@mislav is there an easy way to test the release workflow without releasing a new version? It should work, but I'd like to resolve any issues earlier rather than later.

This may be more code, but keep in mind it provides the flexibility needed to not only fix the problem in #703, but quite a few others I described in the initial commit message. I've testing the upgrade path from 1.14 through built 2.1.0 and 2.2.0, where the latter remembers the user-specified directory.

Many of these changes are not possible with go-msi after reviewing the source (schema was not documented apart from a sample), nor has the project been updated since 2017. The WiX toolset is under continuous development, and 3.11.4 is already installed in windows-latest.

Unfortunately, there is no safe, reliable way of cleaning up the PATH change from the older MSIs that did not correctly remove the environment variables.

@mislav something else to consider is to build the executable for x86 and ship only an x86 MSI. It installs on any Windows architecture (except Server Core if WOW64 was uninstalled, which is incredibly rare). Given it's an EXE, doesn't need GBs of memory, and had network-bound performance anyway, there's no reason it has to be chip-specific. Can still remember the directory it's installed to, though it would still be redirected away from 64-bit-only directories.

Not advocating for it necessarily, but it solves the problem of users picking the "right" package.

something else to consider is to build the executable for x86 and ship only an x86 MSI.

That's a really interesting proposition that we haven't considered at all. Although, at this point I find it slightly risky to downgrade our x64 official binary to x86 just to support 32-bit systems which comprise (in my estimation) less than 5% of our Windows users. So I think it would be safest to provide a separate x86 build that could eventually be retired after we deem it not worthwhile to support 32-bit systems anymore.

Understood. Did you want me to update the workflow to build an x86 version of the EXE and MSI then, or just keep it handy in case it's requested. Anyone installing the existing EXE via MSI (the MSI would've installed, but the EXE wouldn't run), scoop, etc. on an x86 box wouldn't have been able to run it anyway, but I did at least update the WiX authoring to be ready in case you do.

Also, any reply yet regarding the license dialog or the cross-scoping? Removing the license dialog (and then everything related to the license I did) doesn't take much, but cross-scoping normally doesn't work well in practice with the same MSI. From experience, it causes more problems than it solves. There's always scoop or a direct download for users who don't want to or can't elevate - no different than apt-based deployments currently.

Did you want me to update the workflow to build an x86 version of the EXE and MSI then

No, I think we ourselves will be able to add that on top of what you've already set up; thank you!

Also, any reply yet regarding the license dialog or the cross-scoping?

1. Let's remove the license dialog completely;
2. Let's abandon the idea of cross-scoping because it's not that necessary.

@mislav this PR is ready now.

@mislav was there anything else you were waiting on here?

@mislav, I saw 2.1.0 was recently released. It would be nice to get this in for users sooner than later.

I'm leaving for sabbatical soon and it would be great to get this in for the next release.

This is great! I've just tried this locally and it works well.

I'll try to come up with some setup where we can trigger the release manually for testing instead of carving git tags.

Any chance this change can be merged? Dealing with installation as admin then fiddling with PATH would be great to avoid. :)

AFK for a meeting for a bit, but here is where we are:

msi     Build MSI       2022-01-20T19:51:41.9649071Z ##[group]Run name="$(basename "$ZIP_FILE" ".zip")"
msi     Build MSI       2022-01-20T19:51:41.9649603Z name="$(basename "$ZIP_FILE" ".zip")"
msi     Build MSI       2022-01-20T19:51:41.9650206Z "${MSBUILD_PATH}/msbuild.exe" .\build\windows\gh.wixproj /p:SourceDir="$PWD" /p:OutputPath="$PWD" /p:OutputName="$name" /p:ProductVersion="${GITHUB_REF#refs/tags/v}"
msi     Build MSI       2022-01-20T19:51:42.0170618Z shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}
msi     Build MSI       2022-01-20T19:51:42.0171029Z env:
msi     Build MSI       2022-01-20T19:51:42.0171331Z   ZIP_FILE: gh_9.9.9-test_windows_amd64.zip
msi     Build MSI       2022-01-20T19:51:42.0172476Z   MSBUILD_PATH: C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin
msi     Build MSI       2022-01-20T19:51:42.0172843Z ##[endgroup]
msi     Build MSI       2022-01-20T19:51:45.2865355Z Microsoft (R) Build Engine version 16.11.2+f32259642 for .NET Framework
msi     Build MSI       2022-01-20T19:51:45.2866398Z Copyright (C) Microsoft Corporation. All rights reserved.
msi     Build MSI       2022-01-20T19:51:45.2870606Z
msi     Build MSI       2022-01-20T19:51:45.2979338Z MSBUILD : error MSB1008: Only one project can be specified.
msi     Build MSI       2022-01-20T19:51:45.2979990Z Switch: p:SourceDir=/d/a/cli/cli
msi     Build MSI       2022-01-20T19:51:45.2982462Z
msi     Build MSI       2022-01-20T19:51:45.2983257Z For switch syntax, type "MSBuild -help"
msi     Build MSI       2022-01-20T19:51:45.3275004Z ##[error]Process completed with exit code 1.

I'm assuming there is some variable expansion / quoting issue splitting an argument to msbuild

Switch: p:SourceDir=/d/a/cli/cli

I get that bash is being used on a Windows box, but the $PWD directory here seems very odd. Does that seem correct to you?

At any rate, what about just adding MSBUILD_PATH to the PATH env var? That's what we intended for setup-msbuild anyway, though not required. I've had a lot of other problems - even with pwsh shells - getting the quoting issue right since MSBuild itself seems very finicking about where you can put quotes in parameters. But how I had it before worked. I'm assuming quotation of the path itself changed parsing (with cmd it would since the string starts with a quote). So if we add MSBUILD_PATH to PATH that problem would go away.

Trying to repro locally with bash from Git for Windows, it seems we can switch / to - and keep everything as-is.

For posterity, @vilmibm and I discussed offline whether to support any potential customized install directory for the older MSIs and decided against it. Despite the amd64 binary shipping all this time, the MSI was built for x86 and, thus, the gh.exe executable was always installed to [ProgramFilesFolder] e.g., C:\Program Files (x86) (always the 32-bit location). While that wouldn't break anything, it's unexpected and making a clean break to install to the "right" [ProgramFiles64Folder] e.g., C:\Program Files would be preferred.

If, for some reason, we ever did want to support upgrades from the older MSIs that didn't retain the install directory, this is all that's needed:

diff --git a/build/windows/gh.wxs b/build/windows/gh.wxs
index 1e91734f..c244f704 100644
--- a/build/windows/gh.wxs
+++ b/build/windows/gh.wxs
@@ -45,6 +45,12 @@
         <!-- Restore the INSTALLDIR if previously persisted to the registry -->
         <Property Id="INSTALLDIR">
             <RegistrySearch Id="InstallDir" Root="HKLM" Key="SOFTWARE\GitHub\CLI" Name="InstallDir" Type="directory"/>
+            <!-- Try to use install location of previous installer package -->
+            <ComponentSearch Id="OldInstallDirSearch" Guid="{6E6DCB19-3CF6-46D1-AC56-C6FB39485C9D}" Type="file">
+                <DirectorySearch Id="OldInstallDir" AssignToProperty="yes">
+                    <FileSearch Id="OldInstallFile" Name="gh.exe" />
+                </DirectorySearch>
+            </ComponentSearch>
         </Property>
 
         <Feature Id="DefaultFeature" ConfigurableDirectory="INSTALLDIR">

It will use a component search to fine the file and set the INSTALLDIR to the file's parent directory. It's an odd syntax full of nuance as necessitated by Windows Installer that I documented many years ago in the WiX's help docs, so I wanted to include it both for posterity as to why the decision was made to exclude it, as well as what it would look like if it were ever desired. The ComponentSearch/@Type has to match the key path of the component you're searching for, and the intermediate DirectorySearch is what sets the outer Property to the value IIF @AssignToProperty="yes" and the FileSearch/@Id is unique - normally it should be the same as its parent DirectorySearch.

Note also that a registry value is used to store the INSTALLDIR going forward. This is the general property persistence pattern with Windows Installer and generally more useful than using a ComponentSearch that, from a support standpoint (there are registry values but undocumented, and can change), is only supported through an API call or within an MSI package.