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

add tiger hash from https://gitlab.com/rawler/tiger #229

Merged
merged 13 commits into from
Feb 5, 2021

Conversation

myers
Copy link
Contributor

@myers myers commented Feb 4, 2021

Add the tiger hash.

The bulk of this is from Ulrik Mikaelsson but also work from Igor Raits and myself in upgrading to newer versions of the Digest crate.

@tarcieri suggested adding this crate to this repo so that it can be updated as needed.

tiger/Cargo.toml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

tarcieri commented Feb 4, 2021

Looks like you need to rustfmt

tiger/Cargo.toml Outdated
version = "0.1.2"
description = "Tiger hash function"
authors = ["Ulrik Mikaelsson <ulrik.mikaelsson@gmail.com>", "RustCrypto Developers"]
license = "Apache-2.0"
Copy link
Member

Choose a reason for hiding this comment

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

All of our other crates are MIT OR Apache-2.0. Is that acceptable?

Copy link
Contributor Author

@myers myers Feb 4, 2021

Choose a reason for hiding this comment

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

that's a good question. The original work is released under Apache 2.0. From reading https://law.stackexchange.com/questions/6081/can-i-bundle-mit-licensed-components-in-a-apache-2-0-licensed-project it seems like adding this under the MIT license is easy?

Should we ask Ulirk?

Copy link
Member

Choose a reason for hiding this comment

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

It would definitely be good to get the original author's blessing to republish it as MIT+Apache2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have emailed him. I started to make it look like it was under both licenses, but removed that.

This was referenced Feb 4, 2021
@myers
Copy link
Contributor Author

myers commented Feb 4, 2021

based on the failure on thumbv7em-none-eabi the byte-order crate doesn't work under nostd. Looks like what it does you can use byte-tools instead. I'll work on that.

@myers
Copy link
Contributor Author

myers commented Feb 4, 2021

@tarcieri I think I have addressed everything you mentioned above.

If the license question is an issue, I could also go and resurrect #50

@tarcieri
Copy link
Member

tarcieri commented Feb 4, 2021

The license issue is pretty important as we try to ensure our crates all have consistent licensing and a clear IPR story.

We can wait to hear from the author or you can revive #50.

Perhaps you can benchmark the two and see if there's a compelling reason other than licensing to go with one over the other?

@myers
Copy link
Contributor Author

myers commented Feb 5, 2021

From: Ulrik Mikaelsson
Date: Fri, 5 Feb 2021 07:35:53 +0100
Subject: Re: Relicense rust tiger under MIT?

Fun to hear you're using it! I'm ok with licensing it under MIT, but i also
got notice the the PR is now closed in favor of other PR, so not sure it
matters?

I will make the changes to have it listed as under MIT OR Apache 2.0

@myers
Copy link
Contributor Author

myers commented Feb 5, 2021

benchmark of this PR's tiger crate:

$ cd tiger
$ cargo bench
[...]
test bench1_10    ... bench:          20 ns/iter (+/- 3) = 500 MB/s
test bench2_100   ... bench:         158 ns/iter (+/- 17) = 632 MB/s
test bench3_1000  ... bench:       1,678 ns/iter (+/- 240) = 595 MB/s
test bench4_10000 ... bench:      15,735 ns/iter (+/- 2,818) = 635 MB/s

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out; finished in 11.33s

tiger/LICENSE-MIT Outdated Show resolved Hide resolved
@myers
Copy link
Contributor Author

myers commented Feb 5, 2021

I have spent 20 minutes to try to get #50 to compile and pass tests so that I can benchmark, but it's failing tests.

I propose this PR is ready for merging.

@tarcieri
Copy link
Member

tarcieri commented Feb 5, 2021

Looks good, thank you!

I'll cut a 0.1 after merging.

@tarcieri tarcieri merged commit 1d7f23c into RustCrypto:master Feb 5, 2021
@tarcieri
Copy link
Member

tarcieri commented Feb 5, 2021

It's released: https://crates.io/crates/tiger

@myers myers deleted the add-tiger branch February 5, 2021 16:08
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.

2 participants