-
Notifications
You must be signed in to change notification settings - Fork 0
Fix RPATH on linux to point in bin dir #79
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
Conversation
409c066 to
327ac09
Compare
Since USD libraries are always in the same directory as the test executable, works for all cases without needing multi-config logic
327ac09 to
5ff1792
Compare
test/CMakeLists.txt
Outdated
| # (where the single-configuration generators do not) invalidating/changing some predefined paths. | ||
| get_cmake_property(_MULTI_CONFIG GENERATOR_IS_MULTI_CONFIG) | ||
|
|
||
| if (UNIX) |
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.
I would just remove this empty block entirely.
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.
Yeah tried but it fails on platform check bellow, will come up with cleaner code and stop failing in the else...
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.
Just remove the check, it's not needed.
25220fe to
851137a
Compare
Not mandatory for tests to pass, but suppresses USD plugin loading warnings. USD plugins lack embedded RPATH and need help finding their dependencies.
| else() | ||
| # Linux | ||
| set(_rpath "$ORIGIN:$ORIGIN/../lib") | ||
| endif() |
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.
🔴 No, The OpenUSD library location is also based on OPENUSD_INSTALL_PATH (which is the contetn of OPENUSD_INSTALL_PATH as-is when using a specific OpneUSD install or vcpkg installed directory).
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.
Note: Here is the file the code doesn't know yet where are the OpenUSD libraries i.e., only known after the line 200
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.
This RPATH is relative to the loader (an executable in bin/ or library in lib/), there's no reason to have it be absolute.
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.
This RPATH is relative to the loader (an executable in
bin/or library inlib/), there's no reason to have it be absolute.
Agree e.g., HVT library. But the OpenUSD libraries and plugins are not there!
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.
CMake is supposed to set RPATHs automatically according to the linker command line. We shouldn't need an RPATH for the build.
See: https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH.html#variable:CMAKE_SKIP_RPATH
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.
All the USD lib and binaries are in lib and bin folders, we do not use the externals folder for that. It is the same with HVTEx. This also make sense, you can copy these folders and you have everything you need.
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.
All the USD lib and binaries are in lib and bin folders, we do not use the externals folder for that. It is the same with HVTEx. This also make sense, you can copy these folders and you have everything you need.
Nope.
With the HVT project used as standalone, the OpenUSD libraries are not in lib or bin folders.
With the HVT project used as sub-module of HVTEx, the OpenUSD libraries are all in lib or bin folders (because an extra copy is done in HVTEx).
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.
CMake is supposed to set RPATHs automatically according to the linker command line. We shouldn't need an RPATH for the build. See: https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH.html#variable:CMAKE_SKIP_RPATH
Based on my experiments on macOS, the two must be explicit.
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.
I've never had to explicitly set the RPATH, as long as you leave the default CMake RPATH settings on.
| string(COMPARE EQUAL "${CMAKE_CURRENT_SOURCE_DIR}" "${CMAKE_SOURCE_DIR}" _HVT_PROJECT_IS_TOP_LEVEL) | ||
|
|
||
| # Variables that need to be set prior to the project definition. | ||
| # RPATH for Linux and macOS |
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 RPATH computation is needed all the time (i.e., cannot depend on _HVT_PROJECT_IS_TOP_LEVEL).
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.
When building as part gf HVTEx, the RPATH defined there is supposed to be set for other submodules being built.
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.
When building as part gf HVTEx, the RPATH defined there is supposed to be set for other submodules being built.
The OPENUSD_INSTALL_PATH (defined below in the cmake file) takes care of that.
|
Here is my pull request to fix the problem #86 |
Since USD libraries are always in the same directory as the test executable, works for all cases without needing multi-config logic