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

fix: enable rpath on build and install linking #938

Merged
merged 4 commits into from
Sep 11, 2023
Merged

fix: enable rpath on build and install linking #938

merged 4 commits into from
Sep 11, 2023

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Sep 9, 2023

Briefly, what does this PR introduce?

Enable rpath/runpath linking. The goal of this PR is to add the dependency libraries (JANA, podio, EDM4hep, EDM4eic) with their paths into rpath and runpath, and to have all internal library dependencies be explicit as well. Ultimately, no LD_LIBRARY_PATH should need to be set.

And the same on MacOS...

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

@wdconinc wdconinc marked this pull request as ready for review September 11, 2023 00:44
@wdconinc wdconinc temporarily deployed to github-pages September 11, 2023 01:59 — with GitHub Actions Inactive
veprbl
veprbl previously approved these changes Sep 11, 2023
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
wdconinc and others added 2 commits September 11, 2023 12:13
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

This does not break anything on macOS (although also not needed there - all libraries are resolved by full "install name" path).

@wdconinc
Copy link
Contributor Author

Sounds a bit like this experimental feature in spack: https://spack.readthedocs.io/en/latest/config_yaml.html#shared-linking-bind

That is something we've looked into because of the cvmfs-mounted singularity images. Why link against a remote filesystem's generic /use/local/lib (even as rpath) when you can avoid the directory lookup with a direct path?

I haven't looked into how it is implemented, but I think it's some elfutils rewriting that is needed.

@wdconinc wdconinc added this pull request to the merge queue Sep 11, 2023
Merged via the queue into main with commit e4c7d0e Sep 11, 2023
@wdconinc wdconinc deleted the rpath branch September 11, 2023 23:36
@veprbl veprbl temporarily deployed to github-pages September 12, 2023 00:12 — with GitHub Actions Inactive
@wdconinc wdconinc linked an issue Sep 12, 2023 that may be closed by this pull request
github-merge-queue bot pushed a commit that referenced this pull request Sep 15, 2023
### Briefly, what does this PR introduce?
Allow to create shared instead of static algorithms libraries, and allow
skipping the creation of an algorithms plugin.

Depends on:
- [x] #942 (cherry-picked)
- [x] #938 (cherry-picked)

### What kind of change does this PR introduce?
- [x] Bug fix (issue #628 again)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add dependencies to RPATH by default?
2 participants