Add optional binary relocatability in downstream libraries

[traversaro]

🎉 New feature

Closes part of gazebosim/gz-sim#626

Summary

This PR add functionality in gz-cmake3 that permit to enable binary relocatability in downstream Gazebo libraries.

In particular it:

• Adds the gz_add_get_install_prefix_impl CMake function, that can be used in downstream libraries to genate at CMake generation time the implementation of a gz::<libraryname>::getInstallPrefix() method. This method permits to compute the installation prefix (i.e. the value of CMAKE_INSTALL_PREFIX is a relocatable way, as follows:

  • If the library is compiled as shared, the installation prefix is computed by extracting the location of the shared library on the filesystem using the dlfcn's dladdr function, and knowing the relative install location of the shared library.
  • If the library is compiled as static, the implementation falls back to hardcode the ${CMAKE_INSTALL_PREFIX} used at CMake configuration time in the library.
  • The logic to compute the install prefix in a relocatable is only enabled by setting the GZ_ENABLE_RELOCATABLE_INSTALL CMake option to ON in the project that calls gz_add_get_install_prefix_impl (the default value is OFF, as the relocatability logic requires std::filesystem that can create problems due to the mixture of gcc-8 and gcc-9 used in Ubuntu Focal packaging of gz libraries, see Add optional binary relocatability in downstream libraries #334 (comment))
  • For both the shared and static case, the value returned by the gz::<libraryname>::getInstallPrefix() function can be overriden by defining the GZ_<uppercaselibraryname>_INSTALL_PREFIX . This feature has two main use cases:
    • In the cases of static libraries, it can permit to handle the cases in which the files that have been installed with the static library and need to be find via gz::<libraryname>::getInstallPrefix() have been moved to a directory different from the CMAKE_INSTALL_PREFIX used at CMake configuration time
    • In the case the shared libraries are under tests, we want the gz::<libraryname>::getInstallPrefix() to point to the correct installation prefix, not to so location in the build directory in which the expected files are not there. We can achieve this by setting ENVIRONMENT GZ_<uppercaselibraryname>_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX} for the test suite of a given ignition/gazebo library.

• Adds the ENVIRONMENT option in the gz_build_tests function, to support the use case described in the previous bullet point.

Test it

The PR contains test for the gz_add_get_install_prefix_impl CMake function that it adds, as long as GZ_ENABLE_RELOCATABLE_INSTALL is enable in gz-cmake. Furthermore, the first downstream PR that uses it in an actual Gazebo library is gazebosim/gz-rendering#804 .

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

[traversaro]

I see several CI errors on Bionic and Focal, and I did not consider appropriately the effect of this targeting Fortress, while using std::filesystem, see gazebo-tooling/release-tools#639 . I need to think a bit about it, for the time being I will put this PR in draft.

[mjcarroll]

while using std::filesystem

Can we avoid std::filesystem issues by using gz-common? It uses std::filesystem under the hood, but should guarantee that linkage is consistent across all gz libraries.

[mjcarroll]

Thanks for tackling this!

Thinking more about the filesystem issue. This package doesn't have a dependency on gz-common (rather the inverse), so that isn't really an option here.

One option would be to push this to gz-cmake3 and drop the bionic support.

Alternatively, we can push all of this functionality into gz-common, which is where I do similar work with test data prefixes.

[traversaro]

Sorry for the late reply @mjcarroll !

One option would be to push this to gz-cmake3 and drop the bionic support.

From what I understand from gazebo-tooling/release-tools#639, we have a similar mixture of some libraries compiled with gcc-8, some with gcc-9 and std::filesystem being ABI stable only since gcc-9 (so there is an ABI breakage between gcc-8 and gcc-9) also on Focal, or I am missing something?

Migrating this functionality to gz-common and to use gz-common's filesystem is indeed an option, at a first glance it seems that all libraries that currently hardcode their CMAKE_INSTALL_PREFIX already depend on common. However, at the moment gz-common does not currently install CMake functions or any CMake module, so probably we would need to modify something at the CMAke level (see #331).

Another possible option is to:

• set the default value of the IGN_DISABLE_RELOCATABLE to OFF (or rename the option to IGN_ENABLE_RELOCATABLE for clarity)
• leave this logic in ign-cmake/gz-cmake and leave the use of std::filesystem if IGN_DISABLE_RELOCATABLE is ON
• change code in downstream libraries that is used indipendently of the value of IGN_DISABLE_RELOCATABLE from std::filesystem to common::filesystem (see for example https://github.com/gazebosim/gz-rendering/blob/2d00d80bf3fe326a84d1dde881eb794983b60f2e/src/InstallationDirectories.cc#LL31C3-L31C12)

In this way, all the official builds (apt and homebrew) of the ignition/gazebo libraries will continue to hardcode CMAKE_INSTALL_PREFIX in their binaries and they will continue not to use std::filesystem, so that we avoid any gcc8/gcc9/std::filesystem ABI stability-related problem there.
Furthermore, for those builds it is not problematic not to have relocatability.

Instead, packagers that mantain build of gazebo for which relocatability is important (such as conda-forge, conan-center-index, vcpkg, snap, flatpack, etc, etc) can enable the option IGN_ENABLE_RELOCATABLE as need. Anyhow, this system tipically either use modern compilers in which std::filesystem is not part of the C++ standard library (conda-forge) or enforce the use of the same major version of the compiler for all the libraries built (all the others mentioned package managers), so in those case the use of std::filesystem is not problematic at all.

After that, we can switch the default value of GZ_DISABLE_RELOCATABLE to ON once we have a new version of gz-cmake that does not need to support Ubuntu Focal (gz-cmake4/gazebo H?). This would also minimize the possible regressions related to this modifications, as I would test the option for a long time in less used package managers before enabling it for the official builds.

@mjcarroll feel free to let me know which option do you prefer, if moving everything to gz-common or the option I proposed, thanks!

[traversaro]

@mjcarroll feel free to let me know which option do you prefer, if moving everything to gz-common or the option I proposed, thanks!

Gentle ping on this, I would be happy to implement what do you prefer. For a quick TL;DR, the options are:

• Either (option 1) move this logic in ign-common, even if ign-common at the moment does not install CMake modules
• Or (option 2) keep this here, but keep it disabled by default, so apt and homebrew continue to work fine (no std::filesystem ABI compatibility stuff), and people packaging ign/gz for weird package managers can enable it and be happy

I prefer option 2 to minimize the risk of breaking something for apt and homebrew users.

[mjcarroll]

I'm inclined towards option 2, but let me reread the proposal and bring it up with the team before we make a decision. I should have an answer for you by Tuesday of next week.

[traversaro]

I'm inclined towards option 2, but let me reread the proposal and bring it up with the team before we make a decision. I should have an answer for you by Tuesday of next week.

Great, thanks!

[traversaro]

I'm inclined towards option 2, but let me reread the proposal and bring it up with the team before we make a decision. I should have an answer for you by Tuesday of next week.

Did you have the occasion to check this? Sorry for being the "any update?" guy here : ), but unfortunatly given the nature of the change it is tricky to just integrate it in form of patch in conda-forge and then propose it upstream once we have it already working on conda-forge. If it make it simpler we can also have an alignment call on this on Teams/Zoom/Skype Google Meet. : )

[azeey]

Discussion:

• Targetting gz-cmake3 will probably make this a lot easier.
• Consider putting deprecation warnings in config.hh.
• It would be nice to have a consistent way to define macros and have them be overriden by env variables.

[traversaro]

Targetting gz-cmake3 will probably make this a lot easier.

Done, sorry for the long way.

Consider putting deprecation warnings in config.hh.

Done in gazebosim/gz-rendering#804, in general it can be handled in similar way for other downstream libraries.

It would be nice to have a consistent way to define macros and have them be overriden by env variables.

For what regards the installation prefix, this PR adds a way to override it with env variables, while for other existing variables this is not strictly related to this PR.

The PR is ready for review, and the original description was updated to reflect the latest changes.

[traversaro]

There are some warnings on Windows, I will fix them this (european) evening.

[j-rivero]

The whole approach and the PR looks great to me, thanks Silvio for pushing the relocatable effort. I added some minor comments to the PR, can not find any particular error.

I would say that would be great to have a Changelog.md entry for this PR so people knows that the new functions are available. The CI scripts would need to define GZ_DISABLE_RELOCATABLE to ON in order to enable the test of this new feature.

There are some warnings on Windows, I will fix them this (european) evening.

I see the code usingstd::getenv and not getenv so my first impression is that the standard library implemented should be doing the right thing. Is there any particular reason to consider the warning as valid?

[j-rivero]

Could we add please the documentation for the GET_INSTALL_PREFIX_FUNCTION, GET_INSTALL_PREFIX_HEADER and OVERRIDE_INSTALL_PREFIX_ENV_VARIABLE using the same format that the rest of the GzUtils file?

snippet from gz_build_executables

#################################################
# gz_build_executables(SOURCES <sources>
#                       [PREFIX <prefix>]
#                       [LIB_DEPS <library_dependencies>]
#                       [INCLUDE_DIRS <include_dependencies>]
#                       [EXEC_LIST <output_var>]
#                       [EXCLUDE_PROJECT_LIB])
#
# Build executables for an Gazebo project. Arguments are as follows:
#
# SOURCES: Required. The names (without a path) of the source files for your
#          executables.
#
# PREFIX: Optional. This will append <prefix> onto each executable name.
#

[traversaro]

Done in d7708fc . After writing the "Example" part, I wonder if it could make sense to make the arguments optional, and use as default values the one that I put as example. What do you think?

[traversaro]

I would say that would be great to have a Changelog.md entry for this PR so people knows that the new functions are available. The CI scripts would need to define GZ_DISABLE_RELOCATABLE to ON in order to enable the test of this new feature.

Done in d7708fc .

[traversaro]

There are some warnings on Windows, I will fix them this (european) evening.

I see the code usingstd::getenv and not getenv so my first impression is that the standard library implemented should be doing the right thing. Is there any particular reason to consider the warning as valid?

I agree, for that reason I just added a suppression for the warning, just for that specific generated file.

[j-rivero]

Nothing more to add. Thanks again Silvio, awesome work.

[traversaro]

I did not realized that, but this added a dependency on dlfcn-win32 if BUILD_TESTING is enabled.