cudaPackages: apply runpath setup hooks to non-executable ELF files #277213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
While we're at it and since we'll trigger a rebuild anyway, there may be some refactorings to consider. Not meaning to hold the PR though. For one, I'm still unsure if our |
| @@ -9,7 +9,7 @@ elfHasDynamicSection() { | |||
| autoAddOpenGLRunpathPhase() ( | |||
| local outputPaths | |||
| mapfile -t outputPaths < <(for o in $(getAllOutputNames); do echo "${!o}"; done) | |||
| find "${outputPaths[@]}" -type f -executable -print0 | while IFS= read -rd "" f; do | |||
| find "${outputPaths[@]}" -type f -print0 | while IFS= read -rd "" f; do | |||
There was a problem hiding this comment.
-
Here the
|pipe can be replaced with< <( find ...)just as used in the line above. -
Also, is-d ""actually correct? I saw people use-d $'\0'with-print0in other places. No idea how the syntax works but the intent is clear(er)
There was a problem hiding this comment.
- I also wonder if we should consider adding at least some
-iname, just to have fewer reads on average? Like-iname '*.so*' -or -iname '*.dylib*'
There was a problem hiding this comment.
RE: -iname
Might as well delegate this to the user and add something like ${addDriverRunpath_findArgs-}. A derivation we know is going to scan a ton of files of which very few are matches (that happens) could just set some addDriverRunpath_findArgs = [ "-executable" "-iname" "'*.so'" ]
None of this has to happen in this PR though
There was a problem hiding this comment.
Here the | pipe can be replaced with < <( find ...) just as used in the line above.
Funnily, we just had this discussion somewhere else with a colleague about readarray. I didn't get why Nixpkgs uses readarray args < <(some_command), while to me, it's much more idiomatic to do some_command | readarray args (the pipe is more common, it's fewer characters, it doesn't involve a file redirection, etc.).
It turns out the version with | doesn't work because it runs each command in a separate subshell (at least in bash), and commands like readarray and mapfile use bash namerefs to fill the result (here note that outputPaths is passed as a bare symbol), but namerefs don't propagate to parent shell. So, it turns out you can't use | with mapfile or readarray at all to set a variable declared before.
However, it seems it's not the case here. IMHO, the current version with | is simpler and more readable, and I don't see a good reason to imitate the < <( cmd ...) style.
There was a problem hiding this comment.
Idk it's kind of neither here nor there: we're spawning subshells but we're nonetheless doing the bash loop instead of -exec because we want to invoke bash functions... I'm fine with find ... -exec, and I put up with the while ... done < <( ...) loop for when the iteration body is pure bash (I hate the looks, but I pretend it's idiomatic and don't look). Am I a pessimist if I say we draw the worst from both worlds?
There was a problem hiding this comment.
I don't know, it's probably a matter of taste 🙂 the OCamler in me finds reverse application (aka |) left-to-right composition order more readable.
Though I agree that a find -exec would do as well here, and I see your point that piping to a while loop makes it a bit less readable; pipe is probably better for short commands without control-flow.
|
Result of 369 packages marked as broken and skipped:
13 packages failed to build:
106 packages built:
|
|
I believe would be a proper replacement for However, I believe the test here decides if we will proceed with patching the elf file using |
yannham
approved these changes
Jan 9, 2024
There was a problem hiding this comment.
Beside @SomeoneSerge 's remarks (although I feel some of them are a bit outside of the scope of such a PR), I agree that there's no obvious reason to process only files with the executable bit set.
ConnorBaker
approved these changes
Jan 9, 2024
|
I agree that those can be pursued further in a different PR; merging this now to unblock some other stuff. |
|
Sure. I posted the cosmetic comments semi-automatically, I only didn't merge right away because I was unsure about the # of reads, but I guess that's what ssds are for |
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
This prior PR restored the ability for
autoAddOpenGLRunpathHookto be run against shared libraries (which was originally changed by #250639). However, due to the-executableflag in thefindcommand, this only works on shared libraries that also happen to have the executable bit set.I ran into this issue when attempting to update
jetpack-nixosfor 23.11, which also usesautoAddOpenGLRunpathHook. At least for the package I have been working on (cudnn), this does not appear to be a problem with the cudnn redist package in nixpkgs, since the.sofiles have the executable bit set. However, the deb package for cudnn used injetpack-nixosdo not.CC @SomeoneSerge @ConnorBaker
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.