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

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

Merged
merged 15 commits into from
May 18, 2024
Merged

Conversation

sylvestre
Copy link
Contributor

No description provided.

@sylvestre sylvestre force-pushed the cksum-check branch 2 times, most recently from e0becb8 to 6b8ce37 Compare May 10, 2024 07:19
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)

@sylvestre sylvestre force-pushed the cksum-check branch 3 times, most recently from c053907 to 40e00b6 Compare May 12, 2024 08:47
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

src/uu/cksum/Cargo.toml Outdated Show resolved Hide resolved
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)

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 force-pushed the cksum-check branch 2 times, most recently from 5c118cd to 415d655 Compare May 12, 2024 20:46
@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

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