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 11 #110759

Merged
merged 7 commits into from Nov 23, 2020
Merged

chore: update to electron 11 #110759

merged 7 commits into from Nov 23, 2020

Conversation

deepak1556
Copy link
Contributor

@deepak1556 deepak1556 commented Nov 16, 2020

- Compile
condition: and(succeeded(), eq(variables['VSCODE_COMPILE_ONLY'], 'false'))
pool:
vmImage: macOS-latest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now we are cross-compiling but when ci supports native compilation, it will be using a different pool. Hence this is written as a separate stage.

@deepak1556 deepak1556 force-pushed the robo/update_electron_11 branch 2 times, most recently from f056162 to 86c32f1 Compare November 19, 2020 12:47
@@ -191,9 +191,10 @@
"url": "https://github.com/microsoft/vscode/issues"
},
"optionalDependencies": {
"vscode-windows-ca-certs": "^0.3.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version bump cannot be done in current master because of new napi version.

vmImage: macOS-latest
jobs:
- job: macOSARM64
condition: and(succeeded(), eq(variables['VSCODE_BUILD_MACOS_ARM64'], 'true'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turning this off for now because internal mac arm builds are not available yet, there is active work to get a release out sometime next week.

@deepak1556 deepak1556 force-pushed the robo/update_electron_11 branch 2 times, most recently from efa785e to 04787ef Compare November 20, 2020 07:56
@deepak1556 deepak1556 marked this pull request as ready for review November 20, 2020 19:12
package.json Outdated
@@ -1,7 +1,7 @@
{
"name": "code-oss-dev",
"version": "1.52.0",
"distro": "e289d0d793bde8f8b5632625e01c50c218e7e127",
"distro": "bedb1d87b28d90d8cf83deb8f334816a05980fb4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to fix conflicts with distro and get exploration builds out from this branch, will remove this before merging and fix conflicts on distro branch.

@deepak1556
Copy link
Contributor Author

deepak1556 commented Nov 20, 2020

This is now ready for review, also last night's exploration release was made from this branch

@bpasero - can you review the electron api changes
@joaomoreno - can you review the build pipeline changes and install-update api changes
@chrmarti - can you review vscode-windows-ca-certs changes

Thanks!

@@ -1,3 +1,3 @@
disturl "https://electronjs.org/headers"
target "9.3.3"
target "11.0.2"
Copy link
Member

Choose a reason for hiding this comment

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

I think we are not yet having internal builds for this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, will merge once the builds are available, can expect them in beginning of next week.

Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

} finally {
store.done();
}
assert(certCount > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

On a fresh Windows machine the cert store might have very few certificates because Edge downloads and adds to it on demand. Not sure if it can be empty, but if we see this test failing we need to consider that possibility.

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

lgtm

@bpasero
Copy link
Member

bpasero commented Nov 23, 2020

@deepak1556 looks like we can also remove this code now, no?

const isDev = !environment.isBuilt || isExtensionDevelopment;
if (isMacintosh && isDev) {
return 'native'; // not enabled when developing due to https://github.com/electron/electron/issues/3647
}

@egamma egamma mentioned this pull request Nov 23, 2020
65 tasks
@deepak1556
Copy link
Contributor Author

@deepak1556 looks like we can also remove this code now, no?

Good catch, will remove it. Thanks!

@deepak1556
Copy link
Contributor Author

@sbatten can you review c21f001 which addresses #110759 (comment)

Reason for change: Electron has fixed the issue in electron/electron#26395

@deepak1556 deepak1556 merged commit 0baf6bb into master Nov 23, 2020
@deepak1556 deepak1556 deleted the robo/update_electron_11 branch November 23, 2020 23:31
@kieferrm kieferrm mentioned this pull request Dec 14, 2020
73 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants