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

pkgsLinux: set crossSystem instead of localSystem #317651

Draft
wants to merge 5 commits into
base: staging
Choose a base branch
from

Conversation

tie
Copy link
Member

@tie tie commented Jun 6, 2024

Description of changes

As the doc implies, we should be using crossSystem to build for the Linux platform. Using localSystem just happens to occasionally hit cached builds. See also #316659 (comment) and https://github.com/NixOS/nixpkgs/runs/25855763929

See also #282401, #294725

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.

@tie tie requested review from roberth and Gabriella439 June 6, 2024 05:29
@ofborg ofborg bot added 8.has: clean-up 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 6, 2024
@tie
Copy link
Member Author

tie commented Jun 6, 2024

The last tpm2-tss commit would likely cause a lot of rebuilds since it’s systemd dependency. I’ll rebase to the staging branch later.

@tie tie force-pushed the nixos-darwin-cross branch 2 times, most recently from d36910e to d64c253 Compare June 6, 2024 06:35
@ofborg ofborg bot requested a review from baloo June 6, 2024 08:10
@tie
Copy link
Member Author

tie commented Jun 6, 2024

OK, apparently some Linux-specific packages are still getting pulled from hostPkgs (mostly for full systemd, for some reason, e.g. bpftool). Marking the PR as ready for review so we’d at least get an evaluation failure instead of confusing build failure for now.

@tie tie marked this pull request as ready for review June 6, 2024 12:04
@tie tie requested a review from Ericson2314 as a code owner June 6, 2024 12:04
tie added 2 commits June 6, 2024 17:30
As the doc implies, we should be using crossSystem to build *for* the
Linux platform natively instead of hoping that the system has a remote
builder or substituer with the right set of packages.

While this could be considered a breaking change, pkgsLinux attribute is
used exclusively for nixosTests and is a relatively new addition so
there shouldn’t be many external users that would be affected by this
change.
Fixes pkgsLinux.tpm2-tss build on macOS since shadow package is
Linux-specific.
@tie
Copy link
Member Author

tie commented Jun 6, 2024

Ah, I see #293573. That still seems like a really weird and niche use case to me — I mean, it already requires linux builder, so why not run tests there? Instead, I think we should be able to cross-compile NixOS from macOS with some effort.

Copy link
Member

@baloo baloo left a comment

Choose a reason for hiding this comment

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

This will need to retarget to staging (because of tpm2-tss), but for the tpm2 changes I don't see issues.

@tie tie marked this pull request as draft June 6, 2024 15:33
@tie tie changed the base branch from master to staging June 6, 2024 15:33
@tie tie marked this pull request as ready for review June 6, 2024 15:34
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.

See comment.
To summarize, building in a VM is preferable over cross compilation for various reasons.

@@ -249,7 +249,7 @@ let
if stdenv.hostPlatform.isLinux
then self
else nixpkgsFun {
localSystem = lib.systems.elaborate "${stdenv.hostPlatform.parsed.cpu.name}-linux";
crossSystem = lib.systems.elaborate "${stdenv.hostPlatform.parsed.cpu.name}-linux";
Copy link
Member

Choose a reason for hiding this comment

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

This is meant to be a natively built Linux package set, ie a non-cross build that's potentially done on a different machine. Do you think we non-cross would be a clearer description than natively built in the doc above?
Note that the "machine" is easy to set up with for example linux-builder.

By changing to cross compilation, we make the user experience worse and increase cost.

  • Cross compilation, despite how well it works in Nixpkgs, does not permit tests to be run in package builds
  • We don't have a cache for darwin->linux cross builds, afaik. These builds would be unnecessary extra builds, costing compute and cache storage (ofborg, hydra, cache.nixos.org, etc)

Also note that if you're in a team using a mix of Linux and Darwin and you're doing deployment with cross builds, your performing unnecessary redeployments depending on who's initiating it, causing unnecessary disruptions as system services are "updated" to an equivalent package on a different store path. You could solve this by deploying from dedicated infrastructure, but then you might as well use that for remote builds as well.
More importantly though, non-cross builds are more likely to work.
All in all, non-cross deployments are simpler and more robust, so I would not default to cross compilation anywhere, including here.

Perhaps your goal could be achieved by adding pkgsLinuxCross, but then I still wouldn't make the test framework use that for the reasons above.

Copy link
Member Author

@tie tie Jun 6, 2024

Choose a reason for hiding this comment

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

That sounds reasonable given the current state of cross-compilation of Nixpkgs (though I wouldn’t say it works well, rather it works for well-tested {local,cross}System pairs, but that doesn’t cover glibc → glibc static cross-build, NixOS built from macOS, and a lot of other cases).

I think we’ve had a similar discussion in NixOS/nix#10291 about this use case. W.r.t. tests, these do not change the resulting package output. So it doesn’t really matter where the tests are run as long the closures are byte-for-byte identical and reproducible. That is, as long as we can test that a certain set of packages is identical independent of the localSystem, it doesn’t really matter which derivation runs the tests (assuming that at least one does). Sure, that requires content-addressed derivations to get this right, but then there are a lot of other places that should be fixed before that.

I understand your point, but I do want to improve the current state of affairs. I’ll open separate PR for package-specific commits I’ve pushed to this branch, without this particular change.

@roberth
Copy link
Member

roberth commented Jun 6, 2024

I'd previously asked the infra team to provide a Linux builder, but they were concerned about a deadlock issue. I've now opened an issue with alternatives that avoid the problem (and it's also actually properly visible, unlike chat in Matrix).

@ofborg ofborg bot requested a review from typetetris June 6, 2024 16:17
@tie tie marked this pull request as draft June 6, 2024 16:31
@tie tie mentioned this pull request Jun 6, 2024
13 tasks
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 6, 2024
@ofborg ofborg bot requested a review from pSub June 6, 2024 16:54
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 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.

5 participants