-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
bump*: allow auto bumped casks to be manually bumped #20385
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specific change request: this block be kept but be based on e.g.
new_version
/new_base_url
/new_hash
versions being different below.if my understanding is correct: the autobump workflow will update
new_version
andnew_hash
and notnew_base_url
. if so: this error message should get triggered if only the version or hash changes but not if the URL changes.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite what I was referring to.
A standard workflow (as far as I'm aware based on conversations with other maintainers and contributors) is to adjust the cask file locally with any required
livecheck
,url
, etc. changes that are preventing the autobump from occurring.Then rather than having to download the new version manually, compute the checksum, checkout a branch, create the commit and push, you just run
brew bump <token>
and it pulls the minor adjustments from the "dirty" cask file, with the version bump.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bevanjkay Ok, that seems a bit like an unintentional side-effect; I'm surprised that we're just committing any existing changes and considering it a "bump" when it's not really one any more.
If that's the case we're wanting to handle, though, I'd suggest instead:
how's that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that it is somewhat of a side effect, but the burden of these updates is now almost entirely on maintainers, so efficiency is important to me.
Failed autobumps are only really surfaced in CI runs, and I don't think adding these to tracking issues helps contributors get involved.
If a user notices that their favourite software isn't up to date in Homebrew, they can't run
brew bump
or a similar command to try to update it. Which means that they won't discover the reason it is failing and therefore won't help update it. So now that we're autobumping all but 150 or so Casks, I think our primary concern here is maintainer workload, and not presently duplicate PRs.I'm open to any other insights here, but my fear is that we're pushing contributors out almost entirely, except for in the cases of new submissions and widely publicised updates.
All that to say, there's probably some deeper work required here to keep contributors engaged, and reworking some of our documentation around contributing is timely.
I think your suggestions make sense, and is a good middle ground. But it is a shift from the status quo of the last few years, of including small livecheck or url adjustments in version bumps as they're generally "self-reporting".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it no longer on contributors?
What do the fixes/interventions look like here? Could we look at automating these or e.g. opening a PR that we suspect might be 🔴 but as a draft for a maintainer to update?
Yeh, I agree this is a problem to be solved. Unfortunately I think we've somewhat rushed into a messy situation here where it seems we no longer have data on what autobumps are "valid" and what are "we'll find out next time a bump is attempted". Hopefully this is a temporary state. I'd suggest we leave contributor-centric tooling like this as-is until that's resolved and instead provide e.g. workaround scripts/aliases/whatever for maintainers to use until we're past this stage.
I don't understand how saying "don't merge this PR as-is" can be a shift from the status quo?
It feels like the main status quo change that motivated this is "we're enabling autobump on everything even when we don't know if it works or not" and this means contributors can no longer use the tools that prevent updating autobumped casks, no?
If that's the case: I think we should aim to resolve this temporary situation ASAP before adjusting tooling or we should adjust tooling to handle this temporary state without doing what's here as-is.
If I've misunderstood: my apologies and I'd love to get more context/information/data so I can help with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can explain the maintenance burden, and desire for
brew bump
itself a little better, but I'm travelling for work today and tomorrow, and I want to make sure I don't rush the explanation to create more confusion 😊There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bevanjkay I did a small writeup in Slack about this.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the maintenance burden, my primary concern here is that visibility of failures is only in the Autobump CI workflows, we could have a tracking issue, which may help with visibility, but users/contributors would still be unaware of why Casks aren't being updated because they can't run the
brew bump*
command locally.To give some context around these issues, I believe it is more common in
homebrew-cask
thanhomebrew-core
for upstreams to be inconsistent withurl
paths, switching betweendmg
andzip
, changing the.app
path itself (amongst other issues), all of which cause an "Autobump" to fail.Prior to the blocking of
brew bump*
, we had regular contributors opening PRs to bump their favourite software. To be clear, I'm not suggesting that we return to this as our default, as the BrewTestBot workflow is more efficient.Previously, if a contributor/user realises that a software they use has an update that isn't available yet in Homebrew, in the past they would run
brew bump*
and if there were any issues with this process they may be more inclined to fix it because they can replicate the issue locally.Currently, if a Cask hasn't been updated for a couple of days, it requires knowledge of the Autobump workflow, and where to look for a failure reason, because a contributor can't run
brew bump*
themselves to try to replicate any issue. These issues are being solved, because maintainers are checking the failures in the CI run summaries and resolving them.I would suggest that the autobump-related issues are increased at present, but as referenced above, anecdotally
homebrew-cask
seems to have a higher chance of upstream shifting things around, and introducing inconsistencies that cause autobump failures across new versions.One solution here may be to choose which audit/bump failures prevent BrewTestBot from opening a PR, as some would be better fixed in a draft PR with 🔴 as you have already suggested.
Regarding the overall change in terms of maintainer workload, I would say that I don't feel that it is over the top. But in general I would say that maintainers are more engaged with fixing issues that arise than providing reviews.
I see this across
homebrew-core
also. Maintainers do often have the knowledge and experience necessary to fix issues arising in version bumps, but when BrewTestBot opens a majority of PRs it can make it more difficult for contributors to engage with actual fixes, when they can't push to the PR. I don't see this as a significant issue, but just an observation to note.Please know that none of this is a "complaint" about the way things are moving, but I do have interest in the long-term sustainability of the project and ensuring we continue to engage non-maintainer contributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this makes sense to me.
In that case: a simple version-only bump should fail but a URL change bump should pass. That's not what this PR does as-is but I'd be ✅ to that approach.
This is also an option. I'm happy with whichever is easier to land.
All good! I appreciate you @bevanjkay, thanks for all your great work and the great feedback here ❤️.