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

cc-wrapper: clean-up post stdenvAdapters.useLibsFrom: init #281750

Closed
wants to merge 1 commit into from
Closed

cc-wrapper: clean-up post stdenvAdapters.useLibsFrom: init #281750

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions pkgs/build-support/cc-wrapper/default.nix
View file
Open in desktop
Copy link
Contributor Author

@SomeoneSerge SomeoneSerge Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This:

nixpkgs/pkgs/build-support/cc-wrapper/default.nix

Lines 113 to 114 in ff1232c

# Analogously to cc_solib and gccForLibs_solib
libcxx_solib = "${lib.getLib libcxx}/lib";

used to be this:

nixpkgs/pkgs/build-support/cc-wrapper/default.nix

Line 567 in 1a5bd69

echo " -L${lib.getLib libcxx}/lib" >> $out/nix-support/cc-ldflags

Afaiu this was exactly the same logic as

nixpkgs/pkgs/build-support/cc-wrapper/default.nix

Lines 439 to 441 in 1a5bd69

+ optionalString useGccForLibs ''
echo "-L${gccForLibs}/lib/gcc/${targetPlatform.config}/${gccForLibs.version}" >> $out/nix-support/cc-ldflags
echo "-L${gccForLibs_solib}/lib" >> $out/nix-support/cc-ldflags

...is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the description mentions, the prime motivation of the cc-wrapper change was to be able to inherit the nixpkgs' default stdenv's libraries (primarily, libstdc++ on {x86_64,aarch64}-linux) in compat stdenvs, e.g. when using an older gcc.

I still find it questionable e.g. to

  1. pass wrappCCWith the wrapped (g)cc instead of of the unwrapped lib output,
  2. pass the stdenv instead of the lib in stdenvAdapters.useLibsFrom.

Copy link
Contributor Author

@SomeoneSerge SomeoneSerge Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In relation to #281371: it isn't enough to propagate the pkgs.stdenv's libraries to the adapted stdenv, it's also necessary to take the other stdenv's (e.g. pkgs.gcc11Stdenv's) native platform's tools, e.g. coreutils:

nixpkgs/pkgs/build-support/cc-wrapper/default.nix

Lines 90 to 91 in 09f9f1f

# The wrapper scripts use 'cat' and 'grep', so we may need coreutils.
coreutils_bin = optionalString (!nativeTools) (getBin coreutils);

Currently these aren't exposed in passthru or anyhow (except for env, which barely can be considered a part of the public interface)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question regarding

nixpkgs/pkgs/stdenv/adapters.nix

Lines 244 to 246 in 2589d8b

/* NOTE: cc.cc is the unwrapped compiler. Should we respect the old
* wrapper instead? */
cc = targetStdenv.cc.cc;
is "do cc-wrappers compose"? In the current form, useLibsFrom pkgs.stdenv pkgsStatic.gcc11Stdenv is going to discard all of the flags related to static linkage I'm pretty sure

Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,8 @@ stdenv.mkDerivation {
else if givenLibcxx then
{ kind = "libc++"; package = libcxx; solib = libcxx_solib;}
else
# We're probably using the `libstdc++` that came with our `gcc`.
# TODO: this is maybe not always correct?
# TODO: what happens when `nativeTools = true`?
# Fallback to assuming libstdc++. This currently doesn't account e.g. for
# `nativeTools = true`
{ kind = "libstdc++"; package = cc; solib = cc_solib; }
;

Expand Down Expand Up @@ -462,13 +461,6 @@ stdenv.mkDerivation {
echo "-L${gccForLibs}/lib/gcc/${targetPlatform.config}/${gccForLibs.version}" >> $out/nix-support/cc-ldflags
echo "-L${gccForLibs_solib}/lib" >> $out/nix-support/cc-ldflags
''
# The above "fix" may be incorrect; gcc.cc.lib doesn't contain a
# `target-triple` dir but the correct fix may be to just remove the above?
#
# For clang it's not necessary (see `--gcc-toolchain` below) and for other
# situations adding in the above will bring in lots of other gcc libraries
# (i.e. sanitizer libraries, `libatomic`, `libquadmath`) besides just
# `libstdc++`; this may actually break clang.

# TODO We would like to connect this to `useGccForLibs`, but we cannot yet
# because `libcxxStdenv` on linux still needs this. Maybe someday we'll
Expand Down