Skip to content

autoPatchelfHook: fix detection under crossSystem#137351

Merged
symphorien merged 1 commit intoNixOS:masterfrom
impl:autopatchelfhook-cross-fix
Sep 14, 2021
Merged

autoPatchelfHook: fix detection under crossSystem#137351
symphorien merged 1 commit intoNixOS:masterfrom
impl:autopatchelfhook-cross-fix

Conversation

@impl
Copy link
Copy Markdown
Member

@impl impl commented Sep 11, 2021

Motivation for this change

In #84415, autoPatchelfHook was taught to use the correct path to the readelf binary when a crossSystem is specified. Unfortunately, the remainder of the functionality in the script depended on ldd, which only reads ELF files of its own architecture. It has the further unfortunate quality of not reporting any useful error, but rather that the file is not a dynamic executable.

This change uses patchelf to directly analyze the DT_NEEDED tags in the target files instead, which correctly works across architectures. It also updates the use of objdump to be prefix-aware $OBJDUMP (which would have been required in the PR mentioned above, but we never made it that far into the script execution).

Here is an example of a package that can now be properly cross-patched (?, not sure what the terminology for this is):

let
  pkgs = import <nixpkgs> {
    crossSystem = {
      config = "armv6l-unknown-linux-gnueabihf";
      gcc = {
        arch = "armv6kz";
        fpu = "vfp";
      };
    };
  };
in pkgs.callPackage ({ lib, stdenv, fetchFromGitHub, autoPatchelfHook, glibc, gccForLibs }: stdenv.mkDerivation rec {
  pname = "vcdbg";
  version = "1.20210831";

  src = fetchFromGitHub {
    owner = "raspberrypi";
    repo = "firmware";
    rev = version;
    sha256 = "sha256-NhQwg6DRGSwsULxXWJAOUPIW+JvOnZVDTTvIBZnjzgA=";
  };

  nativeBuildInputs = [ autoPatchelfHook ];
  buildInputs = [ gccForLibs.lib ];

  installPhase = ''
    install -m 644 -D -t $out/lib opt/vc/lib/*.so
    install -m 755 -D opt/vc/bin/vcdbg $out/bin/vcdbg
  '';

  meta = with lib; {
    description = "vcdbg";
    homepage = "https://github.com/raspberrypi/firmware";
    license = licenses.unfreeRedistributableFirmware; # See https://github.com/raspberrypi/firmware/blob/master/boot/LICENCE.broadcom
  };
}) {}

This binary runs correctly even under aarch64 on the Raspberry Pi when built.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@impl impl requested a review from aszlig as a code owner September 11, 2021 06:47
@ofborg ofborg Bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Sep 11, 2021
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think this change is doing the right thing because the library path isn't evaluated yet. I'll take another pass at this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

according to man ldd ldd prints all recursive dependencies of the executable while readelf/objdump only print direct dependencies, so that looks like a regression.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, I think I've addressed both of these now. Let me know what you think!

Comment thread pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated
@Mindavi Mindavi added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Sep 11, 2021
@r-rmcgibbo
Copy link
Copy Markdown

Result of nixpkgs-review pr 137351 at fd5670af run on aarch64-linux 1

50 packages marked as broken and skipped:
  • Literate
  • adoptopenjdk-hotspot-bin-13
  • adoptopenjdk-hotspot-bin-14
  • adoptopenjdk-jre-hotspot-bin-13
  • adoptopenjdk-jre-hotspot-bin-14
  • cassandra_2_1
  • cassandra_2_2
  • collectd
  • collectd-data
  • dtools
  • echidna
  • gtkd
  • impressive
  • isabelle
  • jboss
  • odpdown
  • onedrive
  • openems
  • opentsdb
  • ovftool
  • pig
  • polymake
  • processing
  • python38Packages.cnvkit
  • python38Packages.foundationdb51
  • python38Packages.foundationdb52
  • python38Packages.foundationdb60
  • python38Packages.foundationdb61
  • python38Packages.python-csxcad
  • python38Packages.python-openems
  • python38Packages.tensorflowWithCuda
  • python39Packages.cnvkit
  • python39Packages.foundationdb51
  • python39Packages.foundationdb52
  • python39Packages.foundationdb60
  • python39Packages.foundationdb61
  • python39Packages.jax
  • python39Packages.python-csxcad
  • python39Packages.python-openems
  • python39Packages.tensorflowWithCuda
  • qemu_xen
  • qemu_xen-light
  • qemu_xen_4_10
  • qemu_xen_4_10-light
  • rund
  • spark
  • spring
  • springLobby
  • tilix
  • tlaps
51 packages failed to build:
479 packages skipped due to time constraints:
  • DisnixWebService
  • R
  • _1password
  • _7zz
  • abcl
  • adapta-gtk-theme
  • openjdk16-bootstrap (adoptopenjdk-hotspot-bin-15)
  • adoptopenjdk-hotspot-bin-16
  • adoptopenjdk-icedtea-web
  • adoptopenjdk-jre-bin (adoptopenjdk-jre-hotspot-bin-11)
  • ...

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@impl impl force-pushed the autopatchelfhook-cross-fix branch from fd5670a to 77afa34 Compare September 12, 2021 00:14
@impl impl changed the title autoPatchelfHook: Fix detection under crossSystem autoPatchelfHook: fix detection under crossSystem Sep 12, 2021
@impl impl force-pushed the autopatchelfhook-cross-fix branch from 77afa34 to 61733d1 Compare September 12, 2021 00:16
In NixOS#84415, autoPatchelfHook was taught to use the correct path to the
readelf binary when a crossSystem is specified. Unfortunately, the
remainder of the functionality in the script depended on ldd, which only
reads ELF files of its own architecture. It has the further unfortunate
quality of not reporting any useful error, but rather that the file is
not a dynamic executable.

This change uses patchelf to directly analyze the DT_NEEDED tags in the
target files instead, which correctly works across architectures. It
also updates the use of objdump to be prefix-aware $OBJDUMP (which would
have been required in the PR mentioned above, but we never made it that
far into the script execution).
@impl impl force-pushed the autopatchelfhook-cross-fix branch from 61733d1 to b79483d Compare September 12, 2021 22:07
Copy link
Copy Markdown
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

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

could build julia_16-bin natively and cross to aarch64. native julia passes its tests, cross julia can run --version in qemu.

@symphorien symphorien merged commit 3413d61 into NixOS:master Sep 14, 2021
@impl
Copy link
Copy Markdown
Member Author

impl commented Sep 14, 2021

Just to let you know, I have a few more fixes I'll put into a follow-up PR (I rented a big ol' AWS instance and manually reviewed all of the output of nixpkgs-review pr... :) that should improve stability across existing packages as well. Coming shortly!

@impl
Copy link
Copy Markdown
Member Author

impl commented Sep 15, 2021

That PR is #137886, for future onlookers!

@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants