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

generate-cask-api: fall back to previous URL if network fails #18390

Closed
wants to merge 1 commit into from

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Sep 24, 2024

If casks have a url stanza with a block (like transmission@nightly), brew generate-cask-api needs to go to that URL in order to determine the correct download URL for the cask. It's very common for this to fail when generating the API (especially with transmission@nightly), so this PR adds a fallback mechanism. If generate-cask-api is unable to reach the URL specified in the block, it will simply fall back to whatever URL value existed previously in the API. This should hopefully reduce the number of generation failures we see in Homebrew/formulae.brew.sh.

@Rylan12 Rylan12 added the install from api Relates to API installs label Sep 24, 2024
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.

Makes sense, thanks @Rylan12! A few suggested style tweaks; optional and non-blocking.

Comment on lines +26 to +28
prev_url = json_cask["url"]
prev_specs = json_cask["url_specs"] || {}
url = Cask::URL.new(prev_url, **prev_specs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
prev_url = json_cask["url"]
prev_specs = json_cask["url_specs"] || {}
url = Cask::URL.new(prev_url, **prev_specs)
previous_url = json_cask["url"]
previous_specs = json_cask["url_specs"] || {}
url = Cask::URL.new(previous_url, **previous_specs)

url.to_s
rescue
raise unless self.class.generating_hash?
raise unless (json_cask = Homebrew::API::Cask.all_casks[token])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise unless (json_cask = Homebrew::API::Cask.all_casks[token])
json_cask = Homebrew::API::Cask.all_casks.fetch(token).presence
raise unless json_cask

@Bo98
Copy link
Member

Bo98 commented Sep 24, 2024

This is OK but IMO we should ban url do in Homebrew/cask. We did before we imported Homebrew/cask-versions but we never checked the imported casks.

There are only 4 casks that use it so feels weird to have workarounds for such a small number. I think it wouldn't be too unreasonable to try update them to not use url do.

@MikeMcQuaid
Copy link
Member

This is OK but IMO we should ban url do in Homebrew/cask. We did before we imported Homebrew/cask-versions but we never checked the imported casks.

I agree. Note that #18388 will be landing soon so now is a good change to get any deprecations straight into master if @Bo98 or @Rylan12 are tempted to do so.

There are only 4 casks that use it so feels weird to have workarounds for such a small number.

This workaround feels good for unflaking things sooner rather than later, agreed it'd be nicer to remove the need for it soon though.

@Rylan12
Copy link
Member Author

Rylan12 commented Sep 24, 2024

This is OK but IMO we should ban url do in Homebrew/cask. We did before we imported Homebrew/cask-versions but we never checked the imported casks.

Makes sense to me. What was the strategy we used for the casks that needed it before?

@Bo98
Copy link
Member

Bo98 commented Sep 24, 2024

Either:

  • Use livechecks instead (unless it will ping several times a day)
  • Find a redirecting URL and use version :latest

For stable versions, usually one of the above exists so that "download" buttons on webpages work.

For nightlies etc, this sometimes didn't work because they were usually well hidden and for experienced users only. Not sure if that applies to any of the remaining 4 or not.

@Rylan12
Copy link
Member Author

Rylan12 commented Sep 24, 2024

  • Use livechecks instead (unless it will ping several times a day)

This would be for the formulae.brew.sh generation which happens every 15 minutes

  • Find a redirecting URL and use version :latest

I'll look and see what I can find

For nightlies etc, this sometimes didn't work because they were usually well hidden and for experienced users only. Not sure if that applies to any of the remaining 4 or not.

Yes, all of these are nightly or nightly-adjacent:

  • dolphin@dev
  • vlc@nightly
  • transmission@nightly
  • keepassxc@snapshot

@Bo98
Copy link
Member

Bo98 commented Sep 24, 2024

This would be for the formulae.brew.sh generation which happens every 15 minutes

I mean instead of making the cask determine the URL dynamically: we should have livecheck do it so that it opens PRs with checksums that are reviewed properly.

It's the more secure option if possible.

@Rylan12 Rylan12 marked this pull request as draft September 24, 2024 16:12
@Rylan12
Copy link
Member Author

Rylan12 commented Sep 24, 2024

I opened a PR to remove these from homebrew/cask: Homebrew/homebrew-cask#186501

I'll work on adding a rubocop to make sure we don't use any of these in official taps anymore.

@Rylan12 Rylan12 closed this Sep 25, 2024
@Rylan12 Rylan12 deleted the generate-cask-api-add-url-fallback branch September 25, 2024 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install from api Relates to API installs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants