-
Notifications
You must be signed in to change notification settings - Fork 79
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
Improved error handling and documentation #23
Conversation
f0f3a8d
to
58d4936
Compare
src/lib.rs
Outdated
pub fn raise_ctrl_c() { | ||
unsafe { | ||
// This will signal the whole process group. | ||
assert!(self::kernel32::GenerateConsoleCtrlEvent(self::winapi::CTRL_C_EVENT, 0) != 0); |
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.
Ok, this doesn’t work in CMD. We share the console and a process group with cargo, which means cargo also receives the CTRL+C event. I need to find a way to generate a CTRL+C for the current process only, and not the whole group. It doesn’t look like the console API supports this, but I am looking at a workaround.
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.
Maybe some uglyish workaround with launching a new process using new console and running tests there? I wonder if there's a way to check whether the binary is running as a subprocess of Cargo or not. Or then just disable the tests for Windows for the time being.
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.
Yea, I am looking at workaround where we detach from current console and open our own while we run our tests, but I am having trouble getting stdout to work in the new console.
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.
This should be fixed now. I have created a custom test harness for running tests on a secondary console. We should also setup appveyor for CI on windows.
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.
Awesome work! Unfortunately it seems I won't have time to go through this this week. I'll try to merge this (including Appveyor) next week.
One of my motivations behind this pull request is that it allows us to implement something like this #28. |
3a34558
to
1adcb41
Compare
- Avoid panics. - Cleanup resources on error. - Document errors. - Document SIGTERM. - `no_run` examples. - Use nix crate instead of raw libc bindings. - Use std::io::Error for system errors and errno. - Simplify Error enum. - Use sigaction() instead of signal().
- Run tests on a secondary console on windows. - Workaround for GenerateConsoleCtrlEvent() signaling the whole process group. - Enforces serialization and deterministic order for tests. - Uses SetStdHandle(). This breaks Rust's stdout for Rust pre 1.18.0.
1adcb41
to
9f91508
Compare
Improved error handling and documentation
This tries to address some problems with how errors are currently being handled.
Changes:
SIGTERM
, documentation for SIGTERM handling? #21.no_run
examples.