Skip to content

Add configuration to new appcast check#6155

Merged
MikeMcQuaid merged 3 commits intoHomebrew:masterfrom
core-code:master
May 23, 2019
Merged

Add configuration to new appcast check#6155
MikeMcQuaid merged 3 commits intoHomebrew:masterfrom
core-code:master

Conversation

@core-code
Copy link
Copy Markdown
Contributor

@core-code core-code commented May 21, 2019

  • 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 tests with your changes locally?


This a followup to PR #5972
Recap: PR #5972 introduced an optional new flag '--appcast' to 'brew cask audit' that checks whether the version stanza is contained in the downloaded appcast. This allows the Travis CI check to fail for submitted PRs that are for unofficial/beta versions, this preventing automerging by @BrewTestBot.

However, this also prevents auto-merging Cask PRs for a lot of fine PRs. This PR is meant to address that. Changes:
1.) the 'curl_output' call to download the appcast is changed to follow redirects. we've seen quite a lot of failures because the appcast URL actually leads to an redirect. following the redirect in the cask itself is not always appropriate, so the appcast check should be adjusted to be able to follow the redirect
2.) as discussed many times previously the new appcast check is not 'perfect' because many appcasts do not contain the whole version stanza in unmodified form. this PR fixes this problem by allowing the appcast stanza to be annotated to specify which part or which transformation of the version stanza is actually contained in the appcast, therefore allowing the appcast check to succeed and those PRs to be automerged
3.) some appcasts do not contain the version number at all, but automerging PRs for these casks may still be desired. therefore this patch allows to disable the appcast check as needed

examples:

cask 'visual-paradigm'
the appcast ( https://www.visual-paradigm.com/downloads/vp/checksum.html ) does not contain the while version number ( currently 15.2,20190501 ) but just the last part which constitutes the build number. currently all PRs for visual-paradigm fail Travis and can not be automerged. using this PR we can now annotate the appcast stanza with the 'configuration' option which properly makes the appcast check search just for the build number in the appcast:

  appcast 'https://www.visual-paradigm.com/downloads/vp/checksum.html',
          configuration: version.after_comma

other transformations work similarly. i guess you could even specify 'context', if you want the appcast e.g. to contain "Version #{version} released"

another example, cask 'p4v' the appcast ( https://cdist2.perforce.com/perforce/r18.4/bin.macosx1013x86_64/SHA256SUMS ) only contains checksums and therefore doesn't contain the version number at all. the appcast is still useful because it can be observed for changes. however, we don't want the fact that it doesn't contain version numbers prevent auto-merging for this cask. now we can just change the appcast to

  appcast 'https://blabla',
          configuration: :no_check

which instructs the appcast check to skip checking the appcast despite the --appcast flag which travis passes.

cc @reitermarkus @vitorgalvao

Comment thread Library/Homebrew/cask/audit.rb Outdated
Comment thread Library/Homebrew/cask/audit.rb Outdated
Comment thread Library/Homebrew/cask/dsl/appcast.rb Outdated
@core-code
Copy link
Copy Markdown
Contributor Author

core-code commented May 21, 2019

btw, i can also contribute pre-made "configurations" to get around 50 casks to automerge provided this is merged:

"sonos":"#{version.no_dots}"
"routeconverter":"#{version.major_minor}"
"qt3dstudio":"#{version.major_minor}"
"understand":"#{version.major_minor_patch}"
"anka-flow":"#{version.major_minor_patch}"
"android-studio":"#{version.major_minor_patch}"
"fluxcenter":"#{version.major_minor_patch}"
"aspera-connect":"#{version.major_minor_patch}"
"xmind":"#{version.after_comma}"
"wiznote":"#{version.after_comma}"
"jalbum":"#{version.major_minor}"
"scratch":"#{version.after_comma}"
"visual-paradigm":"#{version.after_comma}"
"visual-paradigm-ce":"#{version.after_comma}"
"superduper":"#{version.after_comma}"
"amazon-music":"#{version.after_comma.before_colon}"
"antfileconverter":"#{version.no_dots}"
"iridium":"#{version.major_minor}"
"mbed-studio":"#{version.major_minor}"
"qt-creator":"#{version.major_minor}"
"pagico":"#{version.major_minor}"
"soundplant":"#{version.major_minor}"
"fbreader":"#{version.major_minor}"
"th-makerx":"#{version.major_minor}"
"vmware-horizon-client":"#{version.major_minor}"
"zdoom":"#{version.major_minor}"
"dendroscope":"#{version.dots_to_underscores}"
"tinyumbrella":"#{version.dots_to_underscores}"
"algodoo":"#{version.dots_to_underscores}"
"beersmith":"#{version.dots_to_underscores}"
"astah-professional":"#{version.before_comma.dots_to_underscores}"
"astah-sysml":"#{version.before_comma.dots_to_underscores}"
"visual-paradigm":"#{version.dots_to_underscores}"
"visual-paradigm-ce":"#{version.dots_to_underscores}"
"tableau":"#{version.dots_to_hyphens}"
"tableau-reader":"#{version.dots_to_hyphens}"
"tableau-prep":"#{version.dots_to_hyphens}"
"tableau-public":"#{version.dots_to_hyphens}"
"geogebra":"#{version.dots_to_hyphens}"
"suunto-moveslink":"#{version.dots_to_underscores}"
"second-life-viewer":"#{version.dots_to_underscores}"
"kollaborate-folder-watcher":"#{version.no_dots}"
"keycue":"#{version.no_dots}"
"cycling74-max":"#{version.split('_').first}"
"cocoscreator":"#{version.split('_').first}"
"virtaal":"#{version.split('b').first}"
"nestopia":"#{version.tr('^0-9', '')}"
"cutesdr":"#{version.tr('^0-9', '')}"
"shupapan":"#{version.tr('^0-9', '')}"
"beoplay-software-update":"#{version.tr('^0-9', '')}"
"anylogic":"#{version.chomp(".0")}"
"protopie":"#{version.chomp(".0")}"

Comment thread Library/Homebrew/cask/dsl/appcast.rb Outdated
Comment thread Library/Homebrew/cask/dsl/appcast.rb Outdated
@MikeMcQuaid MikeMcQuaid merged commit c82496c into Homebrew:master May 23, 2019
@core-code
Copy link
Copy Markdown
Contributor Author

thanks!

@core-code
Copy link
Copy Markdown
Contributor Author

core-code commented May 23, 2019

i think this is working fine now, as already evidenced e.g. here Homebrew/homebrew-cask#63701

two more notes regarding cask auto-merging and appcast-checking

1.) some appcasts (e.g. objective-see.com ) are delivered by the webservers in GZIP form. i've done some research but couldn't find a a way to prevent that in the curl call. those servers also ignore the Accept-Encoding flag, so just specifying that doesn't work. i think these servers are so rare that a complicated solution isn't worth it

2.) i think the appcast checking works fine for the main cask repo, but has a higher false-positive rate in other repositories, especially in the versions repo (where most appcast stanzas are low quality) and in the font repo. it could be beneficial to turn on the appcast checking only for the main repo, but i don't know how to do that

@vitorgalvao
Copy link
Copy Markdown
Contributor

vitorgalvao commented May 23, 2019

Great job, @core-code, thank you!

Looking at the PRs, though, I’m seeing .to_s as a common occurrence. Is that part necessary? I was under the impression our version methods already returned strings.

@core-code
Copy link
Copy Markdown
Contributor Author

core-code commented May 23, 2019

was under the impression our version methods already returned strings.

at first i tried e.g.

configuration: "#{version.after_comma}"

but while that worked, it failed the style check (of the caskfile) and style --fix suggested to use

configuration: version.after_comma.to_s

which works too and passes the style check.

maybe it also works without the .to_s - i don't know since i didnt try it.

can do some experiments here if you want.

@vitorgalvao
Copy link
Copy Markdown
Contributor

Yes!

@core-code
Copy link
Copy Markdown
Contributor Author

core-code commented May 23, 2019

thanks, i've edited the top post to remove the unnecessary to_s in case someone comes back here.

in any case, should i try to send pull requests for those 50 casks mentioned above, or is preferred to just add them one demand when and if those casks are updated?

def initialize(uri, **parameters)
@uri = URI(uri)
@parameters = parameters
@configuration = parameters[:configuration] if parameters.key?(:configuration)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need for the if here, it will be nil if the key doesn't exist anyways.

@vitorgalvao
Copy link
Copy Markdown
Contributor

in any case, should i try to send pull requests for those 50 casks mentioned above, or is preferred to just add them one demand when and if those casks are updated?

No preference. If it’s beneficial to you to have them already updated, said updates will be merged. Otherwise, this is not urgent and we can fix cases as they come by.

@core-code
Copy link
Copy Markdown
Contributor Author

ok lets do it as-they-come then ;)

@lock lock bot added the outdated PR was locked due to age label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
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.

4 participants