cudaPackages: multiple outputs for redistributables #240498
Conversation
ConnorBaker
added
0.kind: enhancement
ofborg
bot
added
10.rebuild-darwin: 0
|
Wow, this is huge (no pun intended xD)! I suppose this PR is to be reviewed later, after you mark it ready? Just a comment before then: it seems like this change deals with aarch64 support, cross-compilation support, and splayed outputs support at once. When we start merging this, it might be easier to merge in smaller chunks? I'd also be wary of exposing |
You're right, there was a fair amount of scope creep as I was working on this. It needs to be split up.
Yep! I try not to mark PRs as ready for review until I'm near-done or want to make it a collaborative effort/trigger some CI checks.
Definitely! I can see a few different components in this:
I haven't played much with the module system -- most of the inspiration I took came from utility functions the Haskell packaging has: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/haskell-modules/lib/compose.nix. There's a lot of boilerplate in what's being done in the overrides which is why I started introducing those abstractions. As I was writing some of this code I caught myself trying to make everything point-free (a bad habit from my Haskell tendencies). You're right of course that |
ConnorBaker
force-pushed
the
feat/cuda-redist-multiple-outputs
branch
7 times, most recently
from
fb5aefe
to
ff4dc49
Compare
ofborg
bot
removed
the
2.status: merge conflict
ConnorBaker
force-pushed
the
feat/cuda-redist-multiple-outputs
branch
2 times, most recently
from
7c518af
to
1614286
Compare
|
Focused only on multiple outputs now and rebased on master. Prior to stripping the multiple arch work, I made a copy of it here: https://github.com/ConnorBaker/nixpkgs/tree/feat/cuda-redist-multiple-arch. |
ofborg
bot
added
11.by: package-maintainer
☝🏻 ☝🏻 ☝🏻
First of all, this is more like an automation or code-generation tool to simplify the maintenance, I wouldn't call it "critical". Sort of like poetry2nix or nix-update (also live out-of-tree) only it's nvidia2nix. Bash, for that matter, is more critical to us and isn't maintained in-tree. Once we've generated these augmented manifests, the users don't even need to know the tool was ever involved, unless maybe they look to override something. I also like that the the tool being under Connor's account it's easier to treat it as a blackbox: I was about to complain about pydantic being to heavy, but now that it's isolated I literally don't care.
Now that's an important use-case we may be damaging and which we maybe want to handle later. I suspect these extra jsons may make it harder to diverge (as in, using overlays and overrides) from the nixpkgs tree
If the script sticks around and evolves further maybe we can look into moving it to some organization's namespace, for better consolidation and discoverability. Otherwise, I don't a even a bit mind you having it under yours, as long as it's permissively licensed and fork-able 🤷🏻 To conclude,...from my point of view the only thing holding this PR back is that it touches the documentation and I'd like to see that documentation updated first, because I think it could confuse people in its current form |
Any particular portion of the docs you'd want to see updated, @SomeoneSerge? I purposefully didn't want to change things outside of adding a section on update procedures, but I will if you feel it's necessary. |
ConnorBaker
force-pushed
the
feat/cuda-redist-multiple-outputs
branch
from
cc29e78
to
1c16f6b
Compare
|
Updated with feedback on the docs section (CUDA Toolkit -> CUDA Toolkit runfile installer; explicitly mention |
ConnorBaker
force-pushed
the
feat/cuda-redist-multiple-outputs
branch
from
aae745d
to
2556525
Compare
|
Rebased, updated, squashed, and force-pushed. |
|
Result of 16 packages marked as broken and skipped:
22 packages failed to build:
142 packages built:
|
|
Those failures are all of the usual suspects/reproducible on master. @SomeoneSerge I'm confident this is ready to be merged; have I addressed all your doc concerns? @samuela any blockers you see which would prevent merging? |
|
I intend to merge this tomorrow morning (August 31st) at 13:00 UTC barring any strong objects. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/cuda-team-roadmap-update-2023-08-29/32379/1 |
This change which involves creating multiple outputs for CUDA redistributable packages. We use a script to find out, ahead of time, the outputs each redist package provides. From that, we are able to create multiple outputs for supported redist packages, allowing users to specify exactly which components they require. Beyond the script which finds outputs ahead of time, there is some custom code involved in making this happen. For example, the way Nixpkgs typically handles multiple outputs involves making `dev` the default output when available, and adding `out` to `dev`'s `propagatedBuildInputs`. Instead, we make each output independent of the others. If a user wants only to include the headers found in a redist package, they can do so by choosing the `dev` output. If they want to include dynamic libraries, they can do so by specifying the `lib` output, or `static` for static libraries. To avoid breakages, we continue to provide the `out` output, which becomes the union of all other outputs, effectively making the split outputs opt-in.
ConnorBaker
force-pushed
the
feat/cuda-redist-multiple-outputs
branch
from
2556525
to
d5e5246
Compare
Description of changes
This change which involves creating multiple outputs for CUDA redistributable packages.
We use a script to find out, ahead of time, the outputs each redist package provides. From that, we are able to create multiple outputs for supported redist packages, allowing users to specify exactly which components they require.
Beyond the script which finds outputs ahead of time, there is some custom code involved in making this happen. For example, the way Nixpkgs typically handles multiple outputs involves making
devthe default output when available, and addingouttodev'spropagatedBuildInputs.Instead, we make each output independent of the others. If a user wants only to include the headers found in a redist package, they can do so by choosing the
devoutput. If they want to include dynamic libraries, they can do so by specifying theliboutput, orstaticfor static libraries.To avoid breakages, we continue to provide the
outoutput, which becomes the union of all other outputs, effectively making the split outputs opt-in.Additional changes:
patchelfsucceeds.autoPatchelfIgnoreMissingDepsset to ignorelibcuda.so.1specifically instead of all libs, removing the possibility of breakages by accidentally ignoring libraries we should be linking.Show us the numbers!
All found by building against
masterand this branch, with--impureand a local Nixpkgs configuration ofmaster'scudnnto this branch'scudnn.lib.masterwith this branch (same attribute name).masterwith this branch (same attribute name).cudnn.libinstead ofcudnn.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)