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.

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

Jump to bottom

feat: use electron-winstaller instead of self module #8344

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

feat: use electron-winstaller instead of self module #8344

wants to merge 37 commits into from
+221 −634

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Jul 17, 2024 •
edited
Loading

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 17, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 10931d5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify Netlify
Copy link

netlify bot commented Jul 17, 2024 •
edited
Loading

❌ Deploy Preview for car-park-attendant-cleat-11576 failed.

Name Link
🔨 Latest commit 10931d5
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/6725b77d6078650008f664ad

@Stanzilla
Copy link

Is this package actually still supported by the electron team? Many packages in that org have been dead for a long time.

@mmaietta
Copy link
Collaborator

Is this package actually still supported by the electron team? Many packages in that org have been dead for a long time.

That's a great point. @beyondkmp what bug fixes is this solving for? (Can't tell from the description)

@mmaietta
Copy link
Collaborator

@Stanzilla looks like updates are still being pushed to electron-winstaller, last update was 3mo ago. https://www.npmjs.com/package/electron-winstaller

@beyondkmp
Copy link
Collaborator Author

beyondkmp commented Jul 20, 2024 •
edited
Loading

Is this package actually still supported by the electron team? Many packages in that org have been dead for a long time.

That's a great point. @beyondkmp what bug fixes is this solving for? (Can't tell from the description)

It's a bit much; we can get it from the release page(https://github.com/Squirrel/Squirrel.Windows/releases).

However, what I want most is to update to the latest commit to include this fix: Squirrel/Squirrel.Windows#1862. My plan is to first update to version 2.0.1. If there are no issues, then update to the latest.

@beyondkmp
Copy link
Collaborator Author

beyondkmp commented Jul 20, 2024 •
edited
Loading

This https://github.com/electron/windows-installer is being updated, but some updates are not timely, and some executable files are almost 10 years old. I have identified two issues and submitted PRs for them.

electron/windows-installer#524
electron/windows-installer#525

Currently, I have forked the repository and published electron-squirrel-winstaller to npm. I want to update this library more quickly, and I will start using the electron-squirrel-winstaller library in our company's app production.

@github-actions GitHub Actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Sep 29, 2024
@github-actions github-actions bot removed the windows label Nov 1, 2024
beyondkmp
beyondkmp commented Nov 1, 2024
View reviewed changes
test/src/helpers/packTester.ts
@@ -376,6 +375,55 @@ async function checkMacResult(packager: Packager, packagerOptions: PackagerOptio
}
}

async function listNupkgContents(nupkgPath: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why switch to the 7z command instead of using decompress-zip? In the newer version, the nupkg files generated will have garbled characters when decompressed by decompress-zip (e.g., the file Test App ßW.exe). Moreover, the decompress-zip library hasn't been updated for 4 years. After reviewing the Squirrel Windows source code, I found that they use the 7z command for compression, so I believe it makes more sense to switch to 7z for decompression here.

packages/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts Show resolved Hide resolved
packages/electron-builder-squirrel-windows/package.json Show resolved Hide resolved
packages/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts Outdated
const vendorDirectory = await getBin(
"Squirrel.Windows-2.0.1",
"https://github.com/beyondkmp/electron-builder-binaries/releases/download/Squirrel.Windows-2.0.1/Squirrel.Windows-2.0.1.7z",
"IGIosfkJ25mhpGS6LREBbaSq4uysb3lwXUzt0psM9UBeaVvpOfDz0ZUqat6WAaji35n0oXJqw63WXT24/7ksLA=="
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got this squirrel release from https://github.com/electron/windows-installer/tree/main/vendor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmaietta mmaietta mmaietta left review comments

Assignees
No one assigned
Labels
app-builder-lib Squirrel.Windows Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@beyondkmp @Stanzilla @mmaietta