Skip to content
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

cuda-modules: rewrite setup hooks #302694

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ConnorBaker
Copy link
Contributor

@ConnorBaker ConnorBaker commented Apr 8, 2024

Description of changes

  • Moved to a directory structure for setup hooks
  • More consistent naming convention for our setup hooks (adding aliases when appropriate)
  • Added a logging utility for setup hooks
  • Rewrote markForCudatoolkitRoot and setupCuda to make use of new logging utility and do general cleanup

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ConnorBaker
Copy link
Contributor Author

@SomeoneSerge of some interest -- we're not in the global scope in the body of these setup hooks, I believe. I was running into errors when trying to use the associative arrays cudaHostPathsSeen and cudaOutputToPath in extendcudaHostPathsSeen inside setup-coda.sh. I've kept the -g flag; in the case you're in the global scope, it is a no-op so at the worst it doesn't hurt anything.

@ConnorBaker ConnorBaker force-pushed the feat/cudaPackages-rewrite-setup-hooks branch 2 times, most recently from ff997bd to 0d2e942 Compare April 10, 2024 14:31
@ConnorBaker ConnorBaker force-pushed the feat/cudaPackages-rewrite-setup-hooks branch from 0d2e942 to d0efe91 Compare April 19, 2024 15:54
Comment on lines +17 to +20
markForCudatoolkitRootHook =
mkRenamed "markForCudatoolkitRootHook" "cudaPackages.markForCudatoolkitRoot"
final.markForCudatoolkitRoot; # Added 2024-04-19
setupCudaHook = mkRenamed "setupCudaHook" "cudaPackages.setupCuda" final.setupCuda; # Added 2024-04-19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember a discussion about this, and I agree there's little point in having the suffix, and it's ok not to have it... I don't know why bother renaming old stuff, but no strong opinions here on my part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants