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

[WIP] autoPatchelfHook: search a valid interpreter, or fail #66620

Closed
wants to merge 7 commits into from

Conversation

layus
Copy link
Member

@layus layus commented Aug 14, 2019

Motivation for this change

autoPachelfHook patches everything with the same interpreter
($NIX_CC/nix-support/dynamic-linker) even on incompatible binaries.
This happens in particular when building with multiStdenv.

See https://discourse.nixos.org/t/binary-still-not-working-even-when-properly-patchelfed/1570/3?u=layus for details.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @tadfisher

@layus layus changed the title autoPatchelfHook: search a valid interpreter, or fail [WIP] autoPatchelfHook: search a valid interpreter, or fail Aug 14, 2019
@layus
Copy link
Member Author

layus commented Aug 14, 2019

Tests finished. It seems that this patch breaks a lot of tools based on autoPatchelfHook. For example apktool fails with

searching an interpreted for /nix/store/yypk47fzqcbv0dkd9saaxfnlhcqj7qay-build-tools-28.0.3/libexec/android-sdk/build-tools/28.0.3/zipalign
found interpreter '/nix/store/iykxb0bmfjmi7s53kfg6pjbfpd8jmza6-glibc-2.27/lib/ld-linux-x86-64.so.2' for '/nix/store/yypk47fzqcbv0dkd9saaxfnlhcqj7qay-build-tools-28.0.3/libexec/android-sdk/build-tools/28.0.3/zipalign'
searching for dependencies of /nix/store/yypk47fzqcbv0dkd9saaxfnlhcqj7qay-build-tools-28.0.3/libexec/android-sdk/build-tools/28.0.3/zipalign
  libc++.so -> found: /nix/store/yypk47fzqcbv0dkd9saaxfnlhcqj7qay-build-tools-28.0.3/libexec/android-sdk/build-tools/28.0.3/lib64/libc++.so
setting RPATH to: /nix/store/yypk47fzqcbv0dkd9saaxfnlhcqj7qay-build-tools-28.0.3/libexec/android-sdk/build-tools/28.0.3/lib64
searching an interpreted for /nix/store/yypk47fzqcbv0dkd9saaxfnlhcqj7qay-build-tools-28.0.3/libexec/android-sdk/build-tools/28.0.3/aarch64-linux-android-ld
/nix/store/s50lkmcg73qpd1gdybryndpk46fb8xw9-auto-patchelf-hook/nix-support/setup-hook: line 110: /nix/store/hpzj855nkgjvg58nrhq4910sb9q3kss1-gcc-wrapper-7.4.0/nix-support/dynamic-linker-m32: No such file or directory
objdump: '': No such file
No interpreter found for arch i386 needed by ''
builder for '/nix/store/ffhs8yvjwg51p436dpjjc58a669hky5f-build-tools-28.0.3.drv' failed with exit code 1
cannot build derivation '/nix/store/h1pm5pgkvaaxsg51nknlrlap9kv8swwr-apktool-2.4.0.drv': 1 dependencies couldn't be built

@Mic92 Mic92 requested a review from aszlig August 19, 2019 07:42
@layus
Copy link
Member Author

layus commented Sep 29, 2019

@aszlig Spacechem (openlab-aux/vuizvui#18) needs this to patch correclty the 32bit binary rgb2theora used for solution saves.

Copy link
Member

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request :-)

However, besides the comments mentioned, I'm against introducing unnecessary complexity if there is not a really compelling reason to do so. For the Spacechem example you mentioned, can't you just use callPackage_i686 like this, because it seems that the game is 32bit only?

@@ -6,6 +6,8 @@ deployAndroidPackage {
lib.optional (os == "linux") [ pkgs.glibc pkgs.zlib pkgs.ncurses5 pkgs_i686.glibc pkgs_i686.zlib pkgs_i686.ncurses5 ];
patchInstructions = ''
${lib.optionalString (os == "linux") ''
rm -r $packageBaseDir/{i686,aarch64,mipsel,arm}-linux*
Copy link
Member

Choose a reason for hiding this comment

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

)

echo "searching an '$(getSoArch "$toPatch")' interpreter for $toPatch" >&2
for f in "${linkers[@]}"; do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for f in "${linkers[@]}"; do
for interpreter in "${interpreters[@]}"; do

patchElfInterpreter() {
local toPatch=$1
local linkers=( \
"$NIX_CC/nix-support/dynamic-linker" \
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to use backslashes for continuations here, besides, why have autoPatchelfLinkers and linkers here again?


gatherLibraries() {
autoPatchelfLibs+=("$1/lib")
if [ -f "$1/nix-support/dynamic-linker" ]; then
autoPatchelfLinkers+=("$1/nix-support/dynamic-linker")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
autoPatchelfLinkers+=("$1/nix-support/dynamic-linker")
autoPatchelfLinkers+="$(< "$1/nix-support/dynamic-linker")"

autoPatchelfLinkers+=("$1/nix-support/dynamic-linker")
fi
if [ -f "$1/nix-support/dynamic-linker-m32" ]; then
autoPatchelfLinkers+=("$1/nix-support/dynamic-linker-m32")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
autoPatchelfLinkers+=("$1/nix-support/dynamic-linker-m32")
autoPatchelfLinkers+="$(< "$1/nix-support/dynamic-linker-m32")"

@@ -55,6 +55,10 @@ stdenv.mkDerivation rec {
substitute usr/share/applications/bitwig-studio.desktop \
$out/share/applications/bitwig-studio.desktop \
--replace /usr/bin/bitwig-studio $out/bin/bitwig-studio

# We only support x86_64-linux anyway,
# and these files cannot be correctly autoPatchelfHooked
Copy link
Member

Choose a reason for hiding this comment

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

Why can't they be correctly autoPatchelfHooked?

@layus
Copy link
Member Author

layus commented Sep 30, 2019

The compelling reason was that I needed it for #65069. Not sure if switching to 32bit would be enough. It seems that the canon capt drivers have both 32 and 64 bits binaries packaged.

@tadfisher
Copy link
Contributor

I'm curious if you tried the (somewhat simpler) patch in #51588?

@layus
Copy link
Member Author

layus commented Oct 8, 2019

Not sure how much more simple it is. I did not test it, but the fact that it seemingly does not introduce issues in other packages either means that it is much better than mine, or was just not tested against nixpkgs. Using dynamic-linker* is a good idea however. It makes the code more generic.

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@ryantm ryantm marked this pull request as draft October 23, 2020 03:05
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 23, 2020
@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@Artturin
Copy link
Member

@Artturin Artturin closed this May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants