-
Notifications
You must be signed in to change notification settings - Fork 127
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
Build system refactor #64
Conversation
It appears the changes to the generator that are required for H-Turtle/Rolling aren't backwards-compatible. Is there a way we can deal with this? |
Managed to fix it. This should be good for review! |
Allows IDE to parse dependencies. Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
the compilation syntax Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
a27a5f8
to
bd6ffd2
Compare
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
rclrs/Cargo.toml
Outdated
@@ -13,6 +16,10 @@ core-error = "0.0.0" | |||
parking_lot = {version = "0.11.2", optional = true} | |||
spin = "0.9.2" | |||
|
|||
[dependencies.rclrs_common] | |||
version = "*" | |||
path = "../rclrs_common" |
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.
Is there a way to avoid this? It'd be unfortunate because it's bypassing the ROS dependency resolution mechanism, but it makes rust-analyzer
, it's fine as a compromise. Or maybe, it'd just make sense to integrate rclrs_common
into rclrs
. What do you think?
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.
I had been debating merging the two crates for some time. I will give that a shot and see how it goes.
I am doing something similar in rclrs_examples
as well. I'm assuming it's a problem there as well? What I'd like to do is figure out some way to make ROS 2's dependency resolution work, while also allowing cargo
the freedom to build crates as it likes, since that's what most IDEs will be expecting...
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.
Okay, that took quite a bit more effort than I was expecting, but I have managed to combine most of rclrs_common
with rclrs
. The message traits had to be spun off into their own crate, otherwise we ended up with circular build dependencies.
I can do a bit more thinking on how to avoid the explicit paths in the manifest. Perhaps we should use an auto-generated .cargo/config.toml
file, such as the one that I use for the std_msgs
dependencies?
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
format 3 schema Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
@@ -0,0 +1,19 @@ | |||
<?xml version="1.0"?> |
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.
Correct me if I'm wrong, but it seems that rclrs_msg_utilities
serves a similar purpose as rclrs_common
, that is, code shared between rclrs
and downstream packages / generators. Would it make sense to rework rclrs_common
to include the additions from rclrs_msg_utilities
?
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.
It cannot, as best as I am able to tell. Every time I try, it introduces a circular build dependency.
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.
Oh, sorry, misunderstood your question. So basically what rclrs_msg_utilities
is is just rclrs_common
stripped of everything that doesn't have to do with messages. My goal with it is to eventually use it to help with message generation via CMake.
Since I'm not fully sure what the plan is anymore - We were going to switch over to r2r
, but then I got comments saying that some other people preferred the CMake implementation...
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Please stand by, working on a solution to having explicit paths in the manifests for the packages... |
Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
Okay, should be refactored now. Basically, you feed in the packages you want (and listed in the |
@jhdcs thank you so much! Looks great 👍 |
Closes #61, Closes #34, Closes #38, Closes #31, Closes #13
I have refactored the build system so that things can be build in a more Rust-like way: The biggest surprise people may have is that the
rclrs_examples
crate will generate a.cargo/config.toml
file in order to patch in the file dependency forstd_msgs
.Do the changes look acceptable? While I'm still not super-fond of the dependency search we have to do for
rclrs_examples
, I'm sure that we can find something else to make that easier in another PR.