-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
cc-wrapper: clean-up post stdenvAdapters.useLibsFrom: init
#281750
cc-wrapper: clean-up post stdenvAdapters.useLibsFrom: init
#281750
Conversation
...from "cudaPackages.backendStdenv: useGccForLibs", 210ce38
There was a problem hiding this comment.
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:
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?
There was a problem hiding this comment.
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
- pass
wrappCCWith
the wrapped (g)cc instead of of the unwrappedlib
output, - pass the stdenv instead of the
lib
instdenvAdapters.useLibsFrom
.
stdenvAdapters.useLibsFrom: init
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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; |
useLibsFrom pkgs.stdenv pkgsStatic.gcc11Stdenv
is going to discard all of the flags related to static linkage I'm pretty sure
...from "cudaPackages.backendStdenv: useGccForLibs", 210ce38
Description of changes
#275947 (based on @rrbutani's suggestions and PoC) introduced (without approval) passthru attributes in the cc-wrapper, the purpose of which is to allow propagating the standard c++ libraries used by another wrapper. These attributes are now (ab)used to fix the libstdc++ compatibility issues in
cudaSupport = true
packages and inpython3Packages.tensorflow
, which use older gcc than the rest of nixpkgs.As #275947 (comment) points out, this had introduced more junk in the wrapper, lacks longer-term vision, etc.
This draft is a placeholder just removing the not-so-useful comments introduced by the preceding PR, but I'm hoping we can also have a conversation about whether the passthru solution hits a local minima, about what the other cc-wrapper bits touched by the PR were actually intended for, etc.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.