installer: fix the broken "Enable Pseudo Console Support" option #522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
This helps debugging a lot. Signed-off-by: Johannes Schindelin <[email protected]>
Ever since c800a3f (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 <[email protected]>
dscho
force-pushed
the
fix-pcon-installer-option
branch
from
2b5a99a to
98170e6
Compare
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 c800a3f (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 <[email protected]>
In addition to the exit code, let's make extra certain that the installation succeeded. Signed-off-by: Johannes Schindelin <[email protected]>
As of git-for-windows/git#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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
In 3cc29a6 (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 git-for-windows/git#4571. Reported-by: Erik Falor <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
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 <[email protected]>
…rrectly In git-for-windows/git#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 <[email protected]>
dscho
force-pushed
the
fix-pcon-installer-option
branch
from
98170e6 to
8afe9fc
Compare
|
/add release note bug The installer option to enable support for Pseudo Consoles has been handled incorrectly since Git for Windows v2.41.0, which has been fixed. The workflow run was started |
github-actions bot
pushed a commit
that referenced
this pull request
Aug 28, 2023
The installer option to enable support for Pseudo Consoles [has been handled incorrectly](git-for-windows/git#4571) since Git for Windows v2.41.0, which [has been fixed](#522). Signed-off-by: Johannes Schindelin <[email protected]>
1 task
This comment has been minimized.
This comment has been minimized.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As reported in git-for-windows/git#4571, the Pseudo Console support checkbox does the exact opposite of what it is intended to do.
Turns out that this bug has been with us since Git for Windows v2.41.0, i.e. since June 1st 2023 (or, if you count the pre-releases, since since May 17th, 2023).
This PR fixes the logic and adds a check to prevent regressions in the future.