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

Shutdown explicitly while existing #623

Merged
merged 6 commits into from
Dec 24, 2024

Conversation

evshary
Copy link
Contributor

@evshary evshary commented Oct 18, 2024

🦟 Bug fix

Summary

Shutdown explicitly after creating an entity.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Sorry, something went wrong.

Signed-off-by: ChenYing Kuo <[email protected]>
Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Just checking, do we need to call shutdown() before the previous return calls as well?

Signed-off-by: ChenYing Kuo <[email protected]>
@evshary
Copy link
Contributor Author

evshary commented Oct 25, 2024

Just checking, do we need to call shutdown() before the previous return calls as well?

Good catch! I believe it should be.

Copy link
Contributor

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Instead of multiple rclcpp::shutdown() calls, we could wrap this call inside a callback that will automatically execute once the program terminates by relying on rcpputils::make_scope_exit().

@azeey
Copy link
Contributor

azeey commented Dec 16, 2024

@evshary would you be able to apply @Yadunund's suggestion?

Signed-off-by: ChenYing Kuo <[email protected]>
@evshary
Copy link
Contributor Author

evshary commented Dec 17, 2024

@Yadunund Thanks for your suggestion. I didn't know the tip before. 👍

@evshary
Copy link
Contributor Author

evshary commented Dec 17, 2024

@azeey Do you have any clue why Rpr__ros_gz__ubuntu_noble_amd64 is failing?
I'm not sure why it complains

13:49:54 Traceback (most recent call last):
13:49:54   File "/tmp/ros_buildfarm/scripts/devel/create_devel_task_generator.py", line 21, in <module>
13:49:54     run_module('ros_buildfarm.scripts.devel.create_devel_task_generator', run_name='__main__')
13:49:54   File "<frozen runpy>", line 229, in run_module
13:49:54   File "<frozen runpy>", line 88, in _run_code
13:49:54   File "/tmp/ros_buildfarm/ros_buildfarm/scripts/devel/create_devel_task_generator.py", line 304, in <module>
13:49:54     sys.exit(main())
13:49:54              ^^^^^^
13:49:54   File "/tmp/ros_buildfarm/ros_buildfarm/scripts/devel/create_devel_task_generator.py", line 140, in main
13:49:54     get_binary_package_versions(apt_cache, debian_pkg_names))
13:49:54     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13:49:54   File "/tmp/ros_buildfarm/ros_buildfarm/common.py", line 180, in get_binary_package_versions
13:49:54     raise KeyError("No packages available for '%s'" % (debian_pkg_name,))
13:49:54 KeyError: "No packages available for 'ros-rolling-gz-sim-vendor'"

@azeey
Copy link
Contributor

azeey commented Dec 17, 2024

@ros-pull-request-builder retest this please

@azeey
Copy link
Contributor

azeey commented Dec 17, 2024

@evshary Yes, there was a regression in gz-common due to a change in gz-math. We've made a release of gz-common today, but it'll probably take a couple days for the change to propagate.

Copy link
Contributor

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Add rcpputils to package.xml as <depend>

@Yadunund
Copy link
Contributor

Add rcpputils to package.xml as <depend>

I hit send before finishing my comment 🤦🏼

We should also include the appropriate header file and update CMakeLists.txt to explcitly link the target against rcpputils. I'm assuming this compiles right night through transitive linking against dependences rclcpp exports but it would be good to make this explicit.

Signed-off-by: ChenYing Kuo <[email protected]>
@evshary
Copy link
Contributor Author

evshary commented Dec 20, 2024

Done. I've added the dependency and also header file / CMakefiles.txt

Copy link
Contributor

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for iterating!

@ahcorde ahcorde enabled auto-merge (squash) December 20, 2024 08:43
@ahcorde
Copy link
Collaborator

ahcorde commented Dec 24, 2024

https://github.com/Mergifyio update

Copy link
Contributor

mergify bot commented Dec 24, 2024

update

☑️ Nothing to do

  • #commits-behind > 0 [📌 update requirement]
  • -closed [📌 update requirement]
  • -conflict [📌 update requirement]
  • queue-position = -1 [📌 update requirement]

Sorry, something went wrong.

@ahcorde
Copy link
Collaborator

ahcorde commented Dec 24, 2024

https://github.com/Mergifyio backport jazzy

Copy link
Contributor

mergify bot commented Dec 24, 2024

backport jazzy

✅ Backports have been created

@ahcorde ahcorde merged commit 550ef7b into gazebosim:ros2 Dec 24, 2024
4 checks passed
mergify bot pushed a commit that referenced this pull request Dec 24, 2024
Signed-off-by: ChenYing Kuo <[email protected]>
(cherry picked from commit 550ef7b)
ahcorde pushed a commit that referenced this pull request Dec 24, 2024
Signed-off-by: ChenYing Kuo <[email protected]>
(cherry picked from commit 550ef7b)

Co-authored-by: ChenYing Kuo (CY) <[email protected]>
@evshary evshary deleted the shutdown_explicitly branch December 24, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants