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

Use computed relative extras path #359

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Jul 6, 2023

🦟 Bug fix

Needed by gazebosim/gz-transport#398, follow-up to gazebosim/gz-cmake#360 and #339 (comment)

Summary

Use PROJECT_CMAKE_EXTRAS_INSTALL_DIR and PROJECT_CMAKE_EXTRAS_PATH_TO_PREFIX to improve support for arch-dependent lib folders. In particular, the fixed relative path ../../.. is incorrect when installing to /usr on debian systems, so the computed relative path PROJECT_CMAKE_EXTRAS_PATH_TO_PREFIX is used instead. Also install the other cmake extras to PROJECT_CMAKE_EXTRAS_INSTALL_DIR.

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.

Use PROJECT_CMAKE_EXTRAS_INSTALL_DIR and
PROJECT_CMAKE_EXTRAS_PATH_TO_PREFIX to improve
support for arch-dependent lib folders.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from caguero as a code owner July 6, 2023 05:39
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jul 6, 2023
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #359 (c6ebe78) into main (16de9ef) will not change coverage.
The diff coverage is n/a.

❗ Current head c6ebe78 differs from pull request most recent head ab34acf. Consider uploading reports for the commit ab34acf to get more accurate results

@@           Coverage Diff           @@
##             main     #359   +/-   ##
=======================================
  Coverage   96.89%   96.89%           
=======================================
  Files          28       28           
  Lines        1064     1064           
=======================================
  Hits         1031     1031           
  Misses         33       33           

@scpeters
Copy link
Member Author

scpeters commented Jul 6, 2023

the homebrew build is passing because of the changes in the following branch:

we'll need to make a gz-cmake3 release in order to keep homebrew working

@scpeters
Copy link
Member Author

scpeters commented Jul 6, 2023

CI is happy now that gazebosim/gz-cmake#362 is merged and new nightlies have been built

@j-rivero
Copy link
Contributor

j-rivero commented Jul 7, 2023

We are using in this PR new gz-cmake variables introduced in gazebosim/gz-cmake#362 if I'm not wrong. If that is the case, we are going to need a version requirement in the find_package(gz-cmake) to be sure that the system has a compatible gz-cmake package and does not resolve variables as empty. We probably need to bump gz-cmake to 3.3.0.

@j-rivero
Copy link
Contributor

j-rivero commented Jul 7, 2023

Use PROJECT_CMAKE_EXTRAS_INSTALL_DIR and PROJECT_CMAKE_EXTRAS_PATH_TO_PREFIX to improve support for arch-dependent lib folders. In particular

As a side note, if we are going to use these variables exported from GzConfigureProject we should document it well somehow.

@scpeters
Copy link
Member Author

scpeters commented Jul 7, 2023

Use PROJECT_CMAKE_EXTRAS_INSTALL_DIR and PROJECT_CMAKE_EXTRAS_PATH_TO_PREFIX to improve support for arch-dependent lib folders. In particular

As a side note, if we are going to use these variables exported from GzConfigureProject we should document it well somehow.

this is a great point. We are currently documenting only some of the input parameters to GzConfigureProject, but none of the output parameters. I'll see if I can whip up something quick for this file in preparation for a 3.3.0 release

@scpeters
Copy link
Member Author

scpeters commented Jul 8, 2023

Use PROJECT_CMAKE_EXTRAS_INSTALL_DIR and PROJECT_CMAKE_EXTRAS_PATH_TO_PREFIX to improve support for arch-dependent lib folders. In particular

As a side note, if we are going to use these variables exported from GzConfigureProject we should document it well somehow.

this is a great point. We are currently documenting only some of the input parameters to GzConfigureProject, but none of the output parameters. I'll see if I can whip up something quick for this file in preparation for a 3.3.0 release

@j-rivero I've started on the documentation in gazebosim/gz-cmake#364; please let me know what you think

@scpeters
Copy link
Member Author

@osrf-jenkins run tests please

@scpeters scpeters merged commit 786b42a into main Jul 11, 2023
9 checks passed
@scpeters scpeters deleted the ci_matching_branch/cmake_extras_install branch July 11, 2023 04:28
mjcarroll added a commit that referenced this pull request Jul 12, 2023
This reverts commit 786b42a.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
mjcarroll added a commit that referenced this pull request Jul 12, 2023
* Revert "Use cmake extras path variables (#359)"

This reverts commit 786b42a.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>

* Revert "Unconditionally find the python interpreter (#356)"

This reverts commit 16de9ef.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>

* Revert "Generate messages in downstream builds (#339)"

This reverts commit 0c78b96.

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>

---------

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants