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

fuse -> ROS 2 fuse_msgs : Port package and ignore unported packages for now #277

Merged
merged 4 commits into from
Oct 7, 2022

Conversation

methylDragon
Copy link
Collaborator

@methylDragon methylDragon commented Oct 7, 2022

See: #276

This is just an extremely minor port.

It moves the message definitions and uses the ROS 2 message generation pipeline and adds COLCON_IGNORE files to the remaining unported packages.

I also added linting (fixing a package.xml linting issue).

PS: Should I be bumping the version number? It should be API breaking, so does that mean it should be a new MAJOR version? Or will it have to wait for release first?

Also pinging @svwilliams for visibility.

@methylDragon
Copy link
Collaborator Author

methylDragon commented Oct 7, 2022

Hmm, looks like jenkins is failing on me, but I'm missing some permissions to see what's wrong?
(If I were to guess, it's probably failing because it's trying to catkin build it)

@methylDragon methylDragon changed the title fuse -> ROS 2: Port fuse_msgs and ignore unported packages for now fuse -> ROS 2 fuse_msgs : Port package and ignore unported packages for now Oct 7, 2022
@svwilliams
Copy link
Contributor

In case it is helpful, the buildserver error is:

[2022-10-07T00:19:40.026Z] ERROR: the following packages/stacks could not have their rosdep keys resolved
[2022-10-07T00:19:40.026Z] to system dependencies:
[2022-10-07T00:19:40.026Z] fuse_msgs: Cannot locate rosdep definition for [rosidl_default_runtime]
script returned exit code 1

Pinging @paulbovbel who designed our build system. We are starting the ROS2 port of the fuse package. Are there any steps required to make our build system work with ROS2? Or is the issue that the port is targeting ROS2 Rolling and Locus is using Galactic or whatever official release?

@sloretz
Copy link
Collaborator

sloretz commented Oct 7, 2022

PS: Should I be bumping the version number? It should be API breaking, so does that mean it should be a new MAJOR version? Or will it have to wait for release first?

Usually we bump the version number at the time of release, but in this case I think we could do it at any time. @svwilliams does Locus use a specific versioning system (such as semver.org?) If so, what version number would you prefer for Rolling? (current is 0.4.2)

Pinging @paulbovbel who designed our build system. We are starting the ROS2 port of the fuse package. Are there any steps required to make our build system work with ROS2? Or is the issue that the port is targeting ROS2 Rolling and Locus is using Galactic or whatever official release?

@svwilliams How would you feel about us adding a Rolling PR job and using that as CI?

fuse_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
@svwilliams
Copy link
Contributor

svwilliams commented Oct 7, 2022

How would you feel about us adding a Rolling PR job and using that as CI?

That probably makes the most sense. Locus is not currently targeting Rolling with our CI builds anyway. So even if we get the build configuration worked out to build fuse against ROS2, there is still potential for version compatibility problems.

@svwilliams
Copy link
Contributor

Usually we bump the version number at the time of release, but in this case I think we could do it at any time. @svwilliams does Locus use a specific versioning system (such as semver.org?) If so, what version number would you prefer for Rolling? (current is 0.4.2)

Locus also bumps the version numbers at the time of release, but Locus has its own release schedule. Again, I'm going to defer to @paulbovbel for version number recommendations.

Signed-off-by: methylDragon <methylDragon@gmail.com>
Copy link
Collaborator

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

This looks good to me, though I recommend adding a note to the main repository README saying the Rolling branch is a work in progress with a link to #276.

In absence of a PR job I manually built this repo with colcon build using ROS Rolling on Ubuntu Jammy. Only fuse_msgs was built. colcon test + colcon test-result shows all tests passed.

I manually ran a few commands to double check, and the output looks correct.

jammy:osrf> ros2 interface package fuse_msgs
fuse_msgs/msg/SerializedGraph
fuse_msgs/msg/SerializedTransaction
jammy:osrf> python3 -c 'import fuse_msgs.msg; print(fuse_msgs.msg.SerializedTransaction())'
fuse_msgs.msg.SerializedTransaction(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_id=''), data=[])
jammy:osrf> python3 -c 'import fuse_msgs.msg; print(fuse_msgs.msg.SerializedGraph())'
fuse_msgs.msg.SerializedGraph(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_id=''), plugin_name='', data=[])

@methylDragon
Copy link
Collaborator Author

This looks good to me, though I recommend adding a note to the main repository README saying the Rolling branch is a work in progress with a link to #276.

In absence of a PR job I manually built this repo with colcon build using ROS Rolling on Ubuntu Jammy. Only fuse_msgs was built. colcon test + colcon test-result shows all tests passed.

I manually ran a few commands to double check, and the output looks correct.

jammy:osrf> ros2 interface package fuse_msgs
fuse_msgs/msg/SerializedGraph
fuse_msgs/msg/SerializedTransaction
jammy:osrf> python3 -c 'import fuse_msgs.msg; print(fuse_msgs.msg.SerializedTransaction())'
fuse_msgs.msg.SerializedTransaction(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_id=''), data=[])
jammy:osrf> python3 -c 'import fuse_msgs.msg; print(fuse_msgs.msg.SerializedGraph())'
fuse_msgs.msg.SerializedGraph(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_id=''), plugin_name='', data=[])

Okie, I'll add that note and then merge! Thanks for the review!

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon merged commit e0dd00e into locusrobotics:rolling Oct 7, 2022
@methylDragon methylDragon deleted the rolling branch October 7, 2022 23:49
@methylDragon methylDragon restored the rolling branch October 7, 2022 23:50
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.

None yet

4 participants