Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cmake infrastructure for creating components #784
Cmake infrastructure for creating components #784
Changes from 5 commits
b39d28e
07e04f5
d321e9a
773f0a3
bb4ca14
d7ef517
61d9b17
26fcfc2
f2c4594
08474d1
5e7b834
83a7350
1ffc637
46f410b
81b7ee2
fd32199
e47c90a
3b156a2
4074c75
7f05d46
8e05921
793675d
5aaee75
b257a8d
aeb0d18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@skucheria @jacobperron why do we remove ROS arguments and pass non-ROS arguments to
NodeOptions
? I don't follow. Just for context, I'm having issues with argument validation as whatever is fed here is decidedly not a ROS argument.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 motivation was some of our demos take non-ROS arguments via the command-line. When we went turn them into components, it wasn't obvious how to pass along these non-ROS arguments to the code.
What's the recommended way to get non-ROS arguments?
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.
For example, image_tools parses options by passing the nodes arguments to this function:
https://github.com/ros2/demos/blob/97651912ebad439a6bc62c34671cad6a5bfee785/image_tools/src/options.cpp#L46
IMO, the demos should be refactored to make use of ROS parameters instead of custom CLI arguments (but that's beside the point).
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.
So, we had a discussion about this on #816 and ros2/rclpy#405 (comment). As a result, all
NodeOptions::arguments()
are implicitly ROS arguments. To achieve what you want here, you'd have to do-- non-ros-arg0 ... non-ros-argN
, which is inconvenient but we were operating under the assumption that passing non-ROS arguments to a ROS node was a rare use case.It looks like it's not @wjwwood @ivanpauno. Unless either of you strongly disagrees, I'll force
--ros-args
to be given explicitly in bothrclpy
andrclcpp
.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.
👍 to explicit
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 think that's reasonable. I had forgotten this case.
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 it then possible for
--ros-args
to clash with non-ros args? E.g. what if my node expects a CLI argument named-p
?Does
NodeOptions.arguments()
potentially return something like"--ros-args -p foo:=bar -- -p my_non_ros_arg"
?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.
Currently,
--ros-args
was prepended except if it was already part of the arguments.@hidmic sent a bunch of PRs for changing that, so
--ros-args
won't be prepended more.NodeOptions.arguments({"-p"})
will set a user specific argument.NodeOptions.arguments({"--ros-args", "-p"})
will set a ros specific argument.IDK if that's exactly what you asked or not.
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.
@jacobperron even with the changes I'm making, that could be the case, yes. But here you're removing ROS arguments and thus
--ros-args ... --
sets will be removed. That also means that no--ros-args
will reach your node as local arguments (they may still make it as global arguments) with this implementation, but it doesn't look like it cares about it.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.
Thanks for the explanation.