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

refactor: moved subprojects to separate repositories #454

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

esteve
Copy link
Collaborator

@esteve esteve commented Jan 28, 2025

@esteve esteve requested review from mxgrey, maspe36 and jhdcs January 28, 2025 14:30
@esteve
Copy link
Collaborator Author

esteve commented Jan 28, 2025

I didn't move the docs folder, I thought it'd be good to have them alongside rclrs, but I'm also fine moving that to a separate repo.

@esteve
Copy link
Collaborator Author

esteve commented Jan 28, 2025

I had to tweak the repos files so that they would point to the forks, but those changes will have to be reverted once the corresponding PRs are merged and before merging this one.

@esteve esteve marked this pull request as draft January 29, 2025 10:22
@esteve
Copy link
Collaborator Author

esteve commented Jan 29, 2025

Marked as draft to avoid merging by mistake, the repos files need to be updated once the other PRs are merged.

@mxgrey
Copy link
Collaborator

mxgrey commented Jan 29, 2025

Are we sure about moving examples into another repo? That will make it harder to ensure that the examples stay in sync with the latest version of rclrs, especially with the incoming API changes.

Iirc the main motivation for splitting the repo is so we can eventually release the crates to the build farm while managing their versions separately. I wouldn't expect us to release the example crates, so there shouldn't be any detriment to keeping them in the same repo as rclrs. It's also very common practice in the Rust ecosystem to keep example projects in the same repo as the crate that they're demonstrating.

@esteve
Copy link
Collaborator Author

esteve commented Jan 29, 2025

@mxgrey that's a good point, I was following the structure of the rclcpp and rclpy repositories. However, I wouldn't dismiss releasing the examples as ROS packages anyway, they are released for the other official clients (e.g. https://index.ros.org/p/examples_rclcpp_minimal_publisher/#humble https://index.ros.org/p/examples_rclpy_minimal_publisher/#humble) and I can see their usefulness for demonstrating the functionality of the client libraries.

@maspe36
Copy link
Collaborator

maspe36 commented Jan 30, 2025

Do we need to have 1 ROS package per repo? Couldn't we keep examples in-place and just package them separately? Its more "rusty" IMO to have the examples stay in tree

A few examples from the rust ecosystem

@mxgrey
Copy link
Collaborator

mxgrey commented Jan 30, 2025

Sure, it's worth considering the possibility that we would release the examples on the build farm as well.

I think even in that situation, it wouldn't be a bad thing to always re-release the examples with each release of rclrs and to make the version numbers of the examples always match the version number of rclrs.

The examples would be leaf nodes in the build tree so there wouldn't be cascading rebuilds from unnecessary releases. And they'd need to be rebuilt with each rclrs release anyway since Rust crates can't be updated with a new version of a dependency without being rebuilt.

@esteve
Copy link
Collaborator Author

esteve commented Jan 31, 2025

@mxgrey thinking about it I agree with not moving the examples folder. Especially given that it'd be counterintuitive to release binaries that have been compiled with an older version of rclrs, I'll keep the examples in this repo, but still move the other ones to their respective repositories.

@maspe36 no, we don't need one repo per package. See for example the rclcpp repository (https://github.com/ros2/rclcpp) which contains three packages. Moving rosidl_generator_rs and rosidl_runtime_rs make sense as they are mostly released at a different pace. Plus it'd help us get the generator in the buildfarm as soon as possible until rclrs is in a state that can be released by the buildfarm

@esteve
Copy link
Collaborator Author

esteve commented Feb 5, 2025

@maspe36 @mxgrey @nwn @jhdcs Now that ros2-rust/rosidl_rust#1 and ros2-rust/rosidl_runtime_rs#2 have been merged, this PR is now ready to be reviewed. The changes here should be straightforward to review, I just removed rosidl_runtime_rs and rosidl_generator_rs

Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

These changes look good to me, though I feel a second opinion might be warranted as I wasn't super involved in the reviews for this before now.

Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Everything looks like it's in order 👍

Copy link
Collaborator

@maspe36 maspe36 left a comment

Choose a reason for hiding this comment

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

LGTM

@maspe36 maspe36 merged commit e15100c into ros2-rust:main Feb 6, 2025
6 checks passed
@esteve esteve deleted the split-repos branch February 6, 2025 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants