Swapped usage of rosidl_cmake over to the new rosidl_pycommon. #297

maspe36 · 2022-12-12T03:23:15Z

As of rosidl 3.3.0 (specifically, this commit), the rosidl_cmake Python module was moved to a new rosidl_pycommon package and the Python module in rosidl_cmake was deprecated.

This was throwing these deprecation warnings when building for rolling.

Starting >>> builtin_interfaces
Starting >>> unique_identifier_msgs
--- stderr: unique_identifier_msgs                                                                                 
/opt/ros/rolling/local/lib/python3.10/dist-packages/rosidl_cmake/__init__.py:19: UserWarning: The 'rosidl_cmake' Python module is deprecated. Use 'rosidl_pycommon' instead.
  warnings.warn(
---
Finished <<< unique_identifier_msgs [2.73s]
--- stderr: builtin_interfaces                               
/opt/ros/rolling/local/lib/python3.10/dist-packages/rosidl_cmake/__init__.py:19: UserWarning: The 'rosidl_cmake' Python module is deprecated. Use 'rosidl_pycommon' instead.
  warnings.warn(
---
Finished <<< builtin_interfaces [2.92s]

As of [rosidl 3.3.0](ros2/rosidl@9348ce9), the rosidl_cmake Python module was moved to a new rosidl_pycommon package and the Python module in rosidl_cmake was deprecated.

maspe36 · 2022-12-12T04:44:44Z

rosidl_generator_rs/package.xml

@@ -13,7 +13,8 @@
  <buildtool_depend>rosidl_runtime_rs</buildtool_depend>

  <buildtool_export_depend>ament_cmake</buildtool_export_depend>
-  <buildtool_export_depend>rosidl_cmake</buildtool_export_depend>
+  <buildtool_export_depend condition="$ROS_DISTRO != rolling">rosidl_cmake</buildtool_export_depend>


I may be interpreting REP 149 incorrectly, but to me it reads as if I should be able to use a less/greater than for ROS distros. Unfortunately that doesn't work, and the xml fails to parse. Not sure what to do if rosidl_cmake 3.3.0+ gets back ported to Humble or when Iron comes out

Reads that way to me too. Did you try only > or also >=?

Coming back to this now, looks to only be a problem with less than operators.

Fails to parse
$ROS_DISTRO <= rolling
$ROS_DISTRO < rolling

Successfully parses
$ROS_DISTRO >= rolling
$ROS_DISTRO > rolling

Thankfully this means we can handle this a bit more concisely in the package.xml

<buildtool_export_depend condition="humble >= $ROS_DISTRO">rosidl_cmake</buildtool_export_depend> <buildtool_export_depend condition="$ROS_DISTRO > humble">rosidl_pycommon</buildtool_export_depend>

We know this change is happening after humble (but only on rolling right now), so we can assume anything that's newer than humble will use rosidl_pycommon

esteve · 2022-12-12T15:50:28Z

@maspe36 thanks for the PR. The changes assume that rolling is the next ROS distro, however it's more akin to main in a GitHub repo that gets tagged and release, so these changes won't work with the next ROS distro iron (https://www.ros.org/reps/rep-2000.html#iron-irwini-may-2023-november-2024).

We have two options here:

create a new branch for humble and make main target iron and greater, and backport any changes to humble, galactic, etc.
check the version of one of the rosidl base packages in CMake (e.g. rosidl_parser) and if it's greater or equal than 3.3.0, then don't call find_package on rosidl_cmake. This should work, the problem is that the syntax for package.xml is not expressive enough to have optional dependencies based on the distro

@maspe36 Thoughts? (cc: @jhdcs @nnmm)

nnmm · 2023-02-22T21:31:18Z

This would fit the bill for package.xml, right? Assuming that this won't get backported, which I don't think it will.

<buildtool_export_depend condition="$ROS_DISTRO == foxy or $ROS_DISTRO == galactic or $ROS_DISTRO == humble">rosidl_cmake</buildtool_export_depend>
<buildtool_export_depend condition="$ROS_DISTRO != foxy and $ROS_DISTRO != galactic and $ROS_DISTRO != humble">rosidl_pycommon</buildtool_export_depend>

Then in CMake we could do something a little more elegant like @esteve proposed.

maspe36 · 2023-02-24T02:10:23Z

The changes assume that rolling is the next ROS distro, however it's more akin to main in a GitHub repo that gets tagged and release, so these changes won't work with the next ROS distro iron

The next patch set doesn't assume this and I don't think will break when iron is released.

create a new branch for humble and make main target iron and greater, and backport any changes to humble, galactic, etc.

I think eventually this may need to happen, but for now that's probably more work than its worth. Especially since we can have optional dependencies in the package.xml based on distro.

check the version of one of the rosidl base packages in CMake (e.g. rosidl_parser) and if it's greater or equal than 3.3.0, then don't call find_package on rosidl_cmake

Is this really better than doing the same check from the package.xml in the CMake?

if("$ENV{ROS_DISTRO}" STRLESS_EQUAL "humble")
    find_package(rosidl_cmake REQUIRED)
    ament_export_dependencies(rosidl_cmake)
endif()

Only benefit I see would be if it gets backported but since this is a deprecation I think @nnmm is right, this probably won't get backported.

maspe36 · 2023-02-25T19:58:53Z

Look like the build is failing for the same reason mentioned here? I am able to pass this test, pep257_rosidl_generated_py, locally on humble and rolling at least.

nnmm

From my understanding, this looks good. I'll wait a little bit before merging to give others the chance to take a look too.

rosidl_generator_rs/package.xml

nnmm · 2023-03-20T13:14:56Z

@maspe36 I think you're now running into another small non-backwards-compatible change that was made in rolling: #302 (comment)

I can try to fix it tonight, or you can do it in this PR – it should be easily resolved by conditionally setting the GID length to 16 on rolling where we currently have 24 hardcoded.

An even cleaner solution would be to get rid of the hardcoding – I think the root of the problem is that #define-style constants do not produce a Rust constant in bindgen, so maybe https://github.com/ros2-rust/ros2_rust/blob/main/rclrs/src/rcl_wrapper.h could add some const variable with the value RMW_GID_STORAGE_SIZE, that might work. Not sure though.

nnmm · 2023-03-25T18:13:50Z

@maspe36 Not quite "tonight" as promised, but I have an MR open with the fix, just waiting for review by a maintainer: #309

nnmm · 2023-03-29T17:34:06Z

@maspe36 If you rebase this onto main, CI should be green.

nnmm

Don't we also need rosidl_pycommon in rosidl_generator_rs/CMakeLists.txt? Otherwise LGTM, but I can't merge before @esteve revisits his requested changes.

esteve · 2023-03-30T09:13:10Z

@maspe36 @nnmm I agree, rosidl_common_py needs to be exported in CMakeLists.txt, otherwise, the PR looks good to me. Thanks @maspe36 for all the work and for iterating with us!

maspe36 · 2023-04-06T00:28:28Z

@esteve @nnmm rosidl_pycommon is a python only package, you cannot find it with find_package, and you cannot export it with ament_export_dependencies. Having it in the package.xml should be sufficient for rosdep

nnmm

Sounds good to me. Thanks for taking care of this, and sorry it took forever to merge!

esteve · 2023-04-06T14:12:54Z

@maspe36 thanks again for all the patience! 🙂

maspe36 added 4 commits December 11, 2022 19:20

Swapped usage of rosidl_cmake over to the new rosidl_pycommon.

05383e2

As of [rosidl 3.3.0](ros2/rosidl@9348ce9), the rosidl_cmake Python module was moved to a new rosidl_pycommon package and the Python module in rosidl_cmake was deprecated.

Support builds from older ROS 2 distros.

c6173ac

Fixed build for rolling

ae86ce2

Added test_depend conditional inclusion as well.

f231114

maspe36 commented Dec 12, 2022

View reviewed changes

Swap to a more elegant check

8ee4517

Merge branch 'main' into fix_deprecated_rosidl_cmake_warning

46c0e54

nnmm previously approved these changes Mar 14, 2023

View reviewed changes

esteve requested changes Mar 14, 2023

View reviewed changes

rosidl_generator_rs/package.xml Show resolved Hide resolved

PR Feedback

084fea3

maspe36 dismissed nnmm’s stale review via 084fea3 March 17, 2023 18:13

Merge branch 'ros2-rust:main' into fix_deprecated_rosidl_cmake_warning

0a530d2

maspe36 requested review from esteve and nnmm and removed request for esteve and nnmm March 30, 2023 01:58

nnmm reviewed Mar 30, 2023

View reviewed changes

nnmm approved these changes Apr 6, 2023

View reviewed changes

esteve approved these changes Apr 6, 2023

View reviewed changes

esteve merged commit 047f483 into ros2-rust:main Apr 6, 2023

maspe36 mentioned this pull request Oct 28, 2023

0.4 Release Tracking Issue #340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swapped usage of rosidl_cmake over to the new rosidl_pycommon. #297

Swapped usage of rosidl_cmake over to the new rosidl_pycommon. #297

maspe36 commented Dec 12, 2022 •

edited

Loading

maspe36 Dec 12, 2022

nnmm Feb 22, 2023

maspe36 Feb 24, 2023 •

edited

Loading

esteve commented Dec 12, 2022

nnmm commented Feb 22, 2023 •

edited

Loading

maspe36 commented Feb 24, 2023

maspe36 commented Feb 25, 2023 •

edited

Loading

nnmm left a comment

nnmm commented Mar 20, 2023

nnmm commented Mar 25, 2023

nnmm commented Mar 29, 2023

nnmm left a comment •

edited

Loading

esteve commented Mar 30, 2023

maspe36 commented Apr 6, 2023

nnmm left a comment

esteve commented Apr 6, 2023

Swapped usage of rosidl_cmake over to the new rosidl_pycommon. #297

Swapped usage of rosidl_cmake over to the new rosidl_pycommon. #297

Conversation

maspe36 commented Dec 12, 2022 • edited Loading

maspe36 Dec 12, 2022

Choose a reason for hiding this comment

nnmm Feb 22, 2023

Choose a reason for hiding this comment

maspe36 Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

esteve commented Dec 12, 2022

nnmm commented Feb 22, 2023 • edited Loading

maspe36 commented Feb 24, 2023

maspe36 commented Feb 25, 2023 • edited Loading

nnmm left a comment

Choose a reason for hiding this comment

nnmm commented Mar 20, 2023

nnmm commented Mar 25, 2023

nnmm commented Mar 29, 2023

nnmm left a comment • edited Loading

Choose a reason for hiding this comment

esteve commented Mar 30, 2023

maspe36 commented Apr 6, 2023

nnmm left a comment

Choose a reason for hiding this comment

esteve commented Apr 6, 2023

maspe36 commented Dec 12, 2022 •

edited

Loading

maspe36 Feb 24, 2023 •

edited

Loading

nnmm commented Feb 22, 2023 •

edited

Loading

maspe36 commented Feb 25, 2023 •

edited

Loading

nnmm left a comment •

edited

Loading