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

all: Undo custom exit codes #6162

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

BenWiederhake
Copy link
Collaborator

This PR:

All in all, we should now pass misc/usage_vs_getopt. Let's see whether CI agrees!

Ping @tertsdiepraam, you touched on this topic a long time ago. What do you think? :)

Copy link

GNU testsuite comparison:

GNU test failed: tests/ls/classify. tests/ls/classify is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/usage_vs_getopt is no longer failing!
GNU test error: tests/misc/usage_vs_getopt. tests/misc/usage_vs_getopt is passing on 'main'. Maybe you have to rebase?

@BenWiederhake
Copy link
Collaborator Author

Hooray! It works. It also reveals yet another inconsistency in GNU ls:

$ ls --invalid
ls: unrecognized option '--invalid'
Try 'ls --help' for more information.
[$? = 2]
$ ls --classify=invalid
ls: invalid argument 'invalid' for '--classify'
Valid arguments are:
  - 'always', 'yes', 'force'
  - 'never', 'no', 'none'
  - 'auto', 'tty', 'if-tty'
Try 'ls --help' for more information.
[$? = 1]

@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • rebased on current main
  • fix exit code for --classify=invalid and all other stringly-typed enum-like arguments
  • fix even the weirdly behaving --time-style

Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/dd/nocache_eof is no longer failing!
Congrats! The gnu test tests/ls/stat-free-symlinks is no longer failing!
Congrats! The gnu test tests/misc/usage_vs_getopt is no longer failing!
Congrats! The gnu test tests/tail/inotify-rotate-resources is no longer failing!
GNU test error: tests/dd/nocache_eof. tests/dd/nocache_eof is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/ls/no-cap. tests/ls/no-cap is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/ls/stat-free-color. tests/ls/stat-free-color is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/ls/stat-free-symlinks. tests/ls/stat-free-symlinks is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/misc/usage_vs_getopt. tests/misc/usage_vs_getopt is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/mv/atomic. tests/mv/atomic is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/mv/atomic2. tests/mv/atomic2 is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/tail/inotify-only-regular. tests/tail/inotify-only-regular is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/tail/inotify-rotate-resources. tests/tail/inotify-rotate-resources is passing on 'main'. Maybe you have to rebase?

@BenWiederhake BenWiederhake marked this pull request as draft April 1, 2024 16:07
@BenWiederhake BenWiederhake marked this pull request as ready for review April 1, 2024 21:36
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

Locally this passes misc/usage_vs_getopt again. Let's see whether CI agrees this time! :)

Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/usage_vs_getopt is no longer failing!
GNU test error: tests/misc/usage_vs_getopt. tests/misc/usage_vs_getopt is passing on 'main'. Maybe you have to rebase?

@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • The sed invocation in util/build-gnu.sh was bad; this fixes it. In particular, the argument detection and removal still needs to be relaxed, but the problem was that it was too aggressive and accidentally removed everything. Whoops. All that in a single character!

@@ -89,6 +89,16 @@ pub fn uu_app() -> Command {
.override_usage(format_usage(USAGE))
.after_help(AFTER_HELP)
.infer_long_args(true)
// Since we use value-specific help texts for "--output-error", clap's "short help" and "long help" differ.
Copy link
Member

@tertsdiepraam tertsdiepraam Apr 1, 2024

Choose a reason for hiding this comment

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

I'm not sure about this change. It's more compatible, sure, but is it better? I want to limit all sorts of edge cases in the code and might prefer disabling that test here. Or can we force the bit for --output-error to always be shown?

Note that this will be fixed by uutils-args too by the way, so maybe it just doesn't matter much either way.

(Don't consider this a blocking review)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't quite understand? All that this piece of code does is force the bit for --output-error to always be shown, just like you suggest.

@BenWiederhake
Copy link
Collaborator Author

Fancy new flake discovered in Windows CI:

Error: HttpError: request to https://api.github.com/repos/mozilla/sccache/releases/latest failed, reason: read ECONNRESET
Error: request to https://api.github.com/repos/mozilla/sccache/releases/latest failed, reason: read ECONNRESET

I guess we're (I am) making too many calls and we're getting rate-limited?

Copy link

github-actions bot commented Apr 1, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/misc/usage_vs_getopt is no longer failing!

@BenWiederhake
Copy link
Collaborator Author

Changes since last push: Nothing, just a rebase to show that it still works.

@tertsdiepraam Since the code already does what you suggest (I probably misunderstand you, please clarify), I didn't change anything :)

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Apr 6, 2024

I wanted it by including the same text in the short help (if possible), instead of forcing the long help for the whole util. Again though, not important enough to block on.

Edit: whoops that message was borderline incomprehensible. Fixed it now.

Copy link

github-actions bot commented Apr 6, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/usage_vs_getopt is no longer failing!

@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Rebased to current main, to show that it still works

Is there anything else needed?

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/usage_vs_getopt is no longer failing!
Congrats! The gnu test tests/timeout/timeout is no longer failing!

@sylvestre sylvestre merged commit 4090d46 into uutils:main Apr 25, 2024
66 checks passed
@sylvestre
Copy link
Contributor

well done

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