-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
unpack-bootstrap-tools.sh: use patchelf to undo nuke-refs and nothing else #161925
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@ofborg eval |
This can wait until after staging closes tomorrow and reopens on 2022-May-15. But I would like to try to move forward with it at that point; it's been pending for a pretty long time now. I've "minimized" bunch of my comments that either have been resolved or should be dealt with in a separate PR (if at all) in order to try to make the thread more readable to reviewers. Sorry about bungling the base-branch change and all the noise/mass-review-requests caused by doing so. |
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.
In general this makes sense to me, but I don't feel that qualified to do a proper bootstrap review. I'm not at a pc now, but I'd like to do a build too to build some confidence in this. Planning to do that later when I find some time
pkgs/stdenv/linux/bootstrap-tools/scripts/unpack-bootstrap-tools.sh
Outdated
Show resolved
Hide resolved
I have significantly cleaned this up and rewritten the commit message to clarify the motivation (duplicated as the top comment in this PR). Note that there are two commits in this PR; the first commit is #169746 which needs to be included here in order to pass CI. Only the second commit is specific to this PR. |
The comment for the line removed by this patch states that "A threaded perl build needs glibc/libpthread_nonshared.a, which is not included in bootstrapTools". This is not the reason why it is necessary -- libpthread_nonshared.a part of the bootstrap-files on many platforms. The reason why this workaround was needed is because nuke-refs erases the rpath from libpthread.so, and unpack-bootstrap-tools.sh neglects to undo this using patchelf. Let's apply patchelf to libpthread.so, so we aren't leaving dangling /nix/store/eeee.... refs in libpthread.so, and remove the enableThreading=false override since it is no longer necessary. Additional background: The enableThreading=false attribute not only removes the "-Dusethreads" from the invocation of perl's Configure script, it also patches the script like this: # We need to do this because the bootstrap doesn't have a static libpthread sed -i 's,\(libswanted.*\)pthread,\1,g' Configure Although this prevents the perl interpreter from linking against libpthread.so, it does not prevent the libraries bundled with the interpreter from doing so. Time/HiRes/HiRes.so will still try to link against libpthread.so, and is only prevented from doing so by the fact that unpack-bootstrap-tools.sh neglects to patchelf out all of the nuke-refs'd rpaths. When all of the nuke-refs'd paths are patchelf'ed out, HiRes.so links against libpthread.so in the bootstrap tools, and the stdenv bootstrap fails: Can't load '/nix/store/7ny8kmppjzx6kx4xr5kphjz8fqqrlsll-perl-5.34.1/lib/perl5/5.34.1/x86_64-linux/auto/Time/HiRes/HiRes.so' for module Time::HiRes: libpthread.so.0: cannot open shared object file: No such file or directory at /nix/store/7ny8kmppjzx6kx4xr5kphjz8fqqrlsll-perl-5.34.1/lib/perl5/5.34.1/XSLoader.pm line 93. at /nix/store/7ny8kmppjzx6kx4xr5kphjz8fqqrlsll-perl-5.34.1/lib/perl5/5.34.1/x86_64-linux/Time/HiRes.pm line 94. Compilation failed in require at lib/Autom4te/FileUtils.pm line 42. BEGIN failed--compilation aborted at lib/Autom4te/FileUtils.pm line 42. Compilation failed in require at bin/autom4te line 46. BEGIN failed--compilation aborted at bin/autom4te line 46. make[1]: *** [Makefile:2259: tests/wrapper.in] Error 2 make[1]: *** Waiting for unfinished jobs.... Perl implements its own custom library loading routines for its modules, which likely do not fully implement rpath searching. This problem was revealed during the course of writing PR #161925, which depends on this PR in order to pass CI.
… else The current unpack-bootstrap-tools.sh wields patchelf selectively, guided by unexplained criteria. Specifically, it applies patchelf --set-interpreter to: $out/bin/* $out/libexec/gcc/*/*/* (except */liblto*) and patchelf --set-interpreter --set-rpath to: $out/lib/librt-*.so $out/lib/libpcre* The provenance of these lists is unclear. Some libraries which never had an rpath to begin with (like librt.so) are given an rpath by this script, while other libraries (like libbz2.so) are left with dangling /nix/store/eeee...'s. This makes it very difficult to troubleshoot the behavior of the bootstrap-tools unpacker when porting to new platforms, where it is likely the case that patchelf is being debugged concurrently with nixpkgs. This commit changes unpack-bootstrap-tools.sh to use patchelf *only* to replace /nix/store/eeee... references created by nuke-refs, and to replace *all* such references except where an exception is clearly described and explained. Currently there are two such exceptions: 1. libgcc_s.so is exempted because its rpath leaks into the final stdenv, so we cannot patchelf its rpath without creating a fordbidden stdenv-final->bootstrap-tools requisite. 2. libpthread.so and libc.so are exempted from --set-interpreter because the ELF interpreter of these files does not matter (they are libraries, not executable programs, and unlike ld.so they are not meant to play both roles), and because an unresolved bug in patchelf causes corruption if it is used to set the interpreter here: NixOS/patchelf#368 Making sure to patchelf away all of the nuke-refs'd paths will make the bootstrapping process more robust and less fragile in general. In particular, it will prevent situations like #169746 where the only thing preventing a bug from manifesting was the fact that the arbitrary choice of which libraries to patchelf had been made in a particular way. Situations like that make things more fragile, because now the particular choice which used to be arbitrary has become mandatory, and neither that requirement nor the reason for it is documented anywhere. This commit requires #169746 (which is included in this PR) in order to pass CI.
Latest push just rebases on top of latest #169746; no other changes. |
Note: this PR includes two commits; the first commit is #169746 which needs to merge with or before this one in order to not break CI. Only the second commit is specific to this PR.
The current unpack-bootstrap-tools.sh wields patchelf selectively, guided by unexplained criteria. Specifically, it applies
patchelf --set-interpreter
to:(except
*/liblto*
) andpatchelf --set-interpreter --set-rpath
to:The provenance of these lists is unclear. Some libraries which never had an rpath to begin with (like
librt.so
) are given an rpath by this script, while other libraries (likelibbz2.so
) are left with dangling/nix/store/eeee...
's.This makes it very difficult to troubleshoot the behavior of the bootstrap-tools unpacker when porting to new platforms, where it is likely the case that patchelf is being debugged concurrently with nixpkgs.
This commit changes unpack-bootstrap-tools.sh to use patchelf only to replace
/nix/store/eeee...
references created bynuke-refs
, and to replace all such references except where an exception is clearly described and explained. Currently thereare twois one such exception:libgcc_s.so
is exempted from--set-rpath
because its rpath leaks into the final stdenv, so we cannot patchelf its rpath without creating a fordbidden stdenv-final->bootstrap-tools requisite.2.Edit: no longer required (see third patch in series)libpthread.so
andlibc.so
are exempted from--set-interpreter
because the ELF interpreter of these files does not matter (they are libraries, not executable programs, and unlikeld.so
they are not meant to play both roles), and because an unresolved bug in patchelf causes corruption if it is used to set the interpreter. Since setting the ELF interpreter of a library is seldom done (and indeed does not even need to be done here by nixpkgs), it is not likely that the patchelf bug will get fixed anytime soon, nor is it easy to argue for allocating resources to hunting it down.Making sure to patchelf away all of the nuke-refs'd paths will make the bootstrapping process more robust and less fragile in general. In particular, it will prevent situations like #169746 where the only thing preventing a bug from manifesting was the fact that the arbitrary choice of which libraries to patchelf had been made in a particular way. Situations like that make things more fragile, because now the particular choice which used to be arbitrary has become mandatory, and neither that requirement nor the reason for it is documented anywhere.
Things done
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
./result/bin/
)