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

Major re-org to crate / repository structure #215

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Carter12s
Copy link
Collaborator

@Carter12s Carter12s commented Dec 10, 2024

Description

A major re-organization of the components of roslibrust per: https://github.com/orgs/RosLibRust/discussions/214

Some facets of this:

  • The core roslibrust crate now includes no backends by default. The rosbridge backend is now hidden behind the rosbridge feature flag. This means you don't pay to compile any backend that you aren't using.
  • The rosbridge backend moved under the rosbridge namespace in the roslibrust crate so it is no longer treated as a special backend in the naming conventions.
  • RosApi trait and implementations moved out of roslibrust and into their own crate. Also now generically written so they can be used with any backend.

Fixes

Closes: number

Checklist

  • Update CHANGELOG.md

@Carter12s Carter12s changed the title [DRAFT] Major re-org to split out roslibrust_common and ros1 Major re-org to crate structure Dec 17, 2024
@Carter12s Carter12s changed the title Major re-org to crate structure Major re-org to crate / repository structure Dec 17, 2024
@Carter12s Carter12s requested review from ssnover and lucasw December 17, 2024 18:58
@Carter12s
Copy link
Collaborator Author

@lucasw / @ssnover this is ready for a review now.

It is a pretty big change to overall crate structure, but I think aligns things much more where I want development to head.

I think it sets things up well to eventually include a ros2 backend with either zenoh or dds, and I think it sets us up very well to start making the generic traits for TopicProvider and ServiceProvider the a first class API that both allows folks to swap backends around and to use MockRos for testing.

@ssnover
Copy link
Collaborator

ssnover commented Dec 17, 2024

Reading on mobile right now, but just going to ask clarifying questions and I'll review later:

  • Is the intended entry point crate roslibrust_common now?
  • Should it maybe be called roslibrust_core?

@Carter12s
Copy link
Collaborator Author

For users the intended entrypoint is still roslibrust

The "standard" usage would be to depend on roslibrust with at least one backend enabled via a feature flag.

Depending on roslibrust with no backends enabled can be done (see rosapi) to implement generic behaviors without a particular backend specified as well.

Not expecting anyone outside of roslibrust development to depend on roslibrust_common and I'm super happy to rename that to roslibrust_core or potentially roslibrust_traits <- that is most specifically what is actually inside it at the moment.

Comment on lines 36 to 38
# TODO MAJOR remove this feature
rosapi = []
# Intended for use with tests, includes tests that rely on a locally running rosbridge
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Flagging for myself I still need to clean this up before merge

Copy link
Collaborator

@ssnover ssnover left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks and questions about tracking todos.

example_package/Cargo.toml Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@
#[cfg(feature = "ros1_test")]
mod tests {
use log::*;
use roslibrust::ros1::{NodeError, NodeHandle};
use roslibrust_ros1::{NodeError, NodeHandle};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since integration tests will have similar visibility to internals as user code, I think it's probably best for the integration test to look like what we expect it to look like for users; i.e. keep roslibrust::ros1::

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh looking at this now, I see this is under src instead of tests which is where integration tests ought to go (since they have the same visibility into the crate as a user of the crate does).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch I'll re-org!

roslibrust_common/src/lib.rs Outdated Show resolved Hide resolved
@@ -197,7 +194,7 @@ impl NodeServerHandle {

let md5sum;
let md5sum_res =
roslibrust_codegen::message_definition_to_md5sum(topic_type, msg_definition);
roslibrust_common::md5sum::message_definition_to_md5sum(topic_type, msg_definition);
match md5sum_res {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an aside, but perhaps:

let md5sum = roslibrust_common::md5sum::message_definition_to_md5sum(
    topic_type,
    msg_definition
).map_err(|err| {
    log::error!("{err:?}");
    Err(NodeError::IoError(io::ErrorKind::ConnectionAborted.into()))
})?;

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice to change the name of message_definition_to_md5sum to from_message_definition since it's already namespaced under md5sum module.

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