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

Linux update bugs #3082

Closed
philrz opened this issue Jun 6, 2024 · 1 comment · Fixed by #3084
Closed

Linux update bugs #3082

philrz opened this issue Jun 6, 2024 · 1 comment · Fixed by #3084
Assignees
Labels
bug Something isn't working

Comments

@philrz
Copy link
Contributor

philrz commented Jun 6, 2024

tl;dr

The heavy testing I did related to the merge of #3077 led to the discovery of some Zui update issues on Linux. I've concluded that moving to the latest electron-updater version along with a modest set of Zui code changes will address these issues and also put us in position to hopefully start doing auto-updates on Linux soon (#871).

Background

On macOS and Windows our mac-win-updater.ts uses electron-update's autoUpdater. Because that autoUpdater traditionally did not support Linux, we rolled our own linux-updater.ts to act as an "update helper" of sorts. It asks about the latest available release on GitHub by polling an endpoint of the update.electronjs.org service and, if the service responds indicating a newer release is available, the Zui user is shown a pop-up notification. If the user then clicks the Install button in the pop-up, the brimdata.io Download page page is opened in their browser (or the Zui Insiders "latest release" page on GitHub if they're running Insiders).

This has always been a little hacky because our code actually checks for a later non-Linux release (specifically, macOS) on the assumption that a newer Linux artifact of that same release would also be present. This isn't 100% bulletproof, since it's possible for an Actions build job to fail part way through such that it produces usable artifacts for some platforms but not others. But it's generally been seen as better than just requiring Linux users to always manually check for newer releases.

The two problems with this approach described below are both related to this polling of the Electron update server:

return `https://update.electronjs.org/${repo}/${platform}/${app.getVersion()}`

Problem 1 - The new artifact naming convention makes releases look ineligible as update targets

This was a detail of which I was previously unaware, but the way our Linux update helper polls the Electron update server endpoint relies on the existence of an artifact of the form *(mac|darwin|osx).*(-arm).*.zip to consider a release as an eligible update target. This can be seen if we manually construct a URL similar to the one our Linux update helper uses, but point at a repo that has no artifacts at all.

$ curl -w "%{http_code}" https://update.electronjs.org/brimdata/zui-docs-site/darwin-x64/1.2.3
No updates found (needs asset matching *(mac|darwin|osx).*(-arm).*.zip in public repository)404

Before the merge of #3077 (so, Zui Insiders release v1.7.1-19 and older) the generated artifacts always included a file of this format (e.g., Zui---Insiders-1.7.1-19-mac.zip) But since the merge of #3077 (so, Zui Insiders release v1.7.1-20 and newer) the release automation now creates separate ZIP files for each macOS platform (e.g., Zui---Insiders-1.7.1-23-x64.zip and Zui---Insiders-1.7.1-23-arm64.zip). As you can see, these lack the word mac in their names, so without manual intervention or additional change to the build automation, our Linux update helper would not find recent Zui Insiders releases to be eligible update targets.

In my testing of this in personal forks, I've confirmed two additional things:

  1. I can work around the problem by manually downloading the *-x64.zip artifact for a given release, renaming it to *-mac.zip, then uploading it back so it becomes available as an additional release artifact. I plan to do this with the next GA Zui release as a stopgap measure.

  2. If we wanted to maintain our existing Linux update helper and avoid dependency on these ZIP filenames, we could get equivalent functionality by polling an Electron update URL for platform win32-x64 instead of darwin-x64, since Windows updates don't rely on these ZIP files.

All that said, I determined that trying to fix the existing Linux update helper is probably futile due to the problem described next.

Problem 2 - Our approach for polling the Electron update server appears to have always been broken with certain Zui Insiders build numbers

This only affects Zui Insiders releases because I've seen the problem only occurs with "prerelease" version strings (i.e., - followed by digits, such as in v1.7.1-5). My tests show that version strings that lack the hyphen (such as we use for regular Zui) don't have this problem.

We can observe the problem if we manually construct a valid URL that an older Zui Insiders release like v1.7.1-5 would construct upon launch to check if there's a newer release available.

$ curl -w "%{http_code}" https://update.electronjs.org/brimdata/zui-insiders/darwin-x64/1.7.1-5
{"name":"1.7.1-8","notes":null,"url":"https://github.com/brimdata/zui-insiders/releases/download/v1.7.1-8/Zui---Insiders-1.7.1-8-mac.zip"}200

Notice how this shows that the update server believes 1.7.1-8(*) is the newest available update target, despite the fact that there's many releases (-10, -11, …) newer than that. In my testing I've confirmed that this appears to be due to the update server treating the post-hyphen version numbers alphabetically rather than numerically, e.g., -8 looks "higher" than -10. This is an annoying pattern I've seen in other contexts (#2852). It definitely looks like a bug since the semver spec says quite clearly RE: pre-release versions "Identifiers consisting of only digits are compared numerically". We could raise this as a update.electronjs.org issue and/or try to fix it ourselves and submit a PR. But since the update server is ultimately out of our control, it would probably take a while to force that change through and be able to take advantage, and that would all be in pursuit of maintaining a semi-hacky approach. Therefore it seems worth checking if there's another way.

(*) - Note: If there'd been a 1.7.1-9 build that would have been returned as an eligible update target here, but that one doesn't happen to exist due to a failed Actions build run.

Conclusion

Our electron-updater dependency has been pointing at ^4.3.8 and the latest available electron-updater is now 6.2.1. Hoping it could be a quick fix, I tried pointing at this latest electron-updater and making no other Zui code changes to see either/both of these problems went away. Not only did they not go away, I found enough other things had changed between those electron-updater releases that break update logic for non-Linux Insiders releases as well! (I'll cover details about that in an upcoming PR.) However, at that point I'd spent enough time hacking and looking at debug logs that I can see a set of changes that would allow us to jump to the newer electron-updater, drop the buggy parts of our homegrown Linux update helper, and have updates working correctly on all platforms.

A further advantage to doing this is that in recent releases the electron-update autoUpdater has introduced beta support for auto-updates on Linux (electron-userland/electron-builder#7060). My first attempt at using it in a personal fork test didn't work out so maybe it's too early to rely on 100%, but getting us on to a newer electron-update will get us one step closer.

@philrz philrz added the bug Something isn't working label Jun 6, 2024
@philrz philrz self-assigned this Jun 6, 2024
@philrz
Copy link
Contributor Author

philrz commented Jul 16, 2024

Now that some time has passed since our last GA Zui release, I can verify with production Zui Insiders builds that this is all working as planned. Specifically, back when we were still exposed to this issue, when running an Insiders build ending in .9 the auto-update logic would not have seen a release ending in .10 as an eligible update target, but here's proof on Linux when running 1.8.1-insiders.9 that the auto-update notification came through about the available 1.8.1-insiders.10.

image

Here's proof of it still working on macOS as well:

image

And on Windows:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant