From 59b04a94b05838883cdfa49017927c527f8befc9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 12:00:39 +0200 Subject: [PATCH 01/12] PR build (run the installer): do verify the Git version This is one of those copy/edit fails for which I am so righteously famous: There simply _is no `bundle-artifacts/` directory in this workflow and therefore the code currently greps for the empty string, which is trivially found in the `version` file. Instead, record the intended Git version in the step that creates the sdk-artifact, and then use that information to validate that the intended Git version was installed. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7e10d0d483..110070a897 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -152,6 +152,7 @@ jobs: git clone --bare --depth=1 --single-branch --branch=main $partial \ https://github.com/git-for-windows/git-sdk-${{ matrix.bitness }} .sdk - name: build ${{ matrix.artifact }} artifact + id: build-artifact shell: bash run: | case "${{ matrix.artifact }}" in @@ -166,6 +167,7 @@ jobs: ${{ matrix.artifact }} ;; esac && + echo "git-version=$(sdk-artifact/cmd/git.exe version)" >>$GITHUB_OUTPUT && cygpath -aw "$PWD/sdk-artifact/usr/bin/core_perl" >>$GITHUB_PATH && cygpath -aw "$PWD/sdk-artifact/usr/bin" >>$GITHUB_PATH && cygpath -aw "$PWD/sdk-artifact/mingw${{ matrix.bitness }}/bin" >>$GITHUB_PATH && @@ -195,7 +197,7 @@ jobs: cygpath -aw / && git.exe version --build-options >version && cat version && - grep "$(sed -e 's|^v||' -e 's|-|.|g' Date: Thu, 24 Aug 2023 12:24:10 +0200 Subject: [PATCH 02/12] PR build (run the installer): do show the installer log This helps debugging a lot. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 110070a897..db68d73a8a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -193,6 +193,10 @@ jobs: if: matrix.artifact == 'build-installers' shell: bash run: | + echo '::group::installer.log' + cat installer.log + echo '::endgroup' + set -x && cygpath -aw / && git.exe version --build-options >version && From 00cd1383ba937007ae5e0ec8a11c28af610efc6d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 12:04:55 +0200 Subject: [PATCH 03/12] PR build (run the installer): do install the 32-bit version Ever since c800a3f60 (installer: loudly warn when installing a 32-bit Git for Windows, 2023-02-21), we complained loudly when a user tried to install a 32-bit Git for Windows on a 64-bit Windows. As somewhat intended side effect, it became impossible to install a 32-bit Git for Windows on a 64-bit Windows. One of the unintended consequences, though, was that the automated workflow runs that verify the installer could no longer run the 32-bit installer successfully. Unfortunately, due to a bug in the workflow definition (which will be fixed after this here commit), this problem never caused the workflow runs to fail, and therefore both bugs went undetected for months. This commit fixes the bug where the 32-bit installer would not complete successfully in the workflow runs. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 2 +- installer/install.iss | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index db68d73a8a..412e4ccd6d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -186,7 +186,7 @@ jobs: shell: pwsh run: | $exePath = Get-ChildItem -Path installer-${{ matrix.bitness }}/*.exe | %{$_.FullName} - Start-Process -Wait -FilePath "$exePath" -ArgumentList "/SILENT /VERYSILENT /NORESTART /SUPPRESSMSGBOXES /ALLOWDOWNGRADE=1 /LOG=installer.log" + Start-Process -Wait -FilePath "$exePath" -ArgumentList "/SILENT /VERYSILENT /NORESTART /SUPPRESSMSGBOXES /ALLOWDOWNGRADE=1 /ALLOWINSTALLING32ON64=1 /LOG=installer.log" "$env:ProgramFiles\Git\usr\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH "$env:ProgramFiles\Git\mingw${{env.ARCH_BITNESS}}\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH - name: validate diff --git a/installer/install.iss b/installer/install.iss index b6ff32cd9a..e0f318e023 100644 --- a/installer/install.iss +++ b/installer/install.iss @@ -1098,7 +1098,7 @@ begin Result:=True; if not IsX64 then SuppressibleMsgBox('Git for Windows (32-bit) is nearing its end of support.'+#13+'More information at https://gitforwindows.org/32-bit.html',mbError,MB_OK or MB_DEFBUTTON1,IDOK) - else if SuppressibleMsgBox('Git for Windows (32-bit) is nearing its end of support. It is recommended to install the 64-bit variant of Git for Windows instead.'+#13+'More information at https://gitforwindows.org/32-bit.html'+#13+'Continue to install the 32-bit variant?',mbError,MB_YESNO or MB_DEFBUTTON2,IDNO)=IDNO then + else if not ParamIsSet('ALLOWINSTALLING32ON64') and (SuppressibleMsgBox('Git for Windows (32-bit) is nearing its end of support. It is recommended to install the 64-bit variant of Git for Windows instead.'+#13+'More information at https://gitforwindows.org/32-bit.html'+#13+'Continue to install the 32-bit variant?',mbError,MB_YESNO or MB_DEFBUTTON2,IDNO)=IDNO) then Result:=False; #else if not IsWin64 then begin From 2f1c99859a700c050aeba96b1bfc0b4be441038e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 11:25:58 +0200 Subject: [PATCH 04/12] PR build (run the installer): exit with error if the installer failed When running a PowerShell script, the exit code will be determined by the last command that was run. Unfortunately, in the PR build, we run a couple of commands _after_ running the installer, and since we do not check for the exit code of the installer process, we miss failures. This is particularly bad because we missed the fact that we failed to run the 32-bit installer tests ever since we deprecated installing the 32-bit variant on a 64-bit system (such as the GitHub Actions build agent where the PR build runs...) in c800a3f60 (installer: loudly warn when installing a 32-bit Git for Windows, 2023-02-21). Let's check explictly if the installer failed. Note: We cannot simply use `$?` but need to look at the `ExitCode` attribute of the process returned by `Start-Process` _and_ the `-Wait` option for this to work, due to a bug in `Start-Process` where the `ExitCode` attribute is not populated properly without `-Wait`. And to get to the `ExitCode` we naturally need the `-PassThru` option, too. See https://stackoverflow.com/a/16018287/1860823 for a more complete explanation. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 412e4ccd6d..546f572417 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -186,7 +186,12 @@ jobs: shell: pwsh run: | $exePath = Get-ChildItem -Path installer-${{ matrix.bitness }}/*.exe | %{$_.FullName} - Start-Process -Wait -FilePath "$exePath" -ArgumentList "/SILENT /VERYSILENT /NORESTART /SUPPRESSMSGBOXES /ALLOWDOWNGRADE=1 /ALLOWINSTALLING32ON64=1 /LOG=installer.log" + $installer = Start-Process -PassThru -Wait -FilePath "$exePath" -ArgumentList "/SILENT /VERYSILENT /NORESTART /SUPPRESSMSGBOXES /ALLOWDOWNGRADE=1 /ALLOWINSTALLING32ON64=1 /LOG=installer.log" + $exitCode = $installer.ExitCode + if ($exitCode -ne 0) { + Write-Host "::error::Installer failed with exit code $exitCode!" + exit 1 + } "$env:ProgramFiles\Git\usr\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH "$env:ProgramFiles\Git\mingw${{env.ARCH_BITNESS}}\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH - name: validate From 9526d18509a348f4b9aecf8f576d0a22263e7dd8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 12:53:08 +0200 Subject: [PATCH 05/12] PR build (run the installer): verify that the installer claims success In addition to the exit code, let's make extra certain that the installation succeeded. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 546f572417..e2de7537e0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -203,6 +203,7 @@ jobs: echo '::endgroup' set -x && + grep 'Installation process succeeded' installer.log && cygpath -aw / && git.exe version --build-options >version && cat version && From 92c2f9aa5c82eca1e595c75c267425e77a32b609 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 12:53:59 +0200 Subject: [PATCH 06/12] installer: avoid trying to copy a non-existent file As of https://github.com/git-for-windows/git/pull/4252, the "dashed" command `git-log.exe` is no longer provided by the Git for Windows project. However, our very own installer expected it still to be there, in case a fall-back was needed when hard-linking the "dashed" built-in commands. Which resulted in beautiful (non-fatal) error messages like: Line 3029: Creating copy "C:\Program Files (x86)\Git\tmp\git-wrapper.exe" failed. Line 3053: Deleting temporary "C:\Program Files (x86)\Git\tmp\git-wrapper.exe" failed. Since the intention was always to use the `git-wrapper.exe` file for that fall-back, let's use that file directly instead, and avoid copying any file preemptively, whether it exists or not. Signed-off-by: Johannes Schindelin --- installer/install.iss | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/installer/install.iss b/installer/install.iss index e0f318e023..40aab387f8 100644 --- a/installer/install.iss +++ b/installer/install.iss @@ -2639,7 +2639,7 @@ begin end; if not LinkCreated then begin - if not FileCopy(AppDir+'\tmp\git-wrapper.exe',FileName,False) then begin + if not FileCopy(AppDir+'\{#MINGW_BITNESS}\share\git\git-wrapper.exe',FileName,False) then begin Log('Line {#__LINE__}: Creating copy "'+FileName+'" failed.'); // This is not a critical error, Git could basically be used without the // aliases for built-ins, so we continue. @@ -3024,11 +3024,6 @@ begin end; end; - // Copy git-wrapper to the temp directory. - if not FileCopy(AppDir+'\{#MINGW_BITNESS}\libexec\git-core\git-log.exe',AppDir+'\tmp\git-wrapper.exe',False) then begin - Log('Line {#__LINE__}: Creating copy "'+AppDir+'\tmp\git-wrapper.exe" failed.'); - end; - // Create built-ins as aliases for git.exe. for i:=0 to Count do begin FileName:=AppDir+'\{#MINGW_BITNESS}\bin\'+BuiltIns[i]; @@ -3048,10 +3043,6 @@ begin HardlinkOrCopyGit(AppDir+'\cmd\git-lfs.exe',False); end; - // Delete git-wrapper from the temp directory. - if not DeleteFile(AppDir+'\tmp\git-wrapper.exe') then begin - Log('Line {#__LINE__}: Deleting temporary "'+AppDir+'\tmp\git-wrapper.exe" failed.'); - end; end else LogError('Line {#__LINE__}: Unable to read file "{#MINGW_BITNESS}\{#APP_BUILTINS}".'); From 44a9c8985f0884cc9317e509eb73d3e9b6950a53 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 13:00:59 +0200 Subject: [PATCH 07/12] installer: adjust a misleading log message When readers like myself see the word "failed", all kind of neurotransmitters start doing work. When the only issue at hand is that a check was not performed because it needs to be run in non-admin mode and we happen to have been called as admin and therefore have no way to perform this check, it is incorrect to even say something _failed_. It was _skipped_, is all. In preparation for adding a stringent check to the PR build that verifies that working installers can be built reliably, let's tone down that message. Signed-off-by: Johannes Schindelin --- installer/install.iss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/installer/install.iss b/installer/install.iss index 40aab387f8..b8d7f2e332 100644 --- a/installer/install.iss +++ b/installer/install.iss @@ -1272,7 +1272,7 @@ begin if IsOriginalUserAdmin then begin // detection only works when we're not running as admin - Log('Symbolic link permission detection failed: running as admin'); + Log('Skipping symbolic link permission detection: running as admin'); Result:=False; end else begin // maybe rights assigned through group policy without enabling developer mode? From d034bbbccfa7051b7ef14b35291b91f4b594e1ca Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 13:03:57 +0200 Subject: [PATCH 08/12] PR build (run the installer): report any failure of any kind We really want no failures to be hidden, whether fatal or non-fatal. Regular installations should always succeed without any failures. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e2de7537e0..238980b656 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -204,6 +204,7 @@ jobs: set -x && grep 'Installation process succeeded' installer.log && + ! grep -iw failed installer.log && cygpath -aw / && git.exe version --build-options >version && cat version && From 8e51e656566c106ccdf850e9a8ae0f49ed999441 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 13:43:04 +0200 Subject: [PATCH 09/12] PR build (run the installer): do validate the correct installation When we just installed the 32-bit variant of Git for Windows, we would really actually like to validate that one, not the 64-bit installation that comes as part of the hosted GitHub Actions runners. Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 238980b656..2005941571 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -192,8 +192,13 @@ jobs: Write-Host "::error::Installer failed with exit code $exitCode!" exit 1 } - "$env:ProgramFiles\Git\usr\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH - "$env:ProgramFiles\Git\mingw${{env.ARCH_BITNESS}}\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH + if ("${{ matrix.bitness }}" -eq 32) { + "${env:ProgramFiles(x86)}\Git\usr\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH + "${env:ProgramFiles(x86)}\Git\mingw32\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH + } else { + "$env:ProgramFiles\Git\usr\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH + "$env:ProgramFiles\Git\mingw${{env.ARCH_BITNESS}}\bin" | Out-File -Encoding ascii -Append $env:GITHUB_PATH + } - name: validate if: matrix.artifact == 'build-installers' shell: bash From 47f40872224d7fd72198cb52534adea86df3f5af Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 09:49:34 +0200 Subject: [PATCH 10/12] installer: fix the broken "Enable Pseudo Console Support" option In 3cc29a613 (installer: explicitly turn off pseudo console support, when asked, 2023-05-16), I tried to explicitly turn off pseudo console support when the checkbox is unchecked. However, the change contains a bug: While the indentation clearly relates the intention to have an `if pcon_enabled ... else ...` type of flow, the code block before the `else` was not terminated, and therefore it was essentially an `if pcon_enabled ...` type of flow, i.e. it did _nothing_ when the checkbox was unchecked. Even worse: if the checkbox _was_ checked, the added `else` block would now be triggered if the `git-bash.config` was _written correctly_, but would now incorrectly _overwrite_ the contents (which would read "MSYS=enable_pcon" as intended) with "MSYS=disable_pcon". In essence, the bug where the block before the `else` was not correctly terminated (and the block after the `else` was not correctly opened) leads to _the opposite_ of the intended logic: - If the "Enable Pseudo Console Support" box is checked, we would eventually write the config file such that Pseudo Console support is _disabled_! - If the box is left unchecked, we would not write `git-bash.config` at all, going with the default of the MSYS2 runtime, which is _to enable_ Pseudo Console support! Let's correct this unintentional behavior by fixing the bug. This fixes https://github.com/git-for-windows/git/issues/4571. Reported-by: Erik Falor Signed-off-by: Johannes Schindelin --- installer/install.iss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/installer/install.iss b/installer/install.iss index b8d7f2e332..6866b505f1 100644 --- a/installer/install.iss +++ b/installer/install.iss @@ -3184,7 +3184,7 @@ begin if not SaveStringToFile(ExpandConstant('{app}\etc\git-bash.config'),'MSYS=enable_pcon',False) then begin LogError('Could not write to '+ExpandConstant('{app}\etc\git-bash.config')) end - else + end else begin if not SaveStringToFile(ExpandConstant('{app}\etc\git-bash.config'),'MSYS=disable_pcon',False) then begin LogError('Could not write to '+ExpandConstant('{app}\etc\git-bash.config')) end From f4e9b636146a9ba5198cd8349d6584076d3d284f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 10:13:56 +0200 Subject: [PATCH 11/12] installer checklist: suggest to run the script To verify that Git for Windows installer works as intended, there is a "pre-flight check list" in `checklist.txt`. A few of the steps listed in that file have been performed manually with every Git for Windows release and pre-release for years, until I got tired of doing them manually (and I missed a few every once in a while), which is when I started putting as much as I could into the `run-checklist.sh` script. Instead of duplicating the information in `checklist.txt` that is already contained in the script, let `checklist.txt` suggest to run the script instead. Signed-off-by: Johannes Schindelin --- installer/checklist.txt | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/installer/checklist.txt b/installer/checklist.txt index 3f52e68a93..9c2b16500a 100644 --- a/installer/checklist.txt +++ b/installer/checklist.txt @@ -8,11 +8,8 @@ After building new installers, check that - `gitk` runs and shows the history - `git gui` runs and does not complain about a missing repository - `git help git` opens the page and it has no verbatim `linkgit:` - - `curl --version` lists `IPv6` in the last line - - verify that `git -c http.sslbackend=schannel2 ls-remote - https://github.com/dscho/images` errors out, and that it works with - schannel and openssl - - verify that `git ls-remote` works with a VSTS SSH URL + - `run-checklist.sh` passes (performs a couple automated tests related + to cURL, the Git version, etc) - Git CMD - starts - `git log` in a repository is colorful and stops after the first page From 8afe9fc1351d34277dc3dfc9bdf0d8c2ad800275 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 24 Aug 2023 10:17:45 +0200 Subject: [PATCH 12/12] run-checklist: validate that the Pseudo Console support is handled correctly In https://github.com/git-for-windows/git/issues/4571, it was pointed out that this support was inverted, which we fixed. To verify that it does not regress, let's add an explicit check. Signed-off-by: Johannes Schindelin --- installer/run-checklist.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/installer/run-checklist.sh b/installer/run-checklist.sh index ede2f8694e..706964d069 100644 --- a/installer/run-checklist.sh +++ b/installer/run-checklist.sh @@ -20,4 +20,32 @@ case "$(git version)" in *2.40.*|*2.[123][0-9].*) true;; *) exit 123;; esac git ls-remote git@ssh.dev.azure.com:v3/git-for-windows/git/git main +die () { + echo "$*" >&2 + exit 1 +} + +pcon_choice="$(sed -n 's/^Enable Pseudo Console Support: //p' /etc/install-options.txt)" || +die 'Could not read /etc/install-options.txt' + +if test -n "$pcon_choice" +then + pcon_config="$(cat /etc/git-bash.config)" || + die 'Could not read /etc/git-bash.config' + + case "$pcon_choice" in + Enabled) + test "MSYS=enable_pcon" = "$pcon_config" || + die "Expected enable_pcon in git-bash.config, but got '$pcon_config'" + ;; + Disabled) + test "MSYS=disable_pcon" = "$pcon_config" || + die "Expected disable_pcon in git-bash.config, but got '$pcon_config'" + ;; + *) + die "Unexpected Pseudo Console choice: $pcon_choice" + ;; + esac +fi + echo "All checks passed!" >&2