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 ARM64 support #641

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add ARM64 support #641

wants to merge 7 commits into from

Conversation

Mattiwatti
Copy link

@Mattiwatti Mattiwatti commented Feb 9, 2021

This PR adds ARM64 support to Open-Shell. See also #553 where this feature was requested.

The changes are not spectacularly interesting and mostly consist of adding a third supported platform to the two existing ones. The only part I'm not quite sure about is the FNV hashes of the MSIs that go into the setup; while it seems to work (the installer works on all three platforms for me) it probably wouldn't hurt if someone double-checked this.

Note that WiX has only added support for ARM64 in v3.14.0.3910. This means I've had to raise the required version used in the build script and documentation from 3.7 to 3.14. Edit: and I see the AppVeyor build failed because of this.

@AppVeyorBot
Copy link

@ge0rdi
Copy link
Member

ge0rdi commented Feb 9, 2021

Note that WiX has only added support for ARM64 in v3.14.0.3910

Unfortunately it seems that AppVeyor images contain 3.11 only :(
Maybe there is some way to ask AppVeyor to update it?

@ge0rdi
Copy link
Member

ge0rdi commented Feb 9, 2021

Maybe there is some way to ask AppVeyor to update it?

Actually WiX 3.14 is just development build, not stable one.
So I don't think AppVeyor will add such.

So I guess for now we cannot merge this to master.

I'd suggest to not build ARM64 by default. Just add some possibility if one would like to try it.
Once WiX ARM64 support will get stable and into AppVeyor images we can enable ARM64 builds by default.

Copy link
Member

@ge0rdi ge0rdi left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍
Pity that WiX ARM64 support is not in stable yet :(

Src/ClassicIE/ClassicIEDLL/ClassicIEDLL.vcxproj.filters Outdated Show resolved Hide resolved
Src/Setup/BuildInstaller.bat Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

@Mattiwatti
Copy link
Author

Re: the AppVeyor build: I agree that it's undesirable to have a build batch file that doesn't work on AppVeyor by default. However I see some problems with the 'optional parameter' approach: the build of Setup.sln will fail if the ARM64 MSI is not built, because a required file will be missing to compile the installer resources.

An option could be to make a separate 'Release with ARM64' configuration for the Setup project, so that the default Release configuration includes only the x86 and x64 MSIs, and only the new configuration would include all three MSIs. But this would require some additional changes to the Setup project.

I see another alternative that may work better: while AppVeyor does not support WiX 3.14, it does provide access to curl which can be used to download files. We use this at ScyllaHide to download dependencies that we cannot include on the repository for legal reasons. I've pushed a new commit to test this. Assuming this works, let me know which approach is preferable.

@AppVeyorBot
Copy link

@ge0rdi
Copy link
Member

ge0rdi commented Feb 11, 2021

Assuming this works, let me know which approach is preferable.

Well, I personally don't like idea of using devel version of software just to support very small amount of users.
I also don't like the idea of adding ARM64 version to common installer by default. As it almost doubles its size (from 7MB to 12MB) and vast majority of users will have to download unnecessarily bigger installer (in fact I don't even like the idea of combined x86/x64 installer, but that's another story).

These are my personal views, maybe @coddec, @XenHat have different views on this.

But to be constructive, what about this:

We can keep installer as it was (x86 + x64) and produce separate ARM64 installer (basically just MSI should be enough).
We can push ARM64 as separate artifact to AppVeyor so ARM64 users will be able to test with it.

Only ARM64 build will then use devel version of WiX (that can be downloaded when needed as you proposed).

What do you think?

@coddec
Copy link
Member

coddec commented Feb 15, 2021

Assuming this works, let me know which approach is preferable.

Well, I personally don't like idea of using devel version of software just to support very small amount of users.
I also don't like the idea of adding ARM64 version to common installer by default. As it almost doubles its size (from 7MB to 12MB) and vast majority of users will have to download unnecessarily bigger installer (in fact I don't even like the idea of combined x86/x64 installer, but that's another story).

These are my personal views, maybe @coddec, @XenHat have different views on this.

But to be constructive, what about this:

We can keep installer as it was (x86 + x64) and produce separate ARM64 installer (basically just MSI should be enough).
We can push ARM64 as separate artifact to AppVeyor so ARM64 users will be able to test with it.

Only ARM64 build will then use devel version of WiX (that can be downloaded when needed as you proposed).

What do you think?

I agree with you @ge0rdi
In fact, It's not rare to see people distribute installers for different architecture, e.g. installerxx_x86.exe installerxx_x86.msi installerxx_x64.exe installerxx_arm.exe etc. You get the idea. So I'll support the idea of separating installers for different architectures if that won't introduce any trouble/huge work load etc.
It saves bandwidth, storage etc. not sure if there's any negative sides.
For ARM, suggestion from @ge0rdi seems fine to me, what do you think? Idea of separating the installers for different architectures and ARM? @XenHat @Ibuprophen

@Mattiwatti
Copy link
Author

Thanks for the feedback. Summarizing, so far it seems like the way forward for ARM64 is to make a separate installer (and maybe even split x86 and x64 into separate installers as well? I'll leave this up to you guys). This is fine by me, and as has been pointed out it will reduce the download size for both binaries.

What should be the preferred way to build the ARM64 setup? Should there be (a) new batch file(s) to create it (think e.g. __MakeFinalARM64.bat and __MakeFinalAllLanguagesARM64.bat), or should there be some argument that needs to be passed to the existing batch files that determines which version to build? If the latter, I'm assuming the 'no arguments' case should still build the x86 and x64 combined setup, correct? The reason I'm asking is because presumably it is desirable to make AppVeyor produce automated builds of ARM64 as well as x86/x64.

If the answer to the above is 'don't worry about this, someone else will take care of the installer', or 'do this in another PR', that's fine by me too. I mostly wanted to make this PR to add ARM64 support to Open-Shell, not necessarily Open-Shell's installer :) Though I'm OK with doing it if this is desirable.

@ge0rdi
Copy link
Member

ge0rdi commented Feb 17, 2021

@Mattiwatti
First of all thanks for understanding and I personally appreciate the effort done (as there is not much people contributing to this project).

Should there be (a) new batch file(s) to create it (think e.g. __MakeFinalARM64.bat and __MakeFinalAllLanguagesARM64.bat), or should there be some argument that needs to be passed to the existing batch files that determines which version to build?

I think separate BAT may be easier to do.
But I guess combined script where one can select what should be built will make more sense.

Basically from my POV both ways make sense.

If the latter, I'm assuming the 'no arguments' case should still build the x86 and x64 combined setup, correct?

Yup. That would make sense, so it will be backward compatible.

Though in case when some argument will be present it probably should mean list of platforms to build.
So supplying ARM64 will build ARM64 only and supplying x86 x64 ARM64 will build all platforms.
What do you think?

The reason I'm asking is because presumably it is desirable to make AppVeyor produce automated builds of ARM64 as well as x86/x64.

Right.

Depending on the way the ARM64 build will be integrated we can modify appveyor.yml to build all 3 platforms.

@Mattiwatti
Copy link
Author

I've pushed a new commit that separates the x86/x64 installer from the ARM64 one instead of the previous iteration where the setup was combined. The new ARM64 setup can be built with __MakeFinalARM64.bat or __MakeFinalAllLanguagesARM64.bat, which is equivalent to __MakeFinal.bat ARM64 or __MakeFinalAllLanguages.bat ARM64. So in principle the new batch files are not strictly necessary, since supplying "ARM64" as the optional argument will build the ARM64 setup. But I thought this way would be more obvious to newcomers who are wondering how to compile for a different architecture.
__MakeFinal[AllLanguages].bat without arguments will build the combined x86/x64 installer as before.

Though in case when some argument will be present it probably should mean list of platforms to build. So supplying ARM64 will build ARM64 only and supplying x86 x64 ARM64 will build all platforms.

I agree this would be preferable, but I'll leave this as a "nice to have" TODO for now. I spent some time trying to make this work for generic lists of architecture names (e.g. "x86 x64 ARM64 foo"), but with all the added batch file boilerplate and error checking the batch files (plural, because there are multiple batch files that all need to know which architecture(s) is/are being built) quickly became very unwieldy with most of the lines "wasted" on simply parsing the architecture(s) to build. So I reverted to the simpler method above for now.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@XenHat
Copy link
Member

XenHat commented Jul 10, 2021

Late to the party here. I agree with the sentiment that architectures should be split. What's the status on this? Waiting for Wix to be stable?

@malxau
Copy link

malxau commented Nov 27, 2021

I just bought an ARM64 device and built from Mattiwatti's branch to install. Is there anything I can do to help move this forward? Getting it installed from source worked in the end but was a rough experience (I've never built this code before.)

The only thing I encountered is Utility.rc is trying to include the AMD64 binary (so the x86 binary includes the AMD64 one?) In this build sequence though, BuildBinaries.bat only builds ARM64, not AMD64, so the later x86 build fails because it can't find the AMD64 binary.

@Ibuprophen
Copy link
Member

I'm neutral about having separate installers as long as it's necessary to have...

I'll go with the consensus of the team @ge0rdi, @coddec, @XenHat...

~Ibuprophen

@blackcrack
Copy link

blackcrack commented Aug 12, 2022

@coddec , could Ibuprophen have the "ownerstate" because he is active here and he is everytime here, have a good character and know what he does..
best

@Ibuprophen Ibuprophen added acknowledged by devs This issue has been acknowledged by the Open-Shell team. Enhancement/Feature Request An Enhancement/Feature request by the community low priority Low priority but desired fix tentative long-term goal A potential improvement with no immediate implementation plans. and removed medium priority Non-critical but desired fix labels Aug 13, 2022
@Ibuprophen Ibuprophen assigned Mattiwatti and unassigned Mattiwatti and malxau Aug 13, 2022
@Mattiwatti
Copy link
Author

OP here.

I see that I've been assigned this issue, as well as that the issue has been given the Requires & Awaiting Further Info By OP Issuer tag (among others). I'm not sure what further info is needed at this point.

There is a pending change request by @ge0rdi, but this comment only mentions that the stable release of WiX did not support ARM64 at the time. WiX still does not support ARM64 (in fact, WiX has not had any new stable releases since 2019). However the ARM64 installer builds fine with the current development release of WiX (v3.14.0.6526). I haven't retested the PR against current master since I no longer have an ARM64 device on hand for this, but I don't see any changes that would cause issues.

If needed I can force push a rebased PR, but other than that I'm not sure what else I can do for this.

@XenHat
Copy link
Member

XenHat commented Aug 13, 2022

Could @malxau test the latest code ?
The only ARM64 devices I own beside my phones are Raspberry Pi and I'm not inclined to format them and put windows on them so I can't really test this.

@XenHat
Copy link
Member

XenHat commented Aug 13, 2022

I'm neutral about having separate installers as long as it's necessary to have...

I'll go with the consensus of the team @ge0rdi, @coddec, @XenHat...

~Ibuprophen

I'm fairly neutral, I'm more concerned about confusing our users with picking AMD64 or ARM64. Maybe an auto-detection in the installer?

@blackcrack
Copy link

blackcrack commented Aug 13, 2022

@Ibuprophen
and btw, i have no/0 experience/skills in c or arm coding.. only in scripting in bat/cmd and Linux bash sh, Ibu :)
and i still look anyway this tread Ibu 😃
@XenHat , Arm v6, Arm v7 and Arm64 v8 😉

@Ibuprophen
Copy link
Member

Ibuprophen commented Aug 13, 2022

@blackcrack, my apologies as I was "brain dead" when I had added the list of reviewers... LOL!

~Ibuprophen

@malxau
Copy link

malxau commented Aug 13, 2022

Fwiw, I rebased on master and rebuilt everything successfully. Per above, I had to change Src\Setup\Utility\Utility.rc to point to ReleaseARM64\Utility.exe rather than Release64\Utility.exe . Verified that the installer works, and installs the right version.

I used __MakeFinalARM64.bat for this - is there a reason it needs to build x86? If we have two installers anyway, what function does x86 serve here?

Also, I've since upgraded my ARM64 device to Windows 11 (for x64 support) which means I'm not currently running OpenShell and limits the amount of testing I can really perform. On Windows 11 it's possible to launch the menu via the win key but not by clicking on the start button.

@ge0rdi ge0rdi removed Enhancement/Feature Request An Enhancement/Feature request by the community community feedback Not a bug acknowledged by devs This issue has been acknowledged by the Open-Shell team. awaiting response This issue requires a response or further information from OP. labels Aug 15, 2022
@voicenoise
Copy link

Hello i try to install openshell 4.4.189 on a arm device but it end by a black screen.
Is the any news or a download for a arm version?
Thanks

@Ibuprophen
Copy link
Member

When it comes to ARM32/ARM64 (typically on the Tablets with the Windows OS installed), I don't believe that the Open Shell software (as is) will work out too great (personally).

I believe that Open Shell would require a separate branch (or repo) that would also require some additional (knowledgeable) individuals who would have to work out the coding and such to accommodate these specific architecture types.

Just a thought... 🤔

@ge0rdi?

~Ibuprophen

@malxau
Copy link

malxau commented Mar 1, 2023

I don't think associating tablets with ARM64 matches what's happening today. It's true that ARM32 Windows was for tablets, but we've come a long way since then. My device is a very ordinary looking laptop, and a week or so ago, official support for Windows on Apple laptops was announced: https://support.microsoft.com/en-us/windows/options-for-using-windows-11-with-mac-computers-with-apple-m1-and-m2-chips-cd15fd62-9b34-4b78-b0bc-121baa3c568c

I can take care of the ARM64 port, but right now, the experience on Windows 11 is lacking. If it works well on Intel, it can work well on ARM64.

@bonzibudd
Copy link
Member

@malxau I agree. I imagine many more “standard” form factor systems will be adopting ARM in the near future, so it makes sense to adopt this.

@BeholdersEye
Copy link

BeholdersEye commented Apr 22, 2023

Apple silicon is arm64 and I expect there may be a few people with macbooks who don't like Windows 11's default shell but still need to run Windows in parallels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed low priority Low priority but desired fix tentative long-term goal A potential improvement with no immediate implementation plans.
Projects
None yet
Development

Successfully merging this pull request may close these issues.