cksum: implement -a #4356

howjmay · 2023-02-13T13:01:27Z

Closes #3812

src/uu/cksum/Cargo.toml

sylvestre · 2023-02-13T14:01:41Z

I know it is a draft and we will squash them but could you please make the commit message a bit more explicit :)

howjmay · 2023-02-13T15:01:37Z

I know it is a draft and we will squash them but could you please make the commit message a bit more explicit :)

I'm sorry. Let me squash them and present a little better commit first.

github-actions · 2023-02-13T18:04:34Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!
GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T04:42:03Z

GNU testsuite comparison:

GNU test failed: tests/misc/cksum. tests/misc/cksum is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T04:53:42Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!
GNU test failed: tests/misc/cksum. tests/misc/cksum is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T04:58:50Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!
GNU test failed: tests/misc/cksum. tests/misc/cksum is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T06:41:27Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!
GNU test failed: tests/misc/cksum. tests/misc/cksum is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T07:47:57Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

github-actions · 2023-02-14T07:58:56Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

github-actions · 2023-02-14T09:10:34Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

github-actions · 2023-02-14T09:37:52Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

github-actions · 2023-02-14T10:46:21Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!
GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T13:33:44Z

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T13:39:56Z

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

github-actions · 2023-02-14T13:52:59Z

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

Implement option -a --algorithm. Move digest to src/uucore/src/lib/features and rename it to hash. fix lint fix Cargo.toml

github-actions · 2023-02-18T14:03:04Z

GNU testsuite comparison:

Congrats! The gnu test tests/misc/timeout is no longer failing!

tertsdiepraam

Since it's almost completely implemented in uucore now, maybe we could make the abstraction less leaky? What I mean is that hashsum and cksum do not have to depend on all the separate crates anymore if uucore exposes wrappers around the types from all the separate crates. For instance:

// Note that the field is private, so can't be constructed directly from cksum or hashsum
pub struct Sha1(sha1::Sha1);

impl Digest for Sha1 { ... }

Then detect_algo also just becomes:

match program {
    "sha1" => Sha1::new(),
    ...
}

Where new has the return type Box<dyn Digest>. Do you think that's possible? Ultimately that might even be an interesting crate to publish separately (though not in this PR).

github-actions · 2023-02-19T11:20:03Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

sylvestre · 2023-02-21T10:15:00Z

src/uucore/src/lib/features/sum.rs

@@ -0,0 +1,486 @@
+// spell-checker:ignore memmem algo


please add the license header

tertsdiepraam

I like how the abstraction turned out! I have a few more suggestions, but at some point we should probably just merge this and continue improving it in other PRs.

src/uu/cksum/Cargo.toml

src/uu/cksum/src/cksum.rs

src/uucore/src/lib/features/sum.rs

tertsdiepraam · 2023-02-21T17:02:54Z

The object safety thing is annoying. I tried some things to get rid of it. What worked is to never box it, because we don't really have to. Instead we can do this:

fn cksum_algo<'a, I>(algorithm: &str, files: I) -> UResult<()>
where
    I: Iterator<Item = &'a OsStr>,
{
    match algorithm {
        "sysv" => cksum::<_, SYSV>("SYSV", files),
        "bsd" => cksum::<_, BSD>("BSD", files),
        ...
}

fn cksum<'a, I, D>(algo_name: &'static str, files: I) -> UResult<()>
where
    I: Iterator<Item = &'a OsStr>,
    D: Digest
{ ... }

pub trait Digest {
    const OUTPUT_BITS: usize;
    const OUTPUT_BYTES: usize = (Self::OUTPUT_BITS + 7) / 8;
    fn new() -> Self;
    fn hash_update(&mut self, input: &[u8]);
    fn hash_finalize(&mut self, out: &mut [u8]);
    fn result_str(&mut self) -> String {
        let mut buf: Vec<u8> = vec![0; Self::OUTPUT_BYTES];
        self.hash_finalize(&mut buf);
        encode(buf)
    }
}

But I can also do this after this PR is merged.

tertsdiepraam

Alright, it's time to wrap this up. Like I said, the trait might need a bit of a redesign, but that's not really related to this PR. I have one final comment and then it's good in my opinion.

src/uu/cksum/src/cksum.rs

howjmay · 2023-02-22T03:10:45Z

The object safety thing is annoying. I tried some things to get rid of it. What worked is to never box it, because we don't really have to. Instead we can do this:

fn cksum_algo<'a, I>(algorithm: &str, files: I) -> UResult<()>
where
    I: Iterator<Item = &'a OsStr>,
{
    match algorithm {
        "sysv" => cksum::<_, SYSV>("SYSV", files),
        "bsd" => cksum::<_, BSD>("BSD", files),
        ...
}

fn cksum<'a, I, D>(algo_name: &'static str, files: I) -> UResult<()>
where
    I: Iterator<Item = &'a OsStr>,
    D: Digest
{ ... }

pub trait Digest {
    const OUTPUT_BITS: usize;
    const OUTPUT_BYTES: usize = (Self::OUTPUT_BITS + 7) / 8;
    fn new() -> Self;
    fn hash_update(&mut self, input: &[u8]);
    fn hash_finalize(&mut self, out: &mut [u8]);
    fn result_str(&mut self) -> String {
        let mut buf: Vec<u8> = vec![0; Self::OUTPUT_BYTES];
        self.hash_finalize(&mut buf);
        encode(buf)
    }
}

But I can also do this after this PR is merged.

Cool I can give it a try in another PR.

tertsdiepraam · 2023-02-22T08:37:36Z

src/uu/cksum/src/cksum.rs

+        "sha512" => ("SHA512", Box::new(Sha512::new()) as Box<dyn Digest>, 512),
+        "blake2b" => ("BLAKE2", Box::new(Blake2b::new()) as Box<dyn Digest>, 512),
+        "sm3" => ("SM3", Box::new(Sm3::new()) as Box<dyn Digest>, 512),
+        _ => panic!("unknown algorithm"),


One more thing. This is probably not a great error to show to the user. We can restrict the values clap accepts with Arg::possible_values, so that this panic will never be reached.

but I need a default option in match.

Yeah the _ case will need to be there still, but we can change it to unreachable!("unknown algorithm: clap should have prevented this case")

tertsdiepraam · 2023-02-22T08:39:41Z

I'll do that change to Digest because by experimenting I already basically have a branch ready. I'll ping you for review though, I'll be interested to see your feedback on it.

howjmay · 2023-02-22T13:41:45Z

Thank you!

tertsdiepraam · 2023-02-22T20:14:24Z

src/uu/cksum/src/cksum.rs

+                .long(options::ALGORITHM)
+                .short('a')
+                .help("select the digest type to use. See DIGEST below")
+                .value_name("ALGORITHM"),


Here you can add .value_parser(["bsd", "sysv", ...]) to actually restrict the arguments. Tha't's what I was referring to in the other comment. Here's an example of du doing the same thing: https://github.com/uutils/coreutils/blob/main/src/uu/du/src/du.rs#L877.

Thanks for advice. I have put a fix

github-actions · 2023-02-23T05:06:56Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

tertsdiepraam · 2023-02-23T11:18:15Z

Alright, time to merge this! Thanks!

howjmay · 2023-02-23T16:17:19Z

thank you too!

sylvestre reviewed Feb 13, 2023

View reviewed changes

src/uu/cksum/Cargo.toml Outdated Show resolved Hide resolved

howjmay force-pushed the cksum-a branch from 7565401 to 32ecb9f Compare February 13, 2023 13:55

howjmay force-pushed the cksum-a branch 2 times, most recently from cfd060c to 25a3d86 Compare February 13, 2023 17:04

howjmay force-pushed the cksum-a branch 5 times, most recently from e0fcfe2 to a87ad2e Compare February 14, 2023 07:44

howjmay force-pushed the cksum-a branch 4 times, most recently from f872e52 to 7fc81ec Compare February 14, 2023 12:49

howjmay force-pushed the cksum-a branch 2 times, most recently from c512388 to 8032c96 Compare February 14, 2023 17:59

cksum: Implement option -a

678a11d

Implement option -a --algorithm. Move digest to src/uucore/src/lib/features and rename it to hash. fix lint fix Cargo.toml

howjmay force-pushed the cksum-a branch from e07ee9f to 678a11d Compare February 18, 2023 12:59

howjmay marked this pull request as ready for review February 18, 2023 13:02

howjmay mentioned this pull request Feb 18, 2023

cksum: Add untagged option #4350

Closed

add function profile

1095534

tertsdiepraam reviewed Feb 19, 2023

View reviewed changes

howjmay force-pushed the cksum-a branch from 20c7320 to c53ef18 Compare February 21, 2023 08:36

sylvestre reviewed Feb 21, 2023

View reviewed changes

tertsdiepraam reviewed Feb 21, 2023

View reviewed changes

howjmay force-pushed the cksum-a branch from c53ef18 to d4f7806 Compare February 21, 2023 12:47

Add checksum algorithm abstractions

9322580

howjmay force-pushed the cksum-a branch from d4f7806 to 9322580 Compare February 21, 2023 13:15

tertsdiepraam reviewed Feb 21, 2023

View reviewed changes

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

tertsdiepraam reviewed Feb 22, 2023

View reviewed changes

tertsdiepraam mentioned this pull request Feb 22, 2023

Multi-call hashsum binary is no longer necessary #4414

Open

howjmay force-pushed the cksum-a branch from 0492260 to 446570e Compare February 22, 2023 16:22

tertsdiepraam reviewed Feb 22, 2023

View reviewed changes

add ALGORITHM_HELP_DESC

b1d4951

howjmay force-pushed the cksum-a branch from 446570e to b1d4951 Compare February 23, 2023 04:06

tertsdiepraam merged commit 3554565 into uutils:main Feb 23, 2023

howjmay deleted the cksum-a branch February 23, 2023 16:17

Uh oh!

cksum: implement -a #4356

cksum: implement -a #4356

Uh oh!

Conversation

howjmay commented Feb 13, 2023

Uh oh!

Uh oh!

sylvestre commented Feb 13, 2023

Uh oh!

howjmay commented Feb 13, 2023

Uh oh!

github-actions bot commented Feb 13, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 14, 2023

Uh oh!

github-actions bot commented Feb 18, 2023

Uh oh!

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

github-actions bot commented Feb 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tertsdiepraam commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

howjmay commented Feb 22, 2023

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!

tertsdiepraam commented Feb 22, 2023

Uh oh!

howjmay commented Feb 22, 2023

Uh oh!

tertsdiepraam left a comment •

edited

Loading

tertsdiepraam commented Feb 21, 2023 •

edited

Loading