Skip to content
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

Test native Windows on CI #1745

Merged
merged 29 commits into from
Nov 29, 2023
Merged

Commits on Nov 15, 2023

  1. Add native Windows test jobs to CI matrix

    This expands the CI test matrix in the main testing workflow to
    test on both Ubuntu and Windows, instead of just Ubuntu.
    
    It does not attempt to merge in the Cygwin workflow at this time,
    which may or may not end up being useful to do in the future.
    
    The new Windows test jobs all fail currently: the runs fail as a
    result of various tests being consistently unable to pass but not
    yet being marked xfail (or skip). It should be feasible to mark
    these xfail with informative messages, but this commit doesn't
    include that.
    EliahKagan committed Nov 15, 2023
    Configuration menu
    Copy the full SHA
    2fd79f4 View commit details
    Browse the repository at this point in the history
  2. Add xfail marks for IndexFile.from_tree failures

    8 of the tests that fail on native Windows systems fail due to
    IndexFile.from_tree being broken on Windows, causing gitpython-developers#1630.
    
    This commit marks those tests as xfail. This is part, though not
    all, of the changes to get CI test jobs for native Windows that
    are passing, to guard against new regressions and to allow the code
    and tests to be gradually fixed (see discussion in gitpython-developers#1654).
    
    When fixing the bug, this commit can be reverted.
    EliahKagan committed Nov 15, 2023
    Configuration menu
    Copy the full SHA
    6e477e3 View commit details
    Browse the repository at this point in the history

Commits on Nov 16, 2023

  1. Mark test_clone_command_injection xfail on Windows

    This other GitCommandError on Windows is not related to
    IndexFile.from_tree whose 8 related failing tests were marked xfail
    in the preceding commit.
    
    Also, test_clone_command_injection should not be confused with
    test_clone_from_command_injection, which passes on all platforms.
    
    The problem here appears to be that, on Windows, the path of the
    directory GitPython is intended to clone to -- when the possible
    security vulnerability this test checks for is absent -- is not
    valid. This suggests the bug may only be in the test and that the
    code under test may be working on Windows. But the test does not
    establish that, for which it would need to test with a payload
    clearly capable of creating the file unexpected_path refers to when
    run on its own.
    
    This doesn't currently seem to be reported as a bug, but some
    general context about the implementation can be examined in gitpython-developers#1518
    where it was introduced, and gitpython-developers#1531 where it was modified.
    EliahKagan committed Nov 16, 2023
    Configuration menu
    Copy the full SHA
    cd9d7a9 View commit details
    Browse the repository at this point in the history
  2. Mark test_diff_submodule xfail on Windows

    Everything attempted in the test case method is actually working
    and passes, but when the tearDown method calls shutil.rmtree, it
    fails with "Access is denied" and raises PermissionError. The
    reason and exception are accordingly noted in the xfail decoration.
    
    While adding a pytest import to apply the pytest xfail mark, I also
    improved grouping/sorting of other imports in the test_diff module.
    EliahKagan committed Nov 16, 2023
    Configuration menu
    Copy the full SHA
    f72e282 View commit details
    Browse the repository at this point in the history

Commits on Nov 24, 2023

  1. Mark TestSubmodule.test_rename xfail on Windows

    The test fails when attempting to evaluate the expression:
    
        sm.move(new_path).name
    
    as part of the assertion:
    
        assert sm.move(new_path).name == new_path
    
    But it is the call to sm.move that fails, not the assertion itself.
    The condition is never evaluated, because the subexpression raises
    PermissionError. Details are in the xfail reason string.
    
    This test_submodule.TestSubmodule.test_rename method should not be
    confused with test_config.TestBase.test_rename, which passes on all
    platforms.
    
    On CI this has XPASS status on Python 3.7 and possibly some other
    versions. This should be investigated and, if possible, the xfail
    markings should be made precise. (When working on this change
    before a rebase, I had thought only 3.7 xpassed, and that the XPASS
    was only on CI, never locally. However, I am unable to reproduce
    that or find CI logs of it, so some of these observations may have
    been mistaken and/or or specific to a narrow environment.)
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    42a3d74 View commit details
    Browse the repository at this point in the history
  2. Mark test_conditional_includes_from_git_dir xfail on Windows

    As noted, the second of the config._included_paths() assertions
    fails, which is in the "Ensure that config is included if path is
    matching git_dir" sub-case.
    
    It is returning 0 on Windows. THe GitConfigParser._has_includes
    function returns the expression:
    
        self._merge_includes and len(self._included_paths())
    
    Since _merge_includes is a bool, it appears the first branch of the
    "and" is True, but then _included_paths returns an empty list.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    4abab92 View commit details
    Browse the repository at this point in the history
  3. Improve ordering/grouping of a few imports

    In modules soon to be modified, so if subsequent commits are later
    reverted, these import tweaks are not automatically undone.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    799c853 View commit details
    Browse the repository at this point in the history
  4. Mark test_create_remote_unsafe_url_allowed xfail on Windows

    The test mostly works on Windows, but it fails because the tmp_file
    path is expected to appear in remote_url with backslash separators,
    but (forward) slashes appear instead.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    b284ad7 View commit details
    Browse the repository at this point in the history
  5. Mark unsafe-options "allowed" tests xfail on Windows

    The tests of unsafe options are among those introduced originally
    in gitpython-developers#1521. They are regression tests for gitpython-developers#1515 (CVE-2022-24439).
    The unsafe options tests are paired: a test for the usual, default
    behavior of forbidding the option, and a test for the behavior when
    the option is explicitly allowed. In each such pair, both tests use
    a payload that is intended to produce the side effect of a file of
    a specific name being created in a temporary directory.
    
    All the tests work on Unix-like systems. On Windows, the tests of
    the *allowed* cases are broken, and this commit marks them xfail.
    However, this has implications for the tests of the default, secure
    behavior, because until the "allowed" versions work on Windows, it
    will be unclear if either are using a payload that is effective and
    that corresponds to the way its effect is examined.
    
    What *seems* to happen is this: The "\" characters in the path are
    treated as shell escape characters rather than literally, with the
    effect of disappearing in most paths since most letters lack
    special meaning when escaped. Also, "touch" is not a native Windows
    command, and the "touch" command provided by Git for Windows is
    linked against MSYS2 libraries, causing it to map (some?)
    occurrences of ":" in filenames to a separate code point in the
    Private Use Area of the Basic Multilingual Plane. The result is a
    path with no directory separators or drive letter. It denotes a
    file of an unintended name in the current directory, which is never
    the intended location. The current directory depends on GitPython
    implementation details, but at present it's the top-level directory
    of the rw_repo working tree. A new unstaged file, named like
    "C\357\200\272UsersekAppDataLocalTemptmpc7x4xik5pwn", can be
    observed there (this is how "git status" will format the name).
    
    Fortunately, this and all related tests are working on other OSes,
    and the affected code under test does not appear highly dependent
    on OS. So the fix is *probably* fully working on Windows as well.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    61d1fba View commit details
    Browse the repository at this point in the history
  6. Show PATH on CI

    Along with the version and platform information that is already
    shown. This is to make debugging easier.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    ad07ecb View commit details
    Browse the repository at this point in the history
  7. Show bash and other WSL-relevant info but not PATH

    Only on native Windows systems (since this is for debugging WSL).
    
    The Windows CI runners have WSL itself installed but no WSL systems
    installed. So some of these commands may fail if they call the
    bash.exe in the System32 directory, but this should still be
    illuminating. Results after a WSL system is set up will likely be
    more important, but doing this first allows for comparison.
    
    The reason PATH directories are no longer listed is to decrease
    noise.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    2784e40 View commit details
    Browse the repository at this point in the history
  8. Install WSL system on CI for hook tests

    This makes three of the four hook-related tests pass instead of
    failing, and changes the way the fourth fails.
    
    GitPython uses bash.exe to run hooks that are (or appear to be)
    shell scripts. On many Windows systems, this is the bash.exe in the
    system32 directory, which delegates to bash in a WSL system if at
    least one such system is installed (for the current user), and
    gives an error otherwise. It may be a bug that GitPython ends up
    using this bash.exe when WSL is installed but no WSL systems exist,
    since that is actually a fairly common situation. One place that
    happened was on the GitHub Actions runners used for Windows jobs.
    Those runners have WSL available, and are capable of running WSL 1
    systems (not currently WSL 2 systems), but no WSL systems were
    actually installed.
    
    This commit fixes that cause of failure, for all four tests it
    happened in, by setting up a Debian WSL system on the test runner.
    (This increases the amount of time it takes Windows jobs to run,
    but that might be possible to improve on.) Three of those four
    tests now pass, while the other fails for another reason.
    
    The newly passing tests are:
    
    - test/test_index.py::TestIndex::test_commit_msg_hook_fail
    - test/test_index.py::TestIndex::test_pre_commit_hook_fail
    - test/test_index.py::TestIndex::test_pre_commit_hook_success
    
    The test that still fails, but differently, is:
    
    - test/test_index.py::TestIndex::test_commit_msg_hook_success
    
    I had previously found that test to fail on a local WSL 2 system,
    and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679).
    But the condition I wrote there *appears* to have a bug related to
    the different orders in which subproces.Popen and shutil.which find
    executables, causing it not always to detect when the WSL-related
    bash.exe is the one the Popen call in git.index.fun.run_commit_hook
    will use.
    EliahKagan committed Nov 24, 2023
    Configuration menu
    Copy the full SHA
    9717b8d View commit details
    Browse the repository at this point in the history

Commits on Nov 25, 2023

  1. Fix and expand bash.exe xfail marks on hook tests

    881456b (gitpython-developers#1679) expanded the use of shutil.which in test_index.py
    to attempt to mark test_commit_msg_hook_success xfail when bash.exe
    is a WSL bash wrapper in System32 (because that test currently
    is not passing when the hook is run via bash in a WSL system, which
    the WSL bash.exe wraps). But this was not reliable, due to
    significant differences between shell and non-shell search behavior
    for executable commands on Windows. As the new docstring notes,
    lookup due to Popen generally checks System32 before going through
    directories in PATH, as this is how CreateProcessW behaves.
    
    - https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
    - python/cpython#91558 (comment)
    
    The technique I had used in 881456b also had the shortcoming of
    assuming that a bash.exe in System32 was the WSL wrapper. But such
    a file may exist on some systems without WSL, and be a bash
    interpreter unrelated to WSL that may be able to run hooks.
    
    In addition, one common situation, which was the case on CI before
    a step to install a WSL distribution was added, is that WSL is
    present but no WSL distributions are installed. In that situation
    bash.exe is found in System32, but it can't be used to run any
    hooks, because there's no actual system with a bash in it to wrap.
    This was not covered before. Unlike some conditions that prevent
    a WSL system from being used, such as resource exhaustion blocking
    it from being started, the absence of a WSL system should probably
    not fail the pytest run, for the same reason as we are trying not
    to have the complete *absence* of bash.exe fail the pytest run.
    Both conditions should be xfail. Fortunately, the error message
    when no distribution exists is distinctive and can be checked for.
    
    There is probably no correct and reasonable way to check LBYL-style
    which executable file bash.exe resolves to by using shutil.which,
    due to shutil.which and subprocess.Popen's differing seach orders
    and other subtleties. So this adds code to do it EAFP-style using
    subprocess.run (which itself uses Popen, so giving the same
    CreateProcessW behavior). It tries to run a command with bash.exe
    whose output pretty reliably shows if the system is WSL or not.
    
    We may fail to get to the point of running that command at all, if
    bash.exe is not usable, in which case the failure's details tell us
    if bash.exe is absent (xfail), present as the WSL wrapper with no WSL
    systems (xfail), or has some other error (not xfail, so the user can
    become aware of and proably fix the problem). If we do get to that
    point, then a file that is almost always present on both WSL 1 and
    WSL 2 systems and almost always absent on any other system is
    checked for, to distinguish whether the working bash shell operates
    in a WSL system, or outside any such system as e.g. Git Bash does.
    
    See https://superuser.com/a/1749811 on various techniques for
    checking for WSL, including the /proc/sys/fs/binfmt_misc/WSLInterop
    technique used here (which seems overall may be the most reliable).
    
    Although the Windows CI runners have Git Bash, and this is even the
    bash.exe that appears first in PATH (giving rise to the problem
    with shutil.which detailed above), it would be a bit awkward to
    test the behavior when Git Bash is actually used to run hooks on
    CI, because of how Popen selects the System32 bash.exe first,
    whether or not any WSL distribution is present. However, local
    testing shows that when Git Bash's bash.exe is selected, all four
    hook tests in the module pass, both before and after this change,
    and furthermore that bash.exe is correctly detected as "native",
    continuing to avoid an erroneous xfail mark in that case.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    5d11394 View commit details
    Browse the repository at this point in the history
  2. Simplify/clarify bash.exe check for hook tests; do it only once

    - Make the design of the enum simpler.
    - Move some informaton to the check method's docstring.
    - Call the method once instead of in each decoration.
    
    Doing the check only once makes things a little faster, but the
    more important reason is that the checks can become stale very
    quickly in some situations. This is unlikely to be an issue on CI,
    but locally WSL may fail (or not fail) to start up when bash.exe
    is used in a check, then not fail (or fail) when actaully needed
    in the test, if the reason it fails is due to resource usage, such
    as most RAM being used on the system.
    
    Although this seems like a reason to do the check multiple times,
    doing it multiple times in the decorations still does it ahead of
    when any of the tests is actually run. In contrast, with the change
    here, we may get outdated results by the time the tests run, but
    the xfail effects of the check are always consistent with each
    other and are no longer written in a way that suggests they are
    more current than they really are.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    b215357 View commit details
    Browse the repository at this point in the history
  3. Temporarily don't install WSL system to test xfail

    This reverts "Install WSL system on CI for hook tests",
    commit 9717b8d.
    
    Assuming both cases are found to be working, the removed step will
    either be brought back or replaced with a very similar step that
    installs a different distribution.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    cabb572 View commit details
    Browse the repository at this point in the history
  4. Put back WSL on Windows CI; pare down debug info

    This undoes "Temporarily don't install WSL system to test xfail"
    (cabb572). It keeps Debian as the distribution. (Although the Debian
    WSL system installs pretty fast already, it may still make sense to
    try switching to Alpine in the future. But that might need to wait
    until Vampire/setup-wsl#50 is fixed.)
    
    This also removes most of the commands in the WSL debugging step,
    since the related machinery in test_index.py (_WinBashStatus) seems
    to be in okay shape, condenses the smaller number of commands that
    are retained there, and makes much less extensive reduction in the
    general version and platform information commands as well. This is
    to make workflow output easier to read, understand, and navigate.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    2875ffa View commit details
    Browse the repository at this point in the history
  5. Treat XPASS status as a test failure

    This causes full "failure" output to be printed when a test marked
    xfail unexpectedly passes, and for the test run to be considered
    failing as a result.
    
    The immediate purpose of this change is to facilitate efficient
    identification of recently introduced wrong or overbroad xfail
    markings.
    
    This behavior may eventually become the pytest default (see gitpython-developers#1728
    and references therein), and this could be retained even after the
    current xpassing tests are investigated, to facilitate timely
    detection of tests marked xfail of code that is newly working.
    
    (Individual tests decorated `@pytest.mark.xfail` can still be
    allowed to unexpectedly pass without it being treated like a test
    failure, by passing strict=False explicitly.)
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    0f8cd4c View commit details
    Browse the repository at this point in the history
  6. Correct TestSubmodule.test_rename xfail condition

    The xfail mark was added in 42a3d74, where the XPASS status on 3.7
    was observed but not included in the condition. It turns out this
    seems to XPASS on a much wider range of versions: all but 3.12.
    
    Currently 3.12.0 is the latest stable version and no testing has
    been done with alpha for 3.13. Most likely whatever causes this
    test to fail on 3.12 would also apply to 3.13, but because I don't
    understand the cause, I don't want to guess that, and instead wrote
    the new condition to expect failure only on 3.12.* versions.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    82c361e View commit details
    Browse the repository at this point in the history
  7. Revert "Treat XPASS status as a test failure"

    For its immediate goal, this facilitated identifying the XPASS
    status of TestSubmodule.test_rename and verifying that the revised
    xfail condition eliminated it.
    
    In the test output, XPASS is written as a failure, with strict
    xfail behavior noted. Contributors less familiar with marking tests
    xfail may mistake this to mean some behavior of the code under test
    was broken. So if strict_xfail is to be enabled, it might be best
    to do that in a pull request devoted to it, and maybe even to add
    to the docs, about how to recognize and handle newly xpassing
    tests.
    
    This reverts commit 0f8cd4c.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    0ae5dd1 View commit details
    Browse the repository at this point in the history
  8. Refine TestSubmodule.test_rename xfail condition

    This further improves the condition that was corrected in 82c361e.
    Testing on Python 3.13.0 alpha 2 shows the same failure as on 3.12
    (that I'm at least right now consistently unable to produce on any
    lower versions).
    
    In addition, on both versions of CPython on Windows, the failure
    seems to consistently resolve if two gc.collect() are placed just
    above the code that calls sm.move(). A single call is consistently
    insufficient. I haven't included any such calls in this commit,
    since the focus here is on fixing xfail markings, and becuse such a
    change may benefit from being evaluated separately and may merit
    further accompanying changes. But that this behavior is exhibited
    on both 3.12 and the 3.13 alpha further supports removing the
    upper bound on the xfail marking.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    0b7ee17 View commit details
    Browse the repository at this point in the history
  9. Configuration menu
    Copy the full SHA
    8621e89 View commit details
    Browse the repository at this point in the history
  10. Make _WinBashStatus instances carry all their info

    This changes _WinBashStatus from an enum to a sum type so that,
    rather than having a global _WinBashStatus.ERROR_WHILE_CHECKING
    object whose error_or_process attribute may be absent and, if
    present, carries an object set on the most recent call to check(),
    we get a _WinBashStatus.ErrorWhileChecking instance that carries
    an error_or_process attribute specific to the call.
    
    Ordinarily this would not make a difference, other than that the
    global attribute usage was confusing in the code, because typically
    _WinBashStatus.check() is called only once. However, when
    debugging, having the old attribute on the same object be clobbered
    is undesirable.
    
    This adds the sumtypes library as a test dependency, to allow the
    enum to be rewritten as a sum type without loss of clarity. This is
    a development-only dependency; the main dependencies are unchanged.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    7ff3cee View commit details
    Browse the repository at this point in the history
  11. Use bytes in bash.exe check; retest no-distro case

    This removes text=True from the subprocess.run call, changing str
    literals to bytes where appropriate and (less importantly) using
    "%r" instead of "%s" in log messages so it's clear that printing
    the repr of a bytes object is, at least for now, intentional.
    
    The reason for this is that the encoding of error messages produced
    by running the WSL bash.exe, when it attempts but fails to use a
    WSL system, varies based on what error occurred. When no systems
    are installed, the output can be decoded as UTF-8. When an error
    from "deeper down" is reported, at least for Bash/Service errors,
    the output is encoded in UTF-16LE, and attempting to decode it as
    UTF-8 interleaves lots of null characters in the best case. With a
    bytes object, loss of information is avoided, and it is clear on
    inspection that the output requires decoding.
    
    The most common case of such an error is *probably*:
    
        Insufficient system resources exist to complete the requested service.
        Error code: Bash/Service/CreateInstance/CreateVm/HCS/0x800705aa
    
    However, that is tricky to produce intentionally on some systems.
    To produce a test error, "wsl --shutdown" can be run repeatedly
    while a _WinBashStatus.check() call is in progress. This produces:
    
        The virtual machine or container was forcefully exited.
        Error code: Bash/Service/0x80370107
    
    Assuming the output always includes the text "Error code:", it may
    be feasible to reliably detect which cases it is. This could
    especially improve the log message. But for now that is not done.
    
    In adddition to changing from text to binary mode, this commit also
    temporarily removes the setup-wsl step from the CI workflow again,
    to verify on CI that the text-to-binary change doesn't break the
    WslNoDistro case. Manual testing shows the other cases still work.
    EliahKagan committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    d5ed266 View commit details
    Browse the repository at this point in the history

Commits on Nov 26, 2023

  1. Handle multiple encodings for WSL error messages

    This affects the test suite only. It improves _WinBashStatus.
    
    When bash.exe is the WSL wrapper, and it reports an error that
    prevents bash in a WSL system from ever actually being run, the
    message may be encoded in a narrow or multibyte encoding, or may
    instead be in Windows's native UTF-16LE encoding. This was partly
    handled before, by assuming the message indicating the absence of
    any installed WSL distribuions could be interpreted as UTF-8, but
    matching against bytes and not actually decoding it or other error
    messages. That presented a few problems, which are rememedied here:
    
    - It turns out that this "Windows Subsystem for Linux has no
      installed distributions" message actually can be in UTF-16LE too.
      This happens when it is part of a message that also reports a
      textual error code like:
    
      Bash/Service/CreateInstance/GetDefaultDistro/WSL_E_DEFAULT_DISTRO_NOT_FOUND
    
      Therefore, narrow-encoding matching was not sufficient.
    
    - Logged messages were hard to understand, because they were
      printing the reprs of bytes objects, which sometimes were
      UTF-16LE and thus had many "\x00" interspersed in them.
    
    - The cases were not broken down elegantly. The ErrorWhileChecking
      case could hold a CompletedProcess, a CalledProcessError, or an
      OSError. This is now replaced with a WinError case for the rare
      scenario where CreateProcessW fails in a completely unexpected
      way, and a CheckError case for when the exit status or output of
      bash.exe indicates an error other than one we want to handle.
      CheckError has two attributes: `process` for the CompletedProcess
      (CalledProcessError is no longer used, and CompletedProcess has
      `returncode` and other attributes that provide the same info),
      and `message` for the decoded output.
    EliahKagan committed Nov 26, 2023
    Configuration menu
    Copy the full SHA
    496acaa View commit details
    Browse the repository at this point in the history

Commits on Nov 27, 2023

  1. Don't assume WSL-related bash.exe error is English

    Instead of searching for an English sentence from the error
    message, this searches for the specific aka.ms short URL it
    contains in all languages, which points to a page about downloading
    distributions for WSL from the Microsoft Store (Windows Store).
    
    The URL is controlled and hosted by Microsoft and intended as a
    permalink. Thus this approach should be more reliable even for
    English, as the specific wording of the message might be rephrased
    over time, so this may have a lower risk of false negatives.
    Because it only makes sense to give a link for obtaining a
    distribution in an error message when no distribution is installed
    already, the risk of false positives should also be fairly low.
    
    The URL is included in the output telling the user they have no
    distributions installed even on systems like Windows Server where
    the Microsoft Store is not itself available, and even in situations
    where WSL has successfully downloaded and unpacked a distribution
    but could not run it the first time because it is using WSL 2 but
    the necessary virtualization is unavailable. Because these are
    included among the situations where the hook tests should be marked
    xfail, it is suitable for the purpose at hand.
    
    Furthermore, while this only works if we correctly guess if the
    encoding is UTF-16LE or not, it should be *fairly* robust against
    incorrect decoding of the message where a single-byte encoding is
    treated as UTF-8, or UTF-8 is treated as a single-byte encoding, or
    one single-byte encoding is treated as another (i.e., wrong ANSI
    code page). But some related encoding subtleties remain to be
    resolved.
    
    Although reliability is likely improved even for English, to
    faciliate debugging the WslNoDistro case now carries analogous
    `process` and `message` arguments to the `CheckError` case.
    
    On some Windows systems, wsl.exe exists in System32 but bash.exe
    does not. This is not inherently a problem for run_commit_hook as
    it currently stands, which is just trying to use whatever bash.exe
    is available. (It is not clear that WSL should be preferred.) This
    is currently so, on some systems, where WSL is not really set up;
    wsl.exe can be used to do so. This is a situation where no WSL
    distributions are present, but because the bash.exe WSL wrapper is
    also absent, it is not a problem.
    
    However, I had wrongly claimed in the _WinBashStatus.check
    docstring that bash.exe is in System32 whenever WSL is present. So
    this also revises the docstring to say only that this is usually so
    (plus some further revision for clarity).
    EliahKagan committed Nov 27, 2023
    Configuration menu
    Copy the full SHA
    d779a75 View commit details
    Browse the repository at this point in the history

Commits on Nov 28, 2023

  1. Handle encodings better; make the sum type "public"

    Windows does not have a direct analogue of LANG=C or LC_ALL=C. Some
    programs give them special treatment, but they do not affect the
    way localized behavior of the Windows API operates. In particular,
    the bash.exe WSL wrapper, as well as wsl.exe and wslconfig.exe, do
    not produce their own localized messages (for errors not
    originating in a running distribution) when they are set. Windows
    also provides significant localization through localized versions
    of Windows, so changing language settings in Windows, even
    system-wide, does not always produce the same effect that many or
    most Windows users who use a particular language would experience.
    
    Various encodings may appear when bash.exe is WSL-related but gives
    its own error message. Such a message is often in UTF-16LE, which
    is what Windows uses internally, and preserves any localization.
    That is the more well behaved scenario and already was detected;
    this commit moves, but does not change, the code for that.
    
    The situation where it is not UTF-16LE was previously handled by
    treating it as UTF-8. Because the default strict error treatment
    was used, this would error out in test discovery in some localized
    setups, preventing all tests in test_index from running, including
    the majority of them that are not related to hooks. This fixes that
    by doing better detection that should decode the mesages correctly
    most of the time, that should in practice decode them well enough
    to tell (by the aka.ms URL) if the message is complaining about
    there being no installed distribution all(?) of the time, and that
    should avoid breaking unrelated tests even if that can't be done.
    
    An English non-UTF-16LE message appears on GitHub Actions CI when
    no distribution is installed. Testing of this situation on other
    languages was performed in a virtual machine on a development
    machine.
    
    That the message is output in a narrow character set some of the
    time when bash.exe produces it appears to be a limitation of the
    bash.exe wrapper. In particular, with a zh-CN version of Windows
    (and with the language not changed to anything else), a localized
    message in Simplified Chinese was correctly printed when running
    wsl.exe, but running bash.exe produced literal "?" characters in
    place of Chinese characters (it was not a display or font issue,
    and they were "?" and not Unicode replacement characters). The
    change here does not overcome that; the literal "?" characters will
    be included. But "https://aka.ms/wslstore" is still present if it
    is an error about not having any distributions, so the correct
    status is still inferred.
    
    For more information on code pages in Windows, see:
    https://serverfault.com/a/836221
    
    The following alternatives to all or part of the approach taken
    here were considered but, at least for now, not done, because they
    would not clearly be simpler or more correct:
    
    - Abandoning this "run bash.exe and see what it shows us" approach
      altogether and instead reimplementing the rules CreateProcessW
      uses, to find if the bash.exe the system finds is the one in
      System32, and then, if so, checking the metadata in that
      executable to determine if it's the WSL wrapper. I believe that
      would be even more complex to do correctly than it seems; the
      behavior noted in the WinBashStatus docstring and recent commit
      messages is not the whole story. The documented search order for
      CreateProcessW seems not to be followed in some circumstances.
      One is that the Windows Store version of Python seems always to
      forgo the usual System32 search that precedes seaching directories
      in PATH. It looks like there may also be some subtleties in which
      directories 32-bit builds search in.
    
    - Using chardet. Although the chardet library is excellent, it is
      not clear that the code needed to bridge this highly specific use
      case to it would be simpler than the code currently in use. Some
      of the work might still need to be done by other means; when I
      tested it out for this, this did not detect the UTF-16LE messages
      as such for English. (They are often also valid UTF-8, because
      interleaving null characters is, while strange, permitted.)
    
    - Also calling wsl.exe and/or wslconfig.exe. It's still necessary
      to call bash.exe, because it may not be the WSL bash, even on a
      system with WSL fully set up. Furthermore, those tools' output
      seem to vary in some complex ways, too. Using only one subprocess
      for the detection seemed the simplest. Even using "wsl --list"
      would introduce significant additional logic. Sometimes its
      output is a list of distributions, sometimes it is an error
      message, and if WSL is not set up it may be a help message.
    
    - Using the Windows API to check for WSL systems.
      https://learn.microsoft.com/en-us/windows/win32/api/wslapi/ does
      not currently include functions to list registered distributions.
    
    - Attempting to get wsl.exe to produce an English message using
      Windows API techniques like those used in Locale Emulator. This
      would be complicated, somewhat unintuitive and error prone to do
      in Python, and I am not sure how well it would work on a system
      that does not have an English language pack installed.
    
    - Checking on disk for WSL distributions in the places they are
      most often expected to be. This would intertwine WinBashStatus
      with deep details of how WSL actually operates, and this seems
      like the sort of thing that is likely to change in the future.
    
    However, there may be a more straightforward way to do this (that
    is at least as correct and that remains transparent to debug).
    Especially if the WinBashStatus class remains in test_index for
    long (rather than just being used to aid in debugging existing test
    falures and possible subsequent design decisions for making commit
    hooks work more robustly on Windows in GitPython), then this may
    be worth revisiting.
    
    Thus it is *not* with the intention of treating WinBashStatus as a
    "stable" part of the test suite that it is renamed from
    _WinBashStatus. This is instead done because:
    
    - Like HOOKS_SHEBANG, it makes sense to import it when figuring out
      how the tests work or debugging them.
    
    - When debugging, it is intended that it be imported to call
      check() and examine the resulting `process` and `message`
      information, at least in the CheckError case.
    EliahKagan committed Nov 28, 2023
    Configuration menu
    Copy the full SHA
    9ac2438 View commit details
    Browse the repository at this point in the history
  2. Put back WSL on Windows CI

    So the Windows test jobs can run the three out of four hook tests
    that are able to pass with WSL bash, and so that we are able to
    observe the expected failure on the one that still fails.
    
    This undoes the removal of the setup-wsl step in d5ed266, now that
    the test suite's bash.exe status checking logic, used for xfail
    marks (and also usable in debugging), is internationalized. The CI
    step was omitted during some of this process, to make sure
    WinBashStatus would still detect the specific situation on the
    GitHub Actions runner that occurs when no WSL distro is available.
    
    (This commit thus has much of the same relationship to d5ed266
    as 2875ffa had to cabb572.)
    EliahKagan committed Nov 28, 2023
    Configuration menu
    Copy the full SHA
    b07e5c7 View commit details
    Browse the repository at this point in the history
  3. Improve readability of WinBashStatus class

    - Factor out the code to get the Windows ACP to a helper function,
      and comment to explain why it doesn't use locale.getencoding.
    
    - Remove nonessential material in the WinBashStatus.check docstring
      and reword the rest for clarity.
    
    - Drop reStructuredText notation in the WinBashStatus docstrings,
      because in this case it seems to be making them harder to read in
      the code (we are not generating Sphinx documentation for tests.)
    
    - Revise the comments on specific steps in WinBashStatus._decode
      for accuracy and clarity.
    EliahKagan committed Nov 28, 2023
    Configuration menu
    Copy the full SHA
    3303c74 View commit details
    Browse the repository at this point in the history
  4. Shorten comments on _decode steps

    This removes the parenthesized examples from the per-step comments
    in the WinBashStatus._decode helper.
    EliahKagan committed Nov 28, 2023
    Configuration menu
    Copy the full SHA
    e00fffc View commit details
    Browse the repository at this point in the history