From 7afd1f354ba5e2794a33d5860873b2d388aa8fa1 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Fri, 19 Jan 2024 19:17:27 -0800 Subject: [PATCH 1/2] cc-wrapper: revert speculative commentary This commit deletes speculative comments which were self-merged with no approvals in PR #275947. If you think that "The above 'fix' may be incorrect" the correct response is to submit a PR which removes the 'fix' and get it reviewed. Likewise, if you think that "For clang it's not necessary" you should submit a PR which wraps it in `if !isClang`. `cc-wrapper` is full of too much junk as it is, let's not make things worse. --- pkgs/build-support/cc-wrapper/default.nix | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pkgs/build-support/cc-wrapper/default.nix b/pkgs/build-support/cc-wrapper/default.nix index 0b25d70b14a2b..10b9e0518f843 100644 --- a/pkgs/build-support/cc-wrapper/default.nix +++ b/pkgs/build-support/cc-wrapper/default.nix @@ -462,13 +462,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 From ee9a233b6877f87cb0115162ff76b1d585f67279 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Fri, 19 Jan 2024 20:40:59 -0800 Subject: [PATCH 2/2] cc-wrapper: relocate proprietary-compiler-specific material PR #275947, which was self-merged without approvals, inserted functionality specific to a propriteary closed-source compiler (CUDA) into cc-wrapper. This commit relocates this CUDA-specific functionality into the appropritate place: `cuda-modules`. It is unclear to me exactly what this function is supposed to be doing; much of it (like the `.kind` attributes) do not appear to be used *anywhere* in nixpkgs. Making sure we don't insert unexplained deadcode like this is one of the important functions of the review process. --- pkgs/build-support/cc-wrapper/default.nix | 19 ---------------- .../cuda-modules/backend-stdenv.nix | 22 +++++++++++++++++++ .../cuda-modules/cuda/overrides.nix | 2 +- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/pkgs/build-support/cc-wrapper/default.nix b/pkgs/build-support/cc-wrapper/default.nix index 10b9e0518f843..af9400a99f161 100644 --- a/pkgs/build-support/cc-wrapper/default.nix +++ b/pkgs/build-support/cc-wrapper/default.nix @@ -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 diff --git a/pkgs/development/cuda-modules/backend-stdenv.nix b/pkgs/development/cuda-modules/backend-stdenv.nix index 3b4c8d47c5e8b..7374c45b58d43 100644 --- a/pkgs/development/cuda-modules/backend-stdenv.nix +++ b/pkgs/development/cuda-modules/backend-stdenv.nix @@ -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 diff --git a/pkgs/development/cuda-modules/cuda/overrides.nix b/pkgs/development/cuda-modules/cuda/overrides.nix index d8fab198208cf..69ddab17bac31 100644 --- a/pkgs/development/cuda-modules/cuda/overrides.nix +++ b/pkgs/development/cuda-modules/cuda/overrides.nix @@ -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"); in {