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

Use md5 implementation of RustCrypto organization #1404

Merged
merged 2 commits into from
May 24, 2022

Conversation

petrosagg
Copy link
Contributor

Motivation and Context

I noticed that the S3 SDK was depending to the md5 crate which is not part of the RustCrypto organization. The RustCrypto organization is the de-facto standard of cryptographic primitive implementations in Rust with an active community and already used by aws-smithy-checksums for its sha1 and sha2 checksums.

Description

I changed the md5 crate to the one provided by RustCrypto.

Testing

No behavior changes to be tested

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@petrosagg petrosagg requested a review from a team as a code owner May 23, 2022 13:04
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! These changes look good to me, but I will need to dig into why it's failing in CI. It's kind of a strange failure since the md5 symbol is being used generate the code, and that should pull in the md-5 dependency automatically.

CHANGELOG.next.toml Show resolved Hide resolved
@jdisanti
Copy link
Collaborator

It looks like the md-5 crate renames itself to md5:
https://github.com/RustCrypto/hashes/blob/master/md5/Cargo.toml#L14-L15

Our codegen doesn't currently support that concept, so I think this will require adding a property to CargoDependency and updating either the CargoTomlGenerator to re-rename the crate, or the RustWriter to use the different name.

@petrosagg petrosagg force-pushed the rust-crypto branch 3 times, most recently from 45010fa to f7712ee Compare May 24, 2022 11:39
@petrosagg
Copy link
Contributor Author

@jdisanti I see, I pushed a change that should fix that, let's see what CI thinks about it

@Velfi
Copy link
Contributor

Velfi commented May 24, 2022

@petrosagg It looks like the codegen tests are still failing with a naming issue. You can run codegen locally to check for this with ./gradlew :aws:sdk:cargoCheck. Let me know if that doesn't work for you.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

It looks like that this.name.replace("-", "_") snippet was in two places. Easy fix. Thanks for contributing this!

@jdisanti jdisanti merged commit bb9f2dc into smithy-lang:main May 24, 2022
@petrosagg
Copy link
Contributor Author

@jdisanti oh, great! Thanks for looking into it, it would have been a deep rabbit hole to set up a local devenv for me, I've never worked with Java nor Kotlin before :)

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