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

Add bump-unversioned-casks command. #9423

Merged
merged 16 commits into from Dec 7, 2020

Conversation

reitermarkus
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

As discussed in Homebrew/homebrew-cask#94179 (comment).

@reitermarkus reitermarkus requested a review from a team December 4, 2020 20:43
@BrewTestBot
Copy link
Member

Review period will end on 2020-12-07 at 20:43:04 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 4, 2020
@core-code
Copy link
Contributor

i feel unqualified to comment on ruby code. as mentioned previously i'm still offering the code i have in place for this currently. if you don't need something that actually compiles (only to get the detailed gist of it) this can be done on very short notice. for something that would actually compile i'd need to rip this out from a mountain of dependencies so i'd take longer.

@reitermarkus
Copy link
Member Author

I think the most important part that is (partially) missing is extraction of the version. Any gotchas you encountered in your version that need special handling?

@core-code
Copy link
Contributor

extracting the version number is easy in objc:
[NSDictionary dictionaryWithContentsOfFile:@"path/to/info.plist"]
this nicely deals with both binary as well as xml plists as it used macOS native methods. if this didn't work, macOS couldn't read it either.

on python, you need to jump through some hoops to deal with both text and binary plists, i guess this could be similar in ruby

what i think is more complicated is finding out, and maintaining the currently used 'format' in the version stanza. automated bumps shouldn't disrupt what we've been doing with the cask previously.

for 'us' this is easy. we maintain a database of how the version stanzas 'map' to the actual version numbers embedded to the app. based on the currently known mapping the cask can be updated while maintaining the current format.

if you have troubles there, i can publish our knowledge on how this 'mapping' is done for all casks with static URLs daily if this is helpful for you.

just for reference to the above, 'lossy mapping' means the cask version number has more information than contained in the Info.plist. obviously this can not be bumped automatically by downloading alone.

@reitermarkus reitermarkus force-pushed the bump-unversioned-casks branch 5 times, most recently from 4433458 to fa477ef Compare December 4, 2020 23:22
@vitorgalvao
Copy link
Member

vitorgalvao commented Dec 4, 2020

The mapping shouldn’t be necessary or problematic in this case. Cask versions are weird when url is weird and we change version to accommodate the interpolation. But since this only deals with casks with unversioned url, it means version can be anything, so we should use the pristine CFBundleShortVersionString (or CFBundleShortVersionString,CFBundleVersion, for maximum information) and be done with it.

@core-code
Copy link
Contributor

in any case if you have the intention on change format of the 'version' stanza of those casks please give me some advance notice as this could cause some serious issues on my end. thanks!

@vitorgalvao
Copy link
Member

Changing the format to something specific and predictable seems like the win-win scenario. It’s simpler to automate (and find bugs) and because it’s fixed they can be mapped the same way every time.

Does this also work on unversioned casks with appcast? And if so, will it also remove their sha256?

@reitermarkus
Copy link
Member Author

Does this also work on unversioned casks with appcast? And if so, will it also remove their sha256?

Yes and yes.

@core-code
Copy link
Contributor

seems like the win-win scenario.

if you let me know exactly when everything will be changed before it happens, i am onboard

@reitermarkus reitermarkus force-pushed the bump-unversioned-casks branch 3 times, most recently from ffac6a0 to 4f0d1d7 Compare December 5, 2020 00:26
@reitermarkus
Copy link
Member Author

@reitermarkus reitermarkus force-pushed the bump-unversioned-casks branch 5 times, most recently from f09fb64 to e7fd1f5 Compare December 6, 2020 17:05
@reitermarkus
Copy link
Member Author

@core-code, any pointers on extracting the version from .pkgs containing multiple other .pkgs?

@core-code
Copy link
Contributor

the only one i've found that has the issue is 'iceberg'. i guess since you are also converting casks that were previously :no-version you may have seen more of those.

no solution there, sorry. @vitorgalvao had a look how to support them in pkg-extract but it doesn't seem easily possible

since 'iceberg' was last updated in 2015 i guess this is some obsolete pkg format that is not used anymore so it may not be worthwhile spending too much time on this. did you find other cases?

@reitermarkus
Copy link
Member Author

There are a bunch which install multiple .apps using multiple inner .pkgs, and there is not really a good way to determine which of these is the main .app.

@vitorgalvao
Copy link
Member

There are a bunch which install multiple .apps using multiple inner .pkgs, and there is not really a good way to determine which of these is the main .app.

Not right now, but there could be.

@reitermarkus reitermarkus marked this pull request as ready for review December 7, 2020 22:02
@swrobel
Copy link
Sponsor

swrobel commented Dec 10, 2020

I'm confused by this change in behavior ... It seems that casks with version :latest are being changed to use version numbers, even when they auto-update. For example, this happened to the Spotify cask and now it's trying to install 1.1.47.x over the 1.1.48.x that I currently have installed. Why?

@core-code
Copy link
Contributor

'Spotify' definitely deserves to be an exception. they give any user a random version. you cannot stick a version on it.

@swrobel
Copy link
Sponsor

swrobel commented Dec 10, 2020

Interesting ... still, why do we want the formulae with unversioned download URLs to have a static version number in them? That would mean the cask has to be constantly updated to reflect the latest version number, even when the app auto-updates. It seems that the advice provided here is still relevant, yet this command being run in an automated fashion is changing that implicitly: https://github.com/Homebrew/homebrew-cask/blob/master/doc/development/adding_a_cask.md#examples

@reitermarkus
Copy link
Member Author

even when they auto-update

Whether or not they auto-update is irrelevant. If they have auto_updates true, you will still need --greedy. We want to force an update if we know there has been an update, so a version is preferable to :latest.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 10, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants