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

Find Python3 directly, not with GzPython #472

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

scpeters
Copy link
Member

🦟 Bug fix

Motivated by gazebosim/gz-sim#2249.

Summary

While testing the use of -DPython3_EXECUTABLE=$(which python3) to aid in finding the right version of python3 on macOS (see osrf/homebrew-simulation#2543), I ran into some cmake configuration issues when testing locally (the bottle build seems to be progressing fine though). I was able to fix my local build by consolidating the logic for finding python3 by removing the use of gz-cmake's GzPython in favor of a single call to find_package(Python3). This fixes setting Python3_EXECUTABLE to specify which python version to use and is needed on macOS.

As an aside, I'm going to suggest that we deprecate GzPython on main since it's better and simpler to just find python directly with modern versions of cmake.

Check the diff without whitespace to see a simpler version of the changes without the indentation.

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.

This removes the use of gz-cmake's GzPython in favor
of a single call to find_package(Python3). This allows
setting Python3_EXECUTABLE to specify which python
version to use and is needed on macOS.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters requested a review from caguero as a code owner January 24, 2024 06:27
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Jan 24, 2024
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (764a04b) 87.80% compared to head (505537f) 87.94%.
Report is 1 commits behind head on gz-transport13.

Additional details and impacted files
@@                Coverage Diff                 @@
##           gz-transport13     #472      +/-   ##
==================================================
+ Coverage           87.80%   87.94%   +0.14%     
==================================================
  Files                  59       59              
  Lines                5699     5974     +275     
==================================================
+ Hits                 5004     5254     +250     
- Misses                695      720      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scpeters
Copy link
Member Author

windows failed to find python development, as its search for gz-msgs found a version of python that may have derailed the later search. I'm going to try moving the python search logic before the search for gz-msgs

@@ -22,7 +22,6 @@ option(SKIP_PYBIND11

# Python interfaces vars
include(CMakeDependentOption)
include(GzPython)
Copy link
Member Author

Choose a reason for hiding this comment

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

ah, removing this is probably why windows failed

Copy link
Member Author

Choose a reason for hiding this comment

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

it's fixed as of 505537f

@scpeters
Copy link
Member Author

scpeters commented Feb 4, 2024

I believe this find logic is fixed as of 505537f

I used ci_matching_branch/ to run some CI jobs to reproduce the failure with gz-transport13 built from source and then to confirm the fix with this branch

@scpeters scpeters merged commit 502f519 into gz-transport13 Feb 6, 2024
11 checks passed
@scpeters scpeters deleted the scpeters/fix_find_python branch February 6, 2024 16:29
scpeters added a commit that referenced this pull request Feb 9, 2024
This removes the use of gz-cmake's GzPython in favor
of a single call to find_package(Python3). This allows
setting Python3_EXECUTABLE to specify which python
version to use and is needed on macOS.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this pull request Feb 9, 2024
This removes the use of gz-cmake's GzPython in favor
of a single call to find_package(Python3). This allows
setting Python3_EXECUTABLE to specify which python
version to use and is needed on macOS.

Signed-off-by: Steve Peters <[email protected]>
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.

2 participants