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

gcc: fix building cross-compiler on 32-bit host platforms #82510

Merged
merged 2 commits into from
Mar 22, 2020

Conversation

lopsided98
Copy link
Contributor

Motivation for this change

Fixes building cross-compiler on 32-bit host platforms, which was broken by #81844.

Some quirk/bug of moveToOutput seems to sometimes prevent it from creating the output if it does not already exist. Therefore we need to manually create the output. On 64-bit host platforms, the output gets created here: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/compilers/gcc/builder.sh#L208. Note that the is64bit variable refers to the host platform, although it would probably make more sense for it to refer to the target platform. This is why this bug does not occur for 32-bit ARM cross-compilers.

I'm not sure why #81844 triggered the problem with moveToOutput, as that function's code is hard for me to understand: https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/setup-hooks/multiple-outputs.sh#L93

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.

cc @matthewbauer @misuzu

@Ericson2314
Copy link
Member

Can we just get rid of that is64bit check?, or make it just guard the lib64 part? I rather just delete stupid conditional code than hack around it.

@misuzu
Copy link
Contributor

misuzu commented Mar 13, 2020

There is another fix already in master: b6ae819

@lopsided98
Copy link
Contributor Author

That fix looks pretty broken IMO. It just creates an empty output to satisfy Nix, but the target libraries will be missing.

Once master gets merged into staging, I will revert it in this PR.

@Ericson2314
Copy link
Member

@lopsided98 just updated staging for you

@lopsided98 lopsided98 force-pushed the gcc-i686-static-fix branch from b755e65 to b7b9f82 Compare March 13, 2020 21:39
@@ -201,11 +201,11 @@ postConfigure() {


preInstall() {
mkdir -p "$out/${targetConfig}/lib"
Copy link
Member

Choose a reason for hiding this comment

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

Still want [ -z "$enableMultilib" ] though, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those directories should always be created. I think the logic for the conditional below is correct (other than is64bit referring to wrong platform). On 32-bit platforms, there should only be a lib directory. On 64-bit platforms, there should be lib and lib64, which should be linked unless multilib is enabled.

It doesn't seem to break anything to have lib64 on 32-bit as well (since it happens unintentionally in the armv7l cross compiler), but that kind of change seems out of scope for this PR.

Copy link
Member

@Ericson2314 Ericson2314 Mar 13, 2020

Choose a reason for hiding this comment

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

Well, I'm not sure we ever don't do enableMultilib, but I think without multilib it never prefixes any directories like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multilib is disabled for normal builds. In all the 64-bit GCCs I have built, lib64 is a symlink to lib. The only place I can find in nixpkgs where enableMultilib is true is gcc_multi.

Copy link
Member

Choose a reason for hiding this comment

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

Oh oops I forgot the -z and got it backwards.

@lopsided98 lopsided98 force-pushed the gcc-i686-static-fix branch from b7b9f82 to 83fa0d9 Compare March 13, 2020 21:52
@ofborg ofborg bot requested a review from Synthetica9 March 13, 2020 22:03
@veprbl veprbl added the 6.topic: portability General portability concerns, not specific to cross-compilation or a specific platform label Mar 14, 2020
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.

I see no issue with this. Thanks.

@vcunat vcunat merged commit 4e6a21d into NixOS:staging Mar 22, 2020
vcunat referenced this pull request Mar 23, 2020
@lopsided98 lopsided98 deleted the gcc-i686-static-fix branch October 6, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: portability General portability concerns, not specific to cross-compilation or a specific platform 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants