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

Introduce block syntax for url stanza #22014

Merged
merged 2 commits into from
Jun 19, 2016
Merged

Introduce block syntax for url stanza #22014

merged 2 commits into from
Jun 19, 2016

Conversation

claui
Copy link
Contributor

@claui claui commented Jun 15, 2016

Changes to the core


Issue

Some providers use disposable URLs, which need to be constructed from some sort of landing site (recent example). Doing such expensive things in the middle of a Cask definition has proven difficult in the past. Loading a Cask would become unacceptably slow if HTTP/S requests were done in Cask definitions.

Solution

Albeit rarely, such cases do pop up every now and then; so I decided to write this PR, which introduces an optional block syntax for the url stanza:

url do
  # No known stable URL; fetching disposable URL from landing site
  open('https://example.com/app/landing') do |landing_page|
    content = landing_page.read
    parse(content) # => https://example.com/download?23309800482283
  end
end

The block is only evaluated when needed, for example on download time or when auditing a Cask. It is never evaluated when loading a Cask.

The return value of the block is either a String, or a String, Hash tuple; the latter case is for additional options such as referrer.

Implementation note

For stanza_proxy.rb, I could have used SimpleDelegator but it turned out to be much faster to roll my own logic.

The proxy is designed to be easily reused for other stanzas if needed.

Credits

Thanks @tobadia for contributing the regex/open-uri snippet in audacity.rb, which also triggered the idea for this PR.

Feedback

Pinging @caskroom/maintainers for opinions (and for suggestions to improve my convoluted English in the docs). Thanks!

@claui claui added enhancement awaiting maintainer feedback Issue needs response from a maintainer. core Issue with Homebrew itself rather than with a specific cask. labels Jun 15, 2016
@claui claui mentioned this pull request Jun 16, 2016
# Audacity does not provide a fixed URL
# Their download URL points to a html page that generates a temporary URL embedded within an iframe
# 'open-uri' is required to open that page and grab the temporary URL
require 'open-uri'
Copy link
Member

Choose a reason for hiding this comment

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

Won’t this happen on every cask load? Since this is exclusively needed for url, why not have the require inside the url block? Any reason why that wouldn’t work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does work, and makes for a good improvement.
Changing this, thanks!

@claui
Copy link
Contributor Author

claui commented Jun 16, 2016

@vitorgalvao Changed as per your suggestions, thanks.
I also cut short redundant comments and added the required line:

# fosshub.com/Audacity.html was verified as official when first introduced to the cask

@@ -84,8 +84,49 @@ Also make sure to give the URL for the binary download itself, rather than a pre

## Some Providers Block Command-line Downloads

Some hosting providers actively block command-line HTTP clients (example: FossHub). Such URLs cannot be used in Casks.
Some hosting providers actively block command-line HTTP clients. Such URLs cannot be used in Casks.
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the FossHub example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example has not been removed. I moved it to the next line:

Some providers do not actively block command-line HTTP clients but use URLs that change periodically, or even on each visit (example: FossHub).

I did this for two reasons:

  1. My new text wants to establish a clear difference between hosting providers that actively block CLI clients (e. g. using captchas) vs. those that just use disposable URLs for whatever reason. There are certainly many possible reasons for a provider to go for disposable URLs, even if that choice means breaking CLIs as a side effect. I feel it wouldn’t be unreasonable for us to politely err on the side of the accused, rather than implying that FossHub made their choices for a certain reason without giving actual evidence.
  2. To me, the current wording would imply that both Audacity update url #21952 and Introduce block syntax for url stanza #22014 are acts of circumventing a technological barrier in a way FossHub does not wish to allow. While I am willing to take responsibility for my actions, I certainly don’t feel good about making assumptions which in the worst case might someday be used against the project and its participants. On the other hand, IANAL; my comment is just an account of how I currently feel about the whole thing.

That all said, I dislike how FossHub’s choices makes our lives harder. I’d be glad if someone would inform them about the issue and make them aware of the hoops through which their technical decision makes us jump.

Regarding the docs, I am completely open for feedback/suggestions how to reword this.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Great detail, as always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My verbosity can be both a blessing and a curse. 😄

@vitorgalvao
Copy link
Member

vitorgalvao commented Jun 16, 2016

Everything looks great.

This could provide immense value in the caskroom/versions repo. We could switch all nightlies to use :latest and :no_check and make url always grab the most up-to-date version. Most nightlies users should prefer that to permanently outdated casks.

@claui
Copy link
Contributor Author

claui commented Jun 16, 2016

Committed fixes to url.md regarding typography, spelling, and the link anchor.

tobadia and others added 2 commits June 18, 2016 21:13
Changed Audacity download URL.

Update audacity URL, now grabbed from an embeded iframe within the protected download URL

Update audacity with proper URL and no more style offenses

Update audacity and comments in code for future maintenance
This commit amends the `url` stanza to accept an optional
block

The block is only evaluated when needed, for example on
download time or when auditing a Cask. It is never evaluated
when loading a Cask.

The return value of the block is either a `String`, or a
`String, Hash` tuple; the latter case is for additional
options such as `referer`.

Implementation note: Rolled my own delegate logic because
`SimpleDelegator` turns out to take too much of a
performance hit.
@claui
Copy link
Contributor Author

claui commented Jun 18, 2016

Rebased.

Leaving the PR open for another few hours before I proceed to merge.

@vitorgalvao
Copy link
Member

I wonder in which fun way cask-repair will blow up with these casks. I’ll likely just make it abort if it finds a cask with a block url.

@claui claui merged commit c389d9c into Homebrew:master Jun 19, 2016
@claui claui deleted the url-lazy-evaluation branch June 19, 2016 12:26
@claui claui removed the awaiting maintainer feedback Issue needs response from a maintainer. label Jun 19, 2016
vitorgalvao pushed a commit that referenced this pull request Jun 19, 2016
As [discussed](#22014 (comment)) in #22014, the docs are updated to point to a specific revision for the `audacity.rb` example.
@miccal miccal removed core Issue with Homebrew itself rather than with a specific cask. enhancement labels Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants