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

Fix :svn download errors with :latest casks #13358

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented May 31, 2022

  • 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?

Note: This is an alternative solution to #13354.

This only computes the sha256 for version :latest casks that don't use the subversion download method. Currently with Casks there are three allowed download types.

  1. Curl Get (default)
  2. Curl Post (:post)
  3. Subversion (:svn)

From what I can see both Curl download methods require single compressed files which can be hashed. From the documentation: URL to the .dmg/.zip/.tgz/.tbz2 file that contains the application.

That requirement apparently doesn't exist for Subversion downloads so I added a Cask#checksumable? method that checks if the url is using either of the Curl methods. This should still work with #13232 assuming that is given a different using directive.

Fixes #13359.

@@ -129,19 +129,26 @@ def config_path
metadata_main_container_path/"config.json"
end

def checksumable?
url.using.nil? || url.using == :post
Copy link
Member

@bevanjkay bevanjkay Jun 1, 2022

Choose a reason for hiding this comment

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

I believe once #13232 we can change this to.
But until #13232 is merged this will result in an error.

@MikeMcQuaid Could we merge without support and include the changes in the other PR? I'm not sure on the status quo here.

Suggested change
url.using.nil? || url.using == :post
(url.using.nil? && url.only_paths.nil?) || url.using == :post

Copy link
Member

Choose a reason for hiding this comment

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

Let's just merge what will work for now assuming that #13232 isn't merged or at least gracefully handling it.

Suggested change
url.using.nil? || url.using == :post
url.using.blank? || url.using == :post

or

Suggested change
url.using.nil? || url.using == :post
(url.using.blank? && url.only_paths.blank?) || url.using == :post

Copy link
Member

Choose a reason for hiding this comment

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

For now it'll need to be

Suggested change
url.using.nil? || url.using == :post
url.using.blank? || url.using == :post

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Jun 1, 2022
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.

Fine as-is if it'll work, otherwise pick one of the suggestions and then 👍🏻 to merge.

@@ -129,19 +129,26 @@ def config_path
metadata_main_container_path/"config.json"
end

def checksumable?
url.using.nil? || url.using == :post
Copy link
Member

Choose a reason for hiding this comment

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

Let's just merge what will work for now assuming that #13232 isn't merged or at least gracefully handling it.

Suggested change
url.using.nil? || url.using == :post
url.using.blank? || url.using == :post

or

Suggested change
url.using.nil? || url.using == :post
(url.using.blank? && url.only_paths.blank?) || url.using == :post

@apainintheneck
Copy link
Contributor Author

Okay, I updated url.using.nil? to url.using.blank?. Also, I'll just hop over to the sparse checkout branch and mention the change after this gets merged in.

@MikeMcQuaid MikeMcQuaid merged commit 9b2f2e4 into Homebrew:master Jun 1, 2022
@MikeMcQuaid
Copy link
Member

Thanks again @apainintheneck!

@apainintheneck apainintheneck deleted the fix-latest-svn branch June 8, 2022 19:46
hmarr added a commit to hmarr/brew that referenced this pull request Jun 12, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 9, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to install font-ubuntu-mono-derivative-powerline after commit b85f407e959d0420eaaaca25882335078aa70799
3 participants