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

checksum: simplify, use only sha256 #10247

Merged
merged 1 commit into from
Jan 8, 2021
Merged

Conversation

iMichka
Copy link
Member

@iMichka iMichka commented Jan 7, 2021

We use only one sha type right now.

Needed for #10186

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

@iMichka iMichka requested a review from MikeMcQuaid January 7, 2021 07:54
@BrewTestBot
Copy link
Member

Review period will end on 2021-01-08 at 07:54:12 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 7, 2021
@iMichka
Copy link
Member Author

iMichka commented Jan 7, 2021

There are still a few places that call hash_type and I need to check these. I pushed a few more changes.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good, nice cleanup @iMichka!

# Raised by {Pathname#verify_checksum} when verification fails.
class ChecksumMismatchError < RuntimeError
attr_reader :expected, :hash_type
def initialize(path, expected, actual)
@expected = expected
@hash_type = expected.hash_type.to_s.upcase
super <<~EOS
#{@hash_type} mismatch
Expected: #{Formatter.success(expected.to_s)}
Actual: #{Formatter.error(actual.to_s)}
File: #{path}
To retry an incomplete download, remove the file above.
EOS
end
end
still needs to be updated, I think, but once that's done and CI is 🟢 I'm 👍🏻 to 🚢

@iMichka iMichka force-pushed the sha256 branch 2 times, most recently from 3f1f501 to b04f500 Compare January 7, 2021 11:36
@iMichka
Copy link
Member Author

iMichka commented Jan 7, 2021

All green for me, except the delegate change that I am not able to make work.

@SeekingMeaning
Copy link
Contributor

I would recommend this:

    def sha256(val)
      stable.sha256(val)
    end

Or otherwise using delegate: SeekingMeaning@9faf25527

@SeekingMeaning
Copy link
Contributor

SeekingMeaning commented Jan 7, 2021

Probably should try to add as few :sha256 symbols as possible; we can delete the hash_type variable in bump-formula-pr: SeekingMeaning@7c248d26a

Currently looking into def checksums in software_spec.rb—we might be able to return an array instead of a hash that only has :sha256 as a key (Edit: See SeekingMeaning@c01b1cc80)

@iMichka
Copy link
Member Author

iMichka commented Jan 7, 2021

bump-formula-pr

Done

@SeekingMeaning
Copy link
Contributor

(You missed the hash_type.blank?old_hash.blank? change)

Copy link
Contributor

@SeekingMeaning SeekingMeaning left a comment

Choose a reason for hiding this comment

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

I would also recommend SeekingMeaning@c01b1cc80 but I'm not familiar with all uses of BottleSpecification#checksums so 👍 from me

@iMichka
Copy link
Member Author

iMichka commented Jan 7, 2021

The checksums are coming from https://github.com/Homebrew/brew/pull/10247/files#diff-bb40db554b6955f972e80737c5883dafef80ffe19b09e0ad1ceeea1cb776f694L425-L439 for this part of the code. This should be fine. This is a part of code I will rework with my follow up PRs as the logic is going to change there.

Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/software_spec.rb Outdated Show resolved Hide resolved
We use only one sha type right now.

Needed for Homebrew#10186
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 8, 2021
@MikeMcQuaid MikeMcQuaid disabled auto-merge January 8, 2021 09:43
@MikeMcQuaid MikeMcQuaid merged commit 50ffa38 into Homebrew:master Jan 8, 2021
@MikeMcQuaid
Copy link
Member

Thanks for the cleanup @iMichka!

@iMichka iMichka deleted the sha256 branch January 8, 2021 09:47
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 8, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants