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

addDriverRunpath: init #269475

Merged
merged 3 commits into from
Dec 3, 2023
Merged

addDriverRunpath: init #269475

merged 3 commits into from
Dec 3, 2023

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 23, 2023

Description of changes

Less opinionated version of #196174

Allows for usage of addDriverRunapth, this way the setuphook is available to allow for a larger window for people to transition from addOpenGLRunpath setuphook.

Additional Context: rfc #121

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/)
  • 23.11 Release Notes (or backporting 23.05 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.

Priorities

Add a 👍 reaction to pull requests you find important.

@mweinelt mweinelt requested a review from Atemu November 23, 2023 18:27
@jonringer jonringer marked this pull request as ready for review November 23, 2023 18:39
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog labels Nov 23, 2023
@jonringer
Copy link
Contributor Author

should cudaPackages.autoAddOpenGLRunpathHook be promoted to top-level pkgs? I feel like it should

@mweinelt mweinelt requested a review from a team November 23, 2023 19:03
Copy link
Member

@knedlsepp knedlsepp left a comment

Choose a reason for hiding this comment

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

Much more intuitive! 👍

pkgs/top-level/all-packages.nix Show resolved Hide resolved
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 23, 2023
@SomeoneSerge
Copy link
Contributor

SomeoneSerge commented Nov 23, 2023

This is obviously an improvement.

Since we don't change the names too often, I wonder if we do want the new name to suggest that there should be one "merged" path for all of the impure dependencies: be that libgbm, or libcuda, or whatever is the next thing. This means that if we expose something impure to a program, we expose everything at once.

Btw the link to the rendered rfc is now broken

@jonringer jonringer changed the title addHardwareRunpath: init addDriverRunpath: init Nov 24, 2023
@jonringer
Copy link
Contributor Author

changed the name to addDriverRunpath, as it is more aligned with "this is where the drivers are located" rather than "this is where the hardware is located"

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM!

pkgs/top-level/all-packages.nix Show resolved Hide resolved
@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 25, 2023
Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you so much for spearheading this @jonringer -- an important cleanup!

@delroth delroth added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 25, 2023
@jonringer
Copy link
Contributor Author

cc @infinisil before I start updating the Nixpkgs Manual entries, and doing a treewide rename.

@ConnorBaker ConnorBaker added the 6.topic: cuda Parallel computing platform and API label Nov 30, 2023
@infinisil
Copy link
Member

Just from a glance this looks fine to me :)

@Atemu
Copy link
Member

Atemu commented Dec 3, 2023

Needs a rebase for the 24.05 manual, then we can merge :)

@Atemu Atemu merged commit fd7f5fd into NixOS:master Dec 3, 2023
22 of 23 checks passed
@jonringer jonringer deleted the addhardwarerunpath-mini branch December 4, 2023 18:10
@Scrumplex Scrumplex mentioned this pull request Dec 18, 2023
13 tasks
SomeoneSerge added a commit to SomeoneSerge/nixpkgs that referenced this pull request Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cuda Parallel computing platform and API 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants