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: relocate proprietary-compiler-specific material #282220

Merged
2 commits merged into from Jan 20, 2024
+23 −27
Merged

cc-wrapper: relocate proprietary-compiler-specific material #282220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7afd1f3
cc-wrapper: revert speculative commentary
Jan 20, 2024
ee9a233
cc-wrapper: relocate proprietary-compiler-specific material
Jan 20, 2024
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
26 changes: 0 additions & 26 deletions pkgs/build-support/cc-wrapper/default.nix
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Adam,

Well this is wonderful. It had escaped my attention that the current implementation of stdenvAdapters.useLibsFrom passing the unwrapped cc.cc in gccForLibs means there's nothing on the way to inspect the old wrapper's cxxStdlib and whether it was libc++. This means that the "adapter" is now only functional when gccForLibs is populated from a gcc+libstdc++ stdenv, not from a libc++ one.

Note that there also remains one more cc-wrapper composability issue regarding the propagation of e.g. coreutils when creating new wrappers, as mentioned in #281750 (comment).

Note that this functionality, still half-broken as we've established above, is not in any way specific to any "proprietary compilers", it's specific to the general software that expects older toolchains, e.g. the non-proprietary tensorflow:

nixpkgs/pkgs/top-level/python-packages.nix

Line 14190 in 8b397b2

stdenv = pkgs.stdenvAdapters.useLibsFrom stdenv pkgs.gcc12Stdenv;

This is especially relevant during the post-staging phases (i.e. now) when we get lots of failures from gcc major version updates, including in packages with many reverse dependencies.

A review would have helped a ton in getting things right early, fixing the obvious mistakes, and in pruning the garbage. The PR was open for three weeks for people to object or demand changes. I had notified people adjacent to the cc-wrapper, e.g. rrbutani, whose one year-old patchwork I based the PR off, and Daniel. I had asked for advice on discourse and later in the "Nix/NixOS Contributions" and the "Stdenv" matrix channels. Nobody came up with any suggestions at the time (save for the idea to move the bulk of the logic out into a new "stdenv adapter"), and nobody thought e.g. to redirect me to you. I get that, I do not expect people to be available 365 days in the year. I also don't expect people never to "touch" the parts that I maintain in Nixpkgs unless I intervene in a timely manner.

If you're still convinced I made the wrong call, please feel free to bring this up in any of the public channels suitable for policy discussions. Pushing dramatic commit messages straight into master doesn't really help with anything.

I would really much prefer if you would speak directly to me and react to my messages (#281750 and #275947 (comment)). I'm very glad to see #282221 open. Thanks for bringing it to my attention too, I consider an advancement in this conversation.

I'm interested in having a clean and composable cc-wrapper (or any alternative way to configure toolchains). Whatever we had encountered back in #223664 #225493 #220341 (comment) wasn't clean enough for me to be able to infer any context or build a notion of the public interface. This lead up to us relying on the correct libstdc++ being picked up by the compiler pretty much accidentally (#225661 (comment)) for several months. I get the impression that you must be one deeply invested in the wrapper and set on refactoring it, and that's great: I want to collaborate with somebody with this background. I hope that this message will be the only one where I resort to instigating drama, and that the further exchange will be technical, because I don't want to waste your time or mine.

Cheers

Original file line number Diff line number Diff line change
Expand Up @@ -264,25 +264,6 @@ stdenv.mkDerivation {
inherit bintools;
inherit cc libc libcxx nativeTools nativeLibc nativePrefix isGNU isClang;

# Expose the C++ standard library we're using. See the comments on "General
# libc++ support". This is also relevant when using older gcc than the
# stdenv's, as may be required e.g. by CUDAToolkit's nvcc.
cxxStdlib =
let
givenLibcxx = libcxx.isLLVM or false;
givenGccForLibs = useGccForLibs && gccForLibs.langCC or false;
in
if (!givenLibcxx) && givenGccForLibs then
{ kind = "libstdc++"; package = gccForLibs; solib = gccForLibs_solib; }
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`?
{ kind = "libstdc++"; package = cc; solib = cc_solib; }
;

emacsBufferSetup = pkgs: ''
; We should handle propagation here too
(mapc
Expand Down Expand Up @@ -462,13 +443,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
22 changes: 22 additions & 0 deletions pkgs/development/cuda-modules/backend-stdenv.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,34 @@ let
assertCondition = true;
in

/*
# We should use libstdc++ at least as new as nixpkgs' stdenv's one.
assert let
cxxStdlibCuda = cudaStdenv.cc.cxxStdlib.package;
cxxStdlibNixpkgs = stdenv.cc.cxxStdlib.package;

# Expose the C++ standard library we're using. See the comments on "General
# libc++ support". This is also relevant when using older gcc than the
# stdenv's, as may be required e.g. by CUDAToolkit's nvcc.
cxxStdlib = libcxx:
let
givenLibcxx = libcxx != null && (libcxx.isLLVM or false);
givenGccForLibs = libcxx != null && !(libcxx.isLLVM or false) && (libcxx.isGNU or false);
libcxx_solib = "${lib.getLib libcxx}/lib";
in
if (!givenLibcxx) && givenGccForLibs then
{ kind = "libstdc++"; package = libcxx; solib = libcxx_solib; }
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`?
{ kind = "libstdc++"; package = cc; solib = cc_solib; }
;
in
((stdenv.cc.cxxStdlib.kind or null) == "libstdc++")
-> lib.versionAtLeast cxxStdlibCuda.version cxxStdlibNixpkgs.version;
*/

lib.extendDerivation assertCondition passthruExtra cudaStdenv
2 changes: 1 addition & 1 deletion pkgs/development/cuda-modules/cuda/overrides.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ attrsets.filterAttrs (attr: _: (builtins.hasAttr attr prev)) {
useCcForLibs = true;
gccForLibs = ccForLibs-wrapper.cc;
};
cxxStdlibDir = ccForLibs-wrapper.cxxStdlib.solib;
cxxStdlibDir = ccForLibs-wrapper.cxxStdlib.solib or (throw "necessary to fix CI");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate, which CI you're referring to? The PR had passed the Ofborg checks

Copy link
Contributor

@SomeoneSerge SomeoneSerge Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This branch is now reachable in the default (save for allowUnfree) instantiation of Nixpkgs and breaks the eval:

❯ NIXPKGS_ALLOW_UNFREE=1 nix-instantiate '.' -A tests.cuda
...

       error: necessary to fix CI

in
{

Expand Down