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

etc: Fix install script for rpath removal #15550

Merged
merged 2 commits into from Jul 9, 2014

Conversation

alexcrichton
Copy link
Member

This adds detection of the relevant LD_LIBRARY_PATH-like environment variable
and appropriately sets it when testing whether binaries can run or not.
Additionally, the installation prints a recommended value if one is necessary.

Closes #15545

This adds detection of the relevant LD_LIBRARY_PATH-like environment variable
and appropriately sets it when testing whether binaries can run or not.
Additionally, the installation prints a recommended value if one is necessary.
@pnkfelix
Copy link
Member

pnkfelix commented Jul 9, 2014

This is a good idea, but it does not solve the problem for make install itself.

The reason: The technique that it is using to test whether an installed rustc works or not will be tricked by the build directory itself into thinking that the installed rustc can run, but in fact the needed dependencies are being resolved (at least on Mac OS X), by looking in the local directory and finding the needed libraries there.

The subtle detail here that I am trying to stress is that when the current working directory is the build directory, then all the build products work, despite being moved to the target install directory.

(The observation in the prior paragraph strikes me as a potential footgun, potentially worse than rpath itself; I do not understand the logic that the dynamic library loader is using. Maybe we should be using absolute paths rather than relative ones in the binary that we emit?)

Anyway, here is the transcript:

https://gist.github.com/pnkfelix/8cfa7a8506023f7f1154

In particular, compare this line:
https://gist.github.com/pnkfelix/8cfa7a8506023f7f1154#file-gistfile1-txt-L26
to this line:
https://gist.github.com/pnkfelix/8cfa7a8506023f7f1154#file-gistfile1-txt-L31

(where in the latter all I did was move the current working directory up to the parent of the build tree.)


And of course, note that when I ran make install itself, there was no warning at the end telling me that the installed build products would not actually work without changes to my env vars, as shown here:
https://gist.github.com/pnkfelix/8cfa7a8506023f7f1154#file-gistfile1-txt-L324


Again, I approve of the spirit of this patch, and was close to r+'ing it on that basis. But at this point it does not solve the problem at hand.

@alexcrichton
Copy link
Member Author

This is needed to unblock a snapshot, it may lead to not printing something on OSX, but this is holding back the linux snapshot if it doesn't get merged.

@brson
Copy link
Contributor

brson commented Jul 9, 2014

In @pnkfelix's first example, I don't understand why rustc links correctly when running out of the build directory.

@alexcrichton
Copy link
Member Author

It appears that OSX has a hardcoded path to the library as the last fallback:

$ otool -l x86_64-apple-darwin/stage2/bin/rustc 
...
Load command 27
          cmd LC_LOAD_DYLIB
      cmdsize 112
         name x86_64-apple-darwin/stage1/lib/rustlib/x86_64-apple-darwin/lib/librustrt-4e7c5e5c.dylib (offset 24)
   time stamp 2 Wed Dec 31 16:00:02 1969
      current version 0.0.0
compatibility version 0.0.0
...

I believe it's using these hardcoded paths as the last fallback because nothing is found in DYLD_LIBRARY_PATH

This reverts the promotion from line-comment to doc-comment in 4989a56 to fix
the compiler-docs target.

Closes rust-lang#15553
bors added a commit that referenced this pull request Jul 9, 2014
This adds detection of the relevant LD_LIBRARY_PATH-like environment variable
and appropriately sets it when testing whether binaries can run or not.
Additionally, the installation prints a recommended value if one is necessary.

Closes #15545
@bors bors closed this Jul 9, 2014
@bors bors merged commit 6f8b6c8 into rust-lang:master Jul 9, 2014
@alexcrichton alexcrichton deleted the install-script branch July 10, 2014 03:09
@netvl
Copy link
Contributor

netvl commented Jul 10, 2014

Linux nightly still does not compile with an error about libnative :( http://buildbot.rust-lang.org/builders/nightly-linux/builds/28 It fails on distcheck.

@alexcrichton
Copy link
Member Author

That should be fixed by #15578

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make install (to non-standard prefix) is failing
5 participants