Add TargetPlatform property #11625

chirontt · 2024-03-09T23:48:14Z

Add new TargetPlatform property, to control the specific platform for the GE installer (.msi) package.

Currently the GE installer package is hard-coded to be built for the x86 platform, with C:\Program Files (x86) as the default installation folder. This makes sense to install it on the x86 (i.e. 32-bit) Windows platform, but a bit weird on x64 (i.e. 64-bit) Windows platform (as discussed in #11554), and really feels weird when building an Arm64-specific installer package for Windows on Arm64 platform.

This commit defines a new TargetPlatform property which can take the following values: x64 (default), x86, or arm64 (must be lower-case values, as required by Wix). This property is passed on to the Platform parameter for WiX to create relevant installer package specific to that platform. Like any other property, this TargetPlatform property can be specified on the command line to build a platform-specific installer, rather than using the default x64 platform value.

To build GE, the current build command series still work as before:

dotnet build -c Release
dotnet build -c Release scripts\native.proj
dotnet publish -c Release

and the following build artifacts are produced in the artifacts\Release\publish folder for the x64 platform; for example:

GitExtensions-x64-33.33.33.0-0e12afbf1.msi
GitExtensions-Portable-x64-33.33.33.0-0e12afbf1.zip

The above x64 installer (.msi) package will default to C:\Program Files as the installation folder, and will refuse to install on x86 (i.e. 32-bit) Windows platform. It can be installed on Windows on Arm64 platform, as this platform contains translation layer for running x64 program.

If the TargetPlatform=x86 property value is specified in the publish command, like:

dotnet publish -c Release /p:TargetPlatform=x86

then the following files are produced:

GitExtensions-x86-33.33.33.0-0e12afbf1.msi
GitExtensions-Portable-x86-33.33.33.0-0e12afbf1.zip

The above x86 installer (.msi) package will default to C:\Program Files (x86) as the installation folder, and can install on x86 (i.e. 32-bit) Windows, x64 Windows and Windows on Arm64 platforms.

Similarly, to produce an Arm64-specific installer package, the following build commands should be executed on a Windows on Arm64 box:

dotnet build -c Release
dotnet build -c Release scripts\native.proj /p:TargetPlatform=arm64
dotnet publish -c Release /p:TargetPlatform=arm64

and the following files will be produced for Arm64:

GitExtensions-arm64-33.33.33.0-0e12afbf1.msi
GitExtensions-Portable-arm64-33.33.33.0-0e12afbf1.zip

The above arm64 installer (.msi) package will default to C:\Program Files as the installation folder, and will refuse to install on any Windows platform except the Windows on Arm64 platform.

Fixes #11554

Test methodology

default build GE installer for x64 platform, and check that it installs to the default C:\Program Files folder and works properly on x64 and/or Arm64 Windows platforms. It should refuse to install on an x86 (i.e. 32-bit) Windows platform (but none are available to test.)
build of x86 platform GE installer, by specifying the TargetPlatform=x86 property on the publish command, and check that it installs to the default C:\Program Files (x86) folder and works properly on x64 and/or Arm64 Windows platforms. It should work on an x86 (i.e. 32-bit) Windows platform too, but none are available to test.
build GE installer for Arm64 platform, by specifying the TargetPlatform=arm64 property on the publish command, and check that it installs to the default C:\Program Files folder and works properly on Arm64 Windows platform. Check that it should refuse to install on x64 Windows platform. It should refuse to install on an x86 (i.e. 32-bit) Windows platform too, but none are available to test.

Test environment(s)

Windows 10 & 11 on x64 platform
Windows 11 on Arm64 platform

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.

RussKie

Few questions, but overall, this looks good. Thank you.

RussKie · 2024-03-10T00:44:27Z

Directory.Build.props

+    <!-- Set the target platform manually to be x86 (default), x64, arm64 -->
+    <TargetPlatform Condition=" '$(TargetPlatform)' == '' ">x86</TargetPlatform>


IMO it should be inverted; x86 is a thing of a past, the default should be x64.

Suggested change



<TargetPlatform Condition=" '$(TargetPlatform)' == '' ">x86</TargetPlatform>



<TargetPlatform Condition=" '$(TargetPlatform)' == '' ">x64</TargetPlatform>

Yes, I agree that we should move forward and default to x64 Windows platform, but this comes with a catch: the resulting .msi installer package will refuse to install in x86 (i.e. 32-bit) Windows box. Is this restriction acceptable?

With the default x86 platform, the .msi package will be installable on all 3 Windows platforms: x86, x64 and Arm64; it's the package of the "lowest common denominator." But I guess at some point we need to cut off this x86 support...

Anyway, the AppVeyor build script could be changed to generate packages for both x86 and x64 platforms at the same build time:

dotnet publish -c Release /p:TargetPlatform=x86 dotnet publish -c Release /p:TargetPlatform=x64

and the user can pick whatever .msi package they want to install.

IIRC, there was some flag in the isntaller that forced user to manually uninstall the old app first then install the new one. This way we could migrate the user to the new 64bit app.

Have a look into Setup/EnableUpgrades.wxi var.MinSupportedVersion

I think all these extra requirements (default x64, upgrade strategy, minimum support version) should belong to a different PR than this one, as they require far too much work/consideration as compared to my original intention for this PR.

RussKie · 2024-03-10T00:45:26Z

GitExtensions/Project.Publish.targets

@@ -249,7 +249,8 @@

      <!-- This property comes from GitInfo package, but can be overriden on AppVeyor -->
      <_PublishPortableCommitHashSuffix Condition="'$(GitCommit)' != ''">-$(GitCommit)</_PublishPortableCommitHashSuffix>
-      <_PublishPortableFileName>GitExtensions-Portable$(_PublishPortableVersionSuffix)$(_PublishPortableCommitHashSuffix).zip</_PublishPortableFileName>
+      <_PublishPortableFileName Condition="'$(TargetPlatform)' == 'x86'">GitExtensions-Portable$(_PublishPortableVersionSuffix)$(_PublishPortableCommitHashSuffix).zip</_PublishPortableFileName>
+      <_PublishPortableFileName Condition="'$(TargetPlatform)' != 'x86'">GitExtensions-Portable-$(TargetPlatform)$(_PublishPortableVersionSuffix)$(_PublishPortableCommitHashSuffix).zip</_PublishPortableFileName>


I think we should always add the platform to the name to avoid confusion.

Yes, I agree with this explicit platform in the build artifacts' names.

RussKie · 2024-03-10T00:45:36Z

GitExtensions/Project.Publish.targets

    <PropertyGroup>
      <_AppVeyorSuffix>$(ARTIFACT_BUILD_SUFFIX)</_AppVeyorSuffix>
      <_PublishMsiVersionSuffix>-$(CurrentBuildVersion.ToString())$(_AppVeyorSuffix)</_PublishMsiVersionSuffix>
      <_PublishMsiCommitHashSuffix Condition="'$(GitCommit)' != ''">-$(GitCommit)</_PublishMsiCommitHashSuffix>
      <_PublishMsiCommitHashSuffix Condition="'$(env:APPVEYOR_REPO_COMMIT)' != ''">-$(env:APPVEYOR_REPO_COMMIT)</_PublishMsiCommitHashSuffix>
      <_PublishMsiCommitHashSuffix Condition="'$(env:APPVEYOR_PULL_REQUEST_HEAD_COMMIT)' != ''">-$(env:APPVEYOR_PULL_REQUEST_HEAD_COMMIT)</_PublishMsiCommitHashSuffix>
-      <_PublishMsiFileName>GitExtensions$(_PublishMsiVersionSuffix)$(_PublishMsiCommitHashSuffix)</_PublishMsiFileName>
+      <_PublishMsiFileName Condition="'$(TargetPlatform)' == 'x86'">GitExtensions$(_PublishMsiVersionSuffix)$(_PublishMsiCommitHashSuffix)</_PublishMsiFileName>
+      <_PublishMsiFileName Condition="'$(TargetPlatform)' != 'x86'">GitExtensions-$(TargetPlatform)$(_PublishMsiVersionSuffix)$(_PublishMsiCommitHashSuffix)</_PublishMsiFileName>


RussKie · 2024-03-10T00:46:46Z

scripts/native.proj

@@ -62,7 +62,7 @@
    </Exec>
    <!-- Build GitExtensionsShellEx project, x86 -->
    <Exec
-        Condition="'$(Arm64Build)' != 'true'"
+        Condition="'$(TargetPlatform)' != 'arm64'"


RussKie · 2024-03-10T00:48:06Z

scripts/native.proj

@@ -71,7 +71,7 @@
    </Exec>
    <!-- Build GitExtensionsShellEx project, x64 -->
    <Exec
-        Condition="'$(Arm64Build)' != 'true'"
+        Condition="'$(TargetPlatform)' != 'arm64'"


Should this be constrained to x64 only? That is, this can't be used on x86 platforms.

The default x86 package has always included this 64-bit component during build, but it is restricted (i.e. ignored) at installation time on an x86 (i.e. 32-bit) Windows box, by this condition in Product.wxs:

gitextensions/Setup/Product.wxs

Line 246 in b4100e3

<Condition>VersionNT64</Condition>

So it's safe to keep this thing as-is. But I don't have a 32-bit Windows box to really test it...

I don't think anyone does these days :)

to control the specific platform for the GE installer (.msi) package. Currently the GE installer package is hard-coded to be built for the "x86" platform, with "C:\Program Files (x86)" as default installation folder. This makes sense to install it on the x86 (i.e. 32-bit) or x64 (i.e. 64-bit) Windows platforms, but feels weird when building an Arm64-specific installer package for Windows on Arm64 platform. This commit defines a new "TargetPlatform" property which can take the following values: "x86" (default), "x64", and "arm64". This property is passed on to the "Platform" parameter for WiX to create relevant installer package specific to that platform. Like any other property, this property can be specified on the command line to build a platform-specific installer, rather than using the default "x86" platform value. To build GE, the current build command series still work as before: dotnet build -c Release dotnet build -c Release script\native.proj dotnet publish -c Release and the following build artifacts are produced in the "artifacts\Release\publish" folder for the "x86" platform just like before (for example): GitExtensions-33.33.33.0-0e12afbf1.msi GitExtensions-Portable-33.33.33.0-0e12afbf1.zip If the TargetPlatform=x64 property value is specified in the "publish" command, like: dotnet publish -c Release /p:TargetPlatform=x64 then the following files are produced: GitExtensions-x64-33.33.33.0-0e12afbf1.msi GitExtensions-Portable-x64-33.33.33.0-0e12afbf1.zip The above x64 installer (.msi) package will default to "C:\Program Files" as the installation folder, and will refuse to install on x86 (i.e. 32-bit) Windows platform. Similarly, to produce an Arm64-specific installer package, the following build commands should be executed on a Windows on Arm64 box: dotnet build -c Release dotnet build -c Release script\native.proj /p:TargetPlatform=arm64 dotnet publish -c Release /p:TargetPlatform=arm64 and the following files will be produced for Arm64: GitExtensions-arm64-33.33.33.0-0e12afbf1.msi GitExtensions-Portable-arm64-33.33.33.0-0e12afbf1.zip The above Arm64 installer (.msi) package will default to "C:\Program Files" as the installation folder, and will refuse to install on any Windows platform except the Windows on Arm64 platform.

chirontt · 2024-04-02T03:01:51Z

Branch rebased to latest in master, and updated with the following as discussed:

default target platform is x64 rather than x86 as before
explicit target platform present in the build artifacts' names.

ghost assigned chirontt Mar 9, 2024

RussKie reviewed Mar 10, 2024

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 10, 2024

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 10, 2024

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 23, 2024

chirontt force-pushed the add_target_platform_property branch from d8564ed to 5a73892 Compare April 2, 2024 02:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TargetPlatform property #11625

Add TargetPlatform property #11625

chirontt commented Mar 9, 2024 •

edited

RussKie left a comment

RussKie Mar 10, 2024

chirontt Mar 10, 2024

RussKie Mar 10, 2024

RussKie Mar 10, 2024

chirontt Mar 18, 2024

RussKie Mar 23, 2024

RussKie Mar 10, 2024

chirontt Mar 10, 2024

RussKie Mar 10, 2024

RussKie Mar 10, 2024

RussKie Mar 10, 2024

chirontt Mar 10, 2024 •

edited

RussKie Mar 10, 2024

chirontt commented Apr 2, 2024

		<!-- Set the target platform manually to be x86 (default), x64, arm64 -->
		<TargetPlatform Condition=" '$(TargetPlatform)' == '' ">x86</TargetPlatform>

Add TargetPlatform property #11625

Are you sure you want to change the base?

Add TargetPlatform property #11625

Conversation

chirontt commented Mar 9, 2024 • edited

Test methodology

Test environment(s)

Merge strategy

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chirontt Mar 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chirontt commented Apr 2, 2024

chirontt commented Mar 9, 2024 •

edited

chirontt Mar 10, 2024 •

edited