Miscellaneous scrapping fixes #378

gergondet-woven · 2025-02-04T12:13:20Z

Hello drake-ros maintainers,

Thank you for making this project available. We have been using it to build our software for the past months and we have built up a number of patches that we hope can be useful to everyone.

This PR is the first I'm opening and it contains 4 small patches related to various edge cases in the workspace scrapping that we have encountered.

Below is a short summary of the individual patches, I am happy to submit separate PRs if you prefer.

P1: Remove elements that have a condition attribute on ROS1

This is used by a number of packages that are trying to support both versions at the same time.

This patch is actually part of #336 we actually had our own patch that does pretty much the same thing (including packages that have the $ROS_VERSION == 2 condition instead of excluding the $ROS_VERSION == 1). However, that PR incorporates more changes this particular fix is not available on main yet so I hope it's ok to bring it in with this PR.

P2: Fix an issue with some Python packages that have multiple entries in the egg top_level.txt (seen in `launch_testing for example)

P3: Fix an issue with CMake's JSON where libraries' paths are escaped under some conditions (for example if the workspace path has a ~ character)

P4: Include *.inc files in C++ headers of library, afaik, only the domain_bridge package uses this extension

Thanks again for making bazel_ros2_rules and I'm looking forward to your feedback.

This change is

zachfang · 2025-02-20T16:15:39Z

@IanTheEngineer I assigned you as the reviewer for now just to ensure it's on our radar. Feel free to delegate otherwise.

IanTheEngineer

Thank you for the PR! I kicked off the CI for completeness, but the failures do not necessarily indicate that something is amiss with your PR. I will run some tests locally overnight and report back.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion

bazel_ros2_rules/ros2/resources/cmake_tools/file_api.py line 72 at r1 (raw file):

                    self.link_flags.append(fragment['fragment'])
                elif fragment['role'] == 'libraries':
                    # In the generated JSON a library can be in quotes (to escape some characters?)

Nit: We try to wrap lines at 80 characters whenever possible. This applies to the other files in the PR as well.

gergondet-woven · 2025-02-28T05:55:34Z

Hello @IanTheEngineer

Thanks for the feedback.

We try to wrap lines at 80 characters whenever possible. This applies to the other files in the PR as well.

Sorry about that, I have pushed an extra commit to wrap all lines here under 80 columns.

IanTheEngineer

I took another pass looking through the changes this evening. This looks quite good. I have a couple small questions and nits below. Thank you for iterating on this.

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gergondet-woven)

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/ament_python.py line 17 at r2 (raw file):

    top_level = dist.read_text('top_level.txt')
    top_level = top_level.rstrip('\n')
    return str(dist._path), "\n".join(

Nit: Consider storing this string in a variable and returning that for readability. Also, consider adding a comment as to what this is fixing.

bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 156 at r2 (raw file):

        sandbox(top_level_i)
        for _, top_level in properties.python_packages
        for top_level_i in top_level.split("\n")

Nit: This nested list comprehension is quite clever, but somewhat hard to understand at a glance. Consider breaking it up for readability.

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

                    elements_to_remove.append((parent, child))
                else:
                    child.attrib.pop("condition")

Nit: Consider conforming to the single quote string convention used by the rest of the file.

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

                    elements_to_remove.append((parent, child))
                else:
                    child.attrib.pop("condition")

Looking at the ElementTree iter documentation, it states that the behavior is undefined when iterating over a tree if it is then modified.

Would child.attrib.pop('condition') be considered a modification to the tree structure?

bazel_ros2_rules/ros2/resources/cmake_tools/file_api.py line 76 at r2 (raw file):

                    # This escaping creates problem down the line,
                    # "/usr/lib/libfoo.so" is *not* a path.
                    # We remove the escaping here so they can be treated normally

This comment block is fantastic for conveying the issue and fix. Thanks for this!

gergondet-woven

Thanks again for the feedback and sorry for the delay but I finally got around to fixing the issues you raised!

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @IanTheEngineer)

bazel_ros2_rules/ros2/resources/ros2bzl/templates.py line 156 at r2 (raw file):

Previously, IanTheEngineer (Ian McMahon) wrote…

Nit: This nested list comprehension is quite clever, but somewhat hard to understand at a glance. Consider breaking it up for readability.

Done, hopefully it's clearer

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/ament_python.py line 17 at r2 (raw file):

Previously, IanTheEngineer (Ian McMahon) wrote…

Nit: Consider storing this string in a variable and returning that for readability. Also, consider adding a comment as to what this is fixing.

Done, I have split this into two assignments: one to retrieve the entries inside the the top_level file and one to resolve the path. I have also added a comment clarifying why we need to do this here

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

Previously, IanTheEngineer (Ian McMahon) wrote…

Nit: Consider conforming to the single quote string convention used by the rest of the file.

Sorry about that, I have changes all the double quotes in the patch with single quotes

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/metadata.py line 15 at r2 (raw file):

Previously, IanTheEngineer (Ian McMahon) wrote…

Looking at the ElementTree iter documentation, it states that the behavior is undefined when iterating over a tree if it is then modified.

Would child.attrib.pop('condition') be considered a modification to the tree structure?

I don't think so, attributes are attached to the node so removing them should not modify the tree structure.

IanTheEngineer

Thank you for the iterations here! Things look great. I'll give this a test run tomorrow and we should be clear to merge.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gergondet-woven)

jwnimmer-tri · 2025-04-24T19:07:46Z

Thank you for the iterations here! Things look great. I'll give this a test run tomorrow and we should be clear to merge.

Now that (as of a couple minutes ago) the drake-ros main branch supports Jazzy (only) and not Humble, be aware that this might need Jazzy testing now. Up to you.

jwnimmer-tri · 2025-04-28T16:09:34Z

Also as of #383, this has a merge conflict now which would need to be resolving.

BTW If this a humble-specific fix, another option would be for Ian to add a humble legacy support branch (forked from an earlier point on main), and re-target this PR onto that branch.

ggould-tri

I believe that we are de facto committed to a legacy support branch of some sort, either for jammy or humble; we should avoid if possible having two legacy support branches at once.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gergondet-woven)

jwnimmer-tri · 2025-04-28T16:18:18Z

I believe that we are de facto committed to a legacy support branch of some sort, ...

Not really. TRI only needs such a named branch when we need to land TRI hotfixes for Jammy/Humble, which is unlikely. For TRI purposes at least, we could/would create such a branch only when we need it.

When the workspace path has ~ characters (and possibly others), the JSON output by CMake will be a quoted string, e.g. ``` "lib": "\"/some/ws~~path/libfoo.so\"" ``` The extract path will be `"/some/ws~~path/libfoo.so"` instead of `/some/ws~~path/libfoo.so` and the former will not be treated as path down the line.

This is used in the domain_bridge package.

gergondet-woven · 2025-04-30T01:32:56Z

Thanks for the heads up, I have rebased this branch on the current main to account for the changes.

The patches are not Humble specific and still apply to some packages in Jazzy.

P2: Fix an issue with some Python packages that have multiple entries in the egg top_level.txt (seen in `launch_testing for example)

I have revised my patch for this since it was affected by #383. In Jazzy, the only package doing that - that I have installed locally at least - is launch_testing. Since #383 added an assert that top_level.txt would contain exactly 1 entry, this would fail with this package so I have changed the assert to allow 1 or more package and print a warning when it's more than 1.

ggould-tri

Reviewed all commit messages.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @IanTheEngineer)

bazel_ros2_rules/ros2/resources/ros2bzl/scraping/python.py line 26 at r4 (raw file):

    if len(packages) > 1:
        print(f"Multiple top level entries where found in {name}. "
               "Only the first one will be considered")

BTW: If the only place this is called is line 53 of this file, you could just return the whole list of packages and not build a singleton list around it there?
(I doubt this is especially important, but it would also probably net simplify the code)

ggould-tri

What's the current status on this PR? I see one open discussion and some unclear CI status; I kicked the CI to unwedge that. Who has the next action on this PR?

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @gergondet-woven and @IanTheEngineer)

gergondet-woven · 2025-05-12T10:04:47Z

BTW: If the only place this is called is line 53 of this file, you could just return the whole list of packages and not build a singleton list around it there?

Indeed it's only used there so I have changed the code to correctly handle multiple packages. I was a little concerned with the potential change to properties.python_packages but the possible existence of multiple top levels is correctly handled in configure_package_py_library

I kicked the CI to unwedge that

I don't think the CI failure are related to my changes but you might want to kick it again to test this latest change.

ggould-tri

Is this failure related?
https://github.com/RobotLocomotion/drake-ros/actions/runs/14969360103/job/42075274838?pr=378

Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on @IanTheEngineer)

gergondet-woven · 2025-05-19T09:55:05Z

Is this failure related?

Yes! This one was on me, I had a silly typo in that latest patch!

I pushed a fixed version where I verified locally that all tests passed 🎉

Would you mind re-running the CI on this latest revision?

ggould-tri

Reviewed 2 of 5 files at r2, 3 of 4 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gergondet-woven)

zachfang assigned IanTheEngineer Feb 20, 2025

IanTheEngineer reviewed Feb 27, 2025

View reviewed changes

IanTheEngineer requested changes Mar 5, 2025

View reviewed changes

gergondet-woven commented Mar 21, 2025

View reviewed changes

IanTheEngineer approved these changes Apr 24, 2025

View reviewed changes

ggould-tri reviewed Apr 28, 2025

View reviewed changes

gergondet-woven force-pushed the fix/misc-scrapping-fixes branch from cfe28b4 to 9333462 Compare April 30, 2025 01:14

gergondet-woven added 6 commits April 30, 2025 10:25

Remove elements that have a condition attribute on ROS1

697ee4d

Include *.inc files in C++ headers

8428bbb

This is used in the domain_bridge package.

Wrap to 80 characters

81263b9

Address review comments

d53de6f

Handle the case where top_level.txt has more than one entry

a9113f3

gergondet-woven force-pushed the fix/misc-scrapping-fixes branch from 9333462 to a9113f3 Compare April 30, 2025 01:26

ggould-tri reviewed Apr 30, 2025

View reviewed changes

ggould-tri reviewed May 8, 2025

View reviewed changes

Correctly handle multiple top level entries

30dc664

ggould-tri reviewed May 12, 2025

View reviewed changes

Fix typo in top_levels expression

bf26d80

ggould-tri approved these changes May 19, 2025

View reviewed changes

ggould-tri merged commit 6f5f05d into RobotLocomotion:main May 19, 2025
6 checks passed

Miscellaneous scrapping fixes #378

Miscellaneous scrapping fixes #378

Uh oh!

Conversation

gergondet-woven commented Feb 4, 2025 • edited by EricCousineau-TRI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachfang commented Feb 20, 2025

Uh oh!

IanTheEngineer left a comment

Choose a reason for hiding this comment

Uh oh!

gergondet-woven commented Feb 28, 2025

Uh oh!

IanTheEngineer left a comment

Choose a reason for hiding this comment

Uh oh!

gergondet-woven left a comment

Choose a reason for hiding this comment

Uh oh!

IanTheEngineer left a comment

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri commented Apr 24, 2025

Uh oh!

jwnimmer-tri commented Apr 28, 2025

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri commented Apr 28, 2025

Uh oh!

gergondet-woven commented Apr 30, 2025

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

gergondet-woven commented May 12, 2025

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

gergondet-woven commented May 19, 2025

Uh oh!

ggould-tri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gergondet-woven commented Feb 4, 2025 •

edited by EricCousineau-TRI

Loading