feat: update macOS asset matching #169

setchy · 2024-10-07T01:45:43Z

Resolves #168, #140, #166, #113

Updates the macOS (darwin) asset matching logic to

Add support for electron-builder arm64 asset naming convention
Add support for electron-builder universal asset naming convention
Fallback to universal build if no specific macOS release found (ie: arm64 or x64), or universal asset is deemed more recent.
Ignore assets that don't end with .zip (eg: blockmap assets)
Ensure that suggested latest assets do indeed have a newer version than that currently running

Signed-off-by: Adam Setch <[email protected]>

setchy · 2024-10-08T11:47:20Z

Some before and after examples

Scenario	Current Response	PR Response
http://localhost:56669/gitify-app/gitify/darwin-arm64/5.14.0	No updates found (needs asset matching (mac\|darwin\|osx).(-arm).*.zip in public repository)	`{name: "v1.5.0", url: "https://github.com/setchy/atlassify/releases/download/v1.5.0/Atlassify-1.5.0-arm64-mac.zip"}`
http://localhost:56669/setchy/atlassify/darwin-arm64/1.4.0	No updates found (needs asset matching (mac\|darwin\|osx).(-arm).*.zip in public repository)	`{name: "v5.15.0", url: "https://github.com/gitify-app/gitify/releases/download/v5.15.0/Gitify-5.15.0-universal-mac.zip"}`

MarshallOfSound · 2024-10-08T14:50:06Z

src/updates.js

@@ -210,6 +224,7 @@ class Updates {
    for (const release of releases) {
      if (
        !semver.valid(release.tag_name) ||
+        semver.lt(release.tag_name, version) ||


This change in behavior seems unrelated? Can you explain this change and why it's needed in this PR

Thanks for the review @MarshallOfSound - apologies for not calling out this change.

This change was important to ensure that assets from previous releases weren't being matched.

For example, without this change http://localhost:62178/gitify-app/gitify/darwin-x64/5.14.0 and http://localhost:62178/gitify-app/gitify/darwin-arm64/5.14.0 were both finding the "latest" assets from a really old release https://github.com/gitify-app/gitify/releases/tag/v4.2.1 (the last time we published non-universal artifacts).

This then causes the following empty response and inaccurate log message

update.electronjs.org/src/updates.js

Lines 110 to 113 in 3638132

} else if (semver.lte(latest.version, version)) {

log.debug({ account, repository, platform, version }, "up to date");

noContent(res);

} else {

Adding in the above check ensures that only newer versions are considered eligible for the latest assets object

This isn't an issue with your other changes in this PR right, i.e. you don't need this now that you've added universal fallback?

The regex changes seem safe at a glance, but changing this lte from it's original defense location would require more thinking. If you remove this lt change reviewing this PR will be much easier and will be more likely it lands.

Actually, this change is required as part of this PR for it to work.

Without it, latest is not null, therefore it never falls back to checking if the universal release is newer.

eg:
latest === {name: "v4.2.1", ...}
latestUniversal === {name: "v5.15.0", ...}

This logic can either be where I've placed it, or in the universal release block (ie: semver.gt(universal.version, latest.version)...

I went with the former since that is where the other asset tags are skipped over (prerelease, draft, invalid semver)

Hm, can we modify the fallback logic from: "get x64/arm64, if not exists get universal" or just consider unviersal builds arm64/x64 builds by default if no arm64/x64 specific build is present. i.e. it's a fallback within get asset, rather than a separate call?

i.e. in here

update.electronjs.org/src/updates.js

Line 175 in 3638132

return latest && latest[platform];

Just do effective if (!latest[platform] && platform === A_DARWIN_PLATFORM && latest[universal]) return latest[universal]

Removed the lt check and changes where the latestUniversal replacement occurs

Signed-off-by: Adam Setch <[email protected]>

setchy · 2024-10-08T15:33:58Z

src/updates.js

+
+      if (
+        latestUniversal &&
+        semver.gt(latestUniversal.version, latest.version)


@MarshallOfSound - moved the semver check here to be part of the universal replacement logic

Signed-off-by: Adam Setch <[email protected]>

setchy · 2024-10-08T18:28:12Z

Now with added unit tests for the new universal logic

afonsojramos

Great addition, wasn't aware this wasn't supported yet! We're standing on the shoulders of giants!!

setchy · 2024-10-15T17:22:29Z

@MarshallOfSound - any further feedback you have re: this enhancement proposal? Really looking forward to my two user groups getting back on the auto-upgrade pathway

src/asset-platform.js

Signed-off-by: Adam Setch <[email protected]>

…com/setchy/update.electronjs.org into fix/electron-builder-default-naming

src/asset-platform.js

Signed-off-by: Adam Setch <[email protected]>

setchy · 2024-11-15T12:03:16Z

@MarshallOfSound @erickzhao @dsanders11 - any additional feedback you may offer? If not, could we look to have this enhancement released 🙏

erickzhao

Overall this LGTM.

I see that this does disallow the edge case where the file ZIP is just called mac.zip but the regex for win32 still seems to accept something like win32.zip.

Maybe a good follow-up PR to this would be to add the leading - to the Windows regex patterns as well to get parity between platforms.

setchy · 2024-11-24T20:23:21Z

@erickzhao - thanks for the review 🙏

I can certainly raise a separate PR for the windows regex.

For this PR, what are the next steps to have this change merged and released?

erickzhao · 2024-11-25T19:10:52Z

For this PR, what are the next steps to have this change merged and released?

I was going to ask other maintainers if they had feedback but otherwise I don't think there's anything to do on your end!

VerteDinde

Reviewed the code and it looks reasonable - changes asked for in the PR discussions also seem to be addressed

fix: support latest electron builder default macOS release names

0476c06

Signed-off-by: Adam Setch <[email protected]>

setchy requested a review from a team as a code owner October 7, 2024 01:45

fix: support latest electron builder default macOS release names

ff43355

Signed-off-by: Adam Setch <[email protected]>

setchy mentioned this pull request Oct 7, 2024

triage: confirm that auto updater is behaving as expected setchy/atlassify#204

Open

1 task

setchy added 2 commits October 7, 2024 05:04

fix: update regex to match end of line

ef38952

Signed-off-by: Adam Setch <[email protected]>

fix: update regex to match end of line

f22e472

Signed-off-by: Adam Setch <[email protected]>

setchy changed the title ~~fix: support latest electron builder default macOS release names~~ feat: update macOS asset regex to support electron-builder naming convention, universal builds and exclude blockmaps Oct 7, 2024

setchy changed the title ~~feat: update macOS asset regex to support electron-builder naming convention, universal builds and exclude blockmaps~~ feat: update macOS asset regex matching (electron-builder, universal builds, exclude non zips) Oct 7, 2024

setchy added 3 commits October 7, 2024 06:40

fix: support latest electron builder default macOS release names

2bdcea8

Signed-off-by: Adam Setch <[email protected]>

fix: support latest electron builder default macOS release names

f2fd8d8

Signed-off-by: Adam Setch <[email protected]>

feat: add log statement

c924e0d

Signed-off-by: Adam Setch <[email protected]>

MarshallOfSound reviewed Oct 8, 2024

View reviewed changes

setchy requested a review from MarshallOfSound October 8, 2024 15:16

setchy added 3 commits October 8, 2024 11:32

feat: change where logic guard is for latest universal substitution

7ab3f84

Signed-off-by: Adam Setch <[email protected]>

feat: change where logic guard is for latest universal substitution

08d4032

Signed-off-by: Adam Setch <[email protected]>

feat: change where logic guard is for latest universal substitution

f08728a

Signed-off-by: Adam Setch <[email protected]>

setchy commented Oct 8, 2024

View reviewed changes

setchy added 2 commits October 8, 2024 14:18

test: add unit tests for universal upgrade

8224ad2

Signed-off-by: Adam Setch <[email protected]>

test: add unit tests for universal upgrade

1216fb1

Signed-off-by: Adam Setch <[email protected]>

setchy mentioned this pull request Oct 9, 2024

bug: automated update schedule not finding new assets gitify-app/gitify#1576

Open

1 task

afonsojramos reviewed Oct 10, 2024

View reviewed changes

setchy changed the title ~~feat: update macOS asset regex matching (electron-builder, universal builds, exclude non zips)~~ feat: update macOS asset matching Oct 11, 2024

MarshallOfSound reviewed Nov 2, 2024

View reviewed changes

src/asset-platform.js Outdated Show resolved Hide resolved

setchy added 2 commits November 2, 2024 06:25

incorporate PR feedback on asset matching

b2c09c9

Signed-off-by: Adam Setch <[email protected]>

Merge branch 'fix/electron-builder-default-naming' of https://github.…

e313059

…com/setchy/update.electronjs.org into fix/electron-builder-default-naming

setchy requested a review from MarshallOfSound November 2, 2024 10:26

setchy commented Nov 2, 2024

View reviewed changes

src/asset-platform.js Outdated Show resolved Hide resolved

setchy added 2 commits November 2, 2024 06:39

incorporate PR feedback on asset matching

cb7cc68

Signed-off-by: Adam Setch <[email protected]>

incorporate PR feedback on asset matching

cd8e924

Signed-off-by: Adam Setch <[email protected]>

erickzhao self-requested a review November 15, 2024 20:54

erickzhao approved these changes Nov 21, 2024

View reviewed changes

setchy mentioned this pull request Nov 25, 2024

fix: win32 asset matching pattern #175

Open

VerteDinde approved these changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update macOS asset matching #169

feat: update macOS asset matching #169

setchy commented Oct 7, 2024 •

edited

Loading

setchy commented Oct 8, 2024

MarshallOfSound Oct 8, 2024

setchy Oct 8, 2024 •

edited

Loading

MarshallOfSound Oct 8, 2024

setchy Oct 8, 2024 •

edited

Loading

setchy Oct 8, 2024 •

edited

Loading

MarshallOfSound Oct 8, 2024

MarshallOfSound Oct 8, 2024

setchy Oct 8, 2024

setchy Oct 8, 2024

setchy commented Oct 8, 2024 •

edited

Loading

afonsojramos left a comment

setchy commented Oct 15, 2024

setchy commented Nov 15, 2024

erickzhao left a comment

setchy commented Nov 24, 2024

erickzhao commented Nov 25, 2024

VerteDinde left a comment

	} else if (semver.lte(latest.version, version)) {
	log.debug({ account, repository, platform, version }, "up to date");
	noContent(res);
	} else {

feat: update macOS asset matching #169

Are you sure you want to change the base?

feat: update macOS asset matching #169

Conversation

setchy commented Oct 7, 2024 • edited Loading

setchy commented Oct 8, 2024

MarshallOfSound Oct 8, 2024

Choose a reason for hiding this comment

setchy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

MarshallOfSound Oct 8, 2024

Choose a reason for hiding this comment

setchy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

setchy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

MarshallOfSound Oct 8, 2024

Choose a reason for hiding this comment

MarshallOfSound Oct 8, 2024

Choose a reason for hiding this comment

setchy Oct 8, 2024

Choose a reason for hiding this comment

setchy Oct 8, 2024

Choose a reason for hiding this comment

setchy commented Oct 8, 2024 • edited Loading

afonsojramos left a comment

Choose a reason for hiding this comment

setchy commented Oct 15, 2024

setchy commented Nov 15, 2024

erickzhao left a comment

Choose a reason for hiding this comment

setchy commented Nov 24, 2024

erickzhao commented Nov 25, 2024

VerteDinde left a comment

Choose a reason for hiding this comment

setchy commented Oct 7, 2024 •

edited

Loading

setchy Oct 8, 2024 •

edited

Loading

setchy Oct 8, 2024 •

edited

Loading

setchy Oct 8, 2024 •

edited

Loading

setchy commented Oct 8, 2024 •

edited

Loading