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.

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

Whitelist packages #6

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Whitelist packages #6

merged 4 commits into from
Jun 20, 2024

Conversation

jprestwo
Copy link

No description provided.

norro and others added 2 commits June 13, 2024 10:42
* Adds cmake file that parses package include resp. exclude list
* Filtering cmake dependencies based on include/exclude list

ros2#284

Signed-off-by: norro <[email protected]>
Co-authored-by: James Prestwood <[email protected]>
* CMake passes package include/ignore lists to python scripts generating
the bridge c++ code
* Generation of factories and mappings restricted to packages filtered
by include/ignore lists

ros2#284

Signed-off-by: norro <[email protected]>
Co-authored-by: James Prestwood <[email protected]>
@jprestwo
Copy link
Author

Based on ros2#294 but applied to our local branch

@jprestwo
Copy link
Author

jprestwo commented Jun 14, 2024

So at the moment the only open issue here is how exactly we provide the list of packages. At the moment I'm doing this very crudely with an ansible task:

- name: Create ros1_bridge package list
  lineinfile:
    create: true
    dest: /home/locus/BridgePackageInclude.List
    line: '{{ item }}'
  with_items:
    - 'std_msgs'
    - 'geometry_msgs'

But we need a better option for building the bundle. Open to any ideas.

@jprestwo jprestwo marked this pull request as ready for review June 14, 2024 16:33
@ayrton04
Copy link

Gonna let the experts review this one. I'm not clear on how this package finds all the relevant message packages and does its job, so I don't want to give an inaccurate review.

@ayrton04 ayrton04 removed their request for review June 18, 2024 10:17
@paulbovbel
Copy link

paulbovbel commented Jun 18, 2024

So at the moment the only open issue here is how exactly we provide the list of packages.

I think the best way is to list them as in package.xml (which you will need regardless, for colcon/tailor to work properly), and then scrape the package xml using the catkin_pkg library (which supports ROS2 package.xmls as well :) during the build. single source of truth!

@jprestwo
Copy link
Author

I think the best way is to list them as in package.xml (which you will need regardless, for colcon/tailor to work properly), and then scrape the package xml using the catkin_pkg library (which supports ROS2 package.xmls as well :) during the build. single source of truth!

My concern with this is we would then be adding (potentially) both ROS1 and ROS2 packages to package.xml. I'm not sure what the implications of that are? I would think rosdep isn't going to be happy if a package exists in ROS1 but not ROS2.

@paulbovbel
Copy link

Sorry didn't see your comment @jprestwo, but yeah, ROS1 packages are effectively 'invisible' to the ROS2 dependency tree in package.xml

pkg = parse_package("package.xml")
packages = []

for depend in pkg["build_depends"]:

Choose a reason for hiding this comment

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

so, package.xml here still has a group_depends, for CI purposes could you replace it with a dependency on the message packages you want?

the only ros1_bridge config we have in devel right now i'm aware of are:
https://github.com/locusrobotics/locus_bots/blob/67717d6ec10ebf71c9e015b0585185e43fe7a9cc/origin_common/origin_autonomy/config/ros1_bridge.yaml#L11
https://github.com/locusrobotics/locus_bots/blob/67717d6ec10ebf71c9e015b0585185e43fe7a9cc/omni_common/omni_autonomy/config/ros1_bridge.yaml#L19

though @ayrton04 might have more he wants to bridge over.

Also I think group_depends decodes to a not a <build_depends>, but I can't think of a reason why ros1_bridge would need to on message packages. This may have just been a question of convenience in not have to create <group_build_depends> and etc.

Choose a reason for hiding this comment

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

GS and wifi data topics should be all I need for now.

Copy link
Author

Choose a reason for hiding this comment

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

@paulbovbel yeah I just left package.xml as-is for the context of this PR. But where are groups even defined? I don't really have a preference if we do it via <build_depends> or <group_depends>, but I just don't know where to add packages to the latter (group).

The prior mechanism to generate an ignore/include list was to
use a static file at the workspace root. This doesn't play very
well with our build system so instead use package.xml. Any
<build_depend> package will now be included in the ros1_bridge
build.

This is done using a simple python script to parse package.xml
which is then called by cmake (stdout of the script is the list).
We then feed that list into a modified filter_packages() function
that takes a list of include/ignore packages rather than using
the static list.
The former is deprecated and the build was warning of this.
@jprestwo jprestwo merged commit 1cd2beb into locus-master Jun 20, 2024
0 of 3 checks passed
@jprestwo jprestwo deleted the whitelist-packages branch June 20, 2024 13:01
jprestwo added a commit that referenced this pull request Sep 27, 2024
* Introduces package include and exclude lists

* Adds cmake file that parses package include resp. exclude list
* Filtering cmake dependencies based on include/exclude list

ros2#284

Signed-off-by: norro <[email protected]>
Co-authored-by: James Prestwood <[email protected]>

* Passes package incl./excl. to bridge generation

* CMake passes package include/ignore lists to python scripts generating
the bridge c++ code
* Generation of factories and mappings restricted to packages filtered
by include/ignore lists

ros2#284

Signed-off-by: norro <[email protected]>
Co-authored-by: James Prestwood <[email protected]>

* Create package whitelist from package.xml

The prior mechanism to generate an ignore/include list was to
use a static file at the workspace root. This doesn't play very
well with our build system so instead use package.xml. Any
<build_depend> package will now be included in the ros1_bridge
build.

This is done using a simple python script to parse package.xml
which is then called by cmake (stdout of the script is the list).
We then feed that list into a modified filter_packages() function
that takes a list of include/ignore packages rather than using
the static list.

* Replace rosidl_cmake with rosidl_common

The former is deprecated and the build was warning of this.

---------

Signed-off-by: norro <[email protected]>
Co-authored-by: norro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants