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

Add gulp targets, fix build for Windows on Arm. #85326

Merged
merged 11 commits into from Apr 14, 2020

Conversation

richard-townsend-arm
Copy link
Contributor

@richard-townsend-arm richard-townsend-arm commented Nov 21, 2019

This PR partially fixes #33620, #81854

This PR doesn't get us all the way there because of a problem with vscode-sqlite3. (Essentially, it doesn't build with right version of Visual Studio - 2017 or later is needed, the node-gyp config is slightly wrong and I don't have legal approval to fix it). Once that issue is fixed, you'll be able to do the following to build for Windows on Arm, assuming Node v12.13.1 and node-gyp 5.0.5:

set npm_config_arch=arm64
yarn install --force
yarn run gulp vscode-win32-arm64-archive

Make sure to set up an ARM64 cross-compilation terminal as documented in Electron's upstream instructions.

The result is a zip file which you can manually install and run on the device - seems to work well, built-in extensions like syntax highlighting, PowerShell prompt all seem to function, performance is good too.

PTAL and I'll try to fix any non-SQLite issues which come up. CC @deepak1556

Update: 2020-March-10: the revised SQlite3 module has been published.

@msftclas
Copy link

msftclas commented Nov 21, 2019

CLA assistant check
All CLA requirements met.

console.log(`$ yarn ${args.join(' ')}`);
const result = cp.spawnSync(yarn, args, opts);

// Restore the architecture, if temporarily swapped.
process.env["npm_config_arch"] = oldConfigArch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the build error may come from here. Probably should strengthen the check so it fires only on Windows.

@deepak1556 deepak1556 self-assigned this Nov 22, 2019
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.

Build error is this, notice the undefined:

LINK : fatal error LNK1104: cannot open file 'C:\Users\runneradmin\AppData\Local\node-gyp\Cache\12.4.0\undefined\node.lib' [D:\a\vscode\vscode\remote\node_modules\native-watchdog\build\watchdog.vcxproj]

build/lib/electron.js Outdated Show resolved Hide resolved
build/npm/postinstall.js Outdated Show resolved Hide resolved
@voronoipotato
Copy link

@richard-townsend-arm Don't forget to sign the CLA, that way it will get released :). I'm happy to help test this on my surface pro x.

@richard-townsend-arm
Copy link
Contributor Author

OK, CLA is now signed. I meant to get another patchset out last week, but there've been a few last-minute issues before branching Chromium 80 / Electron 8 which have been consuming my attention. Hopefully get an update out in a few days time.

@bpasero bpasero removed their request for review December 5, 2019 10:05
@bpasero
Copy link
Member

bpasero commented Dec 5, 2019

Change itself looks OK to me, but I think we need to validate the build once we ran it since these changes are all in build scripts. Unassigning myself from review.

@richard-townsend-arm
Copy link
Contributor Author

OK. I'm unsure why the CI is failing to fetch all the deps (something to do with --frozen-lockfile?) There haven't been any changes to packages.json so I'm unsure why it's doing this...

@richard-townsend-arm
Copy link
Contributor Author

Cool, looks like the CI has come back clean this time. Ready for review.

@richard-townsend-arm
Copy link
Contributor Author

Hi guys, happy new year! Any review thoughts?

@snickler
Copy link

snickler commented Jan 8, 2020

I'd love to see this merged in soon. I built from your branch last month and it has been running perfectly for me on my Surface Pro X @richard-townsend-arm .

@voronoipotato
Copy link

Indeed do we have a date estimate for review @joaomoreno / @deepak1556 ? It appears that @richard-townsend-arm has completed your changes.

Copy link
Contributor

@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.

Sorry for the delay on this, LGTM

@richard-townsend-arm
Copy link
Contributor Author

I'm not authorized to merge this change atm.

@snickler
Copy link

It needs @joaomoreno to review it before you can merge.

@nathansoz
Copy link

Would love to see this get merged soon! Built from source but am unable to use the remote development extensions :/

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.

Merging this will only enable building the OSS bits. We will not distribute whatever targets this enables building since that's a much much larger work item and infrastructure investment.

build/gulpfile.reh.js Outdated Show resolved Hide resolved
build/gulpfile.reh.js Outdated Show resolved Hide resolved
build/gulpfile.vscode.win32.js Outdated Show resolved Hide resolved
build/gulpfile.vscode.win32.js Outdated Show resolved Hide resolved
build/gulpfile.vscode.win32.js Outdated Show resolved Hide resolved
build/npm/postinstall.js Outdated Show resolved Hide resolved
@snickler
Copy link

Merging this will only enable building the OSS bits. We will not distribute whatever targets this enables building since that's a much much larger work item and infrastructure investment.

Is there any guidance on the remaining things that would enable a true VS Code build for Windows ARM64? This is something that many of us running WoA would absolutely love to have.

@joaomoreno
Copy link
Member

@snickler As you can imagine, writing the code to build VS Code is only half the story. We have a lot of infrastructure and manual steps to ensure the quality of the bits that we ship to our customers. Basically, if we distribute it, it becomes part of our endgame and continuous testing efforts. We currently don't have the hardware or selfhosting motivation to push this forward. Upvoting #33620 will help with that.

@voronoipotato
Copy link

voronoipotato commented Jan 16, 2020

@joaomoreno Firstly, thanks for taking the time to read and review the pull request. I understand this is a lot of extra work. VS Code and the work you do here is extremely important for Microsoft's Arm initiative. Visual Studio proper can't be meaningfully ported in the near future and emulation of Visual Studio just isn't there yet. Without a tool to write software for windows on ARM there never will be "software for windows on arm". VS Code should be the primary tool for writing code on ARM devices. If you aren't allocated the hardware to test this, and it has downstream impacts that might prevent ARM on windows from being a profitable niche, then that should be remedied. Perhaps you could reach out to the Dot Net team and see what they can do to help connect you with the resources so that ARM has a fair chance in the market. They are currently working on an installer for dotnet core on ARM, and it may be helpful to market this as a coordinated release. Again, thank you for having the patience and compassion to read through the code review. I know you're not heavily incentivized to merge in outside pull requests, so it means a lot. ❤️

dotnet/runtime#1529

@hwchong
Copy link

hwchong commented Mar 13, 2020

Would be great to have this merged. Is it just waiting for @joaomoreno to approve or were more changes requested? It looks like all the checks have passed at this point.

It's sad to think this is being blocked by internal politics at Microsoft.

@harrisonmetz
Copy link

Would be great to have this merged. Is it just waiting for @joaomoreno to approve or were more changes requested? It looks like all the checks have passed at this point.

It's sad to think this is being blocked by internal politics at Microsoft.

Internal politics? The PR is appropriately licensed, and if it works all that is left is to hit the merge button. It's not even that large of a code change. I'd be happy to click the merge button for someone else if they don't feel they can.

@kieferrm kieferrm assigned joaomoreno and unassigned deepak1556 Mar 17, 2020
@kieferrm kieferrm added this to the April 2020 milestone Mar 17, 2020
@voronoipotato
Copy link

I hope @joaomoreno is alright 😕, hopefully they're just on vacation and not hit with the virus.

@voronoipotato
Copy link

voronoipotato commented Mar 31, 2020

hello @joaomoreno ! It appears that you're back. Hopefully all is well. I understand that times have been rough for all and please take the time you need to recenter and breathe. Nobody is as productive as they would like during a pandemic.

If you find that you have the time it would mean a lot to us if you could merge even with an early access or alpha label that way people can use and install it but you don't have as much pressure to work tickets to resolve them, merely merging fixes as you can. Hopefully that could be a decent middle path that allows people to have an option. I personally have not been using my surface pro x as much as I would like in part because it has no native vscode. I really had expected Microsoft to have a unified approach but it appears they have left your team behind in resources. Sorry to hear about that, I think Code is one of Microsoft's best products.

@joaomoreno
Copy link
Member

joaomoreno commented Apr 1, 2020

@voronoipotato, and everyone else: merging this PR does not mean, by all means, that we will start releasing VS Code for ARM. That is a whole different undertaking and requires proper planning and time and resource allocation for our PM team, who is very much aware of the demand for VS Code on ARM for Windows.

This PR will simply enable you to build the Code OSS build for Windows ARM. You can already do that today, you don't need to wait for me:

  1. Clone this repo
  2. Checkout this PR
  3. Build

I will look into merging the PR in April.

@snickler
Copy link

snickler commented Apr 1, 2020

@voronoipotato, and everyone else: merging this PR does not mean, by all means, that we will start releasing VS Code for ARM. That is a whole different undertaking and requires proper planning and time and resource allocation for our PM team, who is very much aware of the demand for VS Code on ARM for Windows.

This PR will simply enable you to build the Code OSS build for Windows ARM. You can already do that today, you don't need to wait for me:

  1. Clone this repo
  2. Checkout this PR
  3. Build

I will look into merging the PR in April.

Nevertheless, we appreciate you looking into this. It'll make it easier for those who wish to create automated Code OSS builds for WoA. After what I've tried to do personally with building some of the components for ARM64, I definitely understand the amount of roadblocks involved with enabling this to be a fully supported VS Code build. After .NET 5 is released, I know a few of those roadblocks will be fixed.

If testers, or ANY help is desired - the team definitely has a group of people they can call upon.

I ask that each of us awaiting this, continue to be patient and contribute any way we can.

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.

I think we should focus on the client bits for now. Let's remove the unnecessary parts and get this in.

@@ -25,6 +25,7 @@ const cp = require('child_process');
const REPO_ROOT = path.dirname(__dirname);

const BUILD_TARGETS = [
{ platform: 'win32', arch: 'arm64', pkgTarget: 'node8-win-arm64' },
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this and focus on the client bits for now.

build/gulpfile.vscode.win32.js Outdated Show resolved Hide resolved
@@ -107,7 +107,7 @@ function getElectron(arch: string): () => NodeJS.ReadWriteStream {
};
}

async function main(arch = process.arch): Promise<void> {
async function main(arch = process.env["npm_config_arch"] || process.arch): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this, since the build isn't fetching electron from here.

if (result.error || result.status !== 0) {
process.exit(1);
}
}

function remoteOpts() {
Copy link
Member

Choose a reason for hiding this comment

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

Again, let's remove this and leave remote for another day.

@joaomoreno
Copy link
Member

May I ask, how is anybody testing this? With what hardware?

@snickler
Copy link

snickler commented Apr 8, 2020 via email

@hwchong
Copy link

hwchong commented Apr 8, 2020 via email

@harrisonmetz
Copy link

Maybe Microsoft should send Surface Pro X's to the entire development team of VS code!

@joaomoreno
Copy link
Member

Got it. Let's see if I can get my hands on one of those.

@pmsjt
Copy link
Member

pmsjt commented Apr 8, 2020

There are quite a few options available for ARM64 based devices. For doing dev work I'd certainly limit the options to those with 8GiB RAM or more. Surface ProX is an obvious choice, especially to MS employees who can get them internally. But the Yoga C630 is an inexpensive candidate, being featured for sale in many stores, being previous generation.

Qualcomm 8cx based:

  • Surface ProX
  • Samsung Galaxy Book S

Qualcomm 850 based:

  • Samsung Galaxy Book 2
  • Lenovo Yoga C630 Snapdragon

Qualcomm 835 based:

  • Asus NovaGo
  • HP Envy x2 Snapdragon
  • Lenovo Miix 630

@voronoipotato
Copy link

wixtoolset/issues#5558

Looks like wix is ready to go for this too!

@snickler
Copy link

wixtoolset/issues#5558

Looks like wix is ready to go for this too!

Yup. Hopefully the 3.14 release comes soon. I was able to pull THIS off using the latest WiX dev release (with a bunch of horrible changes that I could never commit lol)

https://twitter.com/sinclairinat0r/status/1241059487139717121

@joaomoreno joaomoreno merged commit 3b19657 into microsoft:master Apr 14, 2020
@joaomoreno
Copy link
Member

This is now merged, so you should be able to compile VS Code for ARM.

There's a Surface Pro X on the way, so we'll be able to test this soon and evaluate having an official VS Code version.

@voronoipotato
Copy link

voronoipotato commented Apr 14, 2020

Thank you so much João!!! This means so much to me, I really appreciate it. I think once you realize just how amazing it is to not worry about outlets on the go or lugging around a heavy machine you will also come to love the Surface Pro x, and now the battery footprint of VSCode is going to be so tiny! It really is night and day. Truly the killer app for the killer laptop.

@hwchong
Copy link

hwchong commented Apr 14, 2020

Thank you very much for merging this and for working to implement full support for WOA! I've managed to transition almost all my development work to the Surface Pro X with the exception of remote WSL and C/CPP, and so I still keep the x86 binary around for that. Otherwise it works perfectly for most of the other interpreted languages 😊

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.

Support ARM64 build for Windows