Replace CMake Python variables with new ones from FindPython3 module #402

Bi0T1N · 2022-04-02T17:26:47Z

🦟 Bug fix

Fixes #401

Summary

Replaces the old variables with the new ones used by the FindPython3 module. Right now IgnPython cannot be used as it only searches for the interpreter but pybind11 also needs the development components, thus it provides a workaround. The Win32 PYTHON_LIBRARIES (PYTHON_DEBUG_LIBRARIES) variable was also removed as there is no equivalent in the new CMake module for getting the debug libraries.

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

codecov · 2022-04-03T09:09:10Z

Codecov Report

Merging #402 (d9a2a5c) into ign-math6 (4fbd3c8) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           ign-math6     #402   +/-   ##
==========================================
  Coverage      99.65%   99.65%           
==========================================
  Files             67       67           
  Lines           6392     6392           
==========================================
  Hits            6370     6370           
  Misses            22       22

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fbd3c8...d9a2a5c. Read the comment docs.

j-rivero

Mostly fine, a couple of small things

CMakeLists.txt

src/python_pybind11/CMakeLists.txt

chapulina · 2022-06-02T18:41:31Z

Hi @Bi0T1N , thank you for the contribution. Are you still interested in moving on with this PR?

Bi0T1N · 2022-06-11T20:39:50Z

Hi @Bi0T1N , thank you for the contribution. Are you still interested in moving on with this PR?

Sure, but I don't have time for that now. I'll try to come back to this over the next few weeks as I should have more time soon.

codecov · 2022-06-14T17:44:46Z

Codecov Report

Merging #402 (22d802a) into ign-math6 (6276839) will not change coverage.
The diff coverage is n/a.

❗ Current head 22d802a differs from pull request most recent head 43b40dd. Consider uploading reports for the commit 43b40dd to get more accurate results

@@            Coverage Diff             @@
##           ign-math6     #402   +/-   ##
==========================================
  Coverage      99.38%   99.38%           
==========================================
  Files             75       75           
  Lines           7029     7029           
==========================================
  Hits            6986     6986           
  Misses            43       43

there is no equivalent variable in the new FindPython3 module that provides the path to debug libraries Signed-off-by: Bi0T1N <[email protected]>

Signed-off-by: Bi0T1N <[email protected]>

- cannot use IgnPython because it doesn't allow to specify the components to be found - old code probably don't work on old CMake versions < 3.12 as the required FindPython3 module doesn't exist Signed-off-by: Bi0T1N <[email protected]>

This reverts commit 76ffaee. Signed-off-by: Bi0T1N <[email protected]>

Signed-off-by: Bi0T1N <[email protected]>

Bi0T1N · 2022-12-17T13:53:10Z

What is the status on this from your side? Should I update it to match the new gz naming and we merge it or abandon it?

scpeters · 2023-05-09T21:19:16Z

I just resolved the conflicts and will continue review. Thanks for your patience and your help with pull request reviews!

CMakeLists.txt

Signed-off-by: Bi0T1N <[email protected]>

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey · 2023-08-21T23:00:01Z

CMakeLists.txt

+  # the code below can be removed; e.g. pybind needs Interpreter and Development components
+  # see https://pybind11.readthedocs.io/en/stable/cmake/index.html#new-findpython-mode
+  if(${CMAKE_VERSION} VERSION_LESS "3.12.0")
+    set(IGN_PYTHON_VERSION "3")


I added IGN_PYTHON_VERSION "3" here to make it work. It turns out Bionic doesn't have pybind11 2.2 anyway, so this would only be useful for users that build pybind11 from source on Bionic.

azeey · 2023-08-22T01:34:58Z

@j-rivero I believe your requested changes have been resolved. Can you take a look?

azeey · 2023-08-29T23:07:17Z

Removing beta to avoid needing a release and forward ports. This will be merged right after Harmonic is released.

j-rivero

@j-rivero I believe your requested changes have been resolved. Can you take a look?

Sorry for the late response, should be good to go.

Bi0T1N requested a review from scpeters as a code owner April 2, 2022 17:26

github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels Apr 2, 2022

Bi0T1N mentioned this pull request Apr 2, 2022

Add CMake macro for generating pybind11 bindings gazebosim/gz-cmake#215

Open

scpeters mentioned this pull request Apr 3, 2022

Regression of python tests in Jammy CI - "Could not find executable" #401

Closed

chapulina added the scripting Scripting interfaces to Ignition label Apr 4, 2022

chapulina requested a review from j-rivero April 11, 2022 18:45

j-rivero mentioned this pull request Apr 22, 2022

Update CMake Python code to only find Python3 gazebosim/gz-cmake#212

Closed

j-rivero requested changes Apr 27, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

src/python_pybind11/CMakeLists.txt Show resolved Hide resolved

dlech mentioned this pull request May 1, 2022

Pybind11 CMake error when installing ignition-math6 with homebrew osrf/homebrew-simulation#1872

Open

Bi0T1N mentioned this pull request Jul 1, 2022

Support for specifying components of FindPython3 in IgnPython gazebosim/gz-cmake#269

Closed

Bi0T1N requested a review from adityapande-1995 as a code owner July 1, 2022 20:07

Bi0T1N requested a review from j-rivero July 1, 2022 20:08

Bi0T1N added 5 commits July 1, 2022 22:17

Remove PYTHON_LIBRARIES variable for WIN32 debug

f641ce3

there is no equivalent variable in the new FindPython3 module that provides the path to debug libraries Signed-off-by: Bi0T1N <[email protected]>

Migrate to variables from FindPython3

92413f6

Signed-off-by: Bi0T1N <[email protected]>

Improve detection of Python3 for pybind11

8ac2ce9

- cannot use IgnPython because it doesn't allow to specify the components to be found - old code probably don't work on old CMake versions < 3.12 as the required FindPython3 module doesn't exist Signed-off-by: Bi0T1N <[email protected]>

Revert "Remove PYTHON_LIBRARIES variable for WIN32 debug"

60103a7

This reverts commit 76ffaee. Signed-off-by: Bi0T1N <[email protected]>

Restore PYTHON_LIBRARIES for older CMake versions

7de5c7d

Signed-off-by: Bi0T1N <[email protected]>

chapulina added the bug Something isn't working label Jul 23, 2022

Merge branch 'ign-math6' into cmake_python3_fixes

6b4ee27

Merge branch 'ign-math6' into cmake_python3_fixes

2b82268

azeey self-requested a review June 14, 2023 21:28

azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023

azeey reviewed Aug 18, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Bi0T1N and others added 5 commits August 19, 2023 21:38

Search for Python3 interpreter but keep libraries mandatory

b1f2cbb

Signed-off-by: Bi0T1N <[email protected]>

Add IGN_PYTHON_VERSION

7b988f0

Signed-off-by: Addisu Z. Taddese <[email protected]>

Merge branch 'ign-math6' into cmake_python3_fixes

0a3d1e1

Revert pybind11 version change

79e2483

Signed-off-by: Addisu Z. Taddese <[email protected]>

Revert testing changes

6e12c91

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey reviewed Aug 21, 2023

View reviewed changes

azeey approved these changes Aug 21, 2023

View reviewed changes

Merge branch 'ign-math6' into cmake_python3_fixes

3a5a4b0

azeey removed the beta Targeting beta release of upcoming collection label Aug 29, 2023

j-rivero approved these changes Aug 30, 2023

View reviewed changes

Merge branch 'ign-math6' into cmake_python3_fixes

43b40dd

azeey merged commit ea9ae34 into gazebosim:ign-math6 Oct 6, 2023
8 checks passed

Bi0T1N deleted the cmake_python3_fixes branch October 7, 2023 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace CMake Python variables with new ones from FindPython3 module #402

Replace CMake Python variables with new ones from FindPython3 module #402

Bi0T1N commented Apr 2, 2022

codecov bot commented Apr 3, 2022

j-rivero left a comment

chapulina commented Jun 2, 2022

Bi0T1N commented Jun 11, 2022

codecov bot commented Jun 14, 2022 •

edited

Loading

Bi0T1N commented Dec 17, 2022

scpeters commented May 9, 2023

azeey Aug 21, 2023

azeey commented Aug 22, 2023

azeey commented Aug 29, 2023

j-rivero left a comment

Replace CMake Python variables with new ones from FindPython3 module #402

Replace CMake Python variables with new ones from FindPython3 module #402

Conversation

Bi0T1N commented Apr 2, 2022

🦟 Bug fix

Summary

Checklist

codecov bot commented Apr 3, 2022

Codecov Report

j-rivero left a comment

Choose a reason for hiding this comment

chapulina commented Jun 2, 2022

Bi0T1N commented Jun 11, 2022

codecov bot commented Jun 14, 2022 • edited Loading

Codecov Report

Bi0T1N commented Dec 17, 2022

scpeters commented May 9, 2023

azeey Aug 21, 2023

Choose a reason for hiding this comment

azeey commented Aug 22, 2023

azeey commented Aug 29, 2023

j-rivero left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2022 •

edited

Loading