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.

Sign up for GitHub

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

Jump to bottom

lib/systems: use correct config for armv7l-hf-multiplatform #47980

Merged
merged 1 commit into from Oct 10, 2018
Merged

lib/systems: use correct config for armv7l-hf-multiplatform #47980

merged 1 commit into from Oct 10, 2018

Conversation

lopsided98
Copy link
Contributor

Motivation for this change

Elaborating lib.systems.example.armv7l-hf-multiplatform results in unexpected values for some of the fields, breaking cross-compilation of certain packages. In particular linux_hardkernel_4_14 fails because it uses meta.platforms to restrict the host system to armv7l-linux and hostPlatform.system has armv7a-linux instead.

This command reproduces the problem:

# nix-build '<nixpkgs>' --arg crossSystem '(import <nixpkgs/lib>).systems.examples.armv7l-hf-multiplatform' -A linuxPackages_hardkernel_4_14.kernel
error: Package ‘linux-4.14.69-148’ in /elided/nixpkgs/pkgs/os-specific/linux/kernel/manual-config.nix:237 is not supported on ‘armv7a-unknown-linux-gnueabihf’, refusing to evaluate.

Before this PR:

> s = lib.systems.elaborate lib.systems.examples.armv7l-hf-multiplatform
> s.config
"armv7a-unknown-linux-gnueabihf"
> s.parsed.cpu.name
"armv7a"
> s.system
"armv7a-linux" # Should be armv7l-linux

The simple fix seems to be to change config to armv7l-unknown-linux-gnueabihf as is done by this PR, but I'm not confident that this is the correct fix.

I don't really know what the difference between the two triplets is, and why it was chosen in the first place. a02be2b seems like it could have introduced this problem, but before that commit system was arm-linux, which is still wrong. Going further back, 171c7f0 changed config from armv7l-unknown-linux-gnueabi to arm-linux-gnueabihf, but the cross compilation infrastructure has changed so much since then that I'm not sure what implications that had.

After this PR:

> s = lib.systems.elaborate lib.systems.examples.armv7l-hf-multiplatform
> s.config
"armv7l-unknown-linux-gnueabihf"
> s.parsed.cpu.name
"armv7l"
> s.system
"armv7l-linux"

I have tested this only on cross-compiled kernels, which build correctly. I'm afraid it could break packages that use parsed.cpu.name.

cc @Ericson2314

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@andrew-d
Copy link
Contributor

andrew-d commented Oct 7, 2018

Also cc @qolii as someone that uses the Hardkernel kernel

@Ericson2314
Copy link
Member

l is for little endian, a is for the arm variant. Frankly all these names are a mess and both are in wide circulation, but I went with the a because the first component is supposed to describe the CPU, not the ABI, and endianness is a matter of modes while a is a matter of buying a different chip.

I think the proper answer is given the chaos nixpkgs should support both so I'll go fix the meta.platforms check.

@lopsided98
Copy link
Contributor Author

lopsided98 commented Oct 7, 2018 •
edited

Just as a note, native builds use config = "armv7l-unknown-linux-gnueabihf". I didn't check exactly how it works, but I assume that it is elaborated from builtins.currentSystem = "armv7l-linux".

@Ericson2314
Copy link
Member

Yeah it is indeed elaborated from that, which in turn comes from build time uname or autoconf vagueries. It's turtles all the way down.

@lopsided98
Copy link
Contributor Author

One way or another, we almost certainly need to make it so hostPlatform.system = "armv7l-linux". I'm not sure what you are planning to change in the meta.platforms check, but that alone won't be enough to fix the problem. There are many other places that use the value of hostPlatform.system to decide how to build a package, and they won't work correctly with "armv7a-linux".

It also seems like the cross system should match the native system, so I don't see a reason for using "armv7a-unknown-linux-gnueabihf" for cross unless we are going to change how elaboration from builtins.currentSystem works.

@lopsided98
Copy link
Contributor Author

Another problem (somewhat related to @volth's) caused by the incorrect value of hostPlatform.system is that this command fails:

# nix-build '<nixpkgs>' --arg localSystem '(import <nixpkgs/lib>).systems.examples.armv7l-hf-multiplatform' -A stdenv
error: a 'armv7a-linux' is required to build '/nix/store/jmzm5rw1w0jsi6gnf093ki827nwyzffp-stdenv.drv', but I am a 'aarch64-linux'

I've needed this in order to get armv7l packages on aarch64 (I have an armv7l remote builder that actually builds the packages, but I wanted to run them on an aarch64). A workaround is to use { system = "armv7l-linux"; } for localSystem, but there is no good reason the first command should not work.

@Ericson2314
Copy link
Member

Alright this seems good enough for now.

@Ericson2314 Ericson2314 merged commit dd428b8 into NixOS:master Oct 10, 2018
@lopsided98 lopsided98 deleted the armv7l-system-config branch October 21, 2018 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lopsided98 @andrew-d @Ericson2314 @GrahamcOfBorg