nixos/auto-patchelf: Various fixes #40598

dasJ · 2018-05-16T15:34:05Z

Motivation for this change

Maybe I'm just using the tool wrong, but it seems it has two issues (see each individual commit message for more details).

The first thing fixed are shell errors from the findElfs function.
The other thing is the discovery of runtime dependencies, the commit message really explains that more clearly.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

The old code was complaining about '--' not found, probably the wrong shell was used. This fixes the errors.

The old code was just adding the runtimeDependencies to the rpath of each binary. It did, however, not check those directories for library files, so the search would fail if you didn't specify all dependencies as native build inputs. This commit changes that behaviour by adding the lib directories to the search path and search them for the .so files instead of blindly adding them to the rpath

Ericson2314 · 2018-05-16T15:37:23Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

-            shift
-        done
-    ' -- {} +
+    find "$1" -type f | while IFS='' read -r file; do


You know about -print0? That would be better.

Implemented.

Ericson2314 · 2018-05-16T15:39:18Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

+    find "$1" -type f | while IFS='' read -r file; do
+        mimeType="$(file -b -N --mime-type "$file")"
+        if [ "$mimeType" = application/x-executable \
+            -o "${mimeType}" = application/x-sharedlib ]; then


While we're at it, try running https://github.com/koalaman/shellcheck, it's in nixpkgs. I think it will complain about the -o being non-standard. It should also find other useful things.

Fixed all issues in the file.

Ericson2314 · 2018-05-16T15:41:12Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

@@ -156,6 +151,14 @@ autoPatchelf() {
    cachedDependencies+=(
        $(find "$prefix" \! -type d \( -name '*.so' -o -name '*.so.*' \))
    )
+    # Also add all runtime dependencies
+    if [ -n "$runtimeDependencies" ]; then


Where is runtimeDependencies defined anyways?

It can be defined in your derivation. Some CUDA derivation uses it iirc.

bjornfor · 2018-05-16T16:35:21Z

Cc @aszlig

aszlig

Except for the fixup of the runtime dependencies I couldn't find a very convincing reason for the other changes made here. Maybe you can further elaborate on them, especially why they're needed in the way you've implemented them.

aszlig · 2018-05-16T17:01:20Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

+            [ "$mimeType" = application/x-sharedlib ]; then
+            echo "$file"
+        fi
+    done < <(find "$1" -type f -print0)


I don't think this is a big improvement over the old implementation, because IMHO it makes the code less readable. What exactly was the problem with --? Was it something that shellcheck reported or did the error occur at runtime?

When a dependency wasn't found at all, the exact error was:

--: line 5: echo: write error: Broken pipe

Repeated about 20 times

aszlig · 2018-05-16T17:04:06Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

@@ -101,16 +99,11 @@ findDependency() {
 }

 autoPatchelfFile() {
-    local dep rpath="" toPatch="$1"
+    local dep interpreter missing rpath="" toPatch="$1"


Is this also a thing that shellcheck has reported or did you just prefer to declare all locals at the top of the function?

Yes, this was shellcheck's SC2155

aszlig · 2018-05-16T17:08:07Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

+        for dep in $runtimeDependencies; do
+            while IFS='' read -r -d $'\0' file; do
+                cachedDependencies+=("$file")
+            done < <(find "$dep/lib" \! -type d \( -name '*.so' -o -name '*.so.*' \) -print0)


Recursively adding all the .so files from a particular runtime dependency is going to also add files like $dep/lib/somename/plugins/foo.so to the RPATH. So maybe it's generally a better idea to list shared object files explicitly.

This was the old code and the hook is for automatically detecting the .so dependencies instead of listing them in a patchelf command, so I think this use case is legitimate.

aszlig · 2018-05-16T17:08:37Z

pkgs/build-support/setup-hooks/auto-patchelf.sh

+    # shellcheck disable=SC2154
+    while IFS='' read -r -d $'\0' file; do
+        cachedDependencies+=("$file")
+    done < <(find "$prefix" \! -type d \( -name '*.so' -o -name '*.so.*' \) -print0)


Again here, I think this makes the code less readable.

The reason for the while loop is -print0 which was suggested above. I need to use this way (done < <() instead of find | while, because the latter one spawns a subshell and so I'm not able to modify variables (see SC2030)

There is also shopt -s lastpipe btw but that is a fairly recent bash feature (sadly nix-shell uses impure bash).

I thought that was fixed recently...?

@bjornfor Oh was it?!

dasJ · 2018-05-16T17:32:12Z

@aszlig The fixup of the runtime dependencies is something I am not sure about anymore. From the nixpkgs manual:

All packages within the runtimeDependencies environment variable are unconditionally added to executables, which is useful for programs that use dlopen(3) to load libraries at runtime.

So I am basically breaking this use case - they are not added unconditionally, but only when the .so is needed. I could introduce a new variable, or do you see any way to fix my use case? To elaborate my use case more clearly:

My use case

I have a package (Citrix icaclient) that is distributed in binary form only, and that has a lot of .so and executable files. So instead of calling patchelf for every file with the correct packages, this hook seems to automate this work by giving it a list of all packages that could contain the .so files, so the rpath for each individual file is built on the dependencies a file needs.

The only other location where runtimeDependencies is used seems to be cudatoolkit, but the package doesn't add this hook so I guess it's never run anyway.

aszlig · 2018-05-24T00:44:57Z

@dasJ: If I'm understanding correctly then the use case you're describing is the same as the autoPatchelfHook. Initially I wrote the hook for packaging proprietary games mainly because I was tired of getting runtime errors when they add a new library dependency on an upgrade. What I'm not following though is: In which ways does icaclient differ from most other proprietary software and why is it a problem with the hook?

Could you please post the Nix expression of that package (if the binaries are public) to show the nature of the problem?

dasJ added 2 commits May 16, 2018 17:30

nixos/auto-patchelf: Refactor elf detection

02fa61e

The old code was complaining about '--' not found, probably the wrong shell was used. This fixes the errors.

dasJ requested a review from Ericson2314 as a code owner May 16, 2018 15:34

Ericson2314 reviewed May 16, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels May 16, 2018

nixos/auto-patchelf: Fix shellcheck issues

48ad829

aszlig requested changes May 16, 2018

View reviewed changes

dasJ closed this Dec 9, 2018

dasJ deleted the auto-patchelf branch December 9, 2018 17:37

danbst mentioned this pull request Mar 6, 2019

auto-patchelf: don't use grep -q, as it causes Broken pipe #56958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/auto-patchelf: Various fixes #40598

nixos/auto-patchelf: Various fixes #40598

dasJ commented May 16, 2018

Ericson2314 May 16, 2018

dasJ May 16, 2018

Ericson2314 May 16, 2018

dasJ May 16, 2018

Ericson2314 May 16, 2018

dasJ May 16, 2018 •

edited

bjornfor commented May 16, 2018

aszlig left a comment

aszlig May 16, 2018

dasJ May 16, 2018

aszlig May 16, 2018

dasJ May 16, 2018

aszlig May 16, 2018

dasJ May 16, 2018

aszlig May 16, 2018

dasJ May 16, 2018

Ericson2314 May 16, 2018

bjornfor May 16, 2018

Ericson2314 May 16, 2018 •

edited

dasJ commented May 16, 2018 •

edited

aszlig commented May 24, 2018

nixos/auto-patchelf: Various fixes #40598

nixos/auto-patchelf: Various fixes #40598

Conversation

dasJ commented May 16, 2018

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasJ May 16, 2018 • edited

Choose a reason for hiding this comment

bjornfor commented May 16, 2018

aszlig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 May 16, 2018 • edited

Choose a reason for hiding this comment

dasJ commented May 16, 2018 • edited

My use case

aszlig commented May 24, 2018

dasJ May 16, 2018 •

edited

Ericson2314 May 16, 2018 •

edited

dasJ commented May 16, 2018 •

edited