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

cleanup: use better-maintained sha-1 crate instead of sha1 crate #495

Closed
wants to merge 2 commits into from

Conversation

durin42
Copy link

@durin42 durin42 commented Sep 11, 2020

This removes a "duplicate" dependency in the tree of pyoxidizer.

@alex
Copy link

alex commented Sep 11, 2020

sha1 has a higher MSRV than UUID currently does.

@newpavlov
Copy link

sha-1 v0.8 has MSRV equal to 1.21. Ideally together with such change it's worth to replace md5 with md-5 as well, though maintainers have previously rejected my PR with such changes. :/

@KodrAus
Copy link
Member

KodrAus commented Dec 21, 2020

It looks like the original concern came from the weight of those dependencies? I had a look through them and they didn't seem too weighty to me.

I'd be in favour of also bundling md-5 into this PR if you're happy to do that.

@durin42
Copy link
Author

durin42 commented Dec 21, 2020

It was mostly I noticed the two implementations of SHA1 as I was importing crates into a monorepo and figured I should try and consolidate down onto the maintained-looking one. If you're raising your MSRV I'm happy to handle both sha1 and md5, but if you want to keep it low please feel encouraged to reject this PR.

Thanks!

@KodrAus
Copy link
Member

KodrAus commented Dec 23, 2020

We've actually already raised it, so please feel free to roll in both!

@KodrAus
Copy link
Member

KodrAus commented Dec 23, 2020

@durin42 I just pushed a quick merge commit to your branch. Hope you don't mind!

@durin42
Copy link
Author

durin42 commented Dec 25, 2020

Not at all! I'll try and get the md5 upgrade done too over the weekend. Right now my keyboard time is somewhat limited. :)

This removes a "duplicate" dependency in the tree of pyoxidizer.
I feel like it should be easier to go from the GenericArray to the
[u8;16] here, but I can't figure out what it should look like.
@durin42
Copy link
Author

durin42 commented Dec 31, 2020

I've rebased and added a commit for upgrading md5 to md-5, but I got stuck figuring out how to convert the GenericArray into a [u8;16] - please let me know if there's a trick I'm missing!

@durin42
Copy link
Author

durin42 commented Dec 31, 2020

I'm not sure what's up with the cargo clippy presubmit check. Works here?

optional = true
version = "0.7"
version = "0.9.1"

Choose a reason for hiding this comment

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

It could be worth to start with v0.8 versions of md-5 and sha-1 to avoid bumping MSRV. Updating versions can be done later in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

@newpavlov We've actually already bumped the MSRV so there's no reason not to track the latest versions here.


let mut builder = crate::Builder::from_bytes(bytes);
let mut builder = crate::Builder::from_slice(&computed[..16]).unwrap();

Choose a reason for hiding this comment

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

IIUC you don't need the slicing here, i.e. you can replace &computed[..16] with &computed. Same goes for SHA-1.

@@ -20,15 +20,14 @@ impl Uuid {
/// [`NAMESPACE_URL`]: #associatedconstant.NAMESPACE_URL
/// [`NAMESPACE_X500`]: #associatedconstant.NAMESPACE_X500
pub fn new_v3(namespace: &Uuid, name: &[u8]) -> Uuid {
let mut context = md5::Context::new();
let mut hasher = md5::Md5::new();
Copy link

@newpavlov newpavlov Jan 1, 2021

Choose a reason for hiding this comment

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

Use use md5::{Md5, Digest}; import to avoid duplication of md5 here? Same goes for SHA-1.

@KodrAus
Copy link
Member

KodrAus commented Jan 8, 2021

It looks like it's trying to read configuration files for dependencies and is failing on one of them:

error reading Clippy's configuration file `C:\Users\runneradmin\.cargo\registry\src\github.com-1ecc6299db9ec823\typenum-1.12.0\clippy.toml`

We can probably just remove the clippy check in CI if you want to remove the clippy.yml file.

@KodrAus
Copy link
Member

KodrAus commented Jan 8, 2021

I'm not familiar with generic_array but took a quick look and it looks like there's a From impl from GenericArray into [T; N] if we bump our MSRV to 1.41.0, which I'm personally ok with. 1.41.0 was released almost 12 months ago.

@KodrAus
Copy link
Member

KodrAus commented Aug 13, 2021

Hi @durin42 👋

I've ended up doing this in #526. Thanks for working on this!

@KodrAus KodrAus closed this Aug 13, 2021
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.

None yet

4 participants