nohup: added a bunch of nohup fixes based on the failures in the GNU tests #9628
Conversation
| GNU testsuite comparison: |
ChrisDryden force-pushed the nohup_gnu_fixes branch from 316faef to f8ddc57 Compare 
CodSpeed Performance ReportMerging #9628 will not alter performanceComparing Summary
Footnotes
|
| GNU testsuite comparison: |
| .append(true) | ||
| .open(Path::new(NOHUP_OUT)) | ||
| { | ||
| Ok(t) => { |
There was a problem hiding this comment.
When implementing this, it doesn't appear that this matches the GNU implementation, I have added integration tests here and this is not something I came across when trying to replicate the GNU functionality
There was a problem hiding this comment.
I have this line in the Err on the success of this overall function, not just for the open options
| GNU testsuite comparison: |
ChrisDryden force-pushed the nohup_gnu_fixes branch from f8ddc57 to 3dd84fa Compare | GNU testsuite comparison: |
| #[error("{}", translate!("nohup-error-open-failed-both", "first_path" => NOHUP_OUT.quote(), "first_err" => _1, "second_path" => _2.quote(), "second_err" => _3))] | ||
| OpenFailed2(i32, #[source] Error, String, Error), | ||
| | ||
| #[error("")] |
There was a problem hiding this comment.
could you please add a comment why it is empty "" ?
2 participants
This is a bit of a bigger PR but the original goal was to try and add the support of always setting the umask of the output file and when running the tests against the GNU tests I found a few inconsistencies that I'm going to summarize below:
First is that the GNU implementation will always try to set umask of nohop.out to 600 which we werent doing in our tests, I added a wrapper to it to make it always set that way.
Second the error message is based on whether the input or/and the output terminal is a terminal, the current implementation printed the same error message for all use cases and I added error messages specific to each use case.
Third, the error code for failure is set based on the ENV variable POSIXLY_CORRECT which we were already doing in some cases but we need to do that for all cases where it would return a 125 failure
Lastly the show_error macro didn't work for this use case because its possible to fail the output to stdout and then its expected to give a different error code based on whether the stderr was written successfully.
I added rust integ tests for each of these scenarios too, the way the tests are set up in nohup is a bit all over the place, when this gets merged in I'd like to make them all use the same macros and reduce the verbosity a bit