-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
uucore: centralize SIGPIPE handling in main macro #10354
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
Conversation
0ac12ab to
4c6a711
Compare
|
GNU testsuite comparison: |
4c6a711 to
f6381bc
Compare
CodSpeed Performance ReportMerging this PR will improve performance by 3.85%Comparing Summary
Performance Changes
Footnotes
|
|
GNU testsuite comparison: |
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.
Pull request overview
This PR centralizes SIGPIPE handling across all uutils utilities by moving the signal setup logic into the #[uucore::main] procedural macro. Previously, each utility had to manually call enable_pipe_errors() to restore SIGPIPE's default behavior. Now this happens automatically in the macro, with utilities only needing to opt-out when they require special SIGPIPE handling.
Changes:
- Centralized SIGPIPE capture and restoration in the
#[uucore::main]proc macro - Added
disable_pipe_errors()function for utilities that need to override default SIGPIPE behavior - Removed redundant SIGPIPE setup code from 7 utilities (yes, tr, timeout, tail, seq, env, cat)
- Added special SIGPIPE handling for 3 utilities with non-standard requirements (tee, tty, split)
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/uucore_procs/src/lib.rs | Added SIGPIPE state capture at module level and restoration logic before user code executes in the main macro |
| src/uucore/src/lib/features/signals.rs | Added disable_pipe_errors() function and updated documentation for enable_pipe_errors() |
| src/uucore/Cargo.toml | Added "signals" feature to default features to enable SIGPIPE handling for all utilities |
| src/uu/yes/src/yes.rs | Removed redundant enable_pipe_errors() call now handled by macro |
| src/uu/tty/src/tty.rs | Added disable_pipe_errors() to handle broken pipes gracefully and exit with code 3 |
| src/uu/tr/src/tr.rs | Removed redundant SIGPIPE setup now handled by macro |
| src/uu/timeout/src/timeout.rs | Removed redundant enable_pipe_errors() call now handled by macro |
| src/uu/tee/src/tee.rs | Changed logic from enable_pipe_errors() when no output-error to disable_pipe_errors() when output-error is set |
| src/uu/tail/src/tail.rs | Removed redundant SIGPIPE setup now handled by macro |
| src/uu/split/src/split.rs | Added conditional disable_pipe_errors() when using --filter option to handle child process stdin closing |
| src/uu/seq/src/seq.rs | Removed manual SIGPIPE capture and restoration now handled by macro |
| src/uu/env/src/env.rs | Removed redundant enable_pipe_errors() call now handled by macro |
| src/uu/cat/src/cat.rs | Removed redundant SIGPIPE setup now handled by macro |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ChrisDryden this is a good change. I'm OK to merge it. Can you please fix the conflict? |
f6381bc to
847fff3
Compare
|
GNU testsuite comparison: |
|
I think the best way to do this is to use the "Fixes" syntax, then github automatically closes the issues when the PR is merged. |
Instead of adding this code to every utility, hoping to make it centralized and then addressing all of the special cases in the individual utilities. There's definitely a chance that a special case is missed and could be a regression, but this should solve a much larger number of issues for all utilities. Fixes uutils#10325 Fixes uutils#10260 Fixes uutils#10230 Fixes uutils#10214 Fixes uutils#9936 Fixes uutils#8919 Fixes uutils#7252 Fixes uutils#4965
Instead of adding this code to every utility, hoping to make it centralized and then addressing all of the special cases in the individual utilities. There's definitely a chance that a special case is missed and could be a regression, but this should solve a much larger number of issues for all utilities.
Fixes #10325
Fixes #10260
Fixes #10230
Fixes #10214
Fixes #9936
Fixes #8919
Fixes #7252
Fixes #4965