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

Added ARM64 build support #52119

Merged
merged 4 commits into from Sep 11, 2018

Conversation

Projects
None yet
@ava1ar
Contributor

ava1ar commented Jun 17, 2018

Changes required to build master on arm64 machine (Samsung Chromebook+ with ArchLinux running inside crostini container was used). Builds and runs fine with the proposed changes.

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Jun 17, 2018

CLA assistant check
All CLA requirements met.

msftclas commented Jun 17, 2018

CLA assistant check
All CLA requirements met.

@jkol

This comment has been minimized.

Show comment
Hide comment
@jkol

jkol Jul 6, 2018

This is missing an arm64 entry in the object inside of getDebPackageArch in build\gulpfile.vscode.linux.js

jkol commented Jul 6, 2018

This is missing an arm64 entry in the object inside of getDebPackageArch in build\gulpfile.vscode.linux.js

@manfredbrandl

This comment has been minimized.

Show comment
Hide comment
@manfredbrandl

manfredbrandl Jul 17, 2018

Don‘t we need to update to Electron 2.0.x #52782 with this?

manfredbrandl commented Jul 17, 2018

Don‘t we need to update to Electron 2.0.x #52782 with this?

@ava1ar

This comment has been minimized.

Show comment
Hide comment
@ava1ar

ava1ar Jul 17, 2018

Contributor

@manfredbrandl it was updated already when the PR was submitted, but got reverted later...

Contributor

ava1ar commented Jul 17, 2018

@manfredbrandl it was updated already when the PR was submitted, but got reverted later...

@manfredbrandl

This comment has been minimized.

Show comment
Hide comment
@manfredbrandl

manfredbrandl Jul 31, 2018

In #54873 Electron was updated to 2.0.5 .

manfredbrandl commented Jul 31, 2018

In #54873 Electron was updated to 2.0.5 .

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Aug 2, 2018

Member

Guys, no need to keep this PR up-to-date. Supporting ARM64 is more than just accepting a PR. It requires us to update our release and test channels and commit to supporting it in the long run, which is not a development decision. The work here is awesome in which it contains the engineering effort to enable it, but there's a lot more to accepting such a change.

Member

joaomoreno commented Aug 2, 2018

Guys, no need to keep this PR up-to-date. Supporting ARM64 is more than just accepting a PR. It requires us to update our release and test channels and commit to supporting it in the long run, which is not a development decision. The work here is awesome in which it contains the engineering effort to enable it, but there's a lot more to accepting such a change.

@ava1ar

This comment has been minimized.

Show comment
Hide comment
@ava1ar

ava1ar Aug 2, 2018

Contributor

@joaomoreno I am making arm64 builds of vscode for myself, so just wanted to share the required changes with the community, so may be somebody else use it and start getting own builds with arm64. I perfectly understand, that new architecture unboarding requires lot of things to be done, so feel free to do whatever you want with this PR.

Contributor

ava1ar commented Aug 2, 2018

@joaomoreno I am making arm64 builds of vscode for myself, so just wanted to share the required changes with the community, so may be somebody else use it and start getting own builds with arm64. I perfectly understand, that new architecture unboarding requires lot of things to be done, so feel free to do whatever you want with this PR.

@magic-k

This comment has been minimized.

Show comment
Hide comment
@magic-k

magic-k Aug 9, 2018

Allowing an easy way to compile for arm and making arm builds official is not the same thing. I can build for arm64 already out of the box with the current master. This PR simply adds the components for arm64 to build packages. BTW the same lines for armhf are already there.
@joaomoreno i guess you confuse this PR with issues that want official arm builds.

magic-k commented Aug 9, 2018

Allowing an easy way to compile for arm and making arm builds official is not the same thing. I can build for arm64 already out of the box with the current master. This PR simply adds the components for arm64 to build packages. BTW the same lines for armhf are already there.
@joaomoreno i guess you confuse this PR with issues that want official arm builds.

@glebm

This comment has been minimized.

Show comment
Hide comment
@glebm

glebm Aug 16, 2018

@ava1ar The produced deb package is invalid. Adding an architecture mapping fixes it:

sed -i 's|arm: .armhf.|&, arm64: "arm64"|' build/gulpfile.vscode.linux.js

With the mapping added, the deb package seems to work just fine!

glebm commented Aug 16, 2018

@ava1ar The produced deb package is invalid. Adding an architecture mapping fixes it:

sed -i 's|arm: .armhf.|&, arm64: "arm64"|' build/gulpfile.vscode.linux.js

With the mapping added, the deb package seems to work just fine!

@glebm

This comment has been minimized.

Show comment
Hide comment
@glebm

glebm Aug 17, 2018

@roblourens Looks like ripgrep 0.9.0 release is missing the arm64 zip https://github.com/roblourens/ripgrep/releases/tag/0.9.0

glebm commented Aug 17, 2018

@roblourens Looks like ripgrep 0.9.0 release is missing the arm64 zip https://github.com/roblourens/ripgrep/releases/tag/0.9.0

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Aug 17, 2018

Member

arm64 should be there now.

Member

roblourens commented Aug 17, 2018

arm64 should be there now.

@glebm

This comment has been minimized.

Show comment
Hide comment
@glebm

glebm commented Aug 17, 2018

@roblourens Thanks!

@tingxueronghua

This comment has been minimized.

Show comment
Hide comment
@tingxueronghua

tingxueronghua Sep 7, 2018

So, is this problem solved?

tingxueronghua commented Sep 7, 2018

So, is this problem solved?

@magic-k

This comment has been minimized.

Show comment
Hide comment
@magic-k

magic-k Sep 8, 2018

No. Only ripgrep for arm64. Microsoft has currently not the ressources to maintain arm builds. And obviously it's not very high on theit priority list. We have to wait fo arm64 windows notebooks :-)
I track vscode on my own git repo with the arm64 patches. So i can regulary build it on my own.

magic-k commented Sep 8, 2018

No. Only ripgrep for arm64. Microsoft has currently not the ressources to maintain arm builds. And obviously it's not very high on theit priority list. We have to wait fo arm64 windows notebooks :-)
I track vscode on my own git repo with the arm64 patches. So i can regulary build it on my own.

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 9, 2018

Contributor

I'd be really interested in finding out which doors need to be knocked on to get this supported officially.

I've been working to keep my cross-compiled ARM and ARM64 builds up-to-date, but as the build process deviates it gets harder to do so.

There's basically two routes to take at the moment:

  1. Building natively on an ARM/ARM64 device (as in this thread) which I think should be supported in any case.
  2. Cross-compiling from x64 to ARM/ARM64 so that it can be integrated into the existing CI pipelines (which seem to have moved to VSTS now?).

The work to support both of these avenues is now in place with the community so it would really just be a case of having Microsoft take this up.

Obviously committing to support any ARM variation officially is a big deal, but is it really that significant to just add a few lines of code here and there to support building with the CI pipeline and not releasing officially, if such changes don't impact on the official builds? (Some changes have been merged already, but not enough to build a fully native VS Code without a few further edits).

Speaking personally I have folks waiting on the ARM builds I've worked on up until now to use them in an educational setting (Chromebooks) and it's hard to communicate that it's getting more difficult to maintain them over time as the codebase is obviously growing, and due to the lack of being able to merge the compile-time support in.

Any help is much appreciated.

Contributor

headmelted commented Sep 9, 2018

I'd be really interested in finding out which doors need to be knocked on to get this supported officially.

I've been working to keep my cross-compiled ARM and ARM64 builds up-to-date, but as the build process deviates it gets harder to do so.

There's basically two routes to take at the moment:

  1. Building natively on an ARM/ARM64 device (as in this thread) which I think should be supported in any case.
  2. Cross-compiling from x64 to ARM/ARM64 so that it can be integrated into the existing CI pipelines (which seem to have moved to VSTS now?).

The work to support both of these avenues is now in place with the community so it would really just be a case of having Microsoft take this up.

Obviously committing to support any ARM variation officially is a big deal, but is it really that significant to just add a few lines of code here and there to support building with the CI pipeline and not releasing officially, if such changes don't impact on the official builds? (Some changes have been merged already, but not enough to build a fully native VS Code without a few further edits).

Speaking personally I have folks waiting on the ARM builds I've worked on up until now to use them in an educational setting (Chromebooks) and it's hard to communicate that it's getting more difficult to maintain them over time as the codebase is obviously growing, and due to the lack of being able to merge the compile-time support in.

Any help is much appreciated.

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 10, 2018

Member

Yeah, you are right, we can just merge this without actually releasing official builds. Would that be enough for you guys? Why wouldn't you simply have a fork of this repository with these changes on top, and build from there?

Member

joaomoreno commented Sep 10, 2018

Yeah, you are right, we can just merge this without actually releasing official builds. Would that be enough for you guys? Why wouldn't you simply have a fork of this repository with these changes on top, and build from there?

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 10, 2018

Contributor
Contributor

headmelted commented Sep 10, 2018

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 10, 2018

Member

Wait. Please do not start any work. I was talking about this extremely minimal PR. What do you have in mind instead?

Member

joaomoreno commented Sep 10, 2018

Wait. Please do not start any work. I was talking about this extremely minimal PR. What do you have in mind instead?

@glebm

This comment has been minimized.

Show comment
Hide comment
@glebm

glebm Sep 10, 2018

This PR has one small issue, #52119 (comment)

glebm commented Sep 10, 2018

This PR has one small issue, #52119 (comment)

@ava1ar

This comment has been minimized.

Show comment
Hide comment
@ava1ar

ava1ar Sep 10, 2018

Contributor

@glebm @joaomoreno Let me fix the mentioned issue and resolve the merge conflict, give me an hour or so.

Contributor

ava1ar commented Sep 10, 2018

@glebm @joaomoreno Let me fix the mentioned issue and resolve the merge conflict, give me an hour or so.

Fix arm64 deb (thanks to @glebm for the hint)
Removed unused flatpack clean task
Updated electron version for tests
@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 10, 2018

Contributor

@joaomoreno sorry, replied to the email on my phone and got threads crossed.

There are a few other additions here and there needed for ARM64 that I've identified. In fairness, I'm exclusively cross-compiling from x64 so it may be the case that they only impact that scenario.

Basically, this surrounds issues about what "arm" means to the packaging system on the OS - generally it's taken as an alias for "armhf" or "armv7l", neither of which would work on an ARMV8/ARM64 device verbatim. I wouldn't expect this to affect someone compiling directly on ARM64 hardware, as this would be picked up from the OS in that instance.

I previously maintained a fork, the problem with that was that these are not significant changes (effectively just cleaning up a few "arm" references), so it was a bit of a waste for something that really belongs in the core codebase.

In any case, when I've ironed out the details I'll submit a PR that should clarify - as said it's very minimal changes so that you folks can decide if you want to include it or not.

Apologies if this hijacked the thread, just wanted to clarify.

Contributor

headmelted commented Sep 10, 2018

@joaomoreno sorry, replied to the email on my phone and got threads crossed.

There are a few other additions here and there needed for ARM64 that I've identified. In fairness, I'm exclusively cross-compiling from x64 so it may be the case that they only impact that scenario.

Basically, this surrounds issues about what "arm" means to the packaging system on the OS - generally it's taken as an alias for "armhf" or "armv7l", neither of which would work on an ARMV8/ARM64 device verbatim. I wouldn't expect this to affect someone compiling directly on ARM64 hardware, as this would be picked up from the OS in that instance.

I previously maintained a fork, the problem with that was that these are not significant changes (effectively just cleaning up a few "arm" references), so it was a bit of a waste for something that really belongs in the core codebase.

In any case, when I've ironed out the details I'll submit a PR that should clarify - as said it's very minimal changes so that you folks can decide if you want to include it or not.

Apologies if this hijacked the thread, just wanted to clarify.

@ava1ar

This comment has been minimized.

Show comment
Hide comment
@ava1ar

ava1ar Sep 10, 2018

Contributor

@headmelted sounds like extra things you mention are for cross-compilation only, since I am able to successfully build the arm64 version of the vscode using only changes from this PR. I can include required changes to this PR if there are no objections from @joaomoreno and they are small. Otherwise let's split them into different PR to merge this first and cross-compilation related stuff later.

Contributor

ava1ar commented Sep 10, 2018

@headmelted sounds like extra things you mention are for cross-compilation only, since I am able to successfully build the arm64 version of the vscode using only changes from this PR. I can include required changes to this PR if there are no objections from @joaomoreno and they are small. Otherwise let's split them into different PR to merge this first and cross-compilation related stuff later.

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 10, 2018

Contributor
Contributor

headmelted commented Sep 10, 2018

@ava1ar

This comment has been minimized.

Show comment
Hide comment
@ava1ar

ava1ar Sep 10, 2018

Contributor

So, the PR is ready. Let me know if anything still needs to be done to merge it. Thanks!

Contributor

ava1ar commented Sep 10, 2018

So, the PR is ready. Let me know if anything still needs to be done to merge it. Thanks!

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 11, 2018

Contributor

There's a couple of changes I'd make in addition to this that mirror some of the edits in PR #24943. I don't think that PR is likely to be accepted now (it's been some time since it was submitted, and I think the builds have migrated to VSTS from Travis in that time).

Everything in this PR seems fine, so if you guys are happy to merge this PR for ARM64 on ARM64 builds, I can submit a second one with the extra changes needed for ARM64 cross-compiling (they won't impact on other builds, and are quite minor).

If I copy over my additions into a new PR before this PR is merged it'll just replace @ava1ar 's work and I don't want to do that as there's nothing wrong with it and I'd feel like a jackass.

Hope that all makes sense. 👍

Contributor

headmelted commented Sep 11, 2018

There's a couple of changes I'd make in addition to this that mirror some of the edits in PR #24943. I don't think that PR is likely to be accepted now (it's been some time since it was submitted, and I think the builds have migrated to VSTS from Travis in that time).

Everything in this PR seems fine, so if you guys are happy to merge this PR for ARM64 on ARM64 builds, I can submit a second one with the extra changes needed for ARM64 cross-compiling (they won't impact on other builds, and are quite minor).

If I copy over my additions into a new PR before this PR is merged it'll just replace @ava1ar 's work and I don't want to do that as there's nothing wrong with it and I'd feel like a jackass.

Hope that all makes sense. 👍

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 11, 2018

Member

@ava1ar Looks good, I'll check it asap.

ARM64 cross-compiling

@headmelted Sorry for being blunt, but since we do not produce any output in our continuous build, what use would be this for now?

Member

joaomoreno commented Sep 11, 2018

@ava1ar Looks good, I'll check it asap.

ARM64 cross-compiling

@headmelted Sorry for being blunt, but since we do not produce any output in our continuous build, what use would be this for now?

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 11, 2018

Contributor

@joaomoreno It would really be for the benefit of anyone trying to compile the sources for their own use.

As an example, if I pull the source code on an ARM64 device I can build it (the @ava1ar changes in this PR help with that, and should definitely be merged). If I pull the source code on my laptop to cross-compile to ARM64, the build will fail (as is).

There are at least three reasons I can think of as to why you would want to support this.

  1. If your ARM64 device is at the low-end it could take an age to compile (I can confirm this).
  2. You may want to produce pre-compiled Linux packages for non-technical ARM users (I've been doing this for about a year and a half - specifically for classroom use).
  3. Further to the point above, you may want to set up a continuous process (i.e. continuous delivery). Most providers only offer x86 hardware for this, which means you would need to either cross-compile or use QEMU (speaking from experience, this is even worse than compiling directly on low-end hardware as the emulation is extremely slow - and has timed out on any CI provider I've tried to use, preventing updateable releases).

I don't think this is the place to have this discussion, not least as it's now getting away from @ava1ar 's valid PR here, which was never my intent. If someone on the team is happy to discuss this seperately I'd love to work with you guys to get this working once and for all - but if not I can keep hacking around the scripts at my side to make it work for my own builds as I've been doing up until now.

I'd just add that I know of a lot of people who are relying on the builds I've been doing up to now in exciting, non-traditional scenarios (i.e. ARM and Chromebook devices in the classroom) that would really benefit from having this in place, even if it isn't going to be supported officially (which I understand is a lot more complex bureaucratically than just having builds working).

Anyway, as said this is not the place to discuss, and I didn't mean to hijack. @ava1ar , my apologies. Hope you guys can put me in touch with someone who can help me get this in place. 👍

Contributor

headmelted commented Sep 11, 2018

@joaomoreno It would really be for the benefit of anyone trying to compile the sources for their own use.

As an example, if I pull the source code on an ARM64 device I can build it (the @ava1ar changes in this PR help with that, and should definitely be merged). If I pull the source code on my laptop to cross-compile to ARM64, the build will fail (as is).

There are at least three reasons I can think of as to why you would want to support this.

  1. If your ARM64 device is at the low-end it could take an age to compile (I can confirm this).
  2. You may want to produce pre-compiled Linux packages for non-technical ARM users (I've been doing this for about a year and a half - specifically for classroom use).
  3. Further to the point above, you may want to set up a continuous process (i.e. continuous delivery). Most providers only offer x86 hardware for this, which means you would need to either cross-compile or use QEMU (speaking from experience, this is even worse than compiling directly on low-end hardware as the emulation is extremely slow - and has timed out on any CI provider I've tried to use, preventing updateable releases).

I don't think this is the place to have this discussion, not least as it's now getting away from @ava1ar 's valid PR here, which was never my intent. If someone on the team is happy to discuss this seperately I'd love to work with you guys to get this working once and for all - but if not I can keep hacking around the scripts at my side to make it work for my own builds as I've been doing up until now.

I'd just add that I know of a lot of people who are relying on the builds I've been doing up to now in exciting, non-traditional scenarios (i.e. ARM and Chromebook devices in the classroom) that would really benefit from having this in place, even if it isn't going to be supported officially (which I understand is a lot more complex bureaucratically than just having builds working).

Anyway, as said this is not the place to discuss, and I didn't mean to hijack. @ava1ar , my apologies. Hope you guys can put me in touch with someone who can help me get this in place. 👍

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 11, 2018

Member

@headmelted No worries, it was a great explanation, thanks!

Hope you guys can put me in touch with someone who can help me get this in place. 👍

I've brought it up for discussion. 👍

Member

joaomoreno commented Sep 11, 2018

@headmelted No worries, it was a great explanation, thanks!

Hope you guys can put me in touch with someone who can help me get this in place. 👍

I've brought it up for discussion. 👍

@joaomoreno joaomoreno requested review from jrieken, Tyriar and joaomoreno and removed request for Tyriar Sep 11, 2018

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 11, 2018

Member

@jrieken Asking you to review the version bump of electron-mksnapshot.

Member

joaomoreno commented Sep 11, 2018

@jrieken Asking you to review the version bump of electron-mksnapshot.

@jrieken

👍

@joaomoreno joaomoreno merged commit f3dca9a into Microsoft:master Sep 11, 2018

2 checks passed

VS Code #20180910.29 succeeded with issues
Details
license/cla All CLA requirements met.
Details
@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 11, 2018

Member

In it goes! Thanks! 🍻

Member

joaomoreno commented Sep 11, 2018

In it goes! Thanks! 🍻

@joaomoreno joaomoreno modified the milestones: Backlog, September 2018 Sep 11, 2018

@ava1ar

This comment has been minimized.

Show comment
Hide comment
@ava1ar

ava1ar Sep 11, 2018

Contributor

Thanks a lot for merging this! ARM64 users just became a bit happier :)

Contributor

ava1ar commented Sep 11, 2018

Thanks a lot for merging this! ARM64 users just became a bit happier :)

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 11, 2018

Contributor

Looking at this again I think if I adapt my patches to fit in with this PR (I was using different arm aliases - doesn't really matter) I can get the needed additions down to two lines of JS in the same file. Will add when this is merged in.

Contributor

headmelted commented Sep 11, 2018

Looking at this again I think if I adapt my patches to fit in with this PR (I was using different arm aliases - doesn't really matter) I can get the needed additions down to two lines of JS in the same file. Will add when this is merged in.

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 11, 2018

Contributor

PR raised as #58436.

I can re-work my own scripts to fit in with the scheme used here (as it should be). This should be all that's required to allow me (or anyone else) to cross-compile for ARM and ARM64.

Contributor

headmelted commented Sep 11, 2018

PR raised as #58436.

I can re-work my own scripts to fit in with the scheme used here (as it should be). This should be all that's required to allow me (or anyone else) to cross-compile for ARM and ARM64.

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 11, 2018

Member

@headmelted That's great news, merged it. We can then close #24943 ?

Member

joaomoreno commented Sep 11, 2018

@headmelted That's great news, merged it. We can then close #24943 ?

@headmelted

This comment has been minimized.

Show comment
Hide comment
@headmelted

headmelted Sep 11, 2018

Contributor
Contributor

headmelted commented Sep 11, 2018

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 11, 2018

Member

I updated yarn.lock for electron-mksnapshot

Member

roblourens commented Sep 11, 2018

I updated yarn.lock for electron-mksnapshot

roblourens added a commit that referenced this pull request Sep 11, 2018

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 11, 2018

Member

LGTM 👍

Member

Tyriar commented Sep 11, 2018

LGTM 👍

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Sep 11, 2018

Member

I updated yarn.lock for electron-mksnapshot

D'oh

Member

joaomoreno commented Sep 11, 2018

I updated yarn.lock for electron-mksnapshot

D'oh

@stripedpajamas stripedpajamas referenced this pull request Oct 4, 2018

Open

ARM packages. #6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment