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

Implement tee -p #3656

Merged
merged 4 commits into from
Jul 8, 2022
Merged

Implement tee -p #3656

merged 4 commits into from
Jul 8, 2022

Conversation

eds-collabora
Copy link
Contributor

This amounts to enabling pipe errors when -p is not specified, but only on unixish platforms.

This addresses issue #3648

@sylvestre
Copy link
Sponsor Contributor

Could you please add a test ?
See grep pipe tests/by-util/* for similar tests, thanks!

@sylvestre
Copy link
Sponsor Contributor

ping?

@eds-collabora
Copy link
Contributor Author

I've been looking into it; it's a bit more complicated to test, and even more complicated to match GNU tee's behaviour. I do have a fairly complete implementation with tests now, include the extended --output-error option, but I've been fighting with clap to permit an optional value with --output-error. So for example, this command fails:

cat test | tee --output-error foo bar

because clap wants a value for --output-error.

I have fixed the termination/error behaviour to match GNU tee more closely however; this is the last failing test in my set.

@eds-collabora
Copy link
Contributor Author

I should've rebased before pushing; will fix.

@eds-collabora eds-collabora force-pushed the eds/tee_p branch 2 times, most recently from f6f0451 to 04002a5 Compare June 23, 2022 22:04
@eds-collabora
Copy link
Contributor Author

I believe I've fixed the majority of the test failures reported last time:

  • I've masked out my helper functions for Linux only, so that unused function warnings won't fire on other platforms. That also fixes the unhappiness on Windows that my test functions produced with their pipe handling.
  • I ran rustfmt on test_tee (oversight, sorry)
  • I've downgraded clap back to 3.1, to fix the test failure for MinRustV. This downgrade also fixes test_base32 (which broke because the exact text emitted by clap changed).

@eds-collabora
Copy link
Contributor Author

Three failures in the last batch:

  • spellcheck
  • unused imports (fixed by putting everything in a module)
  • windows coverage test seems to have crashed on startup

I'm hoping I've fixed the first two.

@eds-collabora
Copy link
Contributor Author

@sylvestre I don't understand how the remaining two errors relate to this code:

  • tail-2/F-headers appears to be failing, but it passes for me locally with this code. Is this something to do with the runner's environment?
  • Coverage on windows is failing with warnings about undefined references to crossbeam.

I'm not sure how to address these.

@sylvestre
Copy link
Sponsor Contributor

They aren't your fault. For grcov, see mozilla/grcov#849
for the other, it is an intermittent issue
sorry about the noise!

@sylvestre
Copy link
Sponsor Contributor

See that GNU tee.sh is still failing. Do you know why ?
bash util/run-gnu-test.sh tests/misc/tee.sh to run it

@eds-collabora
Copy link
Contributor Author

See that GNU tee.sh is still failing. Do you know why ? bash util/run-gnu-test.sh tests/misc/tee.sh to run it

I've had a quick look, and should be able to fix. There are still quite a few things wrong:

  • I've fixed the "abort on no viable writers" problem that's the most immediate cause of errors (tee is expected to, but the coreutils version wasn't).
  • I've finally discovered that many of the pipe tests fail, because yes is being remapped to the coreutils version. The test expects yes to exit with a pipe error on a broken pipe, and it uses this to detect the correct exit code. Since the coreutils version of yes traps sigpipe (unlike the GNU one), the correct exit code was being detected as 0. This meant all pipe error tests were spuriously failing.
  • I am not sure that the behavior for unwriteable files matches tee, and I think this is the remaining problem.

I'm investigating.

@eds-collabora
Copy link
Contributor Author

I'm not sure all these commits belong in here, and perhaps parts are a little questionable. But I did make the GNU tests for tee pass with these commits. Their main value is just to show the gap between the first commit and a fully passing GNU test set.

  • The main problem to overcome is the unexpected behaviour of yes and timeout (which I have not fixed completely, and I would need to investigate the unit test failure for yes further to see if I caused it - out of time for now).

  • I've added a candidate fix for the apparently fairly longstanding issue in tee that it didn't exit on all writers being unavailable.

  • The fix to timeout is the most dubious of all. It's key to making the tests pass however, because as timeout exists at the moment it does not match the behaviour of the GNU timeout very closely at all with respect to exit status. In particular, it doesn't preserve signal exit codes, it remaps them to exit codes. That leads to some pretty weird and hair pulling effects when you run the GNU tests (because bash will remap signal codes to code + 128, so you get an exit from your program that would be 141, but is now 13 after its wrapped in timeout).

@sylvestre
Copy link
Sponsor Contributor

sylvestre commented Jun 26, 2022

Indeed:

Warning: Congrats! The gnu test tests/misc/tee is no longer failing!

Well done!

@eds-collabora
Copy link
Contributor Author

I believe the remaining test fails are not related to these patches.

I'm happy to modify this PR, whatever makes sense from your perspective. I can drop the extra fixes, or move them into their own PRs, as desired (or leave them here, obviously).

@sylvestre
Copy link
Sponsor Contributor

sorry but it seems that the gnu test regressed:
2022-06-30T13:40:35.4833526Z FAIL: tests/misc/tee.sh

@eds-collabora
Copy link
Contributor Author

sorry but it seems that the gnu test regressed: 2022-06-30T13:40:35.4833526Z FAIL: tests/misc/tee.sh

I'm not sure what happened here. If I run this test locally with what is supposedly the same code I get a test pass for tee. The failure log is empty in the logs from CI (just a nonzero exit code). Any ideas what the variation could be from?

This has the following behaviours. On Unix:

- The default is to exit on pipe errors, and warn on other errors.

- "--output-error=warn" means to warn on all errors

- "--output-error", "--output-error=warn-nopipe" and "-p" all mean
  that pipe errors are suppressed, all other errors warn.

- "--output-error=exit" means to warn and exit on all errors.

- "--output-error=exit-nopipe" means to suppress pipe errors, and to
  warn and exit on all other errors.

On non-Unix platforms, all pipe behaviours are ignored, so the default
is effectively "--output-error=warn" and "warn-nopipe" is identical.
The only meaningful option is "--output-error=exit" which is identical
to "--output-error=exit-nopipe" on these platforms.

Note that warnings give a non-zero exit code, but do not halt writing
to non-erroring targets.
When the monitored process exits, the GNU version of timeout will
preserve its exit status, including the signal state.

This is a partial fix for timeout to enable the tee tests to pass.  It
removes the default Rust trap for SIGPIPE, and kill itself with the
same signal as its child exited with to preserve the signal state.
This is part of fixing the tee tests. 'yes' is used by the GNU test
suite to identify what the SIGPIPE exit code is on the target
platform. By trapping SIGPIPE, it creates a requirement that other
utilities also trap SIGPIPE (and exit 0 after SIGPIPE). This is
sometimes at odds with their desired behaviour.
tee is supposed to exit when there is nothing left to write to. For
finite inputs, it can be hard to determine whether this functions
correctly, but for tee of infinite streams, it is very important to
exit when there is nothing more to write to.
@sylvestre
Copy link
Sponsor Contributor

back
Warning: Congrats! The gnu test tests/misc/tee is no longer failing!

@sylvestre sylvestre merged commit 05823dd into uutils:main Jul 8, 2022
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.

None yet

2 participants