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

lib/systems: elaborate properly with non-matching system / config / parsed args #351608

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

Conversation

wolfgangwalther
Copy link
Contributor

This is broken out of #303849, because even if that PR has stalled for now, imho it's still useful to fix lib.systems.elaborate.


When elaborating a system with both config and system arguments given, they might not match the parsed results. Example:

elaborate {
  config = "i686-unknown-linux-gnu";
  system = "x86_64-linux";
}

This would result in a parsed system for i686, because the config argument is preferred. But since // args // comes after system has been inferred from parsed, it is overwritten again. This results in config and parsed all pointing to i686, while system still tells the story of x86_64.

Inconsistent arguments can also be given when passing parsed directly. This happened in stage.nix for the various package sets.

The solution is simple: One of the three arguments needs to be treated as the ultimate source of truth. system can already be losslessly extracted from parsed. However, config currently can not, for example for various -mingw32 cases. Thus everything must be derived from config.

To do so, system and parsed arguments are made non-overrideable for lib.systems.elaborate. This means, that system will be used to parse when config is not given - and parsed will be ignored entirely (why elaborate an already elaborated system anyway?).

The systemToAttrs helper is exposed on lib.systems, because it's useful to deal with top-level localSystem / crossSystem arguments elsewhere (not in this PR).

@NixOS/stdenv

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Oct 27, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Oct 27, 2024
@wolfgangwalther
Copy link
Contributor Author

Note to myself: I should probably add a test in lib/tests/systems.nix.

…arsed args

When elaborating a system with both "config" and "system" arguments
given, they might not match the parsed results.  Example:

elaborate {
  config = "i686-unknown-linux-gnu";
  system = "x86_64-linux";
}

This would result in a parsed system for i686, because the config
argument is preferred.  But since "// args //" comes after system has
been inferred from parsed, it is overwritten again.  This results in
config and parsed all pointing to i686, while system still tells the
story of x86_64.

Inconsistent arguments can also be given when passing "parsed" directly.
This happened in stage.nix for the various package sets.

The solution is simple: One of the three arguments needs to be treated
as the ultimate source of truth.  "system" can already be losslessly
extracted from "parsed".  However, "config" currently can not, for
example for various -mingw32 cases.  Thus everything must be derived
from "config".

To do so, "system" and "parsed" arguments are made non-overrideable for
systems.elaborate.  This means, that "system" will be used to parse when
"config" is not given - and "parsed" will be ignored entirely.

The systemToAttrs helper is exposed on lib.systems, because it's useful
to deal with top-level localSystem / crossSystem arguments elsewhere.
@wolfgangwalther
Copy link
Contributor Author

Added some tests to show the bad and fixed behavior. Before these changes, the tests fail like this:

% nix-instantiate --eval --strict lib/tests/systems.nix
[
  { expected = "i686"; name = "test_elaborate_config_over_parsed"; result = "x86-64"; }
  { expected = "i686-linux"; name = "test_elaborate_config_over_system"; result = "x86_64-linux"; }
  { expected = "i686"; name = "test_elaborate_system_over_parsed"; result = "x86-64"; }
]

The result is self-conflicting elaborated systems, which leads to problems downstream.

After, the tests pass.

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Nov 3, 2024

@nix-owners nix-owners bot requested a review from infinisil

@infinisil Looking at the ci/OWNERS file, I don't understand why the bot pinged you. Or why it did now, but not before. On the latest force push, I only added tests to /lib/tests/systems.nix. I guess this triggers because you are set up as code owner for /lib overall. But you were not pinged before, when I already made changes to /lib/systems. Is this expected, i.e. will a later rule for a sub directory "take away" your code owner for the top-level? And if so, it's an odd combination that you will be pinged for tests for systems, but not the code itself ;)

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant