-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Introduce a RUNPATH for compiled macros. #20833
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
base: master
Are you sure you want to change the base?
Conversation
guitargeek
left a comment
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.
LGTM! It would just be nice to add some comment avoid the a possible more future-proof solution as a TODO, so we maybe attract effort to implement RUNPATH embedding in ACLiC.
25229a7 to
6e2dcc0
Compare
core/base/src/TSystem.cxx
Outdated
| cmd.ReplaceAll("\"$BuildDir","$BuildDir"); | ||
| cmd.ReplaceAll("$BuildDir","\"$BuildDir\""); | ||
| cmd.ReplaceAll("$BuildDir",build_loc); | ||
| cmd.ReplaceAll("$RPath", "-Wl,-rpath=" + gROOT->GetSharedLibDir()); |
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.
cmd.ReplaceAll("$RPath", "-Wl,-rpath=" + gROOT->GetSharedLibDir());
This syntax is likely to be platform dependent (eg. different on Windows) and thus the -Wl,-rpath= may belong to the compiledata.sh.
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.
humm .. on the other hand for the Windows case, just not having $PATH on the produced line would have the same effect ... still is -Wl,-rpath= for all other platforms?
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.
The Mac linker indeed didn't support -rpath=x, it was -rpath x. Since Linux supports this, too, I changed it to use spaces.
For Windows, there's no rpath as far as I understand, so compiledata for Windows doesn't contain the $RPath placeholder.
Test Results 22 files 22 suites 3d 22h 26m 42s ⏱️ For more details on these failures, see this check. Results for commit 7f5bc86. ♻️ This comment has been updated with latest results. |
6e2dcc0 to
abd03e1
Compare
With the previous commit, users have control over whether an rpath is added or not, so the unconditial addition of an rpath can be replaced with the placeholder.
This follows up on 3161321, where LD_LIBRARY_PATH was removed from roottest.
It cannot be removed without replacement for tests that compile macros. Either LD_LIBRARY_PATH or RUNPATH is needed for those: When ROOT is installed in the system, or when an LD_LIBRARY_PATH is set such that a different ROOT installation is visible, compiled macros can fail. The sequence of events is as follows:
.so.sois loaded..so, e.g.libROOTVecOps.so,libCore.so, ... Given that the library being opened (the.soresulting from the macro) doesn't have any RUNPATHs set, the ROOT libraries it depends on are searched for first in LD_LIBRARY_PATH and then in the system./usr/lib64/libCore.soand similar. This will provoke a lot of warnings, because TClassTable is already populated, and potentially crashes the program.Therefore, in this PR, a RUNPATH is added for ACLiC libraries on Linux and Mac.
TSystem::GetMakeSharedLib()by default now contains$RPath, which gets expanded to-Wl,-rpath,<ROOT library path>.