search

ros-perception / laser_geometry

Provides the LaserProjection class for turning laser scan data into point clouds.
BSD 3-Clause "New" or "Revised" License
157 stars 114 forks source link

Stop using python_cmake_module. #93

Closed clalancette closed 1 month ago

clalancette commented 9 months ago

We really don't need it anymore, and can just use the builtin find_package(Python3).

This must be merged before https://github.com/ros2/ros2/pull/1524 ; see that pull request for more information about this change.

jonbinney commented 9 months ago

@clalancette thank you for doing this! It looks like this PR removes the usage of python_cmake_module but doesn't add find_package(Python3). Is that because that is handled by find_package(ament_cmake_pytest REQUIRED)?

clalancette commented 9 months ago

@clalancette thank you for doing this! It looks like this PR removes the usage of python_cmake_module but doesn't add find_package(Python3). Is that because that is handled by find_package(ament_cmake_pytest REQUIRED)?

That's a really good question. Most of the other places I did this already had separate calls to find_package(Python3), but this one didn't. I did run some preliminary CI on this, and that seemed to pass. But I'll take a closer look to see what is happening here.

clalancette commented 9 months ago

Ah, I see. So it turns out that deep in the bowels of ament_cmake_core, we do a find_package(Python3), which the rest of the CMakeLists.txt is inheriting. So that makes it work in my testing.

But it doesn't seem right to depend on that; theoretically, someday ament_cmake_core could remove it. I'll discuss with some others and see if we should add it to ament_cmake_install_python or something like that.

Relatedly, I realize that this package is missing a dependency on ament_cmake_python, so I'll add that as well.

clalancette commented 9 months ago

So we discussed this.

First, it is our feeling that downstream packages that just depend on ament_cmake_python should not have to call find_package(Python3) themselves, unless they directly reference it. Given that this package does not directly reference any of the Python3 things from CMake, this PR is correct as-is.

The other question had to do about whether we should explicitly do find_package(Python3) within ament_cmake_python. At the end of the day, we decided not to mess with what is working, and so we compromised and just added a comment in https://github.com/ament/ament_cmake/pull/510 that explains that we expect it to be there via ament_cmake_core.

So I think this PR is generally ready to go. It still needs CI run on it, and we'll almost certainly have to merge in https://github.com/ros2/ci/pull/755 first, but I think it is otherwise OK. @jonbinney let me know what you think.

jonbinney commented 9 months ago

@clalancette that sounds good to me and this seems mergeable, but I realized this isn't actually one of the packages I maintain so @mabelzhang should make the call on merging.

clalancette commented 1 month ago

CI for this (along with all of the rest of the changes needed) is in https://github.com/ros2/ros2/pull/1524#issuecomment-2365359588