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

Update electron-builder to v23.0.6 #3410

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Apr 20, 2022

Pull Request Description

[ci no changelog needed]

Task link.

It fixes the build issue on Mac OS 12.3.1 that is caused by removed /usr/bin/python executable.

Also applied enso-formatter to the sources.

Important Notes

We're basically updating for one major electron-builder release - from v22 to v23. I didn't spot anything in the changelog that could affect us. See features + breaking changes excerpt:

Features:

- feat(msi): add fileAssociation support for MSI target (https://github.com/electron-userland/electron-builder/pull/6530)
- feat(mac): ElectronAsarIntegrity in electron@15 - See: https://github.com/electron/electron/pull/30667 (https://github.com/electron-userland/electron-builder/issues/6506 https://github.com/electron-userland/electron-builder/issues/6507)
- feat(snap): add lzo to Snap compression options (also as new default) (https://github.com/electron-userland/electron-builder/pull/6201) Upgraded app-builder-bin dependency required newer version of Go
- feat(msi): support assisted installer for MSI target (https://github.com/electron-userland/electron-builder/pull/6550)

Breaking changes:

- Removing Bintray support since it was sunset. Ref: https://jfrog.com/blog/into-the-sunset-bintray-jcenter-gocenter-and-chartcenter/
- Fail-fast for windows signature verification failures. Adding -LiteralPath to update file path to disregard injected wildcards
- Force strip path separators for backslashes on Windows during update process
- Authentication for local mac squirrel update server
- Disabled advertised shortcuts, since MSIs with advertised Start Menu shortcuts that have a
    Shortcut Property fails to install when deployed machine-wide via GPO but works fine in all
    other contexts. Admins using advertisement must apply an MST to re-enable it. See https://github.com/electron-userland/electron-builder/issues/6508.
- Removing optional NSIS icon ID from config and generating it automatically to synchronize IDs with Advertised Shortcuts and future features

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Before updating Electron, it is important to go trough changelogs and check if there was any change, even super small one, that could somehow affect Enso. If there are any doubts, please list here all the changes that could theoretically affect Enso and lets check them. If there were no such changes, please write about it, so we know the changelog was checked (see the previous electron update PR to see an example of such analysis).

@vitvakatu vitvakatu marked this pull request as draft April 20, 2022 10:53
@vitvakatu vitvakatu marked this pull request as ready for review April 20, 2022 11:27
@vitvakatu
Copy link
Contributor Author

@wdanilo We're not updating Electron, we're updating electron-builder only.
However, git diff contains changes in the package-lock.json, and it seems like the locked version of Electron is indeed bumped in this PR to version 17.1.0. I'm not sure how did this happen, because we updated Electron to 17.1.0 a few months ago, in PR #3316 you mentioned.

Screenshot 2022-04-20 at 14 29 18

The package-lock.json was not updated in that PR for some reason. @mwu-tow does it mean that we were still using the old Electron version?

Anyway, the update to Electron 17.1.0 has been already approved in #3316. I looked through the Changelog of electron-builder and didn't find anything suspicious. You can find an excerpt in the PR description.

@vitvakatu vitvakatu requested a review from wdanilo April 20, 2022 11:35
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

@vitvakatu oh wow, if we are still using old electron this is bad. Please triple-check if the updated Electron works smoothly with the app and then lets merge it.

@mwu-tow
Copy link
Contributor

mwu-tow commented Apr 20, 2022

The package-lock.json was not updated in that PR for some reason. @mwu-tow does it mean that we were still using the old Electron version?

I'm no JS/NPM expert, but I'd rather expect that the lock file gets overridden locally to fit the requirements.

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Apr 21, 2022
@mergify mergify bot merged commit ea33387 into develop Apr 21, 2022
@mergify mergify bot deleted the wip/vitvakatu/electron-builder-update-181944234 branch April 21, 2022 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants