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

Refactor signal handling in yes, tee, and timeout #4588

Merged
merged 1 commit into from Mar 26, 2023

Conversation

anastygnome
Copy link
Contributor

@anastygnome anastygnome commented Mar 23, 2023

Edit : Original title was "yes: remove redundant dependency"

Yes, using libc while using nix was a bit redundant.

@sylvestre
Copy link
Sponsor Contributor

I wonder if we could write something (clippy extension?) to detect this kind of pattern

@anastygnome anastygnome changed the title Refactor yes to remove libc dependency yes: Refactor to remove libc dependency Mar 23, 2023
@tertsdiepraam
Copy link
Member

I wonder if we could write something (clippy extension?) to detect this kind of pattern

I've been thinking about whether could get rid of using libc directly. It should be fairly easy to track if we remove the libc re-export from uucore and just check which utils mention libc in the Cargo.toml. nix covers most cases where we currently use libc and we could contribute missing functionality to nix directly. Like I've done here: nix-rust/nix#1805

@anastygnome
Copy link
Contributor Author

anastygnome commented Mar 23, 2023

The CI fails on 66f6d22 seem unrelated. Can you take a look @tertsdiepraam, looks like a touch issue and some weird syscalls thing on android.

@tertsdiepraam
Copy link
Member

Yeah the touch issue is known and the android issue seems unrelated.

@anastygnome
Copy link
Contributor Author

anastygnome commented Mar 24, 2023

I'll also take a look at

fn enable_pipe_errors() -> Result<()> {

and
fn enable_pipe_errors() -> std::io::Result<()> {

@anastygnome
Copy link
Contributor Author

@tertsdiepraam thanks for the review. Should I put this function in uucore as it is duplicated three times over?

@tertsdiepraam
Copy link
Member

Should I put this function in uucore as it is duplicated three times over?

Yeah I think that makes sense

@anastygnome anastygnome changed the title yes: Refactor to remove libc dependency Refactor signal handling in yes, tee, and timeout Mar 24, 2023
@anastygnome
Copy link
Contributor Author

anastygnome commented Mar 24, 2023

@sylvestre gnu git mirror seems unavailable.
@tertsdiepraam This prevents the tests

src/uucore/src/lib/features/signals.rs Outdated Show resolved Hide resolved
src/uucore/src/lib/features/signals.rs Outdated Show resolved Hide resolved
@tertsdiepraam
Copy link
Member

The CI now seems to work. We'll check in later with the GNU test suite.

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/cp/cp-i is no longer failing!
Congrats! The gnu test tests/cp/debug is no longer failing!
Congrats! The gnu test tests/du/apparent is no longer failing!
Congrats! The gnu test tests/misc/cksum-base64 is no longer failing!
Congrats! The gnu test tests/misc/cksum-raw is no longer failing!
Congrats! The gnu test tests/misc/tee is no longer failing!
Congrats! The gnu test tests/misc/wc-total is no longer failing!
Congrats! The gnu test tests/mv/i-1 is no longer failing!
Congrats! The gnu test tests/mv/i-5 is no longer failing!
GNU test failed: tests/cp/sparse-perf. tests/cp/sparse-perf is passing on 'main'. Maybe you have to rebase?

@tertsdiepraam
Copy link
Member

Those failing GNU test might be due to the bump to GNU coreutils 9.2, you probably don't need to worry about them.

@anastygnome
Copy link
Contributor Author

anastygnome commented Mar 25, 2023

hmm?

the trait uucore::error::UError is not implemented for `nix::errno::Errno

ah seems like most of UError is only implemented for linux and android

Any issue on mac though?

@anastygnome anastygnome marked this pull request as draft March 25, 2023 14:52
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@tertsdiepraam
Copy link
Member

That's unfortunate. I think nix should work for all unix platform, so feel free to change the cfg.

@anastygnome
Copy link
Contributor Author

The errors seem unrelated to this PR

Yes, using libc while using nix was a bit redundant.
Upon investigation, duplicated code was found and moved to uucore.
@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Let's get this merged.

@tertsdiepraam tertsdiepraam merged commit 9991ab2 into uutils:main Mar 26, 2023
37 of 44 checks passed
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

3 participants