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

build: bring in a later compiler for Windows on Arm #18591

Merged
merged 2 commits into from Jun 3, 2019

Conversation

@richard-townsend-arm
Copy link
Contributor

richard-townsend-arm commented Jun 3, 2019

Description of Change

Due to a code-gen defect affecting virtual method calls in the official Clang used in the M76 branch, it's necessary to use a slightly newer compiler for Windows. The issue has been fixed upstream, but for various reasons it was not possible to update the compiler to a version that worked on all devices in M76. This is only intended to be a temporary workaround and will be removed when upstream Chromium updates to a Clang version which fixes the problem.

Checklist

Release Notes

Notes: Added experimental support for building for Windows on Arm

Due to a code-generation defect related to virtual method thunks in the
official compiler used for Chromium M76, it's necessary to build for WoA
with a later version of Clang. When running gclient sync, setting
ELECTRON_BUILD_WOA=1 in the environment will download a corrected
compiler which doesn't have this defect.
@richard-townsend-arm richard-townsend-arm requested a review from electron/wg-upgrades as a code owner Jun 3, 2019
@richard-townsend-arm

This comment has been minimized.

Copy link
Contributor Author

richard-townsend-arm commented Jun 3, 2019

@codebytere codebytere requested a review from jkleinsc Jun 3, 2019

[target_os values]: https://gn.googlesource.com/gn/+/master/docs/reference.md#built_in-predefined-variables-target_os_the-desired-operating-system-for-the-build-possible-values
[target_cpu values]: https://gn.googlesource.com/gn/+/master/docs/reference.md#built_in-predefined-variables-target_cpu_the-desired-cpu-architecture-for-the-build-possible-values

#### Windows on Arm (experimental)
To cross-compile for Windows on Arm, [follow Chromium's guide](https://chromium.googlesource.com/chromium/src/+/refs/heads/master/docs/windows_build_instructions.md#Visual-Studio) to get the necessary dependencies, SDK and libraries, then build with `ELECTRON_BUILDING_WOA=1` in your environment before running `gclient sync`.

This comment has been minimized.

Copy link
@felixrieseberg

felixrieseberg Jun 3, 2019

Member

Nit: Newline after title

Suggested change
To cross-compile for Windows on Arm, [follow Chromium's guide](https://chromium.googlesource.com/chromium/src/+/refs/heads/master/docs/windows_build_instructions.md#Visual-Studio) to get the necessary dependencies, SDK and libraries, then build with `ELECTRON_BUILDING_WOA=1` in your environment before running `gclient sync`.
To cross-compile for Windows on Arm, [follow Chromium's guide](https://chromium.googlesource.com/chromium/src/+/refs/heads/master/docs/windows_build_instructions.md#Visual-Studio) to get the necessary dependencies, SDK and libraries, then build with `ELECTRON_BUILDING_WOA=1` in your environment before running `gclient sync`.
Copy link
Member

felixrieseberg left a comment

Looks alright to me!

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Jun 3, 2019

Copy link
Contributor

jkleinsc left a comment

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Jun 3, 2019

Merging as failing mac test is unrelated to PR and known to not work for fork PRs.

@jkleinsc jkleinsc merged commit 3c8acf3 into electron:master Jun 3, 2019
8 of 9 checks passed
8 of 9 checks passed
build-mac Workflow: build-mac
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jun 3, 2019

Release Notes Persisted

Added experimental support for building for Windows on Arm

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Jun 3, 2019

/trop run backport-to 6-0-x

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Jun 3, 2019

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "6-0-x" here we go! :D

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Jun 3, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@trop trop bot added the needs-manual-bp/6-0-x label Jun 3, 2019
@@ -0,0 +1,37 @@
From b3414d055399d0a21f6166a536467ea752b2aa8a Mon Sep 17 00:00:00 2001

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Jun 4, 2019

Member

Just FYI @jkleinsc @felixrieseberg @richard-townsend-arm this patch file is invalid. Although it works it wasn't generated by the export-patches script which means that the next time someone runs import/export on their patch directory this file will change. E.g. The From line here should be 000000000... and the git version 2.19.1.windows.1 shouldn't be present.

./electron/script/git-export-patches -o ./electron/patches/common/chromium is the command to export a patch applied on top of chromium 👍

This comment has been minimized.

Copy link
@richard-townsend-arm

richard-townsend-arm Jun 4, 2019

Author Contributor

Sounds like the kind of thing a presubmit hook should check for :D

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Jun 4, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #18625

@trop trop bot added in-flight/6-0-x and removed needs-manual-bp/6-0-x labels Jun 4, 2019
@sofianguy sofianguy added this to Fixed in 6.0.0-beta.7 in 6.1.x Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6.1.x
Fixed in 6.0.0-beta.7
4 participants
You can’t perform that action at this time.