-
Notifications
You must be signed in to change notification settings - Fork 7
Relative Module Path Resolution #12
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
base: main
Are you sure you want to change the base?
Conversation
…:` ones. Reworked the rosidl_generator_rs slightly to be a bit simpler. Separate actual templates from the files that reuse them.
| [package.metadata.rclrs] | ||
| reexport = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional metadata I am exporting.
| type FeedbackMessage = super::@(subfolder)::@(type_name)@(ACTION_FEEDBACK_MESSAGE_SUFFIX); | ||
| type SendGoalService = super::@(subfolder)::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX); | ||
| type CancelGoalService = action_msgs::srv::rmw::CancelGoal; | ||
| type GetResultService = crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX); | ||
| type GetResultService = super::@(subfolder)::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the changes look like this, simply replacing crate:: with super::. However, when we referenced external to this module types, (i.e. action_msgs::srv::rmw::CancelGoal) we will need to depend on another mechanism such as a using statement or pulling it in as a separate crate to resolve the module path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still ultimately want this structure, but a crate that wishes to re-export any of these symbols will need to arbitrarily add private use statements for any external modules. Consider something like this
pub mod builtin_interfaces { ... };
pub mod std_msgs {
pub mod msg {
use crate::builtin_interfaces;
include!("path/to/generated/code");
pub mod rmw {
use crate::builtin_interfaces;
include!("path/to/generated/rmw/code");
}
}
}We need to create this rmw module in the generated crates lib.rs file rather than within these generated .rs files directly. This turned out to be more complicated than I bargained for. This was the simplest way I found to get this to export where relative symbols were looked up correctly in both a single crate or a re-export build.
| # Edition 2024+ | ||
| 'gen', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to call attention to this new keyword
| # BoundedSequences can be in the idiomatic API, but the containing type cannot be from the | ||
| # idiomatic API because we do not implement SequenceAlloc for idiomatic types. | ||
| return f'rosidl_runtime_rs::BoundedSequence<{get_rs_type(type_.value_type, current_idiomatic, False)}, {type_.maximum_size}>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really tripped me up. It seems odd that we can have idiomatic types that then hold very non-idiomatic values :D There is nothing in the stdlib to act like a bounded sequence, so we'd need to pull in a dep I believe to get "idiomatic types"
Changed all generated code to use relative module paths (namely,
super::) instead of absolutecrate::onesReworked the rosidl_generator_rs slightly to be a bit simpler.
Separate actual templates from the files that invoke them.
Added some additional package metadata which can be used downstream.
This change should not change any of the functionality of the generated code. Things should still be build-able as crates. This change just gives more flexibility in how we consume messages downstream and opens the door to things like re-exporting symbols under another crate.