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

Don't assume CMAKE_INSTALL_*DIR variables are relative #305

Merged
merged 4 commits into from
Aug 31, 2022
Merged

Don't assume CMAKE_INSTALL_*DIR variables are relative #305

merged 4 commits into from
Aug 31, 2022

Conversation

lopsided98
Copy link
Contributor

🦟 Bug fix

The CMAKE_INSTALL_*DIR variables are allowed to be absolute paths, so they cannot be simply appended to CMAKE_INSTALL_PREFIX. When an absolute path is needed, CMAKE_INSTALL_FULL_*DIR must be used. See here for more information: https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path

Additionally, pkgconfig files should use ${prefix} relative paths if CMAKE_INSTALL_*DIR are relative, but otherwise use absolute paths. This is handled by the custom join_paths() function.

Note that CMake 3.20 provides cmake_path(APPEND) which implements the same functionality as join_paths(), but this CMake version is not available in all supported distros yet (notably Ubuntu older than 22.04).

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

The CMAKE_INSTALL_*DIR variables are allowed to be absolute paths, so they
cannot be simply appended to CMAKE_INSTALL_PREFIX. When an absolute path is
needed, CMAKE_INSTALL_FULL_*DIR must be used.

Additionally, pkgconfig files should use ${prefix} relative paths if
CMAKE_INSTALL_*DIR are relative, but otherwise use absolute paths. This is
handled by the custom join_paths() function.

Note that CMake 3.20 provides cmake_path(APPEND) which implements the same
functionality as join_paths(), but this CMake version is not available in all
supported distros yet (notably Ubuntu older than 22.04).

Signed-off-by: Ben Wolsieffer <[email protected]>
@github-actions github-actions bot added 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels Aug 17, 2022
@lopsided98 lopsided98 changed the title Don't assume CMAKE_INSTALL_*DIR variables are relative Don't assume CMAKE_INSTALL_*DIR variables are relative Aug 17, 2022
@j-rivero
Copy link
Contributor

Thanks Ben for the pointers and the patch. Changes in code looks good to me, my testing a local wokrspace for packages up to gz-transport is fine and I run a testing build of the whole collection in Build Status for Windows (warnings and test are unrelated as far as I can tell).

Note that CMake 3.20 provides cmake_path(APPEND) which implements the same functionality as join_paths(), but this CMake version is not available in all supported distros yet (notably Ubuntu older than 22.04).

I'm going to add a TODO not to forget about this.

@j-rivero j-rivero enabled auto-merge (squash) August 31, 2022 00:06
@j-rivero j-rivero disabled auto-merge August 31, 2022 00:06
@j-rivero j-rivero merged commit 4283fc6 into gazebosim:ign-cmake2 Aug 31, 2022
@lopsided98 lopsided98 deleted the cmake-absolute-paths branch August 31, 2022 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants