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

sococo: update zap #193962

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

zyoshoka
Copy link
Contributor

@zyoshoka zyoshoka commented Dec 3, 2024

Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.

In the following questions <cask> is the token of the cask you're submitting.

After making any changes to a cask, existing or new, verify:

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • Checked the cask was not already refused (add your cask's name to the end of the search field).
  • brew audit --cask --new <cask> worked successfully.
  • HOMEBREW_NO_INSTALL_FROM_API=1 brew install --cask <cask> worked successfully.
  • brew uninstall --cask <cask> worked successfully.

Related to #171006.

The version in the .plist file seems to be outdated, since the newer version appears if you execute console.log(_appVersion) on the client's console, which can be opened from menu bar.

@zyoshoka zyoshoka force-pushed the sococo-update-livecheck branch from 84b0fc9 to c2c8d88 Compare December 3, 2024 04:11
@chenrui333 chenrui333 added the livecheck Issues or PRs related to livecheck label Dec 3, 2024
@zyoshoka zyoshoka changed the title sococo: update livecheck sococo: update livecheck, zap Dec 3, 2024
@bevanjkay
Copy link
Member

Some applications will be out of sync with the client, because they can load a partial update.
If the info.plist is incorrect, then it should be reported to the developer.

@zyoshoka
Copy link
Contributor Author

zyoshoka commented Dec 3, 2024

It is certainly desirable that it be corrected by the developer. In other words, is adopting a version of the client available on the web less reliable than the Info.plist?

@p-linnane p-linnane requested a review from samford December 4, 2024 04:42
Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small suggested tweak to the livecheck block regex, otherwise this looks good to me (on a technical level). I agree that upstream should fix the app plist if the version doesn't correspond to what they're advertising on the website.

Regarding the livecheck block, we only use the ExtractPlist strategy when we can't find accurate version information from any other source. This PR makes it clear that the website provides version information, so it's just a matter of getting upstream to ensure that both the website and app version align.

Casks/s/sococo.rb Outdated Show resolved Hide resolved
@zyoshoka
Copy link
Contributor Author

zyoshoka commented Dec 4, 2024

It is not ensured that the web client's version equals to the desktop version generally (such as Spotify) but in this case I thought adopting the web version is better than plist, because it is same as the desktop version and plist version is incorrect for now. It is certainly possible the consistency could be lost in the furture.

@bevanjkay
Copy link
Member

bevanjkay commented Dec 4, 2024

The version is used to trigger a new download of the desktop client though, not the web client, so I wouldn't say the version found in the web client is relevant to us.

If I understand correctly, updating the version would trigger a re-download of the same desktop client, for anyone using brew upgrade --greedy.

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not ensured that the web client's version equals to the desktop version generally (such as Spotify) but in this case I thought adopting the web version is better than plist, because it is same as the desktop version and plist version is incorrect for now. It is certainly possible the consistency could be lost in the furture.

Ah, I think I'm beginning to understand now. How are you determining that the plist version (from the app) is incorrect? Is it only that it doesn't match the window._appVersion value on the website?

Looking into this more, that global variable is present on more than just the download page, so it seems to be what upstream refers to as the server-side version (corresponding to the versions in the Release Notes), not specifically a version for the desktop app assets on the download page.

From what I'm seeing, it appears that the desktop app has its own versioning scheme that doesn't align with the release versions on the website. For example, the cask was updated to 6.12.2 on 2024-02-09 and the server-side version was something like 6.25.12 at that time (i.e., the 6.25.13 release was on 2022-02-14). There have been a number of server-side releases since then but the desktop app is still 6.12.2, suggesting that the desktop app doesn't follow the same release schedule (unless there's some technical issue with the version used in the desktop app).

As Bevan mentioned, since the cask url is unversioned, changing the cask version is what causes users to redownload the dmg and install the app as a new version. If the version of the app in the dmg hasn't actually changed, updating the cask version would cause users to needlessly redownload the dmg and reinstall the app (thinking they're getting a new version). Similarly, if the app version changes but the _appVersion value on the website doesn't, users would not be updated to a new version when they should be.

We need the version that livecheck surfaces to align with the desktop app version and that doesn't appear to be the case now that I've had a proper look at this (unless I'm misunderstanding or overlooking something). Sorry for any confusion on my part but it looks like we're stuck with ExtractPlist for now.

@zyoshoka
Copy link
Contributor Author

zyoshoka commented Dec 4, 2024

The reason I determined that the version in the Info.plist was incorrect was because it did not match the version that obtained in the desktop app console as I noted in my top comment (and it just happened to match that on the download page), but I'm beginning to think this decision is mistaken, as you say. In the "Updating Sococo Desktop App" page shows an image indicating that the "shell" (desktop) version does not match the server-side version.

I apologize for my lack of confirmation. For now, I'm thinking of changing this PR to only update zap. Would that be okay?

@samford
Copy link
Member

samford commented Dec 4, 2024

For now, I'm thinking of changing this PR to only update zap. Would that be okay?

Sounds good to me 👍

@zyoshoka zyoshoka force-pushed the sococo-update-livecheck branch from b623315 to 0410685 Compare December 4, 2024 14:06
@zyoshoka zyoshoka changed the title sococo: update livecheck, zap sococo: update zap Dec 4, 2024
@zyoshoka
Copy link
Contributor Author

zyoshoka commented Dec 4, 2024

Changed to update only zap. I would like to ask the maintainers to update the checklist in #171006.

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @zyoshoka! I appreciate your understanding as we worked through this.

I updated the linked issue for sococo, so we're good there 👍

@samford samford merged commit 2a293dd into Homebrew:master Dec 4, 2024
11 checks passed
@zyoshoka zyoshoka deleted the sococo-update-livecheck branch December 4, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extract_plist livecheck livecheck Issues or PRs related to livecheck
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants