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

Remove appcast checkpoints? #48051

Closed
commitay opened this issue Jun 4, 2018 · 20 comments
Closed

Remove appcast checkpoints? #48051

commitay opened this issue Jun 4, 2018 · 20 comments

Comments

@commitay
Copy link
Contributor

commitay commented Jun 4, 2018

Refs: #47728, #38376, #43605, #41240 and others

This has been discussed briefly in a few places, opened this for a proper discussion.

This would basically be as the title says, the checkpoint string would be removed along with the _appcast_checkpoint command while keeping the appcast URL that can be parsed with external scripts/tools.

cc @brianmorton @colindunn @gtmgianni @leipert People that use their own scripts/tools for cask updates.

Would also like to know what other contributors think about this so please feel free to comment.

@leipert
Copy link
Contributor

leipert commented Jun 4, 2018

Thanks for pinging me!

Are they used for anything relating to security? If not, remove them! It makes no sense for the casks which @jcbot updates. I always check against the version string, if we need updates and not against the hash of the URL.

@gtmgianni
Copy link
Contributor

Although my scripts rely entirely on the appcast checkpoint, I don't think using the checkpoint is a long term solution for updating casks. This is mainly due to the fact that there are so many casks with outdated checkpoint hashes, that don't actually need a version bump.

I have a blacklist of around 1000 casks that fall into this category, although by the time I run the script which scrubs the blacklist of any invalid entries (i.e. casks whose checkpoint hashes have changed yet again), that number decreases to around 700. In an ideal scenario, all ~300 of those casks that were removed from the blacklist would need a version bump, but in reality many of them are just unstable appcasts which get added back to the blacklist with their new checkpoint hash once I've manually checked them.

I think removing the checkpoint is something we should consider because many appcasts are unstable.

I like the idea of tailored code for each cask being used to find the latest version, as discussed in #41240. Out of curiosity, would we have some sort of bot which automatically submits the PRs, or would different contributors be able to update the casks themselves? If the latter is the case, how would we avoid pull request duplicates?

@vitorgalvao
Copy link
Member

My initial idea:

  • Have the tailored update code for each cask, and store it in a different repo.
  • Have a bot that submit the PRs.
  • In cask-repair, blacklist casks that are updated by the bot.

Since a huge chunk (the majority?) of submissions use cask-repair, blacklisting should be reasonably effective. Eventually, this would mean blacklisting every cask with appcast and ultimately every cask would have some kind of appcast (since code is tailored for each, there are no restrictions).

This means that in time the bot would do every version bump, with humans fixing problems in the casks themselves and the updating mechanism.

@brianbrownton
Copy link
Contributor

brianbrownton commented Jun 4, 2018

Thanks for the ping.

I do have an issue on my own tool's github to basically throwaway the current version of appcast in favor or something tailored to each cask that I would keep on my side (eg. outside HBC)... but it is still an issue an not yet implemented.

All of my current automated scanning relies on url/version sniffing, since I found the appcasts to be a terrible signal-to-noise ratio (as well summarized by @gtmgianni ) as they currently exist.

I would support any move towards automated maintenance/updates of casks.

EDIT: I actually should add that as part of my scripting I do still output the appcasts, as for some casks (particularly if they have a softwall for download, or don't show you what version you're downloading) I will still double check the appcast to verify if an update was posted there after I've found it via url sniffing.

@commitay
Copy link
Contributor Author

commitay commented Jun 8, 2018

Homebrew/brew#4310

vitorgalvao/tiny-scripts#132

#48235

@brianbrownton
Copy link
Contributor

Somewhat ironically, yesterday and today I have implemented a local sqlite database and have resumed appcast scanning (using cityhash instead of the appcast checkpoints) on my side...

@reitermarkus
Copy link
Member

I'm against removing them until we have an actual alternative.

@commitay
Copy link
Contributor Author

commitay commented Jun 9, 2018

We benefit from removing them now as we can add appcasts to everything that can then be used by the current scripts/tools in use by contributors, waiting means removing more appcasts.

@reitermarkus
Copy link
Member

used by the current scripts/tools in use by contributors

I would still like to see an official tool before we remove the checkpoints.

@commitay
Copy link
Contributor Author

commitay commented Jun 9, 2018

I don't see any reason to wait for an offical tool to be implemented.

@reitermarkus
Copy link
Member

reitermarkus commented Jun 9, 2018

I don't see any reason to wait for an offical tool to be implemented.

And I don't see any reason to remove it when an alternative does not exist yet. A checkpoint is still more useful than nothing.

@commitay
Copy link
Contributor Author

commitay commented Jun 9, 2018

A checkpoint is still more useful than nothing.

A checkpoint that changes without a version bump is not useful. An unstable appcast that has been removed is not useful.

@vitorgalvao
Copy link
Member

vitorgalvao commented Jun 9, 2018

A checkpoint is still more useful than nothing.

As someone who was heavily involved in the process for the inception of appcasts and checkpoints, I disagree. As it is, checkpoints aren’t being wildly used and are unreliable. They seemed like a good idea at the time, and while they’re not a bad idea, they’re also not good enough for our present needs.

There was once a system in place that I built that took advantage of checkpoints and there’s a reason I’ve decided to go the way of not checking them rather than improve the old. The old system would warn when an appcast was outdated and then we still had to check and update to the new version. I think we all agree we’d rather the bot checks the appcast and sends the update in one step. Since verifying a checkpoint still requires we download the whole appcast, having it doesn’t really offer any advantage.

Right now, my only interaction with appcasts is having them fail in Travis and having to fix them manually.

While there’s no need to rush to remove them, I do think we eventually should and predict no situation in which keeping them will be the right choice. Since whatever system we implement will be kept outside the casks, even if it uses a form of checkpoint those values can be kept in the system instead of cluttering the cask.

@commitay
Copy link
Contributor Author

commitay commented Jun 9, 2018

Right now, my only interaction with appcasts is having them fail in Travis and having to fix them manually.

👏 💯

Also, I see checkpoints as a hinderance to implementing the update tool as currently appcasts are required to be stable, whereas the purpose of the new tool would be to remove this restriction. I think keeping the checkpoint means we are limiting what appcasts we have available to build and test the tool with.

Removing them would also somewhat simplify implementation of a cask-repair command (#48086) as the checkpoint wouldn't need to be checked and/or updated.

@commitay
Copy link
Contributor Author

commitay commented Jun 9, 2018

Removed another appcast: #48263

@brianbrownton
Copy link
Contributor

To reiterate, I support @commitay and @vitorgalvao in removing the CHECKPOINTS - the appcast URL is still valuable.

@leipert
Copy link
Contributor

leipert commented Jun 14, 2018

@commitay / @vitorgalvao: Could we push for a release of https://github.com/Homebrew/brew? For me running brew cask audit --download fails for any cask, as the app casts checkpoints are now gone:

brew cask audit --download rubymine
==> Downloading https://download.jetbrains.com/ruby/RubyMine-2018.1.3.dmg
Already downloaded: /Users/leipert/Library/Caches/Homebrew/Cask/rubymine--2018.1.3,181.4892.67.dmg
==> Verifying checksum for Cask rubymine
audit for rubymine: failed
 - a checkpoint sha256 is required for appcast
Error: audit failed for casks: rubymine

@brianbrownton
Copy link
Contributor

@leipert its ok for me?

$ brew cask audit --download rubymine
==> Downloading https://download.jetbrains.com/ruby/RubyMine-2018.1.3.dmg
########                                                                  12.1%
$ brew config
HOMEBREW_VERSION: 1.6.8-25-g9b21422
ORIGIN: https://github.com/Homebrew/brew
HEAD: 9b21422b72475068f2c139bc5677bfd43aa68995
Last commit: 8 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 4fbabc99fcf13f3e82254fec472f1fee896cbf3f
Core tap last commit: 4 hours ago
HOMEBREW_PREFIX: /usr/local
HOMEBREW_DEV_CMD_RUN: 1
HOMEBREW_LINKAGE_CACHE: 1
CPU: quad-core 64-bit broadwell
Homebrew Ruby: 2.3.3 => /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.3_2/bin/ruby
Clang: 9.1 build 902
Git: 2.17.1 => /usr/local/bin/git
Curl: 7.54.0 => /usr/bin/curl
Java: N/A
macOS: 10.13.3-x86_64
CLT: 9.3.0.0.1.1521514116
Xcode: 9.3.1
XQuartz: N/A

@MikeMcQuaid
Copy link
Member

@leipert Run brew update again.

@leipert
Copy link
Contributor

leipert commented Jun 15, 2018

@brianmorton / @MikeMcQuaid: Won't help, as I am on brew stable:

$ brew config
HOMEBREW_VERSION: 1.6.8
ORIGIN: https://github.com/Homebrew/brew
HEAD: 3081390ff845a6172b9004cf404c3e028e3be745
[...]
$ brew update
$ brew config
brew config
HOMEBREW_VERSION: 1.6.8
ORIGIN: https://github.com/Homebrew/brew
HEAD: 3081390ff845a6172b9004cf404c3e028e3be745
[...]

tas50 added a commit to chef/homebrew-chef that referenced this issue Jul 5, 2018
Homebrew has been removing these everywhere. See Homebrew/homebrew-cask#48051
Signed-off-by: Tim Smith <tsmith@chef.io>
tas50 added a commit to chef/homebrew-chef that referenced this issue Jul 5, 2018
Homebrew has been removing these everywhere. See Homebrew/homebrew-cask#48051

Signed-off-by: Tim Smith <tsmith@chef.io>
@lock lock bot locked and limited conversation to collaborators Jul 15, 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

No branches or pull requests

7 participants