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 support to build GE for Windows on Arm64 (WoA) #10649
Add support to build GE for Windows on Arm64 (WoA) #10649
Conversation
Related: #10288 |
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.
🎉
The installer will need to be changed as well, see
gitextensions/Setup/Product.wxs
Lines 231 to 249 in 1ee9b75
<!-- Shell extensions --> | |
<?define ShellExCLSID = "{3C16B20A-BA16-4156-916F-0A375ECFFE24}"?> | |
<Component Id="GitExtensionsShellEx32.dll" Guid="*"> | |
<File Name="GitExtensionsShellEx32.dll" Source="$(var.ArtifactsBinPath)\GitExtensionsShellEx\GitExtensionsShellEx32.dll" KeyPath="yes" /> | |
<?foreach ShellExFileName in GitExtensionsShellEx32.dll?> | |
<?include RegisterShellExtension.wxi?> | |
<?endforeach ?> | |
</Component> | |
<Component Id="GitExtensionsShellEx64.dll" Guid="*" Win64="yes"> | |
<Condition>VersionNT64</Condition> | |
<File Name="GitExtensionsShellEx64.dll" Source="$(var.ArtifactsBinPath)\GitExtensionsShellEx\GitExtensionsShellEx64.dll" KeyPath="yes" /> | |
<?foreach ShellExFileName in GitExtensionsShellEx64.dll?> | |
<?include RegisterShellExtension.wxi?> | |
<?endforeach ?> | |
</Component> | |
<!-- End shell extensions --> | |
<Component Id="GitExtSshAskPass.exe" Guid="*"> | |
<File Source="$(var.ArtifactsBinPath)\GitExtSshAskPass\GitExtSshAskPass.exe" /> | |
</Component> |
Also, ConEmu doesn't work on Arm64, so some code paths will need to be conditioned:
scripts/native_arm64.proj
Outdated
<_ProjectArgsARM64> /p:Configuration=$(Configuration) /p:Platform=ARM64</_ProjectArgsARM64> | ||
</PropertyGroup> | ||
|
||
<!-- Build GitExtSshAskPass project, ARM64 --> | ||
<Exec | ||
Command="msbuild.exe $(_GitExtSshAskPassPath) $(_ProjectArgsARM64) /p:OutDir=$(_OutputPath)GitExtSshAskPass\ /bl:$(_ArtifactsLogDir)\GitExtSshAskPass.binlog" | ||
WorkingDirectory="$(_MSBuildCurrentPath)" | ||
EchoOff="true" | ||
ConsoleToMsBuild="true" | ||
StandardOutputImportance="High"> | ||
</Exec> | ||
<!-- Build GitExtensionsShellEx project, ARM64 --> | ||
<Exec | ||
Command="msbuild.exe $(_GitExtensionsShellExPath) $(_ProjectArgsARM64) /p:OutDir=$(_OutputPath)GitExtensionsShellEx\ /bl:$(_ArtifactsLogDir)\GitExtensionsShellEx64.binlog" | ||
WorkingDirectory="$(_MSBuildCurrentPath)" | ||
EchoOff="true" | ||
ConsoleToMsBuild="true" | ||
StandardOutputImportance="High"> | ||
</Exec> |
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.
Is there a reason we can't do this in native.proj?
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.
As I mentioned before, I can't produce the .msi
installer for WoA, due to the WiX incompatibility problem, so I haven't got a chance to see what's needed for it. I've tried to recompile the WiX library for WoA, but it's so old that it throws so many errors (it's pre-net6 program, and .NET SDK in WoA only supports net6+) that I just give it up.
ConEmu problem in WoA was reported almost 5 years ago, and no progress since. I guess we need to wait a couple of more years still. So for now, I just disable ConEmu in my GE settings in WoA. Actually I want to even hide the Console
tab so that I don't accidentally click it and produce a zombie process in the background:
Any idea how to hide it from being displayed on screen?
As to the new native_arm64.proj
file:
Is there a reason we can't do this in native.proj?
Nope, just combining them together would cause compile problems on either platforms: they would share objs between different platforms (on WoA) which is a no-no, the x64 compiler wouldn't know how to compile the ARM64 platform which necessitates some needed conditionals to go different compile path for each platform, etc., that I reckon more research are needed to make it work. For now, using a separate proj file for Arm64 is the simplest solution I can think of.
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.
Any idea how to hide it from being displayed on screen?
Look at the 'Build report' tab logic. I think we don't display it always. Maybe it's just a matter of not adding it to the tab collection.
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.
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.
@gerhardol Yes, the following setting will hide the Console
tab:
2f2a39a
to
ae3bd9b
Compare
I wouldn't rely on a setting, instead I think we should conditionally
exclude the implementation either with `#if ARM64` or `if Platform ==
"arm64"` (pseudo code).
|
FYI: I've made my unofficial build of GE v4.0.2 for Windows Arm64 available on my fork. |
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.
Looks good 👍
Please address the outstanding comments and rebase.
What would be the steps to build a portable version on AppVeyor (since we can't produce an msi)?
Also, we'll need to add build instructions for the ARM64 builds. If you can provide a text, we'll have it added to https://github.com/gitextensions/gitextensions/wiki/How-To%3A-build-instructions.
@@ -43,7 +43,7 @@ | |||
|
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.
Suggest renaming this file to native.amd.proj
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.
OK, and as a result, the reference to it in appveyor.yml
is updated too. So this renaming of native.proj
makes appveyor.yml
become exclusively X64 (and thus should be renamed to appveyor.amd.yml
to be explicitly clear?)
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.
There are few other places where native.proj
is used: https://github.com/gitextensions/gitextensions/search?q=native.proj
So this renaming of
native.proj
makesappveyor.yml
become exclusively X64 (and thus should be renamed toappveyor.amd.yml
to be explicitly clear?)
I don't follow, sorry. We continue to build x86 as well, hence the suggestion to rename to native.amd.proj
and not native.amd64.proj
.
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.
There are few other places where
native.proj
is used: https://github.com/gitextensions/gitextensions/search?q=native.proj
OK, due to these reasons, I've decided to revert to the original native.proj
file name, and have merged the native.arm64.proj
to it so that the native.proj
will now support both Intel/AMD and ARM64 builds via the usage of an extra property. And that means the original appveyor.yml
file won't be changed after all, and such it will continue building the x86/x64 products as originally intended.
I don't have any experience with AppVeyor so I can't do much about how to change appveyor.yml
to support the extra ARM64 build.
With my latest changes, to build the x86/x64 natives would use the same command as originally intended:
dotnet build .\scripts\native.proj
while to build the ARM64 natives would require setting an extra Arm64Build
flag:
dotnet build .\scripts\native.proj /p:Arm64Build=true
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.
You could duplicate the block at line 73-77:
- ps: |
# build .NET
$cmd = "dotnet build -c Release /p:Arm64Build=true --verbosity q --nologo /bl:.\artifacts\log\build.binlog $buildArgs"
Invoke-Expression $cmd
if ($LastExitCode -ne 0) { $host.SetShouldExit($LastExitCode) }
Though it will only come into affect after merging the PR.
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.
That's a good idea. Few things to note:
- since we're build amd and arm we should distingush between the binlogs, hence this one should be named something like build.arm64.binlog;
- we'll need to distinguish the zips as well, i.e., name the zip something like "GitExtensions-portable.arm64.xyz0123.zip".
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.
You could duplicate the block at line 73-77:
No, it wouldn't work. The command:
dotnet build .\scripts\native.proj /p:Arm64Build=true
is meant to be executed on a WoA machine (or VM); it won't work if running on x64 platform or VM such as the one used by AppVeyor (I presume.) The reason is that the MSVC compiler for x86/x64 doesn't know how to generate Arm64 binaries.
This is why I suggested a new appveyor.arm64.yml
to produce the Arm64 GE products, while keeping the existing appveyor.yml
exclusively for x86/x64 GE products. But appveyor.arm64.yml
would only work if AppVeyor could run on a WoA platform, and this is a big IF that I'm not sure about.
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.
Makes sense.
scripts/native_arm64.proj
Outdated
@@ -0,0 +1,63 @@ | |||
<Project DefaultTargets="Build"> |
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.
...and this to native.arm64.proj
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.
OK.
I think to handle Arm64 build, a new appveyor.arm64.yml
file is needed (almost a duplicate of the current appveyor.yml
contents.) But because the dotnet publish
command will fail in WoA which would cause AppVeyor build process to also fail. Is there a way to change the dotnet publish
command to produce the portable zip
distribution only when building for 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.
I don't think it's possible to run multiple build pipelines on a free account. Aren't we able to build both of distros in one pipeline?
Is there a way to change the
dotnet publish
command to produce the portablezip
distribution only when building for Arm64?
The publishing targets are here: https://github.com/gitextensions/gitextensions/blob/master/GitExtensions/Project.Publish.targets#L271. I don't think it should be too difficult to condition CreateMsi
target based on a MSBuild property.
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 don't think it's possible to run multiple build pipelines on a free account. Aren't we able to build both of distros in one pipeline?
I'm out of my depth here, so I can't comment any further.
The publishing targets are here: https://github.com/gitextensions/gitextensions/blob/master/GitExtensions/Project.Publish.targets#L271. I don't think it should be too difficult to condition
CreateMsi
target based on a MSBuild property.
Yes, it's trivial to add a condition to the CreateMsi
target based on a property, but deciding on which property to use is tricky. I've tried various existing properties like ProcessorArchitecture
, RuntimeIdentifier
, Platform
by adding extra options such as -a ARM64
or -r win-arm64
or --use-current-runtime
to the dotnet publish
command, but they have various side-effects in the resulting package. Like, using the -r win-arm64
or --use-current-runtime
on WoA would produce a package for Arm64 having similar problems as mentioned in #9990.
Eventually I decided to define my own property Arm64Build
, so that to produce the x86/x64 distributions the original command would still work as before:
dotnet publish
while to produce Arm64 package would require setting the Arm64Build
flag so that it would skip the CreateMsi
step:
dotnet publish /p:Arm64Build=true
In summary, here are the complete (unchanged) commands to build GE for x86/x64 platform:
dotnet build
dotnet build .\scripts\native.proj
dotnet publish
while to build GE for WoA:
dotnet build
dotnet build .\scripts\native.proj /p:Arm64Build=true
dotnet publish /p:Arm64Build=true
Ideally, the first set of commands should have worked for both situations, without defining the new Arm64Build
property, but I haven't found any existing MSBuild property up to the task. Any help/suggestion is quite welcome.
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.
👍
scripts/native.proj
Outdated
</PropertyGroup> | ||
|
||
<!-- Build GitExtSshAskPass project, x86 --> | ||
<Exec | ||
Condition="!$(Arm64Build)" |
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.
This is error-prone as we don't control the value. We should do this instead:
Condition="!$(Arm64Build)" | |
Condition=" '$(Arm64Build)' != 'true' " |
Ditto for all other similar instances.
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 disagree. Arm64Build
is meant to be a boolean
flag, with true
value being the only value having significant effect; any other value is simply not true
i.e. false
. I'm just following MSBuild docs here. The fact that Arm64Build
is implemented as a string is just a shortcoming of MSBuild, so making it explicit like '$(Arm64Build)' != 'true'
is clunky and unnecessary, in my opinion, as I want to hide its silly string implementation!
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.
dotnet publish /p:Arm64Build=yes
will cause this to fail.
MSBuild doesn't have strong typing, and because of that we should be defensive in our implementations. This is how comparisons are done throughout the .NET SDK.
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.
dotnet publish /p:Arm64Build=yes
will cause this to fail.
Yes, and it should fail for Arm64 build. As I mentioned before, Arm64Build
is a boolean flag with true
having significant meaning, any other values are false
.
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.
MSBuild properties aren't strongly typed. So they have to be compared in a safe way.
@@ -268,7 +268,10 @@ | |||
Creates an MSI. | |||
============================================================ | |||
--> | |||
<Target Name="CreateMsi" AfterTargets="CreatePortable"> | |||
<PropertyGroup> | |||
<Arm64Build>false</Arm64Build> |
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.
Propose a rename to
<Arm64Build>false</Arm64Build> | |
<IsArm64>false</IsArm64> |
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 a better name for the flag would be:
<IsArm64Build>false</IsArm64Build>
thus making it more meaningfully a boolean
property.
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.
Works for me.
@@ -43,7 +43,7 @@ | |||
|
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.
That's a good idea. Few things to note:
- since we're build amd and arm we should distingush between the binlogs, hence this one should be named something like build.arm64.binlog;
- we'll need to distinguish the zips as well, i.e., name the zip something like "GitExtensions-portable.arm64.xyz0123.zip".
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.
As per the earlier comments
@AArnott if you could take this for a spin it'd be great. Also, a review is welcome ;) |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days. It will be closed if no further activity occurs. |
@RussKie sorry I'm just now getting to this. How do I test it? Are there binaries I can download from somewhere? |
Sorry, no binaries, we don't have arm64 infra to build.
If you have an arm64 device you could pull this PR and see if you can build
an arm64-native app.
I think these commands should work:
dotnet build .\scripts\native.proj /p:Arm64Build=true
dotnet publish /p:Arm64Build=true
|
Thanks. Building was as easy as you say. But the result was an .exe that still ran as x64. Its headers (from ILSpy) might suggest a reason:
It's AnyCPU. And it appears to target the .NET Framework 2.0 runtime, which is weird because GitExtensions.dll targets .NET 6. So maybe you're playing some weird tricks anyway. But .NET Framework AnyCPU .exes run as x64 even on an arm64 machine. You have to target arm64 specifically in an .exe to get an arm64 process. |
Please make sure to clone the submodules.
Also the branch is out of date, we no longer use dotnetbootstrapper.
|
Yes, I had to clone the submodules for the |
You can rebase to keep history clean. |
It's a good idea to rebase this branch on top of the latest master.
IIRC dotnetbootstrapper was a .NET Framework host that would download the
required .NET Desktop runtime, if missing, and then start the app. But we
solved the bootstrap problem in another way and removed the dependency.
|
You're welcome to ping me in teams, if you still have troubles ;)
|
History doesn't matter in this case because I'm just testing a PR. I'm not pushing anything. Regardless.... the test worked. I have an arm64 gitextensions.exe process and I poked around the window and switched views and it seemed to work. Thank you.
You shouldn't need arm64 infra to build arm64 binaries. Ever hear of cross-compiling? |
Fyi. https://git-scm.com/book/en/v2/Git-Tools-Rerere You can prep with a merge and solve merge conflicts then reset and rebase. |
I love I'd love to hear how we can get the arm64 build shipping though. |
That's not necessarily true. But in this case, as you said, it doesn't matter.
I think this is how dotnet/windowsdesktop does it, but I never actually looked into it. |
Well thank you for that!
I'll be happy to look into updating your pipeline to include an arm64 build. |
e173f2c
to
a9fcf72
Compare
To build GE for WoA, run the command: dotnet build /p:Arm64Build=true To package GE distribution for WoA, run the commands: dotnet build .\scripts\native.proj /p:Arm64Build=true dotnet publish /p:Arm64Build=true The last command would fail to produce a .msi distribution, due to it using the WiX library which is currently incompatible with the WoA platform, but a portable GE zip distribution for WoA is successfully produced in the folder: artifacts\Debug\publish
a9fcf72
to
f98b438
Compare
Resolves #10674
With this PR, here are the commands to build GE on a Windows on Arm64 (WoA) platform:
The above command produces a
GitExtensionsShellEx64.dll
file for WoA, but GE also requires aGitExtensionsShellEx32.dll
file to exist, so just make a copy ofGitExtensionsShellEx64.dll
to beGitExtensionsShellEx32.dll
as well:The last command would fail to produce a
.msi
distribution, due to it using the WiX library which is currently incompatible with the WoA platform, but a portable GEzip
distribution for WoA will be successfully produced in the folderartifacts\Debug\publish
Proposed changes
Build for Windows on Arm64 platform
Test methodology
Mostly manual tests, done on normal daily usage of the app.
Executing the automated tests, with the
dotnet test
command, mostly fail, with error abouttesthost.dll
file:I guess
testhost.dll
is an x86 library, so it fails to load in an Arm64 GE process.Test environment(s)
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.