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.

Sign up for GitHub

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

Jump to bottom

Stop using python_cmake_module. #93

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Stop using python_cmake_module. #93

merged 2 commits into from
Oct 3, 2024

Conversation

clalancette
Copy link
Contributor

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

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

@jonbinney
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 ament/ament_cmake#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 ros2/ci#755 first, but I think it is otherwise OK. @jonbinney let me know what you think.

@jonbinney
Copy link
Contributor

@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.

quarkytale
quarkytale approved these changes Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@quarkytale quarkytale left a comment

Choose a reason for hiding this comment

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

LGTM, will run CI after the ros2/ci PR gets merged

@clalancette clalancette marked this pull request as draft April 12, 2024 15:34
@clalancette clalancette force-pushed the clalancette/remove-python-cmake-module branch from 5377535 to 3969274 Compare May 14, 2024 13:42
@clalancette clalancette force-pushed the clalancette/remove-python-cmake-module branch from 3969274 to cb69be1 Compare June 4, 2024 18:00
@clalancette clalancette force-pushed the clalancette/remove-python-cmake-module branch from cb69be1 to 3969274 Compare June 25, 2024 13:22
@clalancette
Stop using python_cmake_module.
d4ebb2e 
We really don't need it anymore, and can just use the
builtin find_package(Python3).

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Add in missing ament_cmake_python dependency.
3a2abc7 
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette force-pushed the clalancette/remove-python-cmake-module branch from 3969274 to 3a2abc7 Compare September 21, 2024 23:20
@clalancette clalancette marked this pull request as ready for review October 3, 2024 18:14
@clalancette
Copy link
Contributor Author

CI for this (along with all of the rest of the changes needed) is in ros2/ros2#1524 (comment)

@clalancette clalancette merged commit 291fcb3 into rolling Oct 3, 2024
2 checks passed
@clalancette clalancette deleted the clalancette/remove-python-cmake-module branch October 3, 2024 18:19
CursedRock17 pushed a commit to CursedRock17/laser_geometry that referenced this pull request Nov 2, 2024
@bfjelds @CursedRock17
use find_package to get Boost dependency
f8f22aa 
clean up eigen

export eigen as dependency

laser_geometry should export Eigen3 include dirs

fix includes for case and type

Add visibility header modified from rclcpp

Make it compile, remove PointCloud support, and remove boost

- Compiles on Windows with VS2015/VS2017
- Compiles on Mac with clang
- Compiles on Linux with gcc
- Removed PointCloud support as this is deprecated and might not be needed in ROS 2
- Remove boost as per ROS 2 development guidelines

Build statically but position independent code

- This is necessary to link against shared libraries on Linux

Add tests (remove superfluous test cases) and linters

- Code now lints with standard ament linters
- Added test cases for LaserScan to PointCloud2
- Removed tests that were commented out + tests for LaserScan to PointCloud

Uncrustify

Use correct time unit

Fix cpplint

Disable second test for now

Test needs a correct lookupTransform
It would be best to make that call mockable, but that's not possible with tf2::BufferCore

Fix Windows warnings

Remove several test cases

- On not so fast machines, tests run into timeouts due to exponential explosion: test setup makes for about 7000 test cases
- Keep edge cases

Remove angle dependency as no longer necessary

Add Copyright

Fix package.xml

Add TODO for PointCloud 1 support

Build dynamically using visibility control

Make build export symbols

Increase tests timeout (needed for Mac)

Relicense visibility control file to BSD

Already relicensed in URDF repo.

Create LICENSE (ros-perception#33) (ros-perception#34)

fix eigen dependency name (ros-perception#36)

fixup package.xml

changelogs

2.0.0

Use eigen3_cmake_module

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

2.1.0

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Drop CMake extras redundant with eigen3_cmake_module. (ros-perception#50)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

code style only: wrap after open parenthesis if not in one line (ros-perception#52)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

use target_include_directories

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

use ament_export_targets()

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Changelog.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

2.2.0

increase test timeout

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

update maintainers

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

Use rclcpp::Duration::from_seconds (ros-perception#72)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Changelog.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

2.2.1

Export sensor_msgs, tf2, and rclcpp as dependencies

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

changelog

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

2.2.2

Update python code and tests for ros2 (ros-perception#80)

* Enable projection nose test using ament

* Update python package and tests for ros2

* Remove unneeded python setup file

* Use pytest instead of nose

Nose was outputting xml that xUnit (jenkins plugin) couldn't read.

* Fix pytest warnings

Fix building on running on Windows Debug. (ros-perception#82)

* Fix building on running on Windows Debug.

In particular, we need to set the python executable properly
when running on Windows Debug.  While we are in here, we also
fix up some dependencies in the package.xml and CMakeLists.txt.
We also have to remove WERROR ON, due to some Python
warnings that are outside of our control.  Finally, we heavily
reduce the number of tests being run here so that the tests
complete in a reasonable amount of time, even on (slow) Windows
debug.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

2.3.0

Fix Duration casting issue leading to no undistortion

Signed-off-by: Marco Lampacrescia <Marco.Lampacrescia@de.bosch.com>

Explicit cast to double to prevent loss of precision

Signed-off-by: Marco Lampacrescia <Marco.Lampacrescia@de.bosch.com>

Install headers to include/${PROJECT_NAME} (ros-perception#86)

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Changelog.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

2.4.0

Mirror rolling to ros2

Update Maintainers (ros-perception#88)

* Add Dharini Dutia to CODEOWNERs file

Signed-off-by: Audrow Nash <audrow@openrobotics.org>

Update laser_geometry to C++17. (ros-perception#90)

The main reason to do this is so that we can compile laser_geometry
with the clang static analyzer.  As of clang++-14 (what is in
Ubuntu 22.04), the default still seems to be C++14, so we need
to specify C++17 so that new things in the rclcpp headers work
properly.

Further, due to reasons I don't fully understand, I needed to
set CMAKE_CXX_STANDARD_REQUIRED in order for clang to really use
that version.  So set this as well.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Changelog.

Signed-off-by: Michael Carroll <michael@openrobotics.org>

2.5.0

Changelog

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

2.6.0

Switch to target_link_libraries. (ros-perception#92)

This allows us to hide more of the libraries from downstream
consumers.

While we are in here, do slight cleanups so it is more clear
which libraries are depended on.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Changelog.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

2.7.0

Changelog.

Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com>

2.8.0

Added common linters (ros-perception#96)

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

Changelog.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

2.8.1

Stop using python_cmake_module. (ros-perception#93)

* Stop using python_cmake_module.

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

* Add in missing ament_cmake_python dependency.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quarkytale quarkytale quarkytale approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clalancette @jonbinney @quarkytale