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 desktop to Electron v11 #47555

Merged
merged 16 commits into from
Dec 9, 2020
Merged

Update desktop to Electron v11 #47555

merged 16 commits into from
Dec 9, 2020

Conversation

nsakaimbo
Copy link
Contributor

@nsakaimbo nsakaimbo commented Nov 18, 2020

Description

Update to Electron v11 to support Apple Silicon.

This is ready-to-go but hold off on merging so we can stagger this into our next desktop release.

CI

  • Mac
  • Linux (Circle)
  • Linux (TeamCity)

Manual Smoke Testing

  • Mac
  • Linux
  • Windows

@matticbot
Copy link
Contributor

@nsakaimbo nsakaimbo marked this pull request as ready for review November 18, 2020 17:04
@matticbot
Copy link
Contributor

matticbot commented Nov 18, 2020

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-mac".

@nsakaimbo please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@wp-desktop wp-desktop dismissed their stale review November 18, 2020 19:40

wp-desktop ci passing, closing review

@nsakaimbo
Copy link
Contributor Author

nsakaimbo commented Nov 18, 2020

Update:

CI is passing (save for TeamCity), and all platforms seem to be working as expected during manual smoke-testing.

TeamCity CI is failing with the error:

[22:23:39]v12.18.4 is already installed.
[22:23:40]_XSERVTransmkdir: ERROR: euid != 0,directory /tmp/.X11-unix will not be created.
[22:24:16]Error: Command failed: npx mocha /home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/desktop/e2e/tests/e2e.js --timeout 20000 --exit --reporter mocha-multi-reporters --reporter-options configFile=/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/desktop/e2e/mocha-reporter.json
[22:24:16]    at checkExecSyncError (child_process.js:630:11)
[22:24:16]    at execSync (child_process.js:666:15)
[22:24:16]    at run (/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/desktop/e2e/run.js:139:3) {
[22:24:16]  status: 2,
[22:24:16]  signal: null,
[22:24:16]  output: [ null, null, null ],
[22:24:16]  pid: 558,
[22:24:16]  stdout: null,
[22:24:16]  stderr: null
[22:24:16]}
[22:24:16]error Command failed with exit code 1.
[22:24:16]Process exited with code 1

Inspecting the Electron logs of the process indicates this error:

FATAL:platform_shared_memory_region_posix.cc(255)] This is frequently caused by incorrect permissions on /dev/shm

@scinos Any educated guesses here? There's a SO post on this topic - I might take a stab at the --disable-dev-shm-usage Chrome flag.

Some context for the changes in this PR so far:

After upgrading to Electron v11, both TeamCity and Circle were failing with the error:

Unknown input format: 'avfoundation'

This error was resolved after this change, however a new error appeared:

FATAL:platform_shared_memory_region_posix.cc(255)] This is frequently caused by incorrect permissions on /dev/shm (from the Electron logs in Circle)

Removing these additional Chrome arguments seemed to do the trick - Circle is now passing again, but TeamCity is not. 🤔

@scinos
Copy link
Contributor

scinos commented Nov 19, 2020

I'd try --disable-dev-shm-usage. When working on the migration, I saw it used somewhere in CircleCI docker images (I can't find it right now). It is also recommended by Google itself

@nsakaimbo
Copy link
Contributor Author

nsakaimbo commented Nov 19, 2020

I gave --disable-dev-shm-usage a try and the TC build still bombed, however with a more specific error message:

[452:1119/155121.617675:FATAL:setuid_sandbox_host.cc(158)] The SUID sandbox helper binary 
was found, but is not configured correctly. Rather than run without sandboxing I'm aborting now. 
You need to make sure that /home/teamcity-0/buildAgent/work/c4a9d5b38c1dacde/desktop/node_modules/electron/dist/chrome-sandbox 
is owned by root and has mode 4755.

There's an existing thread in the Electron repo about this issue, although I'm yet to understand what's going on exactly...

@scinos
Copy link
Contributor

scinos commented Nov 19, 2020

I think that is caused by the missing --disable-setuid-sandbox, I remember getting that error before.

Our CI image doesn't have sudo, so we can't easily change the permissions of those files. If having sandboxing working is the only option, we would need to rework the CI image.

@nsakaimbo
Copy link
Contributor Author

nsakaimbo commented Nov 19, 2020

Tried --disable-setuid-sandbox and still no dice, but did get the error message:

[452:1119/221402.136001:FATAL:zygote_host_impl_linux.cc(117)] No usable sandbox! 
Update your kernel or see https://chromium.googlesource.com/chromium/src/+/master/docs/linux/suid_sandbox_development.md 
for more information on developing with the SUID sandbox. 
If you want to live dangerously and need an immediate workaround, 
you can try using --no-sandbox.

Trying --no-sandbox now, but wondering if it will conflict with the sandbox: false security setting that we're enabling as part of this upgrade (link).

@nsakaimbo
Copy link
Contributor Author

Update: Based on last CI run, adding --no-sandbox breaks Circle, and the TC build looks like it's attempting to run but fails with the error:

_XSERVTransmkdir: ERROR: euid != 0,directory /tmp/.X11-unix will not be created.

Some cursory web searching seems to indicate this is also a permissions-related error. Reverting e2e app arguments so at least the Circle build is working while we look into an alternate solution for the TC build.

@nsakaimbo nsakaimbo force-pushed the desktop-use-context-bridge branch from 2bf2174 to 8506b82 Compare November 23, 2020 18:05
@nsakaimbo nsakaimbo force-pushed the desktop-electron-v11 branch 2 times, most recently from abe5a50 to 8d64dca Compare November 23, 2020 18:25
@nsakaimbo nsakaimbo changed the title [WIP] desktop: update to Electron v11 desktop: update to Electron v11 Nov 24, 2020
@nsakaimbo nsakaimbo added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 24, 2020
Base automatically changed from desktop-use-context-bridge to trunk November 24, 2020 21:31
@nsakaimbo nsakaimbo force-pushed the desktop-electron-v11 branch from e5c3e3a to e6751e6 Compare November 24, 2020 21:33
@nsakaimbo
Copy link
Contributor Author

Update: TeamCity build is working and these changes are now ready to land.

@nsakaimbo nsakaimbo force-pushed the desktop-electron-v11 branch from 67f9e39 to e642982 Compare December 3, 2020 17:06
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Update: TeamCity build is working and these changes are now ready to land.

👍

@nsakaimbo nsakaimbo force-pushed the desktop-electron-v11 branch from e642982 to ee62c66 Compare December 8, 2020 16:38
@nsakaimbo nsakaimbo changed the title desktop: update to Electron v11 Update desktop to Electron v11 Dec 9, 2020
@nsakaimbo nsakaimbo merged commit 0d61d98 into trunk Dec 9, 2020
@nsakaimbo nsakaimbo deleted the desktop-electron-v11 branch December 9, 2020 18:23
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants