Skip to content

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Jul 11, 2025

No description provided.

@esteve esteve requested review from maspe36 and jhdcs July 11, 2025 17:59
@esteve
Copy link
Contributor Author

esteve commented Jul 11, 2025

@jhdcs @maspe36 I don't know if it'd be better to only build the generate_docs feature (which contains a shim of the rcl functions) or do it like this PR and install ROS before attempting to release the crate. Let me know what you think.

Copy link
Contributor

@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.

So on one hand, if we use the generate_docs feature we would presumably finish CI faster, which in general is nice. But I feel like especially for a release we should try and be as faithful as possible with our intended users environment.

To me, this makes sense to install ROS. As an aside, what version of ROS is installed?

@esteve
Copy link
Contributor Author

esteve commented Jul 13, 2025

So on one hand, if we use the generate_docs feature we would presumably finish CI faster, which in general is nice. But I feel like especially for a release we should try and be as faithful as possible with our intended users environment.

On the one hand, we only do that once per release, so not a big deal if it takes a bit longer. On the other hand, we already build rclrs for every supported ROS distro when we merge a PR and periodically, so not sure if it's needed for a release in the end. I'd rename generate_docs to something more descriptive for this use case (e.g. build_for_release)

As an aside, what version of ROS is installed?

Actually none, I thought setup-ros would install the latest distro, but turns out that unless the required-ros-distributions option is passed, it just installs the ROS build tools.

@jhdcs
Copy link
Contributor

jhdcs commented Jul 15, 2025

I would agree that renaming generate_docs to something more descriptive would be a good idea. Apart from that, though, I'm afraid I don't have much of an opinion - this is a little outside my normal area of expertise...

maspe36
maspe36 previously approved these changes Aug 3, 2025
Copy link
Contributor

@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.

Actually none, I thought setup-ros would install the latest distro, but turns out that unless the required-ros-distributions option is passed, it just installs the ROS build tools.

If that's the case then lets just go with this approach

@esteve
Copy link
Contributor Author

esteve commented Aug 3, 2025

If that's the case then lets just go with this approach

@maspe36 do you mean installing ROS or releasing with the generate_docs (or a better name) feature on?

@maspe36
Copy link
Contributor

maspe36 commented Aug 3, 2025

Sorry I should've been more clear, lets just do what you're doing already in this PR and install ROS before attempting to release the crate. Either way though, we probably should rename generate_docs to something like no_ros etc

Signed-off-by: Esteve Fernandez <[email protected]>
@esteve
Copy link
Contributor Author

esteve commented Aug 6, 2025

@maspe36 I've been thinking about the approach in this PR and it think it's not the right one, we could install all the supported ROS distros before release, but in the end we can only publish once and we're already testing all the ROS distros in our regular CI. I'm closing in favor of #14, which renames genrate_docs to use_ros_shim and tells release-plz to only use that feature when publishing to crates.io.

Thanks for reviewing the changes in this PR, can you have a look at #14 ?

@esteve esteve closed this Aug 6, 2025
@esteve esteve deleted the install-ros-release-plz branch August 6, 2025 18:42
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.

3 participants