cksum: Improve the GNU compat by sylvestre · Pull Request #6256 · uutils/coreutils

sylvestre · 2024-04-21T09:35:27Z

Should make tests/cksum/cksum-a.sh pass

tests/by-util/test_cksum.rs

github-actions · 2024-04-21T10:09:24Z

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/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

sylvestre · 2024-04-21T10:48:46Z

Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

nice :)

cakebaker · 2024-04-21T11:15:38Z

Did you see that test_check_b2sum_length_duplicate fails on Windows?

github-actions · 2024-04-21T12:47:03Z

GNU testsuite comparison:

Congrats! The gnu test tests/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!
Congrats! The gnu test tests/timeout/timeout is no longer failing!

tests/by-util/test_hashsum.rs

github-actions · 2024-04-28T16:11:02Z

GNU testsuite comparison:

Congrats! The gnu test tests/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

BenWiederhake

This fixes some GNU tests, yes, but also has two major downsides currently:

The code would now falsely claim that files verify with their checksum, without ever actually checking the checksum (VERY bad!)
There are some questionable conditions around --tag and --untagged

Details below.

tests/by-util/test_cksum.rs

src/uu/cksum/src/cksum.rs

tests/by-util/test_cksum.rs

src/uu/cksum/src/cksum.rs

sylvestre · 2024-04-28T19:08:49Z

The code would now falsely claim that files verify with their checksum, without ever actually checking the checksum

I am planning to implement it next :)

tested by gnu/tests/cksum/b2sum.sh

sylvestre · 2024-04-28T19:38:33Z

../gnu/tests/cksum/cksum-c.sh will be next :)

BenWiederhake · 2024-04-28T19:45:55Z

I'm really worried about the --check implementation getting lost in the weeds, or someonw using an intermediate implementation. How about adding a simple if options.check { unimplemented!(); } right after the error handling?

sylvestre · 2024-04-28T19:55:30Z

i am not worried but sure :)

BenWiederhake

Still the check thing, and another iteration on the tag/untagged/binary thing.

src/uu/cksum/src/cksum.rs

tests/by-util/test_cksum.rs

sylvestre · 2024-04-28T20:24:26Z

Still the check thing

I will fix this in a different PR

github-actions · 2024-04-28T20:55:22Z

GNU testsuite comparison:

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

github-actions · 2024-05-02T21:47:21Z

GNU testsuite comparison:

Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

github-actions · 2024-05-02T22:49:24Z

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/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

BenWiederhake

Oh boy, argument parsing sure is finicky.

Migrating away from clap and instead using uutil-args would make this trivially easy, because this sequential "one argument after another" logic seems to be what all GNU utils do under the hood.

src/uu/cksum/src/cksum.rs

tests/by-util/test_cksum.rs

github-actions · 2024-05-06T22:14:31Z

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/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

sylvestre · 2024-05-07T08:02:44Z

I will implement check next. I started but I don't want extend this PR more.

And yes, it is doing hand parsing of the argument but clap doesn't work for this (see the comments) and with our move to the different library, I don't see the point of spending more time on this :)

BenWiederhake

You know what, I think I give up on the binary/text/tag/untagged situation. It's good enough for now, I'll just create an issue once this PR lands, and hope that it doesn't get forgotten to time (like the other ~290 open issues).

Can I ask you to fix the typo, though?

src/uu/cksum/src/cksum.rs

github-actions · 2024-05-07T11:51:58Z

GNU testsuite comparison:

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

github-actions · 2024-05-07T12:20:25Z

GNU testsuite comparison:

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

BenWiederhake

LGTM

(I'm a bit unhappy about the commit subject "handle a corner case", but I think this PR has already been iterated to death, so I don't wanna torture you by requesting yet another change.)

EDIT: Remaining CI failure is #6275 and #6333, as usual.

sylvestre commented Apr 21, 2024

View reviewed changes

tests/by-util/test_cksum.rs Outdated Show resolved Hide resolved

cakebaker reviewed Apr 21, 2024

View reviewed changes

tests/by-util/test_hashsum.rs Show resolved Hide resolved

sylvestre requested a review from cakebaker April 28, 2024 16:27

BenWiederhake requested changes Apr 28, 2024

View reviewed changes

sylvestre added 4 commits April 28, 2024 21:29

hashsum: allow multiple usage of -l and take the last one

835caeb

tested by gnu/tests/cksum/b2sum.sh

cksum: add binary and prepare the rest

a41f925

cksum: handle an error like GNU

04c0fdc

cksum: handle a corner case

a3aee48

sylvestre requested a review from BenWiederhake April 28, 2024 19:38

BenWiederhake requested changes Apr 28, 2024

View reviewed changes

src/uu/cksum/src/cksum.rs Outdated Show resolved Hide resolved

src/uu/cksum/src/cksum.rs Outdated Show resolved Hide resolved

src/uu/cksum/src/cksum.rs Show resolved Hide resolved

tests/by-util/test_cksum.rs Outdated Show resolved Hide resolved

BenWiederhake requested changes May 2, 2024

View reviewed changes

BenWiederhake mentioned this pull request May 6, 2024

cksum: Reverse-engineering implicit behavior of text/binary tag/untagged #6364

Closed

sylvestre requested a review from BenWiederhake May 7, 2024 06:36

BenWiederhake requested changes May 7, 2024

View reviewed changes

src/uu/cksum/src/cksum.rs Outdated Show resolved Hide resolved

src/uu/cksum/src/cksum.rs Outdated Show resolved Hide resolved

src/uu/cksum/src/cksum.rs Show resolved Hide resolved

cksum: --tag & --untagged - the order of usage matters. manage it

5972afb

sylvestre mentioned this pull request May 7, 2024

" --binary --tag --untagged --binary" should display the asterisk #6375

Closed

cksum: add an ignored test for issue 6375

49e2b3d

BenWiederhake approved these changes May 7, 2024

View reviewed changes

BenWiederhake merged commit eb6f1df into uutils:main May 7, 2024

BenWiederhake mentioned this pull request May 7, 2024

cksum: --check is vulnerable, doesn't actually check #6379

Closed

cakebaker mentioned this pull request May 16, 2024

cksum: -b shouldn't be a shortcut of --base64 #6278

Closed

BrewTestBot mentioned this pull request Jun 23, 2024

uutils-coreutils 0.0.27 Homebrew/homebrew-core#175400

Merged

moonfruit mentioned this pull request Jun 24, 2024

uutils-selected 0.0.27 moonfruit/homebrew-tap#114

Merged

Uh oh!

Conversation

sylvestre commented Apr 21, 2024

Uh oh!

Uh oh!

github-actions bot commented Apr 21, 2024

Uh oh!

sylvestre commented Apr 21, 2024

Uh oh!

cakebaker commented Apr 21, 2024

Uh oh!

github-actions bot commented Apr 21, 2024

Uh oh!

Uh oh!

github-actions bot commented Apr 28, 2024

Uh oh!

BenWiederhake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sylvestre commented Apr 28, 2024

Uh oh!

sylvestre commented Apr 28, 2024

Uh oh!

BenWiederhake commented Apr 28, 2024

Uh oh!

sylvestre commented Apr 28, 2024

Uh oh!

BenWiederhake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sylvestre commented Apr 28, 2024

Uh oh!

github-actions bot commented Apr 28, 2024

Uh oh!

github-actions bot commented May 2, 2024

Uh oh!

github-actions bot commented May 2, 2024

Uh oh!

BenWiederhake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 6, 2024

Uh oh!

sylvestre commented May 7, 2024

Uh oh!

BenWiederhake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 7, 2024

Uh oh!

github-actions bot commented May 7, 2024

Uh oh!

BenWiederhake left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

BenWiederhake left a comment •

edited

Loading