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.

Sign up for GitHub

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

Jump to bottom

Limit NO_INSTALL_RPATH to out-of-tree builds. #2935

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Limit NO_INSTALL_RPATH to out-of-tree builds. #2935

wants to merge 1 commit into from
+10 −2

Conversation

hvdijk
Copy link
Contributor

@hvdijk hvdijk commented Dec 16, 2024

For in-tree builds with shared libraries, setting the rpath not only works, but is required.

Fixes #2906

@hvdijk
Limit NO_INSTALL_RPATH to out-of-tree builds.
da1fec9 
For in-tree builds with shared libraries, setting the rpath not only
works, but is required.

Fixes KhronosGroup#2906
@hvdijk
Copy link
Contributor Author

hvdijk commented Dec 16, 2024

The formatting may raise some eyebrows, but this is intentional. To work around #2906, currently, we comment out NO_INSTALL_RPATH by prepending # . This PR keeps NO_INSTALL_RPATH on a line of its own to allow us to continue to use that workaround until we no longer need to worry about older branches. Alternatively, if this (plus any edits as a result of review comments) could also be applied to the branches, we could just delete our workaround entirely.

@svenvh
Copy link
Member

svenvh commented Dec 16, 2024

Alternatively, if this (plus any edits as a result of review comments) could also be applied to the branches, we could just delete our workaround entirely.

Backporting the fix should be fine; which of the older branches are you interested in?

@hvdijk
Copy link
Contributor Author

hvdijk commented Dec 16, 2024

Backporting the fix should be fine; which of the older branches are you interested in?

We use the main, llvm_release_190, and llvm_release_180 branches, thank you.

MrSidims
MrSidims approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrSidims MrSidims MrSidims approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NO_INSTALL_RPATH breaks in-tree builds with shared libs
3 participants
@hvdijk @svenvh @MrSidims