-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add sha256 checksum to rake build:checksum #6022
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
Conversation
|
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
… SHA512 - Until now Rubygems.org has publicly displayed SHA256 checksum for published gems, but has only created the SHA512 checksum for the package via rake build:checksum task - This will allow more gem authors to easily verify integrity of published gems with an existing Rubygems.org feature
previously inconsistent platform discrepancies are normalized to include the following for all: x86_64-darwin-19 x86_64-darwin-20 x86_64-darwin-21
690ec12 to
06f312d
Compare
06f312d to
1c707cc
Compare
|
I am working on a PR updating rubygems.org documentation to be concordant with |
deivid-rodriguez
left a comment
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.
Thanks for fixing this, @pboling! I just have one request regarding lazily loading digest/sha, so that it's only loaded when this task is used.
- address code review feedback
deivid-rodriguez
left a comment
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.
Just to small additional comments, but it's most ready from my side. Thanks so much for working on this!
| file_name = "#{File.basename(built_gem_path)}.sha512" | ||
| require "digest/sha2" | ||
| checksum = ::Digest::SHA512.file(built_gem_path).hexdigest | ||
| CHECKSUMS.each do |extension, digest_klass| |
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.
Super minor but I'm thinking, do you think the CHECKSUMS constant buys us much, over
write_checksum(built_gem_path, "sha256", Digest::SHA256)
write_checksum(built_gem_path, "sha512", Digest::SHA512)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.
It doesn't get us much, agreed.
| def build_checksum(built_gem_path = nil) | ||
| def build_checksums(built_gem_path = nil) | ||
| built_gem_path ||= build_gem | ||
| SharedHelpers.filesystem_access(File.join(base, "checksums")) {|p| FileUtils.mkdir_p(p) } |
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.
Why was this moved inside the write_checksums method? It should be enough to run it just once I think?
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.
Good point. Oversight.
|
I've created a gem that does this same concept, except it supports back to Ruby 2.2. I expect it can be used as an alternative when people are stuck on older versions of Ruby, as I often find myself. It has 98% test coverage, so I will attempt to leverage that in this PR when I have time to move the work over here. I tried pushing the gem to RubyGems.org, but the push was rejected. Perhaps if I change the name to Renaming the gem to I left the repo name as is for now: https://github.com/pboling/gem_checksums |
|
That's interesting, the error message is definitely not great because you just built the gem successfully 🤔 |
|
@deivid-rodriguez Aha! I think it is a likeness collision. It would be great to have more clarity around how this warks and what can be done to successfully rename. https://rubygems.org/gems/gem-checksum Perhaps it would have worked if I'd set up pending trusted publishers? I've never set that up because I don't want to be roped in that hard to GitHub. Or would the trusted publisher setup have warned me that the name would be rejected? Also, the last line of the error is entirely misleading. 🗡️
That would not have helped in this case. |
… in addition to the existing SHA512
What was the end-user or developer problem that led to this PR?
SHA256checksum for published gems, but has only created theSHA512checksum for the package viarake build:checksumtaskWhat is your fix for the problem, implemented in this PR?
Create a SHA256 checksum, in addition to the current SHA512 checksum.
Note that I've changed the Gem helper method from
build_checksumtobuild_checksums. Certainly could keep it the same if that's a problem, but it felt good to be grammatically correct with it. Not sure if internal or external API.Make sure the following tasks are checked