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

Add support for GNU-compatible printf #120

Merged
merged 2 commits into from
Jan 10, 2022
Merged

Conversation

refi64
Copy link
Contributor

@refi64 refi64 commented Dec 17, 2021

This adds initial support for GNU find-compatible printf syntax. As far
as I can tell, it should implement all the specifiers, with the
following exceptions:

  • Using zero as padding instead of space: GNU find isn't consistent with
    which numeric format specifiers support this.
  • Specifying precision (%x.yz): I believe sparseness (%S)
    and seconds-since-epoch (%A@ / %C@ / %T@) are the only ones that
    allow this, making it only partially useful.
  • Printing SELinux contexts (%Z): requires some extra dependencies and
    isn't within our scope.

uucore is added as a dependency, because it has several utility
functions that we need, the complexity of which is high enough that
using the extra dependency is quite helpful.

This also stubs out many of the specifiers for Windows, simply because I
have no idea how these should work there. (The defaults chosen should be
"good enough" for the moment, though.)

Signed-off-by: Ryan Gonzalez ryan.gonzalez@collabora.com


Okay, so some more meta notes on this:

  • Right now, printing the filesystem doesn't work reliably without: uucore::fsext: Fix mountinfo parsing w/ multiple optional fields coreutils#2778
  • I'm not sure if what I put for the copyright in the new file is correct, please point it out if not!
  • Also not sure if it would be worth removing printer.rs in favor of just putting Printf::new("%p").unwrap() (which I believe should be identical) in the main file.

@refi64 refi64 force-pushed the printf branch 2 times, most recently from 1911818 to 361fad3 Compare December 17, 2021 17:41
@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #120 (dd443b9) into master (d385362) will increase coverage by 2.10%.
The diff coverage is 65.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   47.70%   49.80%   +2.10%     
==========================================
  Files          18       19       +1     
  Lines        2111     2594     +483     
  Branches      495      702     +207     
==========================================
+ Hits         1007     1292     +285     
- Misses        933     1046     +113     
- Partials      171      256      +85     
Impacted Files Coverage Δ
src/find/matchers/delete.rs 79.16% <ø> (ø)
src/find/mod.rs 61.36% <ø> (ø)
tests/find_cmd_tests.rs 70.58% <47.36%> (-18.78%) ⬇️
src/find/matchers/mod.rs 65.78% <50.00%> (-0.43%) ⬇️
src/find/matchers/printf.rs 67.70% <67.70%> (ø)
src/lib.rs 25.54% <0.00%> (+2.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d385362...dd443b9. Read the comment docs.

@wlozano0collabora
Copy link

Very strange test failure, it doesn't seem to be related to this PR.

@refi64
Copy link
Contributor Author

refi64 commented Dec 18, 2021

oh oops I had fixed it locally but forgot to push, just did it now

@sylvestre
Copy link
Sponsor Contributor

Would it be possible to have high level tests?
https://github.com/uutils/findutils/blob/master/tests/find_cmd_tests.rs
thanks

@refi64
Copy link
Contributor Author

refi64 commented Dec 23, 2021

@sylvestre I added some high-level tests in src/find/mod.rs with a lot of the others there, does that look okay?

@refi64 refi64 force-pushed the printf branch 3 times, most recently from b0c5bdf to e2bf11b Compare December 23, 2021 02:07
@sylvestre
Copy link
Sponsor Contributor

@sylvestre I added some high-level tests in src/find/mod.rs with a lot of the others there, does that look okay?

could you please move them into the tests directory? I prefer to have all integration tests at the same place. thanks

@refi64
Copy link
Contributor Author

refi64 commented Dec 23, 2021

@sylvestre Ah I misunderstood, just moved it over and actually testing the find binary itself (not find_main).

@refi64 refi64 force-pushed the printf branch 8 times, most recently from da16f97 to f9ab584 Compare December 23, 2021 18:21
@refi64
Copy link
Contributor Author

refi64 commented Dec 23, 2021

Okay got CI on Windows working again with the moved tests 😅 Should be good now. (I also pointed it to uucore from Git so that it has the mentioned fix for grabbing the filesystem (otherwise %F doesn't work).)

@sylvestre
Copy link
Sponsor Contributor

thanks
Seems that the coverage could be improved:
https://codecov.io/github/uutils/findutils/commit/f9ab5847cf4d41bfc15ed9464218ec6915977641
do you think you could add a few more tests ?

@refi64
Copy link
Contributor Author

refi64 commented Dec 27, 2021

@sylvestre I added a few more tests, with that, though, most of the still-remaining red is things that can't easily be tested in CI due to a high dependence on host system config (e.g. filesystem type, block count, sparseness)

@refi64 refi64 force-pushed the printf branch 3 times, most recently from c6998a7 to eae08ae Compare December 27, 2021 16:16
@refi64
Copy link
Contributor Author

refi64 commented Dec 27, 2021

(and CI is passing now, again)

@sylvestre
Copy link
Sponsor Contributor

fails on ubuntu:

---- find::matchers::printf::tests::test_printf_special_types stdout ----
thread 'find::matchers::printf::tests::test_printf_special_types' panicked at 'called `Result::unwrap()` on an `Err` value: Error { depth: 2, inner: Io { path: Some("/dev/shm/multipath"), err: Os { code: 13, kind: PermissionDenied, message: "Permission denied" } } }', src/find/matchers/mod.rs:433:47
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This adds initial support for GNU find-compatible printf syntax. As far
as I can tell, it should implement all the specifiers, with the
following exceptions:

- Using zero as padding instead of space: GNU find isn't consistent with
  which numeric format specifiers support this.
- Specifying precision (`%x.yz`): I believe sparseness (%S)
  and seconds-since-epoch (`%A@` / `%C@` / `%T@`) are the only ones that
  allow this, making it only partially useful.
- Printing SELinux contexts (`%Z`): requires some extra dependencies and
  isn't within our scope.

uucore is added as a dependency, because it has several utility
functions that we need, the complexity of which is high enough that
using the extra dependency is quite helpful.

This also stubs out many of the specifiers for Windows, simply because I
have no idea how these should work there. (The defaults chosen should be
"good enough" for the moment, though.)

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>
@refi64
Copy link
Contributor Author

refi64 commented Dec 27, 2021

@sylvestre Well in retrospect, trying to use /dev here was a terrible idea 😅 new version uses some custom FIFO/socket files instead in order to test special file types

@sylvestre sylvestre merged commit 81402f3 into uutils:master Jan 10, 2022
@sylvestre
Copy link
Sponsor Contributor

thanks and sorry for the latency :(

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.

3 participants