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

install: implement copying from streams #6893

Closed
wants to merge 21 commits into from

Conversation

DaringCuteSeal
Copy link
Contributor

This PR resolves #5080 and #4387 by copying cat's logic to copy from stream to a file.

However, I'm not sure about testing the cases for each stream type (other than standard input which I had provided in the test). I think it's better to be done in a future refactor where the logic for copying files (and streams) are combined into a single common utility.

@DaringCuteSeal
Copy link
Contributor Author

yikes, seems like it's failing on Android. i'll look into that

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@DaringCuteSeal DaringCuteSeal marked this pull request as draft November 27, 2024 22:23
@DaringCuteSeal DaringCuteSeal force-pushed the install-stdin branch 2 times, most recently from ca83797 to 4169fb0 Compare November 27, 2024 22:49
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@DaringCuteSeal DaringCuteSeal marked this pull request as ready for review November 28, 2024 00:44
@DaringCuteSeal
Copy link
Contributor Author

okay fixed the unit test, the Android emulator seem to have /dev/stdin missing so I used /dev/fd/0 instead. should be good now

@DaringCuteSeal DaringCuteSeal force-pushed the install-stdin branch 3 times, most recently from e7fde9c to 4167374 Compare November 28, 2024 13:51
@DaringCuteSeal
Copy link
Contributor Author

oh yeah forgot to ask again. are there anything else i need to add or remove?

@sylvestre
Copy link
Contributor

looks good to me :)
Could we refactor this code to share it with cat?

@DaringCuteSeal
Copy link
Contributor Author

Could we refactor this code to share it with cat?

Maybe we put splice.rs inside uucore. That good?

@DaringCuteSeal
Copy link
Contributor Author

DaringCuteSeal commented Dec 13, 2024

@sylvestre hey i'd love this to get merged since this bug is literally making my kernel disappear after every kernel upgrade 🤣

the pattern install /dev/stdin seem to be a lot more common than i thought--it's also part of Arch Linux's pacman hook for the Linux kernel packages.

I will look into squashing stream-copying functions into uucore--maybe also using it for cp since as of now it doesn't support copying special files (differs from GNU's coreutils).

@sylvestre
Copy link
Contributor

Yeah
Sorry, I was waiting for this :) thanks

Splice is a Linux-specific syscall that allows direct data copying from
one file descriptor to another without user-space buffer. As of now, this
is used by `cp`, `cat`, and `install` when compiled for Linux and Android.
@sylvestre
Copy link
Contributor

i guess you noticed that a few jobs are failing :)

@DaringCuteSeal
Copy link
Contributor Author

DaringCuteSeal commented Dec 17, 2024

i guess you noticed that a few jobs are failing :)

sorry, dont have like 10 virtual machines yet to check my code :} are CI checks discouraged here?

also for the gazillion commits, i will squash them together

@sylvestre
Copy link
Contributor

nope, this is fine :)

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@sylvestre
Copy link
Contributor

once it will be ready to be merged, it would be nice to separate the uucore changes into a separate PR to make the change more atomic
thanks :)

@DaringCuteSeal
Copy link
Contributor Author

once it will be ready to be merged, it would be nice to separate the uucore changes into a separate PR to make the change more atomic

Sure

@DaringCuteSeal
Copy link
Contributor Author

@sylvestre the newly made splice module depends on the pipes module from uucore, how do i add to its dependency??

@sylvestre
Copy link
Contributor

in the feature - features = ["splice"] } ?

@DaringCuteSeal
Copy link
Contributor Author

uh, i'm assuming you mean for each of the utilities? or am i misunderstanding something? i did have that but for some reason i can't pass the FreeBSD tests

@sylvestre
Copy link
Contributor

what is the error message ?

@DaringCuteSeal
Copy link
Contributor Author

DaringCuteSeal commented Dec 17, 2024

     Compiling uucore v0.0.28 (/home/runner/work/coreutils/coreutils/src/uucore)
  error[E0432]: unresolved imports `crate::pipes::splice`, `crate::pipes::splice_exact`
    --> src/uucore/src/lib/features/splice.rs:18:26
     |
  18 | use crate::pipes::{pipe, splice, splice_exact};
     |                          ^^^^^^  ^^^^^^^^^^^^ no `splice_exact` in `features::pipes`
     |                          |
     |                          no `splice` in `features::pipes`
     |
     = help: consider importing this module instead:
             crate::features::splice
  note: found an item that was configured out
    --> src/uucore/src/lib/features/pipes.rs:38:8
     |
  38 | pub fn splice(source: &impl AsFd, target: &impl AsFd, len: usize) -> Result<usize> {
     |        ^^^^^^
  note: the item is gated here
    --> src/uucore/src/lib/features/pipes.rs:37:1
     |
  37 | #[cfg(any(target_os = "linux", target_os = "android"))]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  note: found an item that was configured out
    --> src/uucore/src/lib/features/pipes.rs:48:8
     |
  48 | pub fn splice_exact(source: &impl AsFd, target: &impl AsFd, len: usize) -> Result<()> {
     |        ^^^^^^^^^^^^
  note: the item is gated here
    --> src/uucore/src/lib/features/pipes.rs:47:1
     |
  47 | #[cfg(any(target_os = "linux", target_os = "android"))]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  For more information about this error, try `rustc --explain E0432`.

this help is also misleading since that's just itself :p

     = help: consider importing this module instead:
             crate::features::splice

i dont think i understand how rust crates work lol

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@DaringCuteSeal
Copy link
Contributor Author

nvm works now. my bad, i wrote unix instead of linux & android

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@DaringCuteSeal
Copy link
Contributor Author

i'm opening separate PRs i guess

@sylvestre
Copy link
Contributor

yeah, it will help with the review, thanks :)

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.

install: Accept special files like /dev/stdin
2 participants