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

linux bootstrap tools: fix tests on ppc64 #104742

Merged
merged 1 commit into from Jan 26, 2021
Merged

linux bootstrap tools: fix tests on ppc64 #104742

merged 1 commit into from Jan 26, 2021

Conversation

r-burns
Copy link
Contributor

@r-burns r-burns commented Nov 24, 2020 •
edited

The dynamic loader on powerpc64 is called ld64.so.* rather than ld-linux.so.*, and was not matched by the existing pattern.

Please let me know if there's a more generic way to determine the interpreter name, as it's probably not desirable to add special cases here for all the possible names.

cc @CrystalGamma

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-burns
Copy link
Contributor Author

r-burns commented Nov 24, 2020 •
edited

Alternatively we could refactor bintools-wrapper dynamicLinker a bit so it could be used here as well. This would have the benefit of fixing some other platforms we may wish to bootstrap to in the future.

@andir
Copy link
Member

andir commented Nov 24, 2020

Alternatively we could refactor bintools-wrapper dynamicLinker a bit so it could be used here as well. This would have the benefit of fixing some other platforms we may wish to bootstrap to in the future.

The binutils-wrapper should probable be aware of this detail as otherwise a bunch of packages that rely on that information will not work. I am not sure if you can already us it for the bootstrap tool tho..

@r-burns
Copy link
Contributor Author

r-burns commented Nov 24, 2020

Hmm, I was thinking it was not possible to use $NIX_CC/nix-support/dynamic-linker or binutils.dynamicLinker since they provide full paths, but maybe something like
last (stringSplit "/" binutils.dynamicLinker) would be sufficient? Then we would at least be able to keep bintools-wrapper the single source of truth for this info.

@r-burns
Copy link
Contributor Author

r-burns commented Nov 24, 2020

Passing tests on x86_64 and ppc64le, but I don't have an aarch64 machine to test on.

@FRidh FRidh requested a review from vcunat November 24, 2020 16:27
@r-burns
linux bootstrap tools: fix tests on ppc64
9682c1d 
The dynamic loader on powerpc64 is called ld64.so.2 rather than
ld-linux.so.*, and was not matched by the existing pattern.

We reuse the dynamicLinker name from binutils to match a wider set
of platforms and to avoid specifying this information in two places.
@r-burns
Copy link
Contributor Author

r-burns commented Jan 25, 2021

Also tested on i686-linux (via --argstr localSystem and --extra-platforms) and aarch64-linux (on a RPi4).

vcunat
vcunat approved these changes Jan 26, 2021
View reviewed changes
Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

Nice.

@vcunat vcunat merged commit a648a07 into NixOS:master Jan 26, 2021
@r-burns r-burns deleted the ppc64le-bootstrap-ld64 branch January 26, 2021 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl left review comments

@vcunat vcunat vcunat approved these changes

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

Assignees
No one assigned
Labels
6.topic: stdenv Standard environment 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
@r-burns @andir @veprbl @vcunat