-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/cuda stub setup hook #14
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
Feat/cuda stub setup hook #14
Conversation
Signed-off-by: Connor Baker <[email protected]>
Signed-off-by: Connor Baker <[email protected]>
Signed-off-by: Connor Baker <[email protected]>
|
Uh a more reasonable diffview via https://github.com/NixOS/nixpkgs/compare/master...SomeoneSerge:nixpkgs:feat/cuda-stub-setup-hook?expand=1 |
3721b48 to
3397e9f
Compare
| newRunpathEntries+=("$driverLinkLib") | ||
| ((++driverLinkLibSightings)) | ||
| ((++driverLinkLibSightings )) || true # NOTE: (( 0 )) sets exit code 1 |
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.
Doing the pre-increment is how we avoid (( 0 )), so no need for || true.
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.
Yeah I much prefer explicit || true without the 4LOC comment
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.
|| true is redundant when using a pre-increment on a variable with a lower bound of zero. I'll shrink the comment.
| *-cuda*/lib/stubs) | ||
| *-cuda*-stubs/lib) |
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.
Stubs should not exist at the top-level inside lib; I made symlinks previously so autoPatchelfHook could find them since it does not recurse into lib, but that was a bad approach.
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.
I used to keep stubs in a separate output at precisely this level, and on a separate notice I was about to ask why you merged cudart into a single output.
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.
Build scripts and projects assume the directory layout matches what we've got in the redistributables and patching them to respect multiple outputs is too big a headache. I noticed a number of failures building with CUDA 13 because directory traversal through relative paths was assumed to work but cuda_cudart was split into multiple outputs. It was stupid hard for me to get the CUDA 13 PR merged and I couldn't care about rooting out and fixing all those breakages so I just consolidated -- same thing for cuda_nvcc.
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.
patching them to respect multiple outputs is too big a headach
True, that's why we had the extra output with symlinks. One argument I have for the extra output is that we've been lately trying to guarantee that .sos can be found at predictable locations ${out}/lib/${soname}.so, so ${lib}/lib/stubs/libcuda.so -> ${stubs}/lib/libcuda.so... well ok nits
-- same thing for cuda_nvcc.
Yeah this one we never really managed to split properly, the cyclical references...
3397e9f to
e5f409d
Compare
| printWords >>"''${!outputStubs:?}/nix-support/propagatedNativeBuildInputs" \ | ||
| "${getDev removeStubsFromRunpathHook}" |
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.
It's not enough to just record add the dependency to propagated-native-build-inputs -- that only works if we use stubs exclusively from depsHostHost or later (like buildInputs): https://github.com/ConnorBaker/nix-propagation-behavior/blob/1afbd58f2af1468d4564722b7180cc4d89967ef3/README.md?plain=1#L13-L18
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.
Ah, I had momentary doubts whether propagatedBuildInputs was treated differently from propagatedNativeBuildInputs except for offsets. It must be the (( -1 <= hostOffsetNext && hostOffsetNext <= 1 )) || continue block. So should just use propagatedBuildInputs instead
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.
propagated-build-inputs would mean that we'd have offsets (-1, 0) if the stubs are in nativeBuildInputs and (0, 1) if the stubs are in buildInputs. I think we want (-1, 0) at most.
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.
propagated-build-inputs would mean that we'd have offsets (-1, 0) if the stubs are in nativeBuildInputs
Yes, precisely, so it's within bounds of findInputs
Signed-off-by: Connor Baker <[email protected]>
b6abf26 to
3e172af
Compare
e5f409d to
9d38d18
Compare
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.