Feat/cuda stub setup hook#14
Conversation
Signed-off-by: Connor Baker <ConnorBaker01@gmail.com>
Signed-off-by: Connor Baker <ConnorBaker01@gmail.com>
Signed-off-by: Connor Baker <ConnorBaker01@gmail.com>
|
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.
Doing the pre-increment is how we avoid (( 0 )), so no need for || true.
There was a problem hiding this comment.
Yeah I much prefer explicit || true without the 4LOC comment
There was a problem hiding this comment.
|| 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.
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.
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.
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.
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.
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.
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.
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.
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 <ConnorBaker01@gmail.com>
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.