cudaPackages.backendStdenv.cc: gccForLibs #275947
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SomeoneSerge
force-pushed
the
fix/cuda-cc-wrapper
branch
from
89ecb6e to
489c9f8
Compare
SomeoneSerge
commented
Dec 22, 2023
ofborg
bot
added
10.rebuild-darwin: 0
SomeoneSerge
commented
Dec 22, 2023
SomeoneSerge
force-pushed
the
fix/cuda-cc-wrapper
branch
from
489c9f8 to
90a59ed
Compare
|
Split up and reworded the commits, updated the interface of I'll now see if I can make a stdenv adapter automating this whole business |
SomeoneSerge
force-pushed
the
fix/cuda-cc-wrapper
branch
2 times, most recently
from
4a8bf9e to
2bd5ea5
Compare
ofborg
bot
added
10.rebuild-darwin: 11-100
and removed
10.rebuild-darwin: 0
13 tasks
SomeoneSerge
changed the title
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3259 |
|
Ofborg green, no rebuilds outside the cuda variant. Objections to merging tomorrow? |
SomeoneSerge
commented
Jan 12, 2024
Comment on lines
+241
to
+246
| /* NOTE: cc.cc is the unwrapped compiler. Should we respect the old | ||
| * wrapper instead? */ | ||
| cc = targetStdenv.cc.cc; |
There was a problem hiding this comment.
This is the part I was asking about in https://matrix.to/#/%23stdenv%3Anixos.org/%24jhoyDDs4_NDkhlkmBihy57cv9OSaRkg8zO17cTbr5-0?via=someonex.net&via=matrix.org&via=kde.org&via=matrix.mit.edu
|
A couple of nits I'll add comments for, but other than those I'll say it looks okay -- I haven't however had an opportunity to test this :( |
ConnorBaker
reviewed
Jan 12, 2024
Comment on lines
+113
to
+114
| # Analogously to cc_solib and gccForLibs_solib | ||
| libcxx_solib = "${lib.getLib libcxx}/lib"; |
There was a problem hiding this comment.
I would have expected to see this
Suggested change
| # Analogously to cc_solib and gccForLibs_solib | |
| libcxx_solib = "${lib.getLib libcxx}/lib"; | |
| # Analogously to cc_solib and gccForLibs_solib | |
| libcxx_solib = getLib libcxx | |
| + optionalString (targetPlatform != hostPlatform) "/${targetPlatform.config}"; |
Why do we (or do we not) nest when cross-compiling (beside the pain it causes our tooling, apparently #279952 (comment)).
There was a problem hiding this comment.
Bc it was that way for darwin and i kept it, just moved to a variable
Comment on lines
+279
to
+282
| # TODO: this is maybe not always correct? | ||
| # TODO: what happens when `nativeTools = true`? |
There was a problem hiding this comment.
Any further notes or comments on the two TODOs? If they're still TODO, would you mind adding a bit more description to them or making an issue to follow up on them so they're not lost?
Comment on lines
+461
to
+471
| # 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. |
There was a problem hiding this comment.
Which above fix is this referring to? The other edit you made in the file is almost 200 lines earlier. If it's referring to that, can we move the comment there? This block seems out of place.
There was a problem hiding this comment.
These were left after cherry picking
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
There was a problem hiding this comment.
@rrbutani why did you find it necessary to comment out dc6a8f9#diff-ad7b67d4a92a2db43281c9c45aa74c32341a554fbca0b6b25be593f4648abb5eR330?
I kept things as they were after @danielfullmer's PR
i.e. with the same offsets as nvcc itself
...to fix the configuration error after merging staging-next
SomeoneSerge
force-pushed
the
fix/cuda-cc-wrapper
branch
from
2bd5ea5 to
ff1232c
Compare
|
Result of 395 packages marked as broken and skipped:
86 packages failed to build:
158 packages built:
|
|
RE: Broken on master #280621: All the basic stuff still works in this branch, as expected: I verified that the CPU version of tensorflow works as expected (builds as before using gcc12, links against gcc13's libraries, is importable in python and doesn't conflict e.g. with I still haven't received any reviews on the cc-wrapper specific parts, but seeing how this PR only "really" affects CUDA and tensorflow and how tensorflow blocks a lot of stuff on master, I'm going to merge. If anyone finds some of the comments left after cherry-picking or the final form of |
|
@SomeoneSerge you self-merged this PR without any approvals. I don't think you should have done so for the changes to cc-wrapper, especially with junk like
|
Noted. I had explained my cost/benefit assessment above. I'm happy to put some time into cleaning up at least the mess I have introduced before it reaches any release. And thanks for the feedback! |
13 tasks
This was referenced Jan 20, 2024
ghost
pushed a commit
that referenced
this pull request
Jan 20, 2024
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.
ghost
pushed a commit
that referenced
this pull request
Jan 20, 2024
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.
This was referenced Jan 20, 2024
Closed
mkg20001
pushed a commit
to mkg20001/nixpkgs
that referenced
this pull request
Jan 21, 2024
This commit deletes speculative comments which were self-merged with no approvals in PR NixOS#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.
mkg20001
pushed a commit
to mkg20001/nixpkgs
that referenced
this pull request
Jan 21, 2024
PR NixOS#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.
ghost
mentioned this pull request
peterhoeg
pushed a commit
to peterhoeg/nixpkgs
that referenced
this pull request
Jan 22, 2024
This commit deletes speculative comments which were self-merged with no approvals in PR NixOS#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.
peterhoeg
pushed a commit
to peterhoeg/nixpkgs
that referenced
this pull request
Jan 22, 2024
PR NixOS#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In relation to #226165
Based on:
The actual changes to the cc-wrapper were omitted when cherry-picking because 2946b81 has introduced something potentially equivalent. I haven't had an in-dept look yet
I also want to try pointing
cuda_nvccdirectly at the compatible gcc, either via thesetupCudaHook, or using thenvcc.profileif I get #275922 through first. Then we can start removing thebackendStdenvAccidentally, fixes the tensorflow failure caused by staging-next - we'll probably have more of those in the future
CC @NixOS/cuda-maintainers @rrbutani
Maybe CC #192459
Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.