Add optional binary relocatability #507
Conversation
Signed-off-by: Silvio Traversaro <[email protected]>
There was a problem hiding this comment.
I left a small comment about the change in the dependencies. We also need a small patch for making codecheck happy:
diff --git a/src/InstallationDirectories.cc b/src/InstallationDirectories.cc
index 5f51c076..d9f2711a 100644
--- a/src/InstallationDirectories.cc
+++ b/src/InstallationDirectories.cc
@@ -28,7 +28,8 @@ inline namespace GZ_PHYSICS_VERSION_NAMESPACE {
std::string getEngineInstallDir()
{
- return gz::common::joinPaths(getInstallPrefix(), GZ_PHYSICS_ENGINE_RELATIVE_INSTALL_DIR);
+ return gz::common::joinPaths(getInstallPrefix(),
+ GZ_PHYSICS_ENGINE_RELATIVE_INSTALL_DIR);
}
}
Signed-off-by: Silvio Traversaro <[email protected]>
Codecov Report
@@ Coverage Diff @@
## gz-physics6 #507 +/- ##
===============================================
- Coverage 76.19% 75.78% -0.41%
===============================================
Files 142 143 +1
Lines 7254 7293 +39
===============================================
Hits 5527 5527
- Misses 1727 1766 +39
|
|
@osrf-jenkins retest this please |
| std::string path1 = sanitizeSlashes(checkWindowsPath(_path1)); | ||
| std::string path2 = sanitizeSlashes(checkWindowsPath(_path2), true); | ||
| std::vector<char> combined(path1.length() + path2.length() + 2); | ||
| if (::PathCombineA(combined.data(), path1.c_str(), path2.c_str()) != NULL) |
There was a problem hiding this comment.
in https://build.osrfoundation.org/job/ign_physics-pr-win/2336/console
src\InstallationDirectories.cc(130,9): error C2039: 'PathCombineA': is not a member of '`global namespace''
There was a problem hiding this comment.
maybe we need to include Shlwapi.h?
There was a problem hiding this comment.
I was just getting back to these, I believe that is the case.
There was a problem hiding this comment.
Indeed, this is what I did in conda-forge: conda-forge/gz-physics-feedstock@d548779 . Probably I forgot to report it here.
There was a problem hiding this comment.
Fixed in 2e1a99f .
There was a problem hiding this comment.
Something may need to be linked as well?
InstallationDirectories.obj : error LNK2001: unresolved external symbol __imp_PathCombineA [C:\J\workspace\ign_physics-pr-win\ws\build\gz-physics6\src\gz-physics6.vcxproj]
There was a problem hiding this comment.
Obviously yes, see last commit of https://github.com/conda-forge/gz-physics-feedstock/pull/11/commits , my bad. -_-
There was a problem hiding this comment.
Hopefully fixed by 62f2a32 .
Signed-off-by: Silvio Traversaro <[email protected]>
Signed-off-by: Silvio Traversaro <[email protected]>
|
Alrighty @traversaro maybe just DCO now? |
|
Oh we've had a bunch of merges, I think we are good without any addition DCO work |
|
The only non-merge commit without DCO seems 39b1d44 . |
🦟 Bug fix
Closes part of gazebosim/gz-sim#626
Summary
This PR uses the changes introduced in gz-cmake3 in gazebosim/gz-cmake#334 to support the cmake installation directory to be moved after the
make installprefix, and continue to work without the need to set any special environment variable, as long as the library is compiled as shared. To avoid regressions and problems in Ubuntu Focal due to the use of std::filesystem, this new behaviour is only activated if theGZ_ENABLE_RELOCATABLE_INSTALLoption is enabled, and its default value isOFF.In particular, this PR defines a
gz::physics::getEngineInstallDir()function that should be used in place of theGZ_PHYSICS_ENGINE_INSTALL_DIRmacro to ensure that the library is relocatable.Furthermore, this PR also deprecates the
GZ_PHYSICS_ENGINE_INSTALL_DIRmacros, using the strategy described in https://stackoverflow.com/a/29297970 . That strategy works fine on GCC and Clang, while on MSVC it raise a warning:However, I think that it does anyhow the job of raising some kind of warning, and then at soon as the developer checks the macro definition the kind of warning is clear.
Test it
The test should work as usual. The used CMake machinery is tested in gazebosim/gz-cmake#334 .
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.