Skip to content

du: use uutils-args to fix some bugs due to usage of clap #7739

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

du: use uutils-args to fix some bugs due to usage of clap #7739

wants to merge 3 commits into from

Conversation

BenWiederhake
Copy link
Collaborator

This is a proof-of-concept that using uutils-args would measurably improve compatibility with GNU coreutils, while still passing all existing tests.

Context:

However, this probably shouldn't be merged as-is:

  • This PR uses a specific commit of uutils-args, we should probably consider doing releases of uutils-args at some point. Who can decide that? What are the relevant things to consider here?
  • While uutils-args significantly improves a lot of things (i.e. handling early exit due to --help), there are some edge cases it handles wrong: Only sometimes, a late --help should override other errors uutils-args#130 We should resolve this issue first, as it might change the architecture.
  • Before pushing uutils-args on other people, it should become a little nicer with its error messages first: Typo in #[arg()] leads to opaque error message uutils-args#129 I don't know enough about proc macros, can someone help here?
  • At some point in time (now? later?), the signature fn uu_app() -> Command needs to change. See discord for that discussion; I guess there's going to be vivid discussion.

On the other hand, this is a valid approach, and could in theory be used as-is, hence I didn't mark this as "draft".

What do you think?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/du/inodes. tests/du/inodes is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/du/max-depth. tests/du/max-depth is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/du/threshold. tests/du/threshold is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@BenWiederhake
Add UNRELEASED uutils-args as dependency
86fd6c4 
Note that this links directly to a github commit. Obviously we don't
really want this, but uutils-args currently has no releases, especially
not on crates.io, so I'm forced to go this route.
@BenWiederhake
du: demonstrate importance of argument order
a0fe6d3 
This is essentially the test from uutils/uutils-args#114 .
@BenWiederhake
du: parse blocksize and apparent-size using uutils-args
28a0d5f 
Ideally, we should replace *all* argument parsing by clap in one go,
but that is a monumental task. Let's do it one step at a time instead.
@BenWiederhake BenWiederhake force-pushed the dev-use-uutils-args-for-du branch from f4edc12 to 28a0d5f Compare April 16, 2025 02:49
@BenWiederhake BenWiederhake marked this pull request as draft April 16, 2025 02:49
@BenWiederhake
Copy link
Collaborator Author

Many changes since last push. Let's hope I remember them all:

  • This now uses Make 'complete' available to procmacro's output by conversion to module uutils-args#135 , which is necessary because it won't compile otherwise.
  • I now also include -h and --si in the block size parsing, because these three options overwrite each other, as demonstrated by the new test test_du_order_si_h_b.
  • I moved value parsing to the general parsing stage, not afterwards. One effect is that -B banana -B 1 is no longer accepted, identical to GNU behavior.
  • I also found several new edge cases we currently handle wrongly. Some of them are fixed just by switching to uutils-args, see test_du_error_precedence and test_du_blocksize_bytes_order
  • New edge cases that I couldn't fix but needed to interact with are linked or marked with // FIXME.
  • The generated errors are surprisingly bad. I probably did something wrong. The worst offenders are:
    • du: Invalid value '000' for '-B': (note the trailing space and missing continuation after the colon)
    • du: Invalid value 'banana' for '-B': 'banana' (note the pointless duplication)

In other words, this PR can't be merged as-is, but it's a nice demonstration to what uutils-args can do.

@BenWiederhake BenWiederhake mentioned this pull request Apr 16, 2025
Merged
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/du/inodes. tests/du/inodes is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/du/max-depth. tests/du/max-depth is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/du/threshold. tests/du/threshold is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@BenWiederhake