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

pkgs/top-level: use lib.systems.equals for crossSystem #254763

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

tie
Copy link
Member

@tie tie commented Sep 12, 2023

Description of changes

Fixes otherwise equivalent systems being treated as different by packages that compare stdenv.*Platforms using == operator.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@tie tie marked this pull request as ready for review September 12, 2023 14:54
@tie
Copy link
Member Author

tie commented Sep 12, 2023

See also #237427

I think setting crossSystem just here in pkgs/top-level/default.nix to localSystem depending on the lib.system.equals result should preserve equality for buildPlatform and hostPlatform wrt localSystem and crossSystem across Nixpkgs since it is the main import entrypoint.

@tie tie requested a review from roberth September 12, 2023 15:16
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Sep 12, 2023
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Looks like a good improvement.

Could you add a test? We have some in pkgs/test. Not sure about the best place. This might take a new kind of test that warrants its own file. pkgs/test/top-level/default.nix? You could write some assert expressions and end with a pkgs.emptyFile so that it's nix-build-able.

@tie tie force-pushed the nixpkgs-systems-equals branch 2 times, most recently from 6a549d3 to 26dd291 Compare September 13, 2023 04:11
@tie
Copy link
Member Author

tie commented Sep 13, 2023

@roberth, added tests, nix-build -A tests.top-level.

@tie tie force-pushed the nixpkgs-systems-equals branch 3 times, most recently from a62e776 to 110b8cb Compare September 13, 2023 04:21
Fixes otherwise equivalent systems being treated as different by
packages that compare `stdenv.*Platform`s using `==` operator.
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Sep 13, 2023
@Ericson2314
Copy link
Member

This might be too pendantic, but I wonder if tests that instantiate all of Nixpkgs are more meta and therefore should be imported into pkgs/top-level/release.nix (perhaps not directly) rather than Nixpkgs itself.

(Basically the same issue I have with pkgsCross and friends.)

@roberth
Copy link
Member

roberth commented Sep 13, 2023

@Ericson2314 I kinda agree, but this is the pattern we have, and let's solve one problem at a time. Also pkgsCross seems worse, because at least many tests apply to any pkgs or use some of the same parameters.

@ofborg build tests.top-level

@roberth roberth merged commit 04311ac into NixOS:master Sep 13, 2023
12 checks passed
@github-actions
Copy link
Contributor

Successfully created backport PR for release-23.05:

@github-actions
Copy link
Contributor

Git push to origin failed for release-23.05 with exitcode 1

@tie tie deleted the nixpkgs-systems-equals branch September 13, 2023 11:24
@sternenseemann
Copy link
Member

I don't get this change. If crossSystem is passed, we should cross compile always. It makes it even weirder that you can have native cross, but just if the platform set is different in a way you'd ordinarily wouldn't even consider a different platform (pkgsLLVM, pkgsStatic).

I also thought we had the long term goal of making native compilation being the same as cross compilation, i.e. always cross compiling. If we can't even trigger the “native cross” code path anymore due to this check, it's sort of hard to do any work towards it or play around with it.

@roberth
Copy link
Member

roberth commented Sep 24, 2024

IIRC the problem solved here was that this code relied on a rather unpredictable equality check, making the invocation of Nixpkgs very delicate when it comes to the exact identity (or value address if you will) of the values that are passed in for local and cross. This is basically never a thing in a language that's mostly referentially transparent under its own == operator.
More concretely, users implicitly and reasonably expect that the following produce the same package set:

  • nixpkgs { cross = elaborate "a"; local = elaborate "a"; }
  • let a = elaborate "a"; in nixpkgs { cross = a; local = a; }.

I'm sorry that this change broke your workflow, but I think this is still the right behavior, and we should solve the problem you describe explicitly.

I agree that unifying the native and cross code paths should be a goal (with the possible exception of splicing being slower than no splicing), so until we have achieved that it should definitely be possible to experiment with that code path.
You could work around it by making the platform object subtly different, but perhaps a better way is to add a Nixpkgs config option that enables the "native cross" code path instead of the "non-cross" code path.
Would that solve the problem for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants