fix b2sum, md5sum and hashsum regressions #8760

sylvestre · 2025-09-28T15:51:14Z

No description provided.

github-actions · 2025-09-28T16:12:33Z

GNU testsuite comparison:

GNU test failed: tests/cksum/md5sum. tests/cksum/md5sum is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!
Congrats! The gnu test tests/cksum/cksum-a is no longer failing!

sylvestre · 2025-09-28T16:38:51Z

src/uucore/src/lib/features/checksum.rs

    BitsRequiredForShake256,
+    #[error("--bits required for SHA2")]
+    BitsRequiredForSha2,
+    #[error("Invalid output size for SHA2 (expected 224, 256, 384, or 512), got {0}")]


i noticed that we aren't translating this file, i will fix it next

github-actions · 2025-09-28T17:22:28Z

GNU testsuite comparison:

GNU test failed: tests/cksum/md5sum. tests/cksum/md5sum is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!
Congrats! The gnu test tests/cksum/cksum-a is no longer failing!

codspeed-hq · 2025-09-28T17:36:49Z

CodSpeed Performance Report

Merging #8760 will not alter performance

_{Comparing sylvestre:cksum (06b8692) with main (52c71dc)}

Summary

✅ 41 untouched
⏩ 64 skipped¹

64 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

github-actions · 2025-09-28T18:19:24Z

GNU testsuite comparison:

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

cakebaker · 2025-09-29T08:14:29Z

tests/by-util/test_cksum.rs

+            .arg("lorem_ipsum.txt")
+            .fails_with_code(1)
+            .no_stdout()
+            .stderr_contains("--bits required for SHA2");


The expected error message is incorrect as there is no --bits option.

Suggested change

.stderr_contains("--bits required for SHA2");

.stderr_contains("--length required for SHA2");

GNU cksum itself shows the following error:

cksum: --algorithm=sha2 requires specifying --length 224, 256, 384, or 512

cakebaker · 2025-09-29T08:29:16Z

tests/by-util/test_cksum.rs

+    }
+
+    #[test]
+    fn test_sha2_with_length_224() {


It might make sense to combine the test_sha2_with_length_xxx tests into a single test function that loops over the valid lengths.

Maybe we could leverage rstest here. It offers parametrized tests (the macro generates all the testcases independently). See https://github.com/la10736/rstest?tab=readme-ov-file#parametrize

cakebaker · 2025-09-29T08:52:44Z

tests/by-util/test_cksum.rs

+        // Should contain base64 characters and SHA256 label
+        assert!(result.contains("SHA256"));
+        // Base64 should end with = or == or contain +/
+        assert!(result.contains('=') || result.contains('+') || result.contains('/'));


I would use a single assert and check for the correct output. The second assert is currently true independent of whether the output is base64 because result.contains('=') matches the delimiter between hash name and hash. And the "or" conditions are not necessary because you know the expected output.

cakebaker · 2025-09-29T09:29:27Z

src/uucore/src/lib/features/checksum.rs

+    #[error("--bits required for SHA2")]
+    BitsRequiredForSha2,
+    #[error("Invalid output size for SHA2 (expected 224, 256, 384, or 512), got {0}")]
+    InvalidSha2Length(usize),


For SHA3 we use InvalidOutputSizeForSha3, and so for consistency reasons I would use a similar name:

Suggested change

InvalidSha2Length(usize),

InvalidOutputSizeForSha2(usize),

RenjiSann · 2025-09-29T10:23:05Z

src/uucore/src/lib/features/checksum.rs

+                    let error_key = format!("{}-error-invalid-length", utility_name);
+                    let max_key = format!("{}-error-max-digest-length", utility_name);
+                    show_error!(
+                        "{}",
+                        translate!(&error_key, "length" => DisplayQuotable::quote(length_str))
+                    );
+                    Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        translate!(&max_key, "algorithm" => "BLAKE2b", "max_bits" => 512),
+                    )
+                    .into())


This part is almost a duplicate of the ones above, there might be a way to factorize it with closures ?

cakebaker · 2025-09-29T12:36:11Z

src/uucore/src/lib/features/checksum.rs


+/// Validate BLAKE2b length with Fluent error messages
+/// This function is used by utilities that need localized error messages
+pub fn validate_blake2b_length(length_str: &str, utility_name: &str) -> UResult<Option<usize>> {


I'm struggling with this function as it duplicates the code of validate_blake2b_length_str and calculate_blake2b_length. And thus it makes it easy to introduce sync issues in the future.

cakebaker · 2025-09-29T12:46:53Z

src/uucore/src/lib/features/checksum.rs

 pub const ALGORITHM_OPTIONS_SM3: &str = "sm3";
 pub const ALGORITHM_OPTIONS_SHAKE128: &str = "shake128";
 pub const ALGORITHM_OPTIONS_SHAKE256: &str = "shake256";
+pub const ALGORITHM_OPTIONS_SHA2: &str = "sha2";


A detail: I would move this line up to line 39, so that it is between the consts for SHA1 and SHA3.

cakebaker

Done :)

sylvestre commented Sep 28, 2025

View reviewed changes

sylvestre force-pushed the cksum branch from 976e107 to 90cdc34 Compare September 28, 2025 16:56

sylvestre added 2 commits September 28, 2025 19:02

b2sum: better validation of the args like in tests/cksum/b2sum

3d52a89

cksum: add support for sha2

06b8692

sylvestre force-pushed the cksum branch from 90cdc34 to 4a76833 Compare September 28, 2025 17:02

sylvestre force-pushed the cksum branch from 4a76833 to 06b8692 Compare September 28, 2025 17:59

sylvestre marked this pull request as ready for review September 29, 2025 07:29

sylvestre requested a review from cakebaker September 29, 2025 07:29

cakebaker reviewed Sep 29, 2025

View reviewed changes

RenjiSann reviewed Sep 29, 2025

View reviewed changes

cakebaker reviewed Sep 29, 2025

View reviewed changes

	.stderr_contains("--bits required for SHA2");
	.stderr_contains("--length required for SHA2");

Uh oh!

fix b2sum, md5sum and hashsum regressions #8760

Are you sure you want to change the base?

fix b2sum, md5sum and hashsum regressions #8760

Uh oh!

Conversation

sylvestre commented Sep 28, 2025

Uh oh!

github-actions bot commented Sep 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 28, 2025

Uh oh!

codspeed-hq bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #8760 will not alter performance

Summary

Footnotes

Uh oh!

github-actions bot commented Sep 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cakebaker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq bot commented Sep 28, 2025 •

edited

Loading