-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
env: adds --ignore-signal #6377
Conversation
GNU testsuite comparison:
|
I have fixed styling issues and handled STOP signal and fixed styling issues |
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.
Hi, welcome to uutils! :D
- We try to match GNU behavior closely, so please always check what GNU is doing. It's okay to deviate intentionally if GNU does something suboptimally, or when we offer an extension. However, these cases should be intentional.
- coreutils/uutils is (or will be) used at the heart of the operating system. As such, we strive for high reliability. Part of that is carefully considering each use of
unsafe
, and having thorough testing. Therefore, please always write a few tests to check the main functionality. Additionally, I outlined a handful of bugs that the PR would introduce in its current state. Please use these examples as starting points for tests. - You may also want to take another look at the development and contributing guidelines.
I hope I didn't discourage you. Keep going! :)
No not at all, I really appreciate your encouragement, regarding testing I will add tests today and check code coverage today or tomorrow at the latest. |
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.
Yup, getting better :)
- There still are zero tests. I have lost count of all the things that need to be tested, please fix this.
- Formatting issues and a stray
#[cfg(unix)]
, see CI failures for details. They're there to help you :) - Plus some small things, see below
Sorry for being late, been pretty busy at uni |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
Nearly there! :)
|
I looked at fn test_env_arg_ignore_signal_valid_signals() {
let mut child = new_ucmd!()
.args(&["--ignore-signal=usr1,int,usr2", "sleep", "25"])
.run_no_wait();
let id = child.id();
child.delay(500);
Command::new("kill")
.args(&["-usr1", &format!("{}", id)])
.spawn()
.expect("failed to send signal");
child.delay(100);
assert_eq!(child.is_alive(), true);
Command::new("kill")
.args(&["-usr2", &format!("{}", id)])
.spawn()
.expect("failed to send signal");
child.delay(100);
assert_eq!(child.is_alive(), true);
Command::new("kill")
.args(&["-int", &format!("{}", id)])
.spawn()
.expect("failed to send signal");
child.delay(100);
assert_eq!(child.is_alive(), true);
Command::new("kill")
.args(&["-kill", &format!("{}", id)])
.spawn()
.expect("failed to send signal");
child.delay(100);
assert_eq!(child.is_alive(), false);
} |
Yes, your approach (call sleep, send a signal, check whether it's still running) seems like a good idea. (At least I can't think of a better way.) And yes, please refactor it somehow, ideally with a new |
This commit implement uutils#6164.
8e571ac
to
a4513d2
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.
LGTM, let's see what the CI says …
I looked at CI and these are the errors
Potential Fixes |
349a5d2
to
11a14e8
Compare
You're absolutely right, and thanks for fixing it ^^ In "Potential Fix 2", you removed Windows-support entirely from env. That's fair, Windows doesn't have the same concept, and I don't think there's anything comparable. CI is complaining about a missed opportunity to use
I think that will be the last thing. Thanks for sticking with it! :) |
264de43
to
136acf5
Compare
Thank you for your kind comments and guidance, I had a a lot of fun working on this! |
LGTM, CI is green, and as far as I can see it tests for (nearly) every possible scenario, except #6353, which is unfixable for now. Excellent! :D |
This commit implement #6164.