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

Streamlining Updates #15908

Closed
6 of 8 tasks
adidalal opened this issue Dec 17, 2015 · 17 comments
Closed
6 of 8 tasks

Streamlining Updates #15908

adidalal opened this issue Dec 17, 2015 · 17 comments

Comments

@adidalal
Copy link
Contributor

Some thoughts and a tentative roadmap towards minimizing both the time and effort to keep Casks up to date in homebrew-cask

Currently: Cask updates are submitted as PRs by (our awesome) contributors. A new PR must be made for each version, and then merged into caskroom/homebrew-cask

We have some pieces (ie. appcast URLs and cask-repair to push towards a better process, but there's nothing streamlined yet.


Roadmap (updated based on @vitorgalvao's comments below):

  • Update cask-repair to also update the appcast stanza’s :sha256.
  • Instead of trying to be “smart” and parse different types of appcasts, remove :format altogether.
  • Add :sha256 to all appcasts
  • Make :sha256 in appcasts a requirement.
  • Update cask-repair to also submit a PR automatically. 1
  • Add cask-repair to CONTRIBUTING.md (documentation)

Build a script/bot, that will:

- [ ] Auto-merge PRs only if said PR:
- Consists of a single commit to a single cask.
- Modifies the cask (i.e. not valid for creating casks).
- Modifies exactly (no more, no less) the stanzas: version, sha256, and :sha256 on the appcast if there is one.
- Passes all tests.2
(#15908 (comment))

  • Iterate through the casks with an appcast . Updates would clearly show up as having a changed :sha256.
  • Upon seeing appcast changes, open a new issue in the form of a Cask Request. Includes:
    • The token of the cask (with a link).
    • The contents the appcast.
    • A link to the appcast (in case the 50 lines aren’t enough).

1 At which point cask-repair could be used to submit the PR, set it and forget it. Perhaps it should also be updated (would likely have to use a third-party tool as well, like how prfixmaster uses ghi) to auto-submit the PR, instead of the current behaviour where you still need to go to the actual page and click to submit it.

2 Since @jawshooah made Travis confirm shasums, we know that also means the download is valid.

@adidalal adidalal added enhancement discussion awaiting maintainer feedback Issue needs response from a maintainer. labels Dec 17, 2015
@vitorgalvao
Copy link
Member

I like the sentiment a lot. I’d change the solution a bit, though, since I perceive a hole as it stands. I’ll explain what I mean so we can discuss it and add/replace/remove from your top post, which should (as we usually do) contain the final decisions at any given point.

Basically, I’d change where (part of) the automation is. Instead of making simple PRs automatically, merge them automatically.

Instead of trying to be "smart" and parse different types of appcasts, go back to having just the stanza, with a required shasum. The type could be left in for future work, but it's not currently being used anywhere.

I’m a big fan of “stopping to try to be smart” for this, as #13201 should illustrate. Being smart before having things that work is what caused a lot of the problems in the first place. I’d go as far as removing that second sentence and removing types altogether, though. However, for the current proposal that wouldn’t work.

The problem is that for us to be able to make an automatic PR, we need to know what the new version is. To know that, we need to parse the appcast, and to do that we need to know how it’s structured, hence the types. However, not every type can be trusted to be consistent. For example, the same type of appcast might be different between apps. For example, github appcasts that contain source releases and different OSs in one app, but not in another. Also, there are non-standard appcasts that will add even more overhead. It might not be possible then, to make this work at all without a lot of workarounds for each app, so we should scrape types altogether.

But now, this also makes it impossible for cask-repair to do that job, because it needs you to input an actual version, as it cannot guess it. So my proposal is to update your proposal as follows:

(updated, see first post)


As for brew-cask-upgrade, just went through it and it’s hacky: It basically just checks /opt/homebrew-cask/Caskroom (so right there it’ll fail if you don’t use our default location) and checks the directory names of apps (since we append the version). It works, but isn’t robust enough to be used officially. In addition, since it relies on the directories in the Caskroom having versions, it’ll completely break after #13201.

@vitorgalvao
Copy link
Member

Thanks to @Amorymeltzer, the first item, “update cask-repair to also update the appcast stanza’s :sha256” is now done.

@adidalal adidalal removed the awaiting maintainer feedback Issue needs response from a maintainer. label Dec 20, 2015
@adidalal
Copy link
Contributor Author

Updated the main post with the revised checklist.

@vitorgalvao
Copy link
Member

At which point cask-repair could be used to submit the PR, set it and forget it. Perhaps it should also be updated (would likely have to use a third-party tool as well, like how prfixmaster uses ghi) to auto-submit the PR, instead of the current behaviour where you still need to go to the actual page and click to submit it.

Just updated cask-repair, and it does this now. At the end of the run, it’ll auto-submit the PR.

@adidalal
Copy link
Contributor Author

@vitorgalvao Tested the updated script with #15992 and #15993, seems to work well.

I'd probably put auto-merging PRs as fairly low-prioirity, as it's already so low friction with fastmerge.

The next big step is to mass-add appcast shasums to Casks without one. AFAIK no Cask hosted on Github has one yet.

@vitorgalvao
Copy link
Member

Tested the updated script with #15992 and #15993, seems to work well.

And I see they also worked to update the appcast’s :sha256, which is also good news.

The next big step is to mass-add appcast shasums to Casks without one.

Agreed.

@vitorgalvao
Copy link
Member

“Add :sha256 to all appcasts” is now a check on every repo.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 7, 2016

If cask-repair is stable enough, maybe it could be added to developer/bin instead of semi-officially living in the wild with a bunch of unrelated scripts?

@vitorgalvao
Copy link
Member

@zmwangx From #16134 (comment):

I have a few scripts that are regularly used by some maintainers (and even users) here, but I keep them in my own repo for a few reasons. Some of them aren’t homebrew-cask specific and can easily benefit other projects and the one that is specific to us (cask-repair) isn’t really targeted for development, and can be used by users. I also very much like to keep them organised and even have formulas for them all all my others “tiny-scripts”. I never gave it any thought, really, but perhaps (at least cask-repair) could make sense here.

It is indeed stable enough. It has been relatively stable ever since its release, and problems are usually caught early (sometimes by @adityadalal924).

That said, I have no quarrel whatsoever with unofficial (or semi-official) projects (and like that there are many other scripts and gems made by users). I also enjoy having cask-repair always on hand (i.e. in my PATH), accessible from a standard location (/usr/local/bin), and especially enjoy having a formula for it.

Like I quoted, it’s not out of the question at all, but making it so will lose (some of) those advantages. The fact that it depends on other formula is a big one, even. Though I check to see if they are installed anyway (in case you installed cask-repair manually), having the formula do it for you is so much more convenient. In the end, stangely and in addition to those reasons, it seems to me that the more frequently a script is used, the stronger the case for it to be outside the repo. It is indeed not a developer script (users use it, and may do so frequently), in contrast to those in /developer/bin which are used occasionally and only when you’re solving a difficult problem (as opposed to “oh, an app update, let me run cask-repair”).

@adidalal
Copy link
Contributor Author

adidalal commented Jan 7, 2016

I am in favor of keeping it in tiny scripts, and I'll look into adding a comment or two about it into contributing.md also

@zmwangx
Copy link
Contributor

zmwangx commented Jan 7, 2016

@vitorgalvao What you said makes sense, but there are two problems:

  1. It's hard to keep it updated. One has to clone your whole repo and manually pull changes; Overlooked the part about having a formula, sorry!
  2. The discoverability is too low, especially if you consider it a user script. It's only mentioned all the way down in maintaining.md — it's hard to imagine users reading maintaining.md, and I wasn't aware of it until being told.

@vitorgalvao
Copy link
Member

The discoverability is too low, especially if you consider it a user script.

That is being addressed by @adityadalal924 in #16511. CONTRIBUTING is really the best place to mention it, anyway. Contributors who just want to bump a version won’t look inside /developer (and why should they), so that doesn’t exactly help discoverability; mentioning it in the documentation does.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 7, 2016

Sure.

@vitorgalvao
Copy link
Member

Removed the idea about the auto-merging bot. Thinking about it last night, I realised it could be potentially dangerous, since we allow pretty much anything in version. Picture this: an app that uses a cloudfront or amazonaws URL and has #{version} pretty early on its path. A malicious user could set a version that would change the actual path (using /s) and point to something hosted by them. With human review (that doesn’t actually take that long for these simple submissions), such a change to version would pop out.

@adidalal
Copy link
Contributor Author

#16529 should help make checking simple anyway - Once Travis lights up green and one of the maintainers gives a quick once-over to the PR, it should be okay to merge (which we generally are pretty quick about, anyway).

The "auto-update" script is still a reasonable idea, and would go well with cask-repair

@vitorgalvao
Copy link
Member

The "auto-update" script is still a reasonable idea

I still intend to work on it after I finish updating all appcasts.

and would go well with cask-repair

Have you cask-repair --help lately (do it after an update, as I recently rename some options)? I’ve added some options that should indeed make it more enjoyable, namely giving version and url directly on the command line, without being prompted for it.

I’m also thinking of adding a --blind-submit option so you don’t even have to y afterwards, but am still thinking if it is sensible. I’ll probably add a time delay so you can cancel if you notice a mistake.

@adidalal
Copy link
Contributor Author

adidalal commented Feb 6, 2016

#18237 finishes up this initiative. Closing in favor of that PR.

@adidalal adidalal closed this as completed Feb 6, 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

No branches or pull requests

4 participants