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.

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

cksum: Improve the GNU compat #6256

Merged
merged 6 commits into from
May 7, 2024
Merged

cksum: Improve the GNU compat #6256

merged 6 commits into from
May 7, 2024

Conversation

sylvestre
Copy link
Contributor

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

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

@sylvestre
Copy link
Contributor Author

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

nice :)

@cakebaker
Copy link
Contributor

Did you see that test_check_b2sum_length_duplicate fails on Windows?

@github-actions GitHub Actions
Copy link

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!

@sylvestre sylvestre force-pushed the hash-error1 branch 3 times, most recently from 4a468ac to 3650dba Compare April 28, 2024 15:37
@github-actions GitHub Actions
Copy link

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!

@sylvestre sylvestre requested a review from cakebaker April 28, 2024 16:27
BenWiederhake
BenWiederhake requested changes Apr 28, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
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 Outdated Show resolved Hide resolved
tests/by-util/test_cksum.rs Outdated Show resolved Hide resolved
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 Outdated Show resolved Hide resolved
@sylvestre
Copy link
Contributor Author

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 :)

@sylvestre
Copy link
Contributor Author

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

@sylvestre sylvestre requested a review from BenWiederhake April 28, 2024 19:38
@BenWiederhake
Copy link
Collaborator

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 sylvestre force-pushed the hash-error1 branch 2 times, most recently from f32e625 to cdf959d Compare April 28, 2024 19:54
@sylvestre
Copy link
Contributor Author

i am not worried but sure :)

BenWiederhake
BenWiederhake requested changes Apr 28, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

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

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 Outdated Show resolved Hide resolved
tests/by-util/test_cksum.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Contributor Author

Still the check thing

I will fix this in a different PR

@github-actions GitHub Actions
Copy link

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 GitHub Actions
Copy link

github-actions bot commented May 2, 2024

GNU testsuite comparison:

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

@sylvestre sylvestre force-pushed the hash-error1 branch 2 times, most recently from b57203c to d4b9bc0 Compare May 2, 2024 22:20
@github-actions GitHub Actions
Copy link

github-actions bot commented May 2, 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/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

BenWiederhake
BenWiederhake requested changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

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 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 Show resolved Hide resolved
tests/by-util/test_cksum.rs Outdated Show resolved Hide resolved
@BenWiederhake BenWiederhake mentioned this pull request May 6, 2024
Closed
@sylvestre sylvestre force-pushed the hash-error1 branch 3 times, most recently from 39d1713 to b4e2d59 Compare May 6, 2024 21:44
@github-actions GitHub Actions
Copy link

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

@sylvestre sylvestre requested a review from BenWiederhake May 7, 2024 06:36
@sylvestre
Copy link
Contributor Author

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
BenWiederhake requested changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

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 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
@sylvestre sylvestre mentioned this pull request May 7, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

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)

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

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
BenWiederhake approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

@BenWiederhake BenWiederhake merged commit eb6f1df into uutils:main May 7, 2024
67 of 68 checks passed
@BenWiederhake BenWiederhake mentioned this pull request May 7, 2024
Closed
@cakebaker cakebaker mentioned this pull request May 16, 2024
Closed
@BrewTestBot BrewTestBot mentioned this pull request Jun 23, 2024
Merged
@moonfruit moonfruit mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWiederhake BenWiederhake BenWiederhake approved these changes

@cakebaker cakebaker Awaiting requested review from cakebaker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sylvestre @cakebaker @BenWiederhake