add cask pre-release check #8351
Conversation
|
Should resolve Homebrew/homebrew-cask#86192 |
SMillerDev
force-pushed
the
audit/pre_release
branch
from
409de1e to
caee684
Compare
| @@ -476,7 +537,6 @@ def check_bitbucket_repository | |||
|
|
|||
| def get_repo_data(regex) | |||
| return unless online? | |||
| return unless new_cask? | |||
There was a problem hiding this comment.
Would be nice to use this function everywhere instead of @new_cask
There was a problem hiding this comment.
I don't understand? This is a removal, so this would allow for that
There was a problem hiding this comment.
Yeh, sorry, I'm pointing out that elsewhere in this file it uses @new_cask when I think it'd be nicer to use new_cask?
There was a problem hiding this comment.
I'm not sure what the difference is, but sure
| @@ -14,6 +14,16 @@ def github_repo_data(user, repo) | |||
| nil | |||
| end | |||
|
|
|||
| def github_release_data(user, repo, tag) | |||
| id = "#{user}/#{repo}/#{tag}" | |||
There was a problem hiding this comment.
I'd be tempted to use the full URL as the key.
|
Does it allow the parameter to disable the check on specific cases? That’s essential, because otherwise we’re trading a few false positives on one repo for a lot of false negatives on another. |
Not sure what's meant by "parameter" here but you can add to an allow list in the code. |
Something like: This feature will often need to be off (e.g. when there’s no stable version yet), so we need to control it on a per-cask basis. |
|
I don't think that's really feasible since it adds a lot of manual work. A better option would be to make this run only on the main cask tap for example. |
If you’re referring to the casks themselves, that’s not an issue. Not having the option to disable it on a per-cask basis is what takes more effort, because it requires checking CI every time to make sure it’s it’s a false negative or not, and requires merging non-passing tests (which happens today). We could do the reverse. If the smart check is off by default and we turn it on when needed, fewer casks will need modification. The trade-off is that occasionally a non-stable version might slip up, but that’s what happens now.
It’s not cut-and-dry. Both repos have casks that may need the option on or off. Without the parameter there’s no reason to have the feature, because we could get better results by selectively using the API endpoint or the atom feed. |
|
I can't really see the reasoning here. Either Homebrew/cask thinks (new) prerelease casks or desirable or they are not. If they are desirable: this shouldn't be an audit at all. If they are not: we should build an allow list of casks currently using prereleases and ensure they are eventually migrated away. |
The problem is that developers aren’t consistent. Some apps only have pre-releases until 1.0 (relegating those to homebrew/cask-versions makes little sense and isn’t what users expect) and others label versions as pre-releases despite them being stable (as defined by the authors themselves). This check isn’t a huge necessity; we’ve been doing without it for years. Rather, it’s a nice-to-have that will save us review time by not having us need to check and overturn CI as often. It will also reduce the amount of cases where we need to merge a PR despite CI failing, which has been stated as a goal. If every developer were consistent, we wouldn’t need a toggle. But because they’re not, if we don’t have a simple way to define exceptions we’re not solving anything, just shifting the problem around. That’s why the issue that defines this feature defines the case on the first post. If code complexity in the core is a big consideration to not have the toggle, it’s better to abandon the feature entirely and go the API-URL-in-appcast route directly (assuming we won’t get into rate-limiting problems). |
|
Developers aren't consistent in the software that core ships either. That's why I think we should just ask them for clarification and not try and fix a human issue with technical solutions.
I'd say this should be exactly what users expect.
You can add it to the whitelist or tell upstream that we can't ship it as a pre-release |
It's not that as much as I think an audit that's sometimes on and sometimes off is more confusing to contributors.
They aren't consistent because we allow them to not be. I think we should be consistent for the benefit of our users. That said, if we choose to ignore this and remove this audit for cask: that seems fine, too. |
But it isn’t. This the way Homebrew Cask has worked forever, from way before it was merged with Homebrew, so that expectation only lowered with time. And I still think it makes sense. If we followed what you’re suggesting, we’d require frequent tap migrations and would get a lot of duplicate submissions to the main repo. Both of those are unnecessary work that mostly affects maintainers.
Perhaps you can explain that better. I’m not following why a whitelist—which is a separate file—is better than a parameter—something we can see at a glance and in scope.
I agree! I refuse plenty of workarounds in casks for that very reason. Every time there’s a bug or oddity in a software and a user tries to circumvent it in the cask, I make a point to not merge and ask them to request the fix upstream, so everyone can benefit. What’s maybe not coming across here is that |
Having a parameter encourages users to "add the parameter to make the error go away".
Agreed, same with audits. Both exist for us to enforce standards we think are beneficial so users can have a consistent experience. To me (although I defer to you): "we don't include prereleases in Homebrew/homebrew-cask" or "we include them" is an easier mental model for contributors and users than "it depends". |
I see your point. Not a problem here because we never ask users to run the Seems like the best course of action here will be to change some high-profile casks to use the API URL, see how that goes, and reevaluate. @core-code Would you need any changes on your checks? |
i guess basically the question is, how many casks that are hosted on github do not have a stable release yet and therefore should (ideally) have this additional check turned off? if its just a few then an additional parameter doesn't make much sense as we can just manually merge them. hard data would be nice (*)
thanks for the heads-up, doesn't look that way ;) __ @SMillerDev the github release API (both asking for all as well just the latest release) will return NOTHING for projects which don't have binaries in their releases. e.g. cask 'zotero' and a few dozen others. a fall-back to the atom feed is necessary here. forgive the noise if you've already taken care of that. (*) ok so i've run a short test to generate some data. i've looked at around 3200 casks. it seems only 28 of those have all releases marked as pre-release and therefore need an exception. i think those are so few that we can approve updates for them manually and don't need a flag: |
I think in line with #8351 (comment) it's pretty reasonable to ask these upstreams to change their naming.
I think that's just if they don't make GitHub releases but tags right? If not, do you have a link for me? Cause that would be an improvement for core as well. |
could be they are just tags, though they show up in the release page and the release atom feed: https://github.com/zotero/zotero/releases versus |
while i generally agree that problems should be fixed upstream, this is not a 'problem'. if a project has no stable release yet, marking it as stable would just be wrong. i don't think we should ask projects to do wrong things just because it makes it easier for us. |
In #8351 (comment) would suggest that sometimes upstream is using pre-releases when they shouldn't be according to the common definition of a pre-release. I don't think they should mark something stable when it isn't, but if they mark something unstable when it isn't that is just as incorrect. Either way, 28/3200 is not a terrible number and I'll leave it up to you how you want to deal with this. |
SMillerDev
force-pushed
the
audit/pre_release
branch
from
caee684 to
0279dda
Compare
|
@core-code @vitorgalvao can I get some yay/nays on this? |
|
i am absolutely positive that we need this feature, many thanks for working on this! regarding the implementation, sorry i cannot give any feedback on ruby code. regarding the recently discussed issue, i think the consensus is that it isn't a problem if we have to manually merge updates for projects that only have 'unstable' releases and will therefore fail this new check. as mentioned it should affect less than 30 casks. so i think further complicating the implementation for these corner cases is not necessary. the only problem i see here is that it seems to be impossible to merge casks that fail CI due to some recent changes made just a few days ago, so that could pose a real problem here. |
Can you link me some more info on this? |
|
there has been some discussion here: |
|
That thread seems to indicate that it's still possible, just difficult. I guess I can merge this then? |
|
Yes, it's now possible if a maintainer applies the |
Co-authored-by: Markus Reiter <me@reitermark.us>
|
Did this end up receiving the per-repo disabling (i.e. don’t do the pre-release check on homebrew/cask-versions)? |
|
It didn't, ik try and fix that today. Unless someone beats me to it. |
brew stylewith your changes locally?brew testswith your changes locally?