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

Fix hardcoded pkg-config library in examples to fix tests on non-Debian Linux systems #163

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Mar 31, 2021

🦟 Bug fix

Fixes test failures on platforms (such as non-Debian Linux) in which CMAKE_INSTALL_LIBDIR is not equal to lib.

Summary

ignition-cmake by default installs the pkg-config files in ${CMAKE_INSTALL_LIBDIR}\pkgconfig (see https://github.com/ignitionrobotics/ign-cmake/blob/b03ca34191e538d334f8d9bb285711a4b487e2a4/cmake/IgnPackaging.cmake#L201). Later, in the tests before this PR it is assumed that they are installed by default in lib\pkgconfig. However, there are several platforms (such as a any non-Debian Linux system) in which ${CMAKE_INSTALL_LIBDIR} is not lib, so in those platforms the test would fail.

This PR fixes this by using ${CMAKE_INSTALL_LIBDIR} also in the tests.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Mar 31, 2021
@traversaro
Copy link
Contributor Author

Unfortunately the value of CMAKE_INSTALL_LIBDIR is not lib even on Debian if the install prefix is /usr, so a slightly more complex fix is necessary.

Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
Comment on lines +178 to +181
# On Debian and if the install prefix is /usr, the default CMAKE_INSTALL_LIBDIR of this project
# as set by GNUInstallDirs will be different from the one of the example project,
# so let's hardcode it in that case
if(EXISTS "/etc/debian_version" AND "${CMAKE_INSTALL_PREFIX}" MATCHES "^/usr/?$")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to check that the prefix is /usr, should we remove the ??

According to the CMake Regex Specification, ? is defined as Matches preceding pattern zero or once only. So, couldn't this mean that there would be zero matches, resulting in the if being true when it should really be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that the ? is meant to match both ^/usr$ and ^/usr/$. Just as a reference, this logic was taken from the relevant code in CMake itself: https://gitlab.kitware.com/cmake/cmake/-/blob/v3.20.0/Modules/GNUInstallDirs.cmake#L242 .

@adlarkin
Copy link
Contributor

adlarkin commented Apr 7, 2021

Changes look okay to me. @j-rivero, mind taking a quick look?

Copy link
Contributor

@j-rivero j-rivero 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. Thanks @traversaro

@j-rivero j-rivero merged commit c34873d into gazebosim:ign-cmake2 Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants