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

Add optional binary relocatability #804

Merged
merged 4 commits into from
May 17, 2023

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Jan 16, 2023

🎉 New feature

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 install prefix, 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 the GZ_ENABLE_RELOCATABLE_INSTALL option is enabled, and its default value is OFF .

In particular, this PR defines a gz::rendering::getResourcePath() and gz::rendering::getEngineInstallDir() that should be used in place of the GZ_RENDERING_RESOURCE_PATH and GZ_RENDERING_ENGINE_INSTALL_DIR macros to ensure that the library is relocatable.

Furthermore, this PR also deprecates the GZ_RENDERING_RESOURCE_PATH and GZ_RENDERING_ENGINE_INSTALL_DIR macros, 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:

warning C4068: unknown pragma

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

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@traversaro
Copy link
Contributor Author

The PR is failing as it requires the merge and release of gazebosim/gz-cmake#334 . However, it was important to already open it to permit to understand how gazebosim/gz-cmake#334 would be used.

@traversaro traversaro marked this pull request as draft January 16, 2023 22:54
@traversaro traversaro changed the base branch from ign-rendering6 to gz-rendering7 April 16, 2023 13:00
@traversaro traversaro force-pushed the relocbin branch 4 times, most recently from 9d6c2af to 9859efe Compare April 16, 2023 22:39
@traversaro traversaro marked this pull request as ready for review April 16, 2023 22:55
@traversaro
Copy link
Contributor Author

traversaro commented Apr 21, 2023

I updated this PR to target gz-rendering7 (i.e. Gazebo Garden). The related change in gz-cmake3 has been merged in gazebosim/gz-cmake#334, I guess we need a release of gz-cmake3 before going forward here.

@traversaro traversaro changed the title Enable binary relocatability Add optional binary relocatability Apr 21, 2023
@traversaro traversaro closed this Apr 24, 2023
@traversaro traversaro reopened this Apr 24, 2023
Signed-off-by: Silvio Traversaro <[email protected]>

std::string getEngineInstallDir()
{
return gz::common::joinPaths(getInstallPrefix(), GZ_RENDERING_ENGINE_RELATIVE_INSTALL_DIR);
Copy link
Contributor

Choose a reason for hiding this comment

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

CI picked up a couple of minor issues here:

  /github/workspace/src/InstallationDirectories.cc:31:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/InstallationDirectories.cc:36:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I did not notice that as I tought the CI errors were all due to the gz-cmake 3.1.0 missing on Focal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c412915 .

Signed-off-by: Silvio Traversaro <[email protected]>
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #804 (55d5061) into gz-rendering7 (91f5fe9) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 55d5061 differs from pull request most recent head 51c4d52. Consider uploading reports for the commit 51c4d52 to get more accurate results

@@                Coverage Diff                @@
##           gz-rendering7     #804      +/-   ##
=================================================
- Coverage          77.14%   77.14%   -0.01%     
=================================================
  Files                169      170       +1     
  Lines              14712    14716       +4     
=================================================
+ Hits               11349    11352       +3     
- Misses              3363     3364       +1     
Impacted Files Coverage Δ
ogre2/src/Ogre2RenderEngine.cc 76.03% <100.00%> (ø)
src/InstallationDirectories.cc 100.00% <100.00%> (ø)
src/RenderEngineManager.cc 77.25% <100.00%> (ø)
src/base/BaseScene.cc 89.10% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me.

@mjcarroll do you need to test this with bazel?

@mjcarroll
Copy link
Contributor

I already tested the physics one, since the remainder follow suit, i don't have concerns here.

@iche033 iche033 merged commit a14b582 into gazebosim:gz-rendering7 May 17, 2023
@traversaro traversaro deleted the relocbin branch May 17, 2023 21:01
@PetervdPerk-NXP
Copy link

PetervdPerk-NXP commented May 30, 2023

This PR triggers following CMake error for me.
Which package should provide the gz_add_get_install_prefix_impl cmake command?

Somehow gz-cmake didn't update for me running latest solves this issue.

CMake Error at src/CMakeLists.txt:18 (gz_add_get_install_prefix_impl):
  Unknown CMake command "gz_add_get_install_prefix_impl".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants