-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
cudaPackages.buildRedist: automatically remove runpath entries for stubs #459416
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
Changes from all commits
58d0c43
bd6334f
caac266
ee1f087
9d38d18
85ff775
6d9255a
d2f2407
413abbf
eca21ba
4944bbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # shellcheck shell=bash | ||
|
|
||
| # getRunpathEntries | ||
| # Append the runpath entries of the dynamically linked ELF file at path to the indexed array referenced by | ||
| # outputArrRef. | ||
| # | ||
| # NOTE: This function does not check if path is a valid ELF file. | ||
| # | ||
| # Arguments: | ||
| # - path: a path to an ELF file with a dynamic section | ||
| # - outputArrRef: a reference to an indexed array (mutated only by appending) | ||
| # | ||
| # Returns 0 if the file is dynamically linked and the runpath was appended to the output array. | ||
| # Returns 1 if patchelf fails (e.g., the ELF file is not dynamically linked, so patchelf fails to print the rpath). | ||
| getRunpathEntries() { | ||
| if (($# != 2)); then | ||
| nixErrorLog "expected two arguments!" | ||
| nixErrorLog "usage: getRunpathEntries path outputArrRef" | ||
| exit 1 | ||
| fi | ||
|
|
||
| local -r path="$1" | ||
| local -rn outputArrRef="$2" | ||
|
|
||
| if [[ ! -f $path ]]; then | ||
| nixErrorLog "first argument path $path is not a file" | ||
| exit 1 | ||
| elif ! isDeclaredArray "${!outputArrRef}"; then | ||
| nixErrorLog "second argument outputArrRef must be a reference to an indexed array" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Declare runpath separately to avoid masking the return value of patchelf. | ||
| local runpath | ||
| # Files that are not dynamically linked cause patchelf to exit with a non-zero status and print to stderr. | ||
| # If patchelf fails to print the rpath, we assume the file is not dynamically linked. | ||
| runpath="$(patchelf --print-rpath "$path" 2>/dev/null)" || return 1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /me wonders if stdenv sets
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But no, unless someone sets it explicitly in a phase they're not set. |
||
|
|
||
| # If the runpath is empty and we feed it to mapfile, it gives us a singleton array with an empty string. | ||
| # We want to avoid that, so we check if the runpath is empty before trying to populate runpathEntries. | ||
| local -a runpathEntries=() | ||
| if [[ -n $runpath ]]; then | ||
| mapfile -d ':' -t runpathEntries < <(echo -n "$runpath") | ||
SomeoneSerge marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| fi | ||
|
|
||
| outputArrRef+=("${runpathEntries[@]}") | ||
|
|
||
| return 0 | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| callPackages, | ||
| isDeclaredArray, | ||
| makeSetupHook, | ||
| patchelf, | ||
| }: | ||
| makeSetupHook { | ||
| name = "getRunpathEntries"; | ||
| propagatedBuildInputs = [ | ||
| isDeclaredArray | ||
| patchelf | ||
| ]; | ||
| passthru.tests = callPackages ./tests.nix { }; | ||
| meta.description = "Appends runpath entries of a file to an array"; | ||
| } ./getRunpathEntries.bash |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| # NOTE: Tests related to getRunpathEntries go here. | ||
| { | ||
| emptyFile, | ||
| getRunpathEntries, | ||
| hello, | ||
| lib, | ||
| pkgsStatic, | ||
| stdenv, | ||
| testers, | ||
| }: | ||
| let | ||
| inherit (lib.attrsets) recurseIntoAttrs; | ||
| inherit (testers) | ||
| shellcheck | ||
| shfmt | ||
| testBuildFailure' | ||
| testEqualArrayOrMap | ||
| ; | ||
|
|
||
| check = | ||
| { | ||
| name, | ||
| elfFile, | ||
| runpathEntries, | ||
| }: | ||
| (testEqualArrayOrMap { | ||
| inherit name; | ||
| expectedArray = runpathEntries; | ||
| script = '' | ||
| set -eu | ||
| nixLog "running getRunpathEntries with ''${elfFile@Q} to populate actualArray" | ||
| getRunpathEntries "$elfFile" actualArray || { | ||
| nixErrorLog "getRunpathEntries failed" | ||
| exit 1 | ||
| } | ||
| ''; | ||
| }).overrideAttrs | ||
| (prevAttrs: { | ||
| inherit elfFile; | ||
| nativeBuildInputs = prevAttrs.nativeBuildInputs or [ ] ++ [ getRunpathEntries ]; | ||
| meta = prevAttrs.meta or { } // { | ||
| platforms = lib.platforms.linux; | ||
| }; | ||
| }); | ||
| in | ||
| recurseIntoAttrs { | ||
| shellcheck = shellcheck { | ||
| name = "getRunpathEntries"; | ||
| src = ./getRunpathEntries.bash; | ||
| }; | ||
|
|
||
| shfmt = shfmt { | ||
| name = "getRunpathEntries"; | ||
| src = ./getRunpathEntries.bash; | ||
| }; | ||
| } | ||
| # Only tested on Linux. | ||
| // lib.optionalAttrs stdenv.hostPlatform.isLinux { | ||
| # Not an ELF file | ||
| notElfFileFails = testBuildFailure' { | ||
| name = "notElfFileFails"; | ||
| drv = check { | ||
| name = "notElfFile"; | ||
| elfFile = emptyFile; | ||
| runpathEntries = [ ]; | ||
| }; | ||
| expectedBuilderLogEntries = [ | ||
| "getRunpathEntries failed" | ||
| ]; | ||
| }; | ||
|
|
||
| # Not a dynamic ELF file | ||
| staticElfFileFails = testBuildFailure' { | ||
| name = "staticElfFileFails"; | ||
| drv = check { | ||
| name = "staticElfFile"; | ||
| elfFile = lib.getExe pkgsStatic.hello; | ||
| runpathEntries = [ ]; | ||
| }; | ||
| expectedBuilderLogEntries = [ | ||
| "getRunpathEntries failed" | ||
| ]; | ||
| }; | ||
|
|
||
| hello = check { | ||
| name = "hello"; | ||
| elfFile = lib.getExe hello; | ||
| runpathEntries = [ | ||
| "${lib.getLib stdenv.cc.libc}/lib" | ||
| ]; | ||
| }; | ||
|
|
||
| libstdcplusplus = check { | ||
| name = "libstdcplusplus"; | ||
| elfFile = "${lib.getLib stdenv.cc.cc}/lib/libstdc++.so"; | ||
| runpathEntries = [ | ||
| "${lib.getLib stdenv.cc.cc}/lib" | ||
| "${lib.getLib stdenv.cc.libc}/lib" | ||
| ]; | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,15 +6,14 @@ | |
| autoAddDriverRunpath, | ||
| autoPatchelfHook, | ||
| backendStdenv, | ||
| config, | ||
| cudaMajorMinorVersion, | ||
| cudaMajorVersion, | ||
| cudaNamePrefix, | ||
| fetchurl, | ||
| lib, | ||
| manifests, | ||
| markForCudatoolkitRootHook, | ||
| setupCudaHook, | ||
| removeStubsFromRunpathHook, | ||
| srcOnly, | ||
| stdenv, | ||
| stdenvNoCC, | ||
|
|
@@ -24,6 +23,7 @@ let | |
| inherit (_cuda.lib) getNixSystems _mkCudaVariant mkRedistUrl; | ||
| inherit (lib.attrsets) | ||
| foldlAttrs | ||
| getDev | ||
| hasAttr | ||
| isAttrs | ||
| attrNames | ||
|
|
@@ -55,6 +55,7 @@ let | |
| ; | ||
| inherit (lib.strings) | ||
| concatMapStringsSep | ||
| optionalString | ||
| toUpper | ||
| stringLength | ||
| substring | ||
|
|
@@ -138,6 +139,8 @@ extendMkDerivation { | |
|
|
||
| # Fixups | ||
| appendRunpaths ? [ ], | ||
| includeRemoveStubsFromRunpathHook ? elem "stubs" finalAttrs.outputs, | ||
| postFixup ? "", | ||
|
|
||
| # Extra | ||
| passthru ? { }, | ||
|
|
@@ -201,7 +204,10 @@ extendMkDerivation { | |
| outputPython = [ "python" ]; | ||
| outputSamples = [ "samples" ]; | ||
| outputStatic = [ "static" ]; | ||
| outputStubs = [ "stubs" ]; | ||
| outputStubs = [ | ||
| "stubs" | ||
| "lib" | ||
| ]; | ||
| }, | ||
| ... | ||
| }: | ||
|
|
@@ -266,6 +272,9 @@ extendMkDerivation { | |
| # in typically /lib/opengl-driver by adding that | ||
| # directory to the rpath of all ELF binaries. | ||
| # Check e.g. with `patchelf --print-rpath path/to/my/binary | ||
| # TODO(@connorbaker): Given we'll have stubs available, we can switch from autoPatchelfIgnoreMissingDeps to | ||
| # allowing autoPatchelf to find and link against the stub files and rely on removeStubsFromRunpathHook to | ||
| # automatically find and replace those references with ones to the driver link lib directory. | ||
|
Comment on lines
+275
to
+277
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be made into an issue after merge. |
||
| autoAddDriverRunpath | ||
| markForCudatoolkitRootHook | ||
| ] | ||
|
|
@@ -332,6 +341,16 @@ extendMkDerivation { | |
|
|
||
| inherit doInstallCheck; | ||
| inherit allowFHSReferences; | ||
| inherit includeRemoveStubsFromRunpathHook; | ||
|
|
||
| postFixup = | ||
| postFixup | ||
| + optionalString finalAttrs.includeRemoveStubsFromRunpathHook '' | ||
| nixLog "installing stub removal runpath hook" | ||
| mkdir -p "''${!outputStubs:?}/nix-support" | ||
| printWords >>"''${!outputStubs:?}/nix-support/propagated-build-inputs" \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to propagate this to runtime? Why no dev output?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn github email integration broke again, I had replied to this days ago but the message doesn't appear. TLDR: |
||
| "${getDev removeStubsFromRunpathHook}" | ||
ConnorBaker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ''; | ||
|
|
||
| passthru = passthru // { | ||
| inherit redistName release; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker: In future we probably want to replace this with a more general patchelf-structuredAttrs adapter, although patchelf is far from the only tool that suffers from this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed... :(