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: start building Windows on Arm builds #19780

Merged
merged 9 commits into from
Aug 21, 2019
Merged

build: start building Windows on Arm builds #19780

merged 9 commits into from
Aug 21, 2019

Conversation

jkleinsc
Copy link
Contributor

@jkleinsc jkleinsc commented Aug 15, 2019

Description of Change

Adds configurations to build Windows on Arm builds.
This includes:

  1. a zip manifest for Windows on Arm so that we can verify if the zip manifest contents have changed for Windows on Arm. Resolves this issue: fix: crash reporting on Windows on Arm #19391 (comment)
  2. Appveyor changes to support Windows on Arm builds.
  3. Azure Pipelines configuration to test on Windows on Arm. This test is currently disabled as there are issues running non-interactively: build: start building Windows on Arm builds #19780 (comment). Enabling these tests will be handled under a separate PR.

Checklist

Release Notes

Notes: no-notes

(cherry picked from commit 4064e1f)
@jkleinsc jkleinsc requested a review from a team as a code owner August 15, 2019 18:40
@richard-townsend-arm
Copy link
Contributor

Current Azure Pipeline failure could be related to microsoft/azure-pipelines-tasks#8418 (summary: they're including an x64 version of 7Zip, even for x86 platforms - Windows on Arm does not support emulating x64 stuff).

@richard-townsend-arm
Copy link
Contributor

The current failure looks like VSTS is trying to execute electron.exe without a desktop attached (I saw similar issues trying to run it on a device with a remote session). Downloading dist.zip onto an arm64 device, unpacking it and running electron.exe seems to work fine. The WinAppDriver (https://blogs.windows.com/windowsdeveloper/2019/05/13/announcing-ui-tests-in-ci-cd-for-desktop-app-developers/#Mzaq4j4qGai8lDt2.97) may let you run things interactively.

@richard-townsend-arm
Copy link
Contributor

Actually, I also found https://docs.microsoft.com/en-us/azure/devops/pipelines/agents/v2-windows?view=azure-devops#download-and-configure-the-agent - probably best to make sure that the agent is running in interactive mode (though there are some security implications for doing this).


- script: |
cd src
set npm_config_nodedir=%cd%\out\Default\gen\node_headers
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably also need to set npm_config_arch=arm64 here too if it's not being set somewhere else in the environment.

@jkleinsc jkleinsc changed the title build: add zip manifest for Windows on Arm build: start building Windows on Arm builds Aug 19, 2019
@richard-townsend-arm
Copy link
Contributor

Cool, so the latest test result I have on the master branch (1dc02e6), running on Windows 10 Version 1903 (build 18362.295, current retail release) should have approximately 3-4 unit test failures.

@codebytere codebytere self-requested a review August 20, 2019 18:47
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM, left some minor style comments.

@@ -89,6 +89,10 @@ build_script:
- appveyor PushArtifact out/Default/dist.zip
- appveyor PushArtifact out/Default/chromedriver.zip
- appveyor PushArtifact out/ffmpeg/ffmpeg.zip
- 7z a node_headers.zip out\Default\gen\node_headers
Copy link
Member

Choose a reason for hiding this comment

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

Can you rely on utility from standard powershell https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.archive/compress-archive?view=powershell-6 to avoid some accidental breakage in future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deepak1556 it looks like the version of Powershell we have on our CI vms doesn't support this command: https://ci.appveyor.com/project/electron-bot/electron-ldhmv/builds/26839690

cd src
echo "Running test suite"
.\out\Default\electron.exe --version
.\out\Default\electron.exe electron\spec --ci --enable-logging
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we also run spec-main ? would be better to just call npm run test ?

$localArtifactPath = "$pwd\dist.zip"
$serverArtifactPath = "$env:APPVEYOR_URL/buildjobs/$env:APPVEYOR_JOB_ID/artifacts/dist.zip"
Invoke-RestMethod -Method Get -Uri $serverArtifactPath -OutFile $localArtifactPath -Headers @{ "Authorization" = "Bearer $env:APPVEYOR_TOKEN" }
& "${env:ProgramFiles(x86)}\7-Zip\7z.exe" x -osrc\out\Default -y $localArtifactPath
Copy link
Member

Choose a reason for hiding this comment

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

Same here for 7z

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

@jkleinsc we must make sure that d3dcompiler_47.dll version difference doesn't impact users when relying on our release. I don't have the hardware to test this scenario, a task for followup.

@jkleinsc jkleinsc merged commit 4bc7b3b into master Aug 21, 2019
@release-clerk
Copy link

release-clerk bot commented Aug 21, 2019

No Release Notes

@trop
Copy link
Contributor

trop bot commented Aug 21, 2019

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

@trop
Copy link
Contributor

trop bot commented Aug 21, 2019

I have automatically backported this PR to "7-0-x", please check out #19874

jkleinsc pushed a commit that referenced this pull request Aug 23, 2019
@trop
Copy link
Contributor

trop bot commented Aug 23, 2019

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

jkleinsc pushed a commit that referenced this pull request Aug 26, 2019
* build: start building Windows on Arm builds (#19780)

(cherry picked from commit 4bc7b3b)

* Update manifest to include 6-0-x file
@jkleinsc jkleinsc deleted the add_woa_manifest branch August 26, 2019 20:37
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.

None yet

3 participants