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

Always setup rpath #27426

Closed
wants to merge 4 commits into from
Closed

Conversation

Neumann-A
Copy link
Contributor

Reasoning:
a) vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) -> builds .so
b) static libs links the above
c) a code generator links the above (e.g. qsb in #27279 complaining about not finding libGLX.so.0 while building qtdeclarative)

as such vcpkg needs to always fix the rpath because otherwise required build tools might be looking for shared libraries.

cc @Osyotr

github-actions[bot]
github-actions bot previously approved these changes Oct 24, 2022
@Osyotr
Copy link
Contributor

Osyotr commented Oct 24, 2022

vcpkg needs to always fix the rpath

This was discussed in the original PR. #23035 (comment)
I would like to make it default on x64-linux, too.
@vicroms

@LilyWangLL LilyWangLL added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Oct 24, 2022
github-actions[bot]
github-actions bot previously approved these changes Oct 24, 2022
@Neumann-A
Copy link
Contributor Author

hmm @autoantwort does vcpkg need a similar elf patch on osx? Seems like the current approach of @Osyotr doesn't work there?

@autoantwort
Copy link
Contributor

I have no idea. I don't use arm64-osx-dynamic.

@Neumann-A
Copy link
Contributor Author

I have no idea. I don't use arm64-osx-dynamic.

This is not about osx-dynamic but more about vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) and if we have to do the same as linux does with fixing rpaths on osx.

https://stackoverflow.com/questions/12521802/print-rpath-of-an-executable-on-macos
bazelbuild/bazel#4589
https://wincent.com/wiki/@executable_path,_@load_path_and_@rpath

scripts/ports.cmake Outdated Show resolved Hide resolved
osx needs another way to fix loader paths
Comment on lines +36 to +39
# TODO: Read out current rpath
# - nuke out any vcpkg related paths
# - remove just $ORIGIN paths (because we add them any way)
# - prepend our paths
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Osyotr What is your opinion on this TODO?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect something other than $ORIGIN and /path/to/vcpkg_installed to be in the path?
If it's an absolute path, I don't think we should keep it. On the other hand, $ORIGIN stuff can theoretically point to something other than lib/, so we could try to keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you expect something other than $ORIGIN and /path/to/vcpkg_installed to be in the path?

Yes if the user set it up in the toolchain itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we will be able to tell whether a path was set in toolchain or in library itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in #36056 I came to the conclusion that it is enough to support rpaths which point to the installed tree itself.

@Adela0814
Copy link
Contributor

Pinging @Neumann-A for response. Is work still being done for this PR?

@Neumann-A
Copy link
Contributor Author

@Adela0814. Ideally the TODO needs to be taken care of.

@Neumann-A
Copy link
Contributor Author

superseded by #36056

@Neumann-A Neumann-A closed this Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants