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

Don't register SIGTERM handler on Windows #153

Merged
merged 1 commit into from
Dec 28, 2020
Merged

Conversation

subfuzion
Copy link
Member

@subfuzion subfuzion commented Dec 28, 2020

Fixes #151

Registering a SIGTERM handler on Windows raises an exception.

@kevmoo PTAL. Things to consider: this explicitly checks for Windows, but it might be better to just try to catch the exception and then log that the SIGTERM handler couldn't be registered for the host OS.

Before merging this, we should get someone with Windows expertise to weigh in. I left this comment in the issue, copied here for convenience:

While the exception would seem to indicate that SIGTERM isn't supported, I found this old Microsoft doc that seems to indicate that SIGTERM is supported. 🤷🏻‍♂️

This might or might not be the appropriate fix. There might be an issue with Dart? According to this Microsoft doc, Windows includes SIGTERM for ANSI compatibility:

The SIGILL and SIGTERM signals are not generated under Windows. They are included for ANSI compatibility.

Copy link

@YazeedAlKhalaf YazeedAlKhalaf left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
Tried the changes on my Windows machine and it works!

@subfuzion
Copy link
Member Author

@YazeedAlKhalaf Thanks for confirming. I'd still like to get at least one more review from either @kevmoo or @grant just to make sure they also think this is the "right" fix.

Copy link
Collaborator

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

We should do a follow-up to at least run a sanity check of all tests on windows

@subfuzion subfuzion merged commit af9496f into main Dec 28, 2020
@subfuzion subfuzion deleted the windows-sigterm branch December 28, 2020 21:29
@subfuzion
Copy link
Member Author

We should do a follow-up to at least run a sanity check of all tests on windows

We should be able to do this with a GitHub CI build ... a dedicated job that only runs a set of dart tests without the bash dependencies we've got (like tool/ci.sh).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example not working on Windows 😁
4 participants