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

build-support/cc-wrapper: add libstdc++fs into default library path for clang #213021

Merged
merged 1 commit into from
Jan 28, 2023
Merged

build-support/cc-wrapper: add libstdc++fs into default library path for clang #213021

merged 1 commit into from
Jan 28, 2023

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jan 27, 2023 •
edited
Loading

After #210004 usbmuxd2 started failing to build as:

usbmuxd2-unstable> .../ld: cannot find -lstdc++fs: No such file or directory
usbmuxd2-unstable> clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

This started happening because #210004 exposed a long-standing bug in gcc derivation: cc.lib is missing libstdc++fs library:

$ find $(nix-build --no-link -A stdenv.cc.cc.lib) | fgrep libstdc | unnix

/<<NIX>>/gcc-11.3.0-lib/lib/libstdc++fs.la

/<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.la
/<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.so.6.0.29
/<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.so
/<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.so.6

It was not moved from cc.out output:

$ find $(nix-build --no-link -A stdenv.cc.cc) | fgrep libstdc | unnix
/<<NIX>>/gcc-11.3.0/lib/libstdc++.a
/<<NIX>>/gcc-11.3.0/lib/libstdc++fs.a

This change adds cc library lookup path back to staging-next until gcc is fixed.`

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@trofi trofi mentioned this pull request Jan 27, 2023
Merged
13 tasks
@trofi trofi marked this pull request as draft January 27, 2023 22:25
@trofi
Copy link
Contributor Author

trofi commented Jan 27, 2023

15k rebuilds, more than I expected, but maybe ~ok for staging-next?

$ ./maintainers/scripts/rebuild-amount.sh HEAD^
Estimating rebuild amount by counting changed Hydra jobs (parallel=unset).
      1 pkgs-lib-tests
      1 x86_64-darwin
  15523 x86_64-linux

I expected only affect the only packages that use clangStdenv in one form or another. But maybe it's something like mesa?

@trofi
Copy link
Contributor Author

trofi commented Jan 27, 2023

Yeah, it's most of graphical stack.

@trofi trofi marked this pull request as ready for review January 27, 2023 22:30
ghost
ghost approved these changes Jan 28, 2023
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

LGTM but could you mention in the commit message (maybe even in the title) that the extra flags are added only for clang?

Rebuilt full workstation on:

  • x86_64-linux (except chromium+firefox, still building)
  • powerpc64le-linux (except ghc, chromium+firefox, still building)

pkgs/build-support/cc-wrapper/default.nix
@@ -321,6 +321,11 @@ stdenv.mkDerivation {
&& !(stdenv.targetPlatform.useLLVM or false)
&& gccForLibs != null) ''
echo "--gcc-toolchain=${gccForLibs}" >> $out/nix-support/cc-cflags

# Pull in 'cc.out' target to get 'libstdc++fs.a'. It should be in
# 'cc.lib'. But it's a gcc package bug.
Copy link

@ghost ghost Jan 28, 2023 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

I agree, libraries belong in lib.

Unfortunately a ton of packages that have a $lib output are putting their .a files in $out. I think autotools even does this by default when you build both static and dynamic libraries (i.e. it doesn't use --libdir for the .a files). On my system, when scanning /nix/store/*-lib/ for .a files, the only hits are things that don't use autotools. Or it might be the way nixpkgs invokes ./configure. Anyways, the problem affects a lot more than just gcc.

This should probably be fixed nixpkgs-wide in setup.sh someday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately a ton of packages that have a $lib output are putting their .a files in $out.

From what I understand how library lookup works splitting .a and .so into different directories will effectively cause only one of them to be used as ld will use first found. I would expect most of the packages to put both together in a single directory.

I think autotools even does this by default when you build both static and dynamic libraries (i.e. it doesn't use --libdir for the .a files). On my system, when scanning /nix/store/*-lib/ for .a files, the only hits are things that don't use autotools. Or it might be the way nixpkgs invokes ./configure.

My understanding is that nixpkgs tries hard not to build both if possible (--enable-shared --disable-static / --disable-shared --enable-static). autotools should put both into --libdir. Even FHS distributions rely a lot on --libdir correct handling to put 32-vs-64 bit code into a correct location to make -m32/-m64 just work.

Anyways, the problem affects a lot more than just gcc.

Sure. Though most programs don't implement the split between host/target libraries. For simpler programs it should be only a matter of fixing build system to put results into a correct directory (--libdir or equivalent). gcc case is slightly more complicated than that. Otherwise we would not need this huge driver-wrapper that incorrectly reimplements gcc's assumed library search.

@trofi
build-support/cc-wrapper: add libstdc++fs into default library path f…
4763533 
…or clang

After NixOS#210004 `usbmuxd2` started
failing to build as:

    usbmuxd2-unstable> .../ld: cannot find -lstdc++fs: No such file or directory
    usbmuxd2-unstable> clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

This started happening because NixOS#210004 exposed a long-standing bug in
`gcc` derivation: `cc.lib` is missing `libstdc++fs` library:

    $ find $(nix-build --no-link -A stdenv.cc.cc.lib) | fgrep libstdc | unnix

    /<<NIX>>/gcc-11.3.0-lib/lib/libstdc++fs.la

    /<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.la
    /<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.so.6.0.29
    /<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.so
    /<<NIX>>/gcc-11.3.0-lib/lib/libstdc++.so.6

It was not moved from `cc.out` output:

    $ find $(nix-build --no-link -A stdenv.cc.cc) | fgrep libstdc | unnix
    /<<NIX>>/gcc-11.3.0/lib/libstdc++.a
    /<<NIX>>/gcc-11.3.0/lib/libstdc++fs.a

This change adds `cc` library lookup path back to `staging-next` until
`gcc` is fixed.`
@trofi trofi changed the title build-support/cc-wrapper: add libstdc++fs into default library path build-support/cc-wrapper: add libstdc++fs into default library path for clang Jan 28, 2023
@trofi
Copy link
Contributor Author

trofi commented Jan 28, 2023

LGTM but could you mention in the commit message (maybe even in the title) that the extra flags are added only for clang?

Added for clang to commit summary.

@vcunat vcunat merged commit 710d53b into NixOS:staging-next Jan 28, 2023
@trofi trofi deleted the cc-wrapper-cc.out branch January 28, 2023 07:50
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

@primeos primeos Awaiting requested review from primeos

@davidtwco davidtwco Awaiting requested review from davidtwco

@dtzWill dtzWill Awaiting requested review from dtzWill

@lovek323 lovek323 Awaiting requested review from lovek323

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trofi @vcunat