-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
Conversation
build/npm/postinstall.js
Outdated
console.log(`$ yarn ${args.join(' ')}`); | ||
const result = cp.spawnSync(yarn, args, opts); | ||
|
||
// Restore the architecture, if temporarily swapped. | ||
process.env["npm_config_arch"] = oldConfigArch |
There was a problem hiding this comment.
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.
There was a problem hiding this 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]
@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. |
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. |
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. |
edd2cd7
to
52d3410
Compare
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... |
52d3410
to
40d74d4
Compare
Cool, looks like the CI has come back clean this time. Ready for review. |
Hi guys, happy new year! Any review thoughts? |
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 . |
Indeed do we have a date estimate for review @joaomoreno / @deepak1556 ? It appears that @richard-townsend-arm has completed your changes. |
There was a problem hiding this 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
I'm not authorized to merge this change atm. |
It needs @joaomoreno to review it before you can merge. |
Would love to see this get merged soon! Built from source but am unable to use the remote development extensions :/ |
There was a problem hiding this 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.
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. |
@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. |
40d74d4
to
8b73106
Compare
@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. ❤️ |
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. |
I hope @joaomoreno is alright 😕, hopefully they're just on vacation and not hit with the virus. |
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. |
@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:
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. |
There was a problem hiding this 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.
build/gulpfile.reh.js
Outdated
@@ -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' }, |
There was a problem hiding this comment.
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/lib/electron.ts
Outdated
@@ -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> { |
There was a problem hiding this comment.
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.
build/npm/postinstall.js
Outdated
if (result.error || result.status !== 0) { | ||
process.exit(1); | ||
} | ||
} | ||
|
||
function remoteOpts() { |
There was a problem hiding this comment.
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.
May I ask, how is anybody testing this? With what hardware? |
Surface Pro X here.
…-------- Original message --------
From: Jeremy Sinclair <[email protected]>
Date: 8/4/20 10:05 pm (GMT+10:00)
To: microsoft/vscode <[email protected]>
Cc: Hon Weng Chong <[email protected]>, Comment <[email protected]>
Subject: Re: [microsoft/vscode] Add gulp targets, fix build for Windows on Arm. (#85326)
I've tested this compiling it from my ASUS NovaGo and most recently my Surface Pro X using the instructions above. I was able to compile a build from this PR while even merging master into it.
Sent from Outlook Mobile<https://aka.ms/blhgte>
________________________________
From: João Moreno <[email protected]>
Sent: Wednesday, April 8, 2020 8:01:19 AM
To: microsoft/vscode <[email protected]>
Cc: Jeremy Sinclair <[email protected]>; Mention <[email protected]>
Subject: Re: [microsoft/vscode] Add gulp targets, fix build for Windows on Arm. (#85326)
May I ask, how is anybody testing this? With what hardware?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fvscode%2Fpull%2F85326%23issuecomment-610917179&data=02%7C01%7C%7Cbe342319d2e34384e9b508d7dbb48cee%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637219440805126929&sdata=8J34yN2lUCK6PKWFOzqx9HrbyRH7dX9ULj128mD8ZTY%3D&reserved=0>, or unsubscribe<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAA6URJPNA7XG4HZLKFYMDOTRLRRQ7ANCNFSM4JQGXUWQ&data=02%7C01%7C%7Cbe342319d2e34384e9b508d7dbb48cee%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637219440805136926&sdata=aEGuSLMtTvUyg0kaLY47YFvRZb6NOAZ6jLcKsRNEINI%3D&reserved=0>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#85326 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAC734VEOOTYUVMSAICUWVDRLRSB3ANCNFSM4JQGXUWQ>.
|
Maybe Microsoft should send Surface Pro X's to the entire development team of VS code! |
Got it. Let's see if I can get my hands on one of those. |
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:
Qualcomm 850 based:
Qualcomm 835 based:
|
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 |
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. |
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. |
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 😊 |
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: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.