8000 cksum: implement check (Closes: #5705) by sylvestre · Pull Request #6390 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cksum: implement check (Closes: #5705) #6390

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

Merged
merged 15 commits into from
May 18, 2024
Merged

cksum: implement check (Closes: #5705) #6390

merged 15 commits into from
May 18, 2024

Conversation

sylvestre
Copy link
Contributor

No description provided.

Copy link

GNU testsuite comparison:

GNU test failed: tests/cksum/cksum-a. tests/cksum/cksum-a is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/cksum/cksum-c is no longer failing!

Copy link

GNU testsuite comparison:

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

@sylvestre
Copy link
Contributor Author

should fail but getting close

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

Copy link

GNU testsuite comparison:

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

@sylvestre sylvestre marked this pull request as ready for review May 12, 2024 11:17
@sylvestre
Copy link
Contributor Author

coreutils/coreutils@b5ce9fb I contributed this upstream as they were missing

Copy link

GNU testsuite comparison:

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

Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/cksum/cksum-c 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
Copy link

GNU testsuite comparison:

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

@sylvestre sylvestre requested a review from cakebaker May 12, 2024 21:04
Copy link

GNU testsuite comparison:

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

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.

I found a few places where we produce the wrong result, and even a panic :)

I'm weirdly proud of finding so much. Clearly it's already good code, it passes the GNU tests and even some that you created for them, and yet there are a lot of interesting edge cases! :D

Comment on lines 493 to 494
io::ErrorKind::Other,
"no properly formatted checksum lines found",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reason, this does not always trigger for me:

$ ../gnu/src/cksum -a blake2b README.md | tee foo.sums
BLAKE2b (README.md) = c0a230cfdbbb686f91071119c915a19df28cb2e4fc575f19d5c6b81bf4e3ca2e83e6df49f14d8b89ece14eb4090be435a7af6fed2c981774e076caa8fe62b04c
$ ../gnu/src/cksum -a sha1 -c foo.sums 
../gnu/src/cksum: foo.sums: no properly formatted checksum lines found
[$? = 1]
$ cargo run -q cksum -a sha1 -c foo.sums 
cksum: WARNING: 1 line is improperly formatted

I suspect that this has to do with -a sha1 being interpreted as "every line MUST use the specified algorithm, even if tagged otherwise" or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This bug still exists. We definitely shouldn't accept files if they aren't correct.

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

Copy link

GNU testsuite comparison:

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

@sylvestre
Copy link
Contributor Author

@BenWiederhake I added tests on our side (ignored) for stuff that we don't support yet (and probably quite niche)
As GNU doesn't have tests either, i proposed that we land this PR + i will contribute these tests upstream.
thanks!

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.

Most of the problems I addressed are indeed fixed, and I mostly agree that it's a good idea to move most of the remainder for later. Hooray!

There are two exceptions that I think should be fixed in this PR:

  • The new-ish issue SHA4 (README.md) = 00000000, see above
  • The bit-rounding-down issue, which causes us to accept dubious and obviously-wrong hashes. This should be just a simple div-mod, right?

Things that should be fixed eventually, but perhaps not right now:

  • "accept files with explicit algo but algos mismatch" details
  • inconsistent behavior caused by the inclusion of b in the algo-name regex:
$ echo 'bSD (README.md) = 0000' > foo.sums; cargo run -q cksum -c foo.sums
README.md: FAILED
cksum: WARNING: 1 computed checksum did NOT match
[$? = 1]
$ echo 'BsD (README.md) = 0000' > foo.sums; cargo run -q cksum -c foo.sums
cksum: foo.sums: no properly formatted checksum lines found
cksum: WARNING: 1 line is improperly formatted
[$? = 1]

@sylvestre
Copy link
Contributor Author

Done :)

inconsistent behavior caused by the inclusion of b in the algo-name regex:
fixed too

Copy link

GNU testsuite comparison:

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

@sylvestre sylvestre requested a review from BenWiederhake May 18, 2024 06:02
@sylvestre sylvestre changed the title cksum: implement check cksum: implement check (Closes: #5705) May 18, 2024
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.

No more crashes, no more false-positives in the checking, and I couldn't find any new edge cases in the new code. Yay! This is ready to merge in my eyes.

@BenWiederhake
Copy link
Collaborator

Remaining failures seem to be the usual (#6275, #6333, and that new "year 1677" thing on mac), so it's as green as it can be, currently.

@BenWiederhake BenWiederhake merged commit b718f95 into uutils:main May 18, 2024
66 of 68 checks passed
@sylvestre sylvestre deleted the cksum-check branch May 26, 2024 16:54
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
0