search

gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.21k stars 484 forks source link

Add SIGINT signal handlers for Windows #3167

Closed Ace314159 closed 1 year ago

Ace314159 commented 2 years ago

This makes ctrl+c more reliable at actually killing the process.

Ace314159 commented 2 years ago

Is there anything else I should do to get this PR merged?

Ace314159 commented 2 years ago

@j-rivero I'd really appreciate a review, thank you!

j-rivero commented 2 years ago

Hey @Ace314159, sorry for the radio silence. I was looking into yes, code changes looks good to me but I don't have a working Windows environment these days. I'm going to approve but let's wait until someone can verify that indeed improves the runtime shutdown.

Thanks for the contribution!

traversaro commented 2 years ago

I restarted the CI to check if the failure were flaky.

Ace314159 commented 2 years ago

Do you think the failures are caused by my changes? I'm having trouble actually looking at the failing tests. Most of them just say that the test didn't run.

talregev commented 1 year ago

Can you rebase to the latest gazebo11 dev branch?

Ace314159 commented 1 year ago

@talregev I've rebased

talregev commented 1 year ago

@traversaro Can you review it?

traversaro commented 1 year ago

@traversaro Can you review it?

Sure, as @j-rivero mentioned it would be nice to verify that this indeed improve the ctrl+c behavior on Windows. @talregev did you tested this improvements yourself? If yes, I think we can proceed.

talregev commented 1 year ago

@Ace314159 Can you record your screen while you press ctrl+c and post it here? I will also try to do it when I have time and post it here.

talregev commented 1 year ago

@traversaro I build gazebo with vcpkg for x86 on this branch, and I can confirm that when I press crtl+c, it close all windows immediately. I am using windows 11.

@traversaro Let me know what do you think.

https://github.com/gazebosim/gazebo-classic/assets/7043539/b105be23-50e1-446b-b596-3a131b234aeb

traversaro commented 1 year ago

Perfect, thanks! I will wait two days for anyone else to comment, then I will merge. Feel free to ping then, thanks!

talregev commented 1 year ago

@traversaro gentle ping about this PR.

traversaro commented 1 year ago

Thanks!