-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
base: main
Are you sure you want to change the base?
Conversation
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.
🙏 🙏 🙏
Fixes Homebrew/homebrew-cask#218095 too |
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 needs more discussion, sorry.
Are people using brew bump
to open URL-only changes that don't include version changes? This messaging should stick around for e.g. versioning only changes. As-is this seems likely to bring back duplicate PRs.
We should also ensure we're aligning how we handle formulae and casks.
No, it would only be used in the case of version bumps, but I would suggest that a majority of the time, It should not introduce any duplicate PRs as we are still using the duplicate PR checks, that look at the GitHub API. |
This check is not reliable unfortunately.
Thanks. I would suggest we continue to block version-only bumps and basically anything that autobump/livecheck can/will handle and loosen this error instead of removing it entirely. |
@MikeMcQuaid Can you speak a bit more to what you would see as acceptable here? |
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.
If the maintainers who spend the most time on Homebrew/cask think this is a good idea (and it seems like many/most of them do), then we should probably defer to them here.
It's easy to roll back, and we can do that if it turns out to be necessary.
Duplicate PRs aren't really as big of an issue in Homebrew/cask as they are in Homebrew/core (because we don't have self-hosted CI there), in any case.
odie <<~EOS unless cask.tap.allow_bump?(cask.token) | ||
Whoops, the #{cask.token} cask has its version update | ||
pull requests automatically opened by BrewTestBot every ~3 hours! | ||
We'd still love your contributions, though, so try another one | ||
that is excluded from autobump list (i.e. it has 'no_autobump!' | ||
method or 'livecheck' block with 'skip'.) | ||
EOS | ||
|
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
and new_hash
and not new_base_url
. if so: this error message should get triggered if only the version or hash changes but not if the URL changes.
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:
- ensure that the message is printed unless there's additional local changes being committed
- either prompt the user for a new commit message or intelligently fill the commit message to differentiate from a normal "version only" bump
- ensure that the behaviour is mostly consistent with formulae
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.
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.
Why is it no longer on contributors?
Failed autobumps are only really surfaced in CI runs, and I don't think adding these to tracking issues helps contributors get involved.
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?
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.
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 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".
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.
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?
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
than homebrew-core
for upstreams to be inconsistent with url
paths, switching between dmg
and zip
, 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.
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 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.
@bevanjkay Have hopefully done that! Please feel free to ask for any additional clarification. Apologies for previous ambiguity.
@carlocab I disagree. As Project Leader, it is one of my responsibilities to guide, if not make, these sorts of decisions.
It's easy to roll back, technically. Culturally, it's terrible for contributors to have us yo-yo on a day-by-day basis because we've not taken time to properly discuss this. We have had a bunch of complaints about (the opposite of) these sorts of changes in the past. That should encourage us to be slower and more careful making them in future. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?The
no_autobump!
stanza is being removed from all casks inhomebrew-cask
.Because all casks will no be checked on a roughly 3 hour interval, the original motivation for this blocker is likely not going to be an issue moving forward.
Using
brew bump
andbrew bump-cask-pr
can be a valuable way to open PRs easily when Casks require small changes such as a url update as a part of the version bump.CC: @Homebrew/cask