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

[MM-47279] Upgrade to Electron v21 #2270

Merged
merged 3 commits into from
Oct 11, 2022
Merged

[MM-47279] Upgrade to Electron v21 #2270

merged 3 commits into from
Oct 11, 2022

Conversation

devinbinnie
Copy link
Member

Summary

Upgrading to Electron v21

Ticket Link

https://mattermost.atlassian.net/browse/MM-47279

Upgrade to Electron v21.0.1

@devinbinnie devinbinnie added the 2: Dev Review Requires review by a core committer label Oct 4, 2022
@devinbinnie devinbinnie added this to the v5.2.0 milestone Oct 4, 2022
tboulis
tboulis reviewed Oct 5, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
m1lt0n
m1lt0n approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@m1lt0n m1lt0n left a comment

Choose a reason for hiding this comment

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

Left a minor non-blocking comment about dependencies.

package.json Outdated
@@ -173,6 +173,7 @@
"mmjstool": "github:mattermost/mattermost-utilities#3b4506b0f6b14fbb402f9f8ef932370e459e3773",
"mocha-circleci-reporter": "0.0.3",
"mochawesome": "7.1.3",
"node-abi": "3.25.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

(question) Is the explicit dependency needed here? All the packages that need it have it in their dependencies (e.g. electron-rebuild etc). Is this needed to pin it to a specific version?

Copy link
Member Author

@devinbinnie devinbinnie Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should be able to remove this eventually, but for now it's needed because of how new Electron v21 is. Some of the downstream dependencies are using older versions of node-abi so we need to make sure we have the latest.

@devinbinnie
Copy link
Member Author

So unfortunately this is now blocked due to issues with the NodeJS Native Abstractions, there's a PR waiting to be merged to fix it here: nodejs/nan#943

@devinbinnie devinbinnie added Do Not Merge Should not be merged until this label is removed 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Oct 5, 2022
@devinbinnie devinbinnie mentioned this pull request Oct 5, 2022
Merged
@m1lt0n
Copy link
Contributor

m1lt0n commented Oct 6, 2022

Good to know @devinbinnie , so I guess this one should be closed and the one that upgrades electron to the latest 19.1.1 will be the relevant one.

@devinbinnie
Copy link
Member Author

Good to know @devinbinnie , so I guess this one should be closed and the one that upgrades electron to the latest 19.1.1 will be the relevant one.

@m1lt0n We can leave this one open for the time being, since I'd still like to upgrade to v21 once we're unblocked. It just won't be merged for a while.

@amyblais amyblais removed this from the v5.2.0 milestone Oct 6, 2022
@devinbinnie devinbinnie removed the 4: Reviews Complete All reviewers have approved the pull request label Oct 11, 2022
@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer Build Apps for PR Builds signed builds for testing labels Oct 11, 2022
@mattermod mattermod removed the Build Apps for PR Builds signed builds for testing label Oct 11, 2022
@mattermod
Copy link
Contributor

Building app in separate branch.

@devinbinnie devinbinnie added Build Apps for PR Builds signed builds for testing and removed Do Not Merge Should not be merged until this label is removed labels Oct 11, 2022
@mattermod mattermod removed the Build Apps for PR Builds signed builds for testing label Oct 11, 2022
@mattermod
Copy link
Contributor

Building app in separate branch.

@devinbinnie devinbinnie added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Oct 11, 2022
@mattermod
Copy link
Contributor

Successfully building:
https://circleci.com/gh/mattermost/desktop/23300
https://circleci.com/gh/mattermost/desktop/23299
https://circleci.com/gh/mattermost/desktop/23298

1 similar comment
@mattermod
Copy link
Contributor

Successfully building:
https://circleci.com/gh/mattermost/desktop/23300
https://circleci.com/gh/mattermost/desktop/23299
https://circleci.com/gh/mattermost/desktop/23298

devinbinnie
devinbinnie commented Oct 11, 2022
View reviewed changes
package.json
@@ -174,6 +174,8 @@
"mmjstool": "github:mattermost/mattermost-utilities#3b4506b0f6b14fbb402f9f8ef932370e459e3773",
"mocha-circleci-reporter": "0.0.3",
"mochawesome": "7.1.3",
"nan": "2.17.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

These were added to ensure compatibility with native code.

devinbinnie
devinbinnie commented Oct 11, 2022
View reviewed changes
package.json
@@ -27,7 +27,7 @@
"build": "npm-run-all build:*",
"build:main": "webpack-cli --config webpack.config.main.js",
"build:renderer": "webpack-cli --config webpack.config.renderer.js",
"build-robotjs": "electron-rebuild -v 19.1.2 -m ./node_modules/robotjs",
"build-robotjs": "cross-env CL='/std:c++17' electron-rebuild -v 21.1.0 -m ./node_modules/robotjs",
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to add a compiler flag for building RobotJS on Windows

@devinbinnie
Copy link
Member Author

@amyblais @tboulis @m1lt0n Potential known issue I'd like to call out: I had to blow out my C:\Users\<username>\AppData\Roaming\Electron folder to get things working on Windows. It could have been a fluke with my system, but just in case.

@devinbinnie devinbinnie added this to the v5.2.0 milestone Oct 11, 2022
@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Oct 11, 2022
@mattermod
Copy link
Contributor

Artifact links:
https://output.circle-artifacts.com/output/job/98711560-a26d-40be-894d-82963efa2922/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-linux-x64.tar.gz
https://output.circle-artifacts.com/output/job/902d1dc4-9eff-4f90-9e30-92a8849efa55/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-mac-m1.dmg
https://output.circle-artifacts.com/output/job/902d1dc4-9eff-4f90-9e30-92a8849efa55/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-mac-x64.dmg
https://output.circle-artifacts.com/output/job/2503a39e-e670-4166-a81c-84b3b2de6d65/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-win-ia32.zip
https://output.circle-artifacts.com/output/job/2503a39e-e670-4166-a81c-84b3b2de6d65/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-win-x64.zip

1 similar comment
@mattermod
Copy link
Contributor

Artifact links:
https://output.circle-artifacts.com/output/job/98711560-a26d-40be-894d-82963efa2922/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-linux-x64.tar.gz
https://output.circle-artifacts.com/output/job/902d1dc4-9eff-4f90-9e30-92a8849efa55/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-mac-m1.dmg
https://output.circle-artifacts.com/output/job/902d1dc4-9eff-4f90-9e30-92a8849efa55/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-mac-x64.dmg
https://output.circle-artifacts.com/output/job/2503a39e-e670-4166-a81c-84b3b2de6d65/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-win-ia32.zip
https://output.circle-artifacts.com/output/job/2503a39e-e670-4166-a81c-84b3b2de6d65/artifacts/0/tmp/artifacts/mattermost-desktop-5.2.0-develop.1-win-x64.zip

@mattermod
Copy link
Contributor

Failed to delete ref. @mattermost/core-build-engineers have been notified. Error
DELETE https://api.github.com/repos/mattermost/desktop/git/refs/heads/build-pr-2270-7d1a231: 422 Reference does not exist []

@devinbinnie
Copy link
Member Author

devinbinnie commented Oct 11, 2022 •
edited
Loading

There's an issue with the loading spinner I'm currently sorting out:
image

@devinbinnie devinbinnie merged commit 0ac5360 into master Oct 11, 2022
@devinbinnie devinbinnie deleted the MM-47279 branch October 11, 2022 18:02
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermost-build mattermost-build mentioned this pull request Oct 11, 2022
Merged
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 11, 2022
mattermost-build pushed a commit to mattermost-build/desktop that referenced this pull request Oct 11, 2022
@devinbinnie @mattermost-build
[MM-47279] Upgrade to Electron v21 (mattermost#2270)
db09e43 
* WIP

* Fixed robotjs on windows

(cherry picked from commit 0ac5360)
devinbinnie added a commit that referenced this pull request Oct 11, 2022
@mattermost-build @devinbinnie
[MM-47279] Upgrade to Electron v21 (#2270) (#2275)
6b7d844 
* WIP

* Fixed robotjs on windows

(cherry picked from commit 0ac5360)

Co-authored-by: Devin Binnie <52460000+devinbinnie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m1lt0n m1lt0n m1lt0n approved these changes

@tboulis tboulis tboulis approved these changes

Assignees

@devinbinnie devinbinnie

Labels
4: Reviews Complete All reviewers have approved the pull request CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone release-note
Projects
None yet
Milestone
v5.2.0
Development

Successfully merging this pull request may close these issues.
6 participants
@devinbinnie @m1lt0n @mattermod @tboulis @amyblais @mm-cloud-bot