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: link $lib/lib -> $lib/$targetConfig correctly and consistently #296219

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

ConnorBaker
Copy link
Contributor

@ConnorBaker ConnorBaker commented Mar 15, 2024

Note

This PR is a re-upload of #282236 (part of #284796)

When native-compiling, gcc will install libraries into:

/nix/store/...-$targetConfig-gcc-$version-lib/lib

When cross-compiling, gcc will install libraries into:

/nix/store/...-$targetConfig-gcc-$version-lib/$targetConfig

When cross-compiling, we intended to create a link from $lib/lib to $lib/$targetConfig, so that downstream users can always safely assume that "${lib.getLib stdenv.cc.cc}/lib" is where the gcc libraries are, regardless of whether stdenv.cc.cc is a cross compiler or a native compiler.

Unfortunately, there were two problems with how we were trying to create these links:

  1. The link would be created only when enableLibGccOutput==true

  2. The link was being created from the incorrect source $lib/lib/lib instead of $lib/lib.

Both of these mistakes are my fault. This commit corrects them by creating the link using ln -Ts (which is more predictable) and by creating the link from gcc/common/builder.nix rather than from gcc/common/libgcc.nix.

Description of changes

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.05 Release Notes (or backporting 23.05 and 23.11 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.

When native-compiling, gcc will install libraries into:

  /nix/store/...-$targetConfig-gcc-$version-lib/lib

When cross-compiling, gcc will install libraries into:

  /nix/store/...-$targetConfig-gcc-$version-lib/$targetConfig

When cross-compiling, we intended to create a link from $lib/lib to
$lib/$targetConfig, so that downstream users can always safely
assume that "${lib.getLib stdenv.cc.cc}/lib" is where the gcc
libraries are, regardless of whether `stdenv.cc.cc` is a cross
compiler or a native compiler.

Unfortunately, there were two problems with how we were trying to
create these links:

1. The link would be created only when `enableLibGccOutput==true`

2. The link was being created from the incorrect source
   `$lib/lib/lib` instead of `$lib/lib`.

Both of these mistakes are my fault.  This commit corrects them by
creating the link using `ln -Ts` (which is more predictable) and by
creating the link from `gcc/common/builder.nix` rather than from
`gcc/common/libgcc.nix`.
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Symlink is correct this time!

Comment on lines +202 to +205
# symlink), this ensures that in every case we can assume that
# $lib/lib contains the .so files
lib.optionalString (with stdenv; targetPlatform.config != hostPlatform.config) ''
ln -Ts "''${!outputLib}/''${targetConfig}/lib" $lib/lib
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to come up with any adverse side-effects...

I imagine that without this PR we can use some kind of buildEnv/symlinkJoin to generate FHS prefixes with cross-compilation tools that would somehow approximate what you get with ubuntu-like distributions?..

All in all, I don't see any reason not to merge this, but I lack familiarity with wrapper

@ConnorBaker ConnorBaker merged commit c7293f7 into NixOS:staging Mar 16, 2024
35 of 36 checks passed
@ConnorBaker ConnorBaker deleted the fix/gcc-cross-nested-libs branch March 16, 2024 15:06
@SomeoneSerge
Copy link
Contributor

I think darwin tests hadn't finished?

@ConnorBaker
Copy link
Contributor Author

@SomeoneSerge any idea if there's an easy way to see the actions that were triggered by a PR (meaning, can I see if the action is still running or if it has failed for this PR)?

Failing that when I get home I can try doing a nixpkgs-review on my laptop for aarch64-darwin and x86_64-darwin, though that looks like it'll be about 8000 packages :l

@SomeoneSerge
Copy link
Contributor

Maybe https://github.com/NixOS/nixpkgs/pull/296219/checks?check_run_id=22722654479 and the adjacent ones

@ConnorBaker
Copy link
Contributor Author

I don't know how I missed the tab labeled Checks...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants