-
Notifications
You must be signed in to change notification settings - Fork 416
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
Implement ctrlc in place of signal hook on Windows #1123
Conversation
ec88336
to
70e917d
Compare
Many thanks for the contribution! |
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.
Would it please be possible to add a CI job to make sure the new code builds as expected on Windows?
Should note that there is no Windows job with --features metrics_process flag because Prometheus doesn't work on Windows and I didn't want to introduce more complexity just for that |
Cargo.toml
Outdated
tiny_http = { version = "0.12", optional = true } | ||
|
||
[target.'cfg(windows)'.dependencies] | ||
ctrlc = "3.2.5" |
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.
Just from curiosity, why not using 3.4.2 ctrlc
?
IIUC it should support our MSRV: https://github.com/Detegr/rust-ctrlc/blob/3.4.2/Cargo.toml#L15
e7cb794
to
217b475
Compare
Since this is a binary project, we should use the latest compatible dependency versions. Updated:
This follows Rust's MSRV policy where MSRV bumps are not considered breaking changes. |
e7f0abe
to
3bede84
Compare
3bede84
to
42b987d
Compare
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.
Many thanks!
Satisfies #709, utilizing ctrlc in place of signal hook on Windows only.