-
Notifications
You must be signed in to change notification settings - Fork 467
implement std.sha256 #607
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
implement std.sha256 #607
Conversation
* explicitly load git_repository rule * load googletest as a git_repository * stop using bind (see bazelbuild/bazel#1952)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to leave the old one at all? Why not just always use OpenSSL version if we depend on it anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards compatibility. We don't want to force people to redeploy their entire infrastructure because they were using md5 to generate unique names for things.
Thanks, adding more standard hash functions is a good thing.
Unfortunately, going from 0 external dependencies to one (two with absl) is a pretty inconvenient. We need to support the users who don't use Bazel.
Are there any good header-only hash function libs? That would be much more convenient here. However, OpenSSL is going to be available on pretty much any system anyway and it provides us with all the crypto we may need. I'm not aware of any other reasonable option, so we probably should use it. But then we need to make it work with a regular Makefile, add dependency information to README and make the new requirement very clear during the release.
I don't think that absl::BytesToHexString is worth getting absl. It sounds like quite a pain to get working with Make and it's much less likely that the users have it already in their system.
I wonder what @sparkprime thinks about this.
The only reason Mike did this is that someone internally wanted us to provide an alternative to MD5 in case anyone felt tempted to use MD5 in a context where crytographic hashing was needed. Personally I'm not really convinced it's worthwhile since this is a config language.
As for absl dependency, I'm sure we can write our own absl::BytesToHexString :)
There are already build problems on windows and mac and stuff like this will probably exacerbate them.
Maybe we should pull off the bandaid and start adding stuff to go that's not in C++?
If we will go forward with parseYaml we will probably want to add shared library dependencies anyway, so adding openSSL should't change that much.
Adds a dep on openssl and absl. I'm not sure how problematic that is.