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

RPATH support using ld wrapper (WIP) #1613

Merged
merged 6 commits into from
Nov 14, 2016
Merged

Conversation

rjeschmi
Copy link
Contributor

I am working to getting this working to spec, but I thought I'd start the PR with a version using the wrapper. It works well enough, but I haven't removed the module deps yet (so LD_LIBRARY_PATH is still populated)

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2678/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

descr = ("RPATH options", "changes linking to include rpaths")

opts = OrderedDict({
'rpath': ("Enable RPATH support", None, 'store_true', False),
Copy link
Member

Choose a reason for hiding this comment

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

do you expect other options here?

if not, this shouldn't be an option group by itself?

@boegel
Copy link
Member

boegel commented Feb 13, 2016

@rjeschmi: this looks pretty basic (good!), but I think wrapper ld is not what we plan to do in the end, right?

@rjeschmi
Copy link
Contributor Author

I'll probably leave it as an "option"

I feel like having a wrapper available with some more generic hooks is a good thing and might lead to helpful debugging or special case handling in the future.

The way it is injected makes it low risk for inclusion even if it is not the default method

Ultimately for some cases this might work better than setting environment variables and is a good option to have around.

dir=`pwd`
for (( i=${#@}; i >= 0; i-- )); do
if [[ ${!i} == "--enable-new-dtags" ]]; then
## we are removing this flag if passed as it creates a copy of rpath to runpath.
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we at the mercy of what the default is set to here? We probably should (always) be adding --disable-new-dtags to the arguments as well as removing --enable-new-dtags

@boegel boegel added this to the v2.7.0 milestone Feb 16, 2016
@boegel
Copy link
Member

boegel commented Feb 17, 2016

TODO:

  • add sanity check (using https://github.com/eliben/pyelftools)
  • allow rpath methods: env, ld wrapper, compilers wrappers (multiple options at the same)
  • no $LD_LIBRARY_PATH in modules (and check for it when loading dep modules)

@hpcugentbot
Copy link

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2776/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel boegel modified the milestones: v2.8.0, v2.7.0 Mar 2, 2016
@boegel boegel modified the milestones: v2.8.0, v2.x May 10, 2016
@boegel boegel merged commit 1ee637d into easybuilders:develop Nov 14, 2016
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.

4 participants