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.

Sign up for GitHub

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

Jump to bottom

Let XML executables/nodes be "required" (like in ROS 1) (#751) #764

Merged
merged 1 commit into from Mar 20, 2024
Merged

Let XML executables/nodes be "required" (like in ROS 1) (#751) #764

merged 1 commit into from Mar 20, 2024

Conversation

Timple
Copy link

@Timple Timple commented Mar 5, 2024

Backport of #751 for ROS iron

Thanks @m-elwin for the works!

@m-elwin @Timple
Let XML executables/nodes be "required" (like in ROS 1) (ros2#751)
e8714b9 
* Let XML nodes be "required"

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

* Added tests for yaml

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
@fujitatomoya
Copy link

@mjcarroll can you review this and run CI?

@mjcarroll mjcarroll self-assigned this Mar 14, 2024
mjcarroll
mjcarroll approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM with Green CI

@mjcarroll
Copy link
Member

mjcarroll commented Mar 14, 2024 •
edited

CI running against iron

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@Timple
Copy link
Author

Timple commented Mar 15, 2024

Flake8 complains: ./launch/launch_context.py:24:1: F401 'typing.Mapping' imported but unused.
But it's not part of this PR. Would you like me to fix it, or is this out of scope?

@mjcarroll
Copy link
Member

Would you like me to fix it, or is this out of scope?

Let's go ahead and fix it here so we can keep CI green.

@Timple
Copy link
Author

Timple commented Mar 18, 2024

Now the tests are stating:

/launch/launch_context.py:66:40: F821 undefined name 'Mapping'

Which I removed because 'Mapping' was indicated as unused import.
I hope this can be considered out of scope for this PR as I don't have an idea how to solve this 🙁 Locally all tests are happy.

@mjcarroll mjcarroll merged commit adef012 into ros2:iron Mar 20, 2024
3 checks passed
@Timple
Copy link
Author

Timple commented Apr 4, 2024

@Yadunund since you did the last release, do you know what the policy is for getting a new release on iron? 🙂

@Yadunund
Copy link
Member

Yadunund commented Apr 4, 2024

Version bumps to core packages (including launch) would be part of a patch release and I was aiming to do the next one around 19th April.

@Timple
Copy link
Author

Timple commented Apr 4, 2024

Cool 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

@methylDragon methylDragon Awaiting requested review from methylDragon methylDragon is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees

@mjcarroll mjcarroll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Timple @fujitatomoya @mjcarroll @Yadunund @m-elwin