Skip to content

feat: update macOS asset matching #169

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Jan 3, 2025
Merged

feat: update macOS asset matching #169

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0476c06
fix: support latest electron builder default macOS release names
setchy Oct 7, 2024
ff43355
fix: support latest electron builder default macOS release names
setchy Oct 7, 2024
ef38952
fix: update regex to match end of line
setchy Oct 7, 2024
f22e472
fix: update regex to match end of line
setchy Oct 7, 2024
2bdcea8
fix: support latest electron builder default macOS release names
setchy Oct 7, 2024
f2fd8d8
fix: support latest electron builder default macOS release names
setchy Oct 7, 2024
c924e0d
feat: add log statement
setchy Oct 7, 2024
7ab3f84
feat: change where logic guard is for latest universal substitution
setchy Oct 8, 2024
08d4032
feat: change where logic guard is for latest universal substitution
setchy Oct 8, 2024
f08728a
feat: change where logic guard is for latest universal substitution
setchy Oct 8, 2024
8224ad2
test: add unit tests for universal upgrade
setchy Oct 8, 2024
1216fb1
test: add unit tests for universal upgrade
setchy Oct 8, 2024
b2c09c9
incorporate PR feedback on asset matching
setchy Nov 2, 2024
e313059
Merge branch 'fix/electron-builder-default-naming' of https://github.…
setchy Nov 2, 2024
cb7cc68
incorporate PR feedback on asset matching
setchy Nov 2, 2024
cd8e924
incorporate PR feedback on asset matching
setchy Nov 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/asset-platform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
const { PLATFORM_ARCH } = require("./constants");

const assetPlatform = (fileName) => {
if (/.*(mac|darwin|osx).*(-arm).*\.zip/i.test(fileName)) {
return PLATFORM_ARCH.DARWIN_ARM64;
}

if (/.*(mac|darwin|osx).*\.zip/i.test(fileName) && !/arm64/.test(fileName)) {
if (/.*(mac|darwin|osx).*\.zip$/i.test(fileName)) {
if (/universal/.test(fileName)) return PLATFORM_ARCH.DARWIN_UNIVERSAL;
if (/arm64/.test(fileName)) return PLATFORM_ARCH.DARWIN_ARM64;
return PLATFORM_ARCH.DARWIN_X64;
}

Expand Down
1 change: 1 addition & 0 deletions src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const PLATFORM = {
const PLATFORM_ARCH = {
DARWIN_X64: "darwin-x64",
DARWIN_ARM64: "darwin-arm64",
DARWIN_UNIVERSAL: "darwin-universal",
WIN_X64: "win32-x64",
WIN_IA32: "win32-ia32",
WIN_ARM64: "win32-arm64",
Expand Down
19 changes: 17 additions & 2 deletions src/updates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,16 +95,30 @@ class Updates {
}

async handleUpdate(res, account, repository, platform, version) {
const latest = await this.cachedGetLatest(
let latest = await this.cachedGetLatest(
account,
repository,
platform,
version
);

if (!latest && platform.includes(PLATFORM.DARWIN)) {
const latestUniversal = await this.cachedGetLatest(
account,
repository,
PLATFORM_ARCH.DARWIN_UNIVERSAL,
version
);

if (latestUniversal) {
log.info("Falling back to universal build for darwin");
latest = latestUniversal;
}
}

if (!latest) {
const message = platform.includes(PLATFORM.DARWIN)
? "No updates found (needs asset matching *(mac|darwin|osx).*(-arm).*.zip in public repository)"
? "No updates found (needs asset matching *(mac|darwin|osx).*.zip in public repository)"
: "No updates found (needs asset containing win32-{x64,ia32,arm64} or .exe in public repository)";
notFound(res, message);
} else if (semver.lte(latest.version, version)) {
Expand Down Expand Up @@ -210,6 +224,7 @@ class Updates {
for (const release of releases) {
if (
!semver.valid(release.tag_name) ||
semver.lt(release.tag_name, version) ||
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@setchy setchy Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@setchy setchy Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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", ...}

Copy link
Contributor Author

@setchy setchy Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the lt check and changes where the latestUniversal replacement occurs

release.draft ||
release.prerelease
) {
Expand Down
16 changes: 16 additions & 0 deletions test/asset-platform.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,22 @@ test("assetPlatform() matches the right platform", (t) => {
name: "Electron.Fiddle-darwin-x64-0.27.3.zip",
platform: PLATFORM_ARCH.DARWIN_X64,
},
{
name: "Electron-Builder-1.2.3-mac.zip",
platform: PLATFORM_ARCH.DARWIN_X64,
},
{
name: "Electron-Builder-1.2.3-universal-mac.zip",
platform: PLATFORM_ARCH.DARWIN_UNIVERSAL,
},
{
name: "Electron-Builder-1.2.3-arm64-mac.zip",
platform: PLATFORM_ARCH.DARWIN_ARM64,
},
{
name: "Electron.Fiddle-1.62.6-mac.zip.blockmap",
platform: false,
},
];

for (const release of releases) {
Expand Down
8 changes: 4 additions & 4 deletions test/update.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,15 +346,15 @@ test("Updates", async (t) => {
let body = await res.text();
t.equal(
body,
"No updates found (needs asset matching *(mac|darwin|osx).*(-arm).*.zip in public repository)"
"No updates found (needs asset matching *(mac|darwin|osx).*.zip in public repository)"
);

res = await fetch(`${address}/owner/repo-win32-zip/darwin/0.0.0`);
t.equal(res.status, 404);
body = await res.text();
t.equal(
body,
"No updates found (needs asset matching *(mac|darwin|osx).*(-arm).*.zip in public repository)"
"No updates found (needs asset matching *(mac|darwin|osx).*.zip in public repository)"
);
});
});
Expand Down Expand Up @@ -491,7 +491,7 @@ test("Updates", async (t) => {
const body = await res.text();
t.equal(
body,
'Unsupported platform: "linux". Supported: darwin-x64, darwin-arm64, win32-x64, win32-ia32, win32-arm64.'
'Unsupported platform: "linux". Supported: darwin-x64, darwin-arm64, darwin-universal, win32-x64, win32-ia32, win32-arm64.'
);
});
});
Expand All @@ -503,7 +503,7 @@ test("Updates", async (t) => {
const body = await res.text();
t.equal(
body,
'Unsupported platform: "os". Supported: darwin-x64, darwin-arm64, win32-x64, win32-ia32, win32-arm64.'
'Unsupported platform: "os". Supported: darwin-x64, darwin-arm64, darwin-universal, win32-x64, win32-ia32, win32-arm64.'
);
});
});
Expand Down