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

chore: update to electron 16 #137241

Merged
merged 125 commits into from
Feb 8, 2022
Merged

chore: update to electron 16 #137241

merged 125 commits into from
Feb 8, 2022

Conversation

deepak1556
Copy link
Collaborator

@deepak1556 deepak1556 commented Nov 15, 2021

Todo:

Fixes #120431
Fixes #133820
Fixes #131714
Fixes #93615
Fixes #136787
Fixes #50386 , Depends on #126113
Fixes #138721
Fixes #84453

@deepak1556 deepak1556 self-assigned this Nov 15, 2021
@deepak1556 deepak1556 added this to the November 2021 milestone Nov 15, 2021
@deepak1556 deepak1556 force-pushed the electron-16.x.y branch 2 times, most recently from bf2fe22 to f704913 Compare November 18, 2021 03:26
@deepak1556 deepak1556 changed the title chore: update to electron 15 chore: update to electron 16 Nov 18, 2021
@rzhao271
Copy link
Contributor

I'm getting an issue where clicking on another file in the file explorer results in several terminals conhost.exe launching and exiting.
Based on the VS Code process explorer as well as the Process explorer tool, the processes seem to be coming from the git extension in the extension host.
I remember with process reuse enabled in Electron 13 on macOS, there were a lot of child git processes that'd be created. On Windows with process reuse enabled in Electron 13, the same thing with the child processes was probably happening, but there wasn't the issue of terminals launching.

Screencap of described issue

@rzhao271
Copy link
Contributor

The issue does not reproduce as much with extensions (including Git, GitHub, and GitHub Authentication) disabled, but even with the extensions disabled, when I launch OSS, I get a command prompt telling me that types from types-registry are being installed, as well as another command prompt from ripgrep.

@deepak1556 deepak1556 marked this pull request as ready for review February 7, 2022 09:36
Copy link
Contributor

@rzhao271 rzhao271 left a comment

Choose a reason for hiding this comment

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

I was interested in seeing what changed. Left some comments

src/vs/base/node/languagePacks.js Outdated Show resolved Hide resolved
@@ -50,7 +50,7 @@ function terminateProcess(process: cp.ChildProcess, cwd?: string): Promise<Termi
if (cwd) {
options.cwd = cwd;
}
const killProcess = cp.execFile('taskkill', ['/T', '/F', '/PID', process.pid.toString()], options);
const killProcess = cp.execFile('taskkill', ['/T', '/F', '/PID', process.pid!.toString()], options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I get a 500 error when trying to view the nodejs docs. Did the potential return values of process.pid change recently? In the worst case, we would need to add more fallback logic if it really is the case that sometimes, process.pid isn't valid anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no behavior change in this version, documentation was updated to reflect the correct behavior and types were updated accordingly nodejs/node@a3c564bead

pid will be undefined only when the child process fails to launch and the process is not connected to a shell. Feel free to update this utility for the case when pid is undefined and that change can target main branch today instead of changing in this PR.

src/bootstrap-fork.js Show resolved Hide resolved
scripts/test-remote-integration.sh Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

👏

@bpasero bpasero merged commit 01df559 into main Feb 8, 2022
@bpasero bpasero deleted the electron-16.x.y branch February 8, 2022 19:09
deepak1556 added a commit that referenced this pull request Feb 24, 2022
This reverts commit 01df559.
This reverts commit 4630133
bpasero added a commit that referenced this pull request Feb 24, 2022
* Revert "chore: update to electron 16 (#137241)"

This reverts commit 01df559.
This reverts commit 4630133

* ci: fix remote compiler for sdl-scan

* chore: fix remote/.yarnrc

* chore: fix build/npm/postinstall.js

* chore: rm crash reporter from shared process

* chore: rm crash reporter from ext host

* chore: fix build/lib/layersChecker.ts

* :chore: preserve some more changes

* fix tests

Co-authored-by: Benjamin Pasero <[email protected]>
@kieferrm kieferrm mentioned this pull request Mar 6, 2022
87 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.