Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux-builder: don’t read nixpkgs.hostPlatform #1319

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emilazy
Copy link
Collaborator

@emilazy emilazy commented Feb 3, 2025

This is discouraged by NixOS and broke with recent changes.

This is discouraged by NixOS and broke with recent changes.
@@ -124,9 +124,9 @@ in

systems = mkOption {
type = types.listOf types.str;
default = [ cfg.package.nixosConfig.nixpkgs.hostPlatform.system ];
default = [ cfg.package.nixosConfig._module.args.pkgs.stdenv.hostPlatform.system ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this instead?

Suggested change
default = [ cfg.package.nixosConfig._module.args.pkgs.stdenv.hostPlatform.system ];
default = [ cfg.package.nixosConfig.nixpkgs.pkgs.stdenv.hostPlatform.system ];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is it is better to read pkgs from _module.args, because nixpkgs.pkgs is allowed to be undefined, and _module.args.pkgs is also allowed to be overridden e.g. using mkForce.

So the "canonical" pkgs instance is _module.args.pkgs, or I guess _module.specialArgs.pkgs or _module.args.pkgs technically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a difference between the two, then nixpkgs.pkgs would match the previous behavior more, right? Because if you override _module.args.pkgs and don't affect nixpkgs.pkgs that way, then the previous nixpkgs.hostPlatform might only be represented in nixpkgs.pkgs, not _module.args.pkgs.

Copy link
Collaborator Author

@emilazy emilazy Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the difference is that cfg.package.nixosConfig.nixpkgs.pkgs.stdenv.hostPlatform.system is an evaluation error, because nixpkgs.pkgs is not set.

Since @MattSturgeon is correct that this should be checking specialArgs too, the resulting construct is unwieldy, and the Nixpkgs PR has been reverted, I’m going to draft this PR for now until it’s clearer what we should be doing here.

Copy link
Contributor

@MattSturgeon MattSturgeon Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a difference between the two, then nixpkgs.pkgs would match the previous behavior more, right?

Technically, yes.1 But only because the previous behaviour was incorrect.

The intention is to re-use the platform information from whatever pkgs instance is actually in use by the outer configuration.

Footnotes

  1. Although even then, nixpkgs.pkgs is mostly unrelated to nixpkgs.*Platform, because it is used to provide an externally constructed pkgs instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently broken for anyone who sets nixpkgs.hostPlatform to a string, which is an allowed value for nixpkgs.hostPlatform.

If we're not merging this PR at the moment, can I create one that just at least tests if it's a string so we can fix this error for now?

error:
       … while evaluating the attribute 'value'
         at /nix/store/6zgxj37rf9nsf5r0pd4pvwdazpbpnqi4-source/lib/modules.nix:846:9:
          845|     in warnDeprecation opt //
          846|       { value = addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          847|         inherit (res.defsFinal') highestPrio;

       … while evaluating the option `system.build':

       … while evaluating the attribute 'mergedValue'
         at /nix/store/6zgxj37rf9nsf5r0pd4pvwdazpbpnqi4-source/lib/modules.nix:881:5:
          880|     # Type-check the remaining definitions, and merge them. Or throw if no definitions.
          881|     mergedValue =
             |     ^
          882|       if isDefined then

       … while evaluating definitions from `/nix/store/ddd45r3mm6ikg5bqhwwasl09g7vcq178-source/modules/system':

       … while evaluating the option `nix.buildMachines."[definition 1-entry 1]".systems':

       … while evaluating definitions from `/nix/store/ddd45r3mm6ikg5bqhwwasl09g7vcq178-source/modules/nix/linux-builder.nix':

       … while evaluating the option `nix.linux-builder.systems':

       … while evaluating definitions from `/nix/store/ddd45r3mm6ikg5bqhwwasl09g7vcq178-source/modules/nix/linux-builder.nix':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: expected a set but found a string: "aarch64-linux"
    hostPlatform = lib.mkOption {
      type = lib.types.either lib.types.str lib.types.attrs;
      example = {
        system = "aarch64-darwin";
      };
      description = ''
        Specifies the platform where the nix-darwin configuration will run.

        To cross-compile, set also `nixpkgs.buildPlatform`.

        Ignored when `nixpkgs.pkgs` is set.
      '';
    };

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work fine with Nixpkgs master, as the PR that caused the issue was reverted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. I guess that's not in nixpkgs-unstable yet.. do you have a pr number? is it NixOS/nixpkgs#376988 ? asking because that one broke other stuff for me as well and it seems related 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like that error is caused by the combination of:

  1. nix-darwin reading config.nixpkgs.*Platform instead of pkgs.stdenv.*Platform (this PR fixes that).
  2. nix-darwin having backported the removal of config.nixpkgs.*Platform's apply function, that coerced string definitions into attrs.

The proper fix is to finish & merge this PR. For the reasons discussed above; reading nixpkgs.*Platform is unlikely to be what you actually want anyway.

But reverting the apply removal would also work, assuming a compatible nixpkgs revision is in use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think the apply thing in nix-darwin matters here, since the relevant configuration is a NixOS one. The revert on our end is waiting for the Nixpkgs channels, though.

do you have a pr number?

https://nixpk.gs/pr-tracker.html?pr=379615

The proper fix is to finish & merge this PR. For the reasons discussed above; reading nixpkgs.*Platform is unlikely to be what you actually want anyway.

FWIW I don’t disagree with this but my assumption is that we’ll get something better than (cfg.package.nixosConfig._module.specialArgs.pkgs or cfg.package.nixosConfig._module.args.pkgs).stdenv.hostPlatform which is awful enough that I’d rather hold off rather than change something that has basically worked for ages twice.

@hraban
Copy link
Contributor

hraban commented Feb 6, 2025

TLDR for anyone coming from Google: something is broken in nixpkgs/nixpkgs-unstable atm. The last working version built by hydra is 7abfe415360545836eded3dc9a4ebe08e637fd1c, a fix is in master and you can track progress here https://nixpkgs-tracker.ocfox.me/?pr=379615

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants