From 2fd79f41fe1304be7bcaebec84394ab9e45961c5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 15 Oct 2023 02:35:44 -0400 Subject: [PATCH 01/29] 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. --- .github/workflows/pythonpackage.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 35fecf16c..95b12004c 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -10,18 +10,19 @@ permissions: jobs: build: - runs-on: ubuntu-latest - strategy: fail-fast: false matrix: + os: ["ubuntu-latest", "windows-latest"] python-version: ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"] include: - experimental: false + runs-on: ${{ matrix.os }} + defaults: run: - shell: /bin/bash --noprofile --norc -exo pipefail {0} + shell: bash --noprofile --norc -exo pipefail {0} steps: - uses: actions/checkout@v4 From 6e477e3a06e4b11d43ea118a9f18cae03fa211fd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 14 Nov 2023 00:41:57 -0500 Subject: [PATCH 02/29] 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 #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 #1654). When fixing the bug, this commit can be reverted. --- test/test_docs.py | 11 +++++++++-- test/test_fun.py | 17 ++++++++++++++--- test/test_index.py | 40 ++++++++++++++++++++++++++++++++++++++++ test/test_refs.py | 11 +++++++++++ 4 files changed, 74 insertions(+), 5 deletions(-) diff --git a/test/test_docs.py b/test/test_docs.py index 2f4b2e8d8..2ff1c794a 100644 --- a/test/test_docs.py +++ b/test/test_docs.py @@ -8,11 +8,10 @@ import pytest +from git.exc import GitCommandError from test.lib import TestBase from test.lib.helper import with_rw_directory -import os.path - class Tutorials(TestBase): def tearDown(self): @@ -207,6 +206,14 @@ def update(self, op_code, cur_count, max_count=None, message=""): assert sm.module_exists() # The submodule's working tree was checked out by update. # ![14-test_init_repo_object] + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_directory def test_references_and_objects(self, rw_dir): # [1-test_references_and_objects] diff --git a/test/test_fun.py b/test/test_fun.py index 566bc9aae..8ea5b7e46 100644 --- a/test/test_fun.py +++ b/test/test_fun.py @@ -3,10 +3,13 @@ from io import BytesIO from stat import S_IFDIR, S_IFREG, S_IFLNK, S_IXUSR -from os import stat +import os import os.path as osp +import pytest + from git import Git +from git.exc import GitCommandError from git.index import IndexFile from git.index.fun import ( aggressive_tree_merge, @@ -34,6 +37,14 @@ def _assert_index_entries(self, entries, trees): assert (entry.path, entry.stage) in index.entries # END assert entry matches fully + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) def test_aggressive_tree_merge(self): # Head tree with additions, removals and modification compared to its predecessor. odb = self.rorepo.odb @@ -291,12 +302,12 @@ def test_linked_worktree_traversal(self, rw_dir): rw_master.git.worktree("add", worktree_path, branch.name) dotgit = osp.join(worktree_path, ".git") - statbuf = stat(dotgit) + statbuf = os.stat(dotgit) self.assertTrue(statbuf.st_mode & S_IFREG) gitdir = find_worktree_git_dir(dotgit) self.assertIsNotNone(gitdir) - statbuf = stat(gitdir) + statbuf = os.stat(gitdir) self.assertTrue(statbuf.st_mode & S_IFDIR) def test_tree_entries_from_data_with_failing_name_decode_py3(self): diff --git a/test/test_index.py b/test/test_index.py index 3d1042be8..9fe35d42b 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -179,6 +179,14 @@ def add_bad_blob(): except Exception as ex: assert "index.lock' could not be obtained" not in str(ex) + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_repo("0.1.6") def test_index_file_from_tree(self, rw_repo): common_ancestor_sha = "5117c9c8a4d3af19a9958677e45cda9269de1541" @@ -229,6 +237,14 @@ def test_index_file_from_tree(self, rw_repo): # END for each blob self.assertEqual(num_blobs, len(three_way_index.entries)) + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_repo("0.1.6") def test_index_merge_tree(self, rw_repo): # A bit out of place, but we need a different repo for this: @@ -291,6 +307,14 @@ def test_index_merge_tree(self, rw_repo): self.assertEqual(len(unmerged_blobs), 1) self.assertEqual(list(unmerged_blobs.keys())[0], manifest_key[0]) + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_repo("0.1.6") def test_index_file_diffing(self, rw_repo): # Default Index instance points to our index. @@ -425,6 +449,14 @@ def _count_existing(self, repo, files): # END num existing helper + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_repo("0.1.6") def test_index_mutation(self, rw_repo): index = rw_repo.index @@ -778,6 +810,14 @@ def make_paths(): for absfile in absfiles: assert osp.isfile(absfile) + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_repo("HEAD") def test_compare_write_tree(self, rw_repo): """Test writing all trees, comparing them for equality.""" diff --git a/test/test_refs.py b/test/test_refs.py index 6ee385007..a1573c11b 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -4,8 +4,11 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ from itertools import chain +import os from pathlib import Path +import pytest + from git import ( Reference, Head, @@ -215,6 +218,14 @@ def test_head_checkout_detached_head(self, rw_repo): assert isinstance(res, SymbolicReference) assert res.name == "HEAD" + @pytest.mark.xfail( + os.name == "nt", + reason=( + "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n" + "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'." + ), + raises=GitCommandError, + ) @with_rw_repo("0.1.6") def test_head_reset(self, rw_repo): cur_head = rw_repo.head From cd9d7a9d558273a1f82527890a1d69529a3ef1d5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 09:55:06 -0500 Subject: [PATCH 03/29] 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 #1518 where it was introduced, and #1531 where it was modified. --- test/test_repo.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/test_repo.py b/test/test_repo.py index e39aee05e..f853e2b36 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -1365,6 +1365,11 @@ def test_do_not_strip_newline_in_stdout(self, rw_dir): r.git.commit(message="init") self.assertEqual(r.git.show("HEAD:hello.txt", strip_newline_in_stdout=False), "hello\n") + @pytest.mark.xfail( + os.name == "nt", + reason=R"fatal: could not create leading directories of '--upload-pack=touch C:\Users\ek\AppData\Local\Temp\tmpnantqizc\pwn': Invalid argument", # noqa: E501 + raises=GitCommandError, + ) @with_rw_repo("HEAD") def test_clone_command_injection(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: From f72e2821a3073c4e87915fe7e721512e16e4fa74 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 12:19:10 -0500 Subject: [PATCH 04/29] 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. --- test/test_diff.py | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/test/test_diff.py b/test/test_diff.py index 1678e737d..1d138a086 100644 --- a/test/test_diff.py +++ b/test/test_diff.py @@ -3,26 +3,17 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ -import ddt +import os +import os.path as osp import shutil import tempfile -from git import ( - Repo, - GitCommandError, - Diff, - DiffIndex, - NULL_TREE, - Submodule, -) -from git.cmd import Git -from test.lib import ( - TestBase, - StringProcessAdapter, - fixture, -) -from test.lib import with_rw_directory -import os.path as osp +import ddt +import pytest + +from git import NULL_TREE, Diff, DiffIndex, GitCommandError, Repo, Submodule +from git.cmd import Git +from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory def to_raw(input): @@ -318,6 +309,11 @@ def test_diff_with_spaces(self): self.assertIsNone(diff_index[0].a_path, repr(diff_index[0].a_path)) self.assertEqual(diff_index[0].b_path, "file with spaces", repr(diff_index[0].b_path)) + @pytest.mark.xfail( + os.name == "nt", + reason='"Access is denied" when tearDown calls shutil.rmtree', + raises=PermissionError, + ) def test_diff_submodule(self): """Test that diff is able to correctly diff commits that cover submodule changes""" # Init a temp git repo that will be referenced as a submodule. From 42a3d74f529f34382b205c0ada46fb18df80c034 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 14:14:44 -0500 Subject: [PATCH 05/29] 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.) --- test/test_submodule.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/test_submodule.py b/test/test_submodule.py index 5be1a1077..68dbcc491 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -949,6 +949,17 @@ def test_remove_norefs(self, rwdir): sm.remove() assert not sm.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=( + "The sm.move call fails. Submodule.move calls os.renames, which raises:\n" + "PermissionError: [WinError 32] " + "The process cannot access the file because it is being used by another process: " + R"'C:\Users\ek\AppData\Local\Temp\test_renamekkbznwjp\parent\mymodules\myname' " + R"-> 'C:\Users\ek\AppData\Local\Temp\test_renamekkbznwjp\parent\renamed\myname'" + ), + raises=PermissionError, + ) @with_rw_directory def test_rename(self, rwdir): parent = git.Repo.init(osp.join(rwdir, "parent")) From 4abab92cb0f4caf569cc4bd9a8084994a80733ce Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 14:54:53 -0500 Subject: [PATCH 06/29] 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. --- test/test_config.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/test_config.py b/test/test_config.py index 0e1bba08a..c1b26c91f 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -6,19 +6,15 @@ import glob import io import os +import os.path as osp from unittest import mock +import pytest + from git import GitConfigParser from git.config import _OMD, cp -from test.lib import ( - TestCase, - fixture_path, - SkipTest, -) -from test.lib import with_rw_directory - -import os.path as osp from git.util import rmfile +from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory _tc_lock_fpaths = osp.join(osp.dirname(__file__), "fixtures/*.lock") @@ -239,6 +235,11 @@ def check_test_value(cr, value): with GitConfigParser(fpa, read_only=True) as cr: check_test_value(cr, tv) + @pytest.mark.xfail( + os.name == "nt", + reason='Second config._has_includes() assertion fails (for "config is included if path is matching git_dir")', + raises=AssertionError, + ) @with_rw_directory def test_conditional_includes_from_git_dir(self, rw_dir): # Initiate repository path. From 799c8536800e75d7ddaef2b588754234db363245 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 19:57:15 -0500 Subject: [PATCH 07/29] 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. --- test/test_remote.py | 27 ++++++++++++++------------- test/test_repo.py | 25 ++++++++++++------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index 12de18476..711c7b1bc 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -3,36 +3,37 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +import os.path as osp +from pathlib import Path import random import tempfile -import pytest from unittest import skipIf +import pytest + from git import ( - RemoteProgress, + Commit, FetchInfo, - Reference, - SymbolicReference, + GitCommandError, Head, - Commit, PushInfo, + Reference, + Remote, + RemoteProgress, RemoteReference, + SymbolicReference, TagReference, - Remote, - GitCommandError, ) from git.cmd import Git -from pathlib import Path from git.exc import UnsafeOptionError, UnsafeProtocolError +from git.util import rmtree, HIDE_WINDOWS_FREEZE_ERRORS, IterableList from test.lib import ( + GIT_DAEMON_PORT, TestBase, - with_rw_repo, - with_rw_and_rw_remote_repo, fixture, - GIT_DAEMON_PORT, + with_rw_and_rw_remote_repo, + with_rw_repo, ) -from git.util import rmtree, HIDE_WINDOWS_FREEZE_ERRORS, IterableList -import os.path as osp # Make sure we have repeatable results. diff --git a/test/test_repo.py b/test/test_repo.py index f853e2b36..3cde25184 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -8,6 +8,7 @@ from io import BytesIO import itertools import os +import os.path as osp import pathlib import pickle import sys @@ -17,22 +18,22 @@ import pytest from git import ( + BadName, + Commit, + Git, + GitCmdObjectDB, + GitCommandError, + GitDB, + Head, + IndexFile, InvalidGitRepositoryError, - Repo, NoSuchPathError, - Head, - Commit, Object, - Tree, - IndexFile, - Git, Reference, - GitDB, - Submodule, - GitCmdObjectDB, Remote, - BadName, - GitCommandError, + Repo, + Submodule, + Tree, ) from git.exc import ( BadObject, @@ -43,8 +44,6 @@ from git.util import bin_to_hex, cygpath, join_path_native, rmfile, rmtree from test.lib import TestBase, fixture, with_rw_directory, with_rw_repo -import os.path as osp - def iter_flatten(lol): for items in lol: From b284ad70291d24024b840fa659f54276cf9ceaa5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 20:17:51 -0500 Subject: [PATCH 08/29] 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. --- test/test_remote.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/test_remote.py b/test/test_remote.py index 711c7b1bc..f9f35e5d8 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -3,6 +3,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +import os import os.path as osp from pathlib import Path import random @@ -767,6 +768,11 @@ def test_create_remote_unsafe_url(self, rw_repo): Remote.create(rw_repo, "origin", url) assert not tmp_file.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=R"Multiple '\' instead of '/' in remote.url make it differ from expected value", + raises=AssertionError, + ) @with_rw_repo("HEAD") def test_create_remote_unsafe_url_allowed(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: From 61d1fba6dd318e7f68b93b0e8b46050db0d03fa8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 15 Nov 2023 21:04:04 -0500 Subject: [PATCH 09/29] Mark unsafe-options "allowed" tests xfail on Windows The tests of unsafe options are among those introduced originally in #1521. They are regression tests for #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. --- test/test_remote.py | 27 +++++++++++++++++++++++++++ test/test_repo.py | 18 ++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/test/test_remote.py b/test/test_remote.py index f9f35e5d8..080718a1c 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -831,6 +831,15 @@ def test_fetch_unsafe_options(self, rw_repo): remote.fetch(**unsafe_option) assert not tmp_file.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=( + "File not created. A separate Windows command may be needed. This and the " + "currently passing test test_fetch_unsafe_options must be adjusted in the " + "same way. Until then, test_fetch_unsafe_options is unreliable on Windows." + ), + raises=AssertionError, + ) @with_rw_repo("HEAD") def test_fetch_unsafe_options_allowed(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: @@ -890,6 +899,15 @@ def test_pull_unsafe_options(self, rw_repo): remote.pull(**unsafe_option) assert not tmp_file.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=( + "File not created. A separate Windows command may be needed. This and the " + "currently passing test test_pull_unsafe_options must be adjusted in the " + "same way. Until then, test_pull_unsafe_options is unreliable on Windows." + ), + raises=AssertionError, + ) @with_rw_repo("HEAD") def test_pull_unsafe_options_allowed(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: @@ -955,6 +973,15 @@ def test_push_unsafe_options(self, rw_repo): remote.push(**unsafe_option) assert not tmp_file.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=( + "File not created. A separate Windows command may be needed. This and the " + "currently passing test test_push_unsafe_options must be adjusted in the " + "same way. Until then, test_push_unsafe_options is unreliable on Windows." + ), + raises=AssertionError, + ) @with_rw_repo("HEAD") def test_push_unsafe_options_allowed(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: diff --git a/test/test_repo.py b/test/test_repo.py index 3cde25184..033deca1e 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -294,6 +294,15 @@ def test_clone_unsafe_options(self, rw_repo): rw_repo.clone(tmp_dir, **unsafe_option) assert not tmp_file.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=( + "File not created. A separate Windows command may be needed. This and the " + "currently passing test test_clone_unsafe_options must be adjusted in the " + "same way. Until then, test_clone_unsafe_options is unreliable on Windows." + ), + raises=AssertionError, + ) @with_rw_repo("HEAD") def test_clone_unsafe_options_allowed(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: @@ -364,6 +373,15 @@ def test_clone_from_unsafe_options(self, rw_repo): Repo.clone_from(rw_repo.working_dir, tmp_dir, **unsafe_option) assert not tmp_file.exists() + @pytest.mark.xfail( + os.name == "nt", + reason=( + "File not created. A separate Windows command may be needed. This and the " + "currently passing test test_clone_from_unsafe_options must be adjusted in the " + "same way. Until then, test_clone_from_unsafe_options is unreliable on Windows." + ), + raises=AssertionError, + ) @with_rw_repo("HEAD") def test_clone_from_unsafe_options_allowed(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: From ad07ecb0151e64f3fa774099d8c69232df6bda23 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 22 Nov 2023 11:00:54 -0500 Subject: [PATCH 10/29] Show PATH on CI Along with the version and platform information that is already shown. This is to make debugging easier. --- .github/workflows/cygwin-test.yml | 1 + .github/workflows/pythonpackage.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 96728e51a..36329f93f 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -73,6 +73,7 @@ jobs: python -c 'import sys; print(sys.platform)' python -c 'import os; print(os.name)' python -c 'import git; print(git.compat.is_win)' # NOTE: Deprecated. Use os.name directly. + printenv PATH | tr ':' '\n' - name: Test with pytest run: | diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 95b12004c..890681b7d 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -65,6 +65,7 @@ jobs: python -c 'import sys; print(sys.platform)' python -c 'import os; print(os.name)' python -c 'import git; print(git.compat.is_win)' # NOTE: Deprecated. Use os.name directly. + printenv PATH | tr ':' '\n' - name: Check types with mypy run: | From 2784e403d6fd812650f29f1842db1e67344170a3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 22 Nov 2023 12:25:22 -0500 Subject: [PATCH 11/29] 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. --- .github/workflows/cygwin-test.yml | 1 - .github/workflows/pythonpackage.yml | 22 +++++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 36329f93f..96728e51a 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -73,7 +73,6 @@ jobs: python -c 'import sys; print(sys.platform)' python -c 'import os; print(os.name)' python -c 'import git; print(git.compat.is_win)' # NOTE: Deprecated. Use os.name directly. - printenv PATH | tr ':' '\n' - name: Test with pytest run: | diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 890681b7d..d21ea3949 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -65,7 +65,27 @@ jobs: python -c 'import sys; print(sys.platform)' python -c 'import os; print(os.name)' python -c 'import git; print(git.compat.is_win)' # NOTE: Deprecated. Use os.name directly. - printenv PATH | tr ':' '\n' + + # For debugging hook tests on native Windows systems that may have WSL. + - name: Show where bash.exe may be found + if: startsWith(matrix.os, 'windows') + run: | + set +e + type -a bash.exe + python -c 'import shutil; print(shutil.which("bash.exe"))' + bash.exe --version + python -c 'import subprocess; p = subprocess.run(["bash.exe", "--version"]); print(f"result: {p!r}")' + bash.exe -c 'echo "$BASH"' + python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", """echo "$BASH" """]); print(f"result: {p!r}")' + bash.exe -c 'echo "$BASH_VERSION"' + python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", """echo "$BASH_VERSION" """]); print(f"result: {p!r}")' + bash.exe -c 'printenv WSL_DISTRO_NAME' + python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", "printenv WSL_DISTRO_NAME"]); print(f"result: {p!r}")' + bash.exe -c 'ls -l /proc/sys/fs/binfmt_misc/WSLInterop' + python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", "ls -l /proc/sys/fs/binfmt_misc/WSLInterop"]); print(f"result: {p!r}")' + bash.exe -c 'uname -a' + python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", "uname -a"]); print(f"result: {p!r}")' + continue-on-error: true - name: Check types with mypy run: | From 9717b8d847b514f761ecaa423226054c73f2a325 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 17 Nov 2023 11:22:12 -0500 Subject: [PATCH 12/29] 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 (#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. --- .github/workflows/pythonpackage.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index d21ea3949..c2bbc1bbb 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -35,6 +35,12 @@ jobs: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} + - name: Set up WSL + if: startsWith(matrix.os, 'windows') + uses: Vampire/setup-wsl@v2.0.2 + with: + distribution: Debian + - name: Prepare this repo for tests run: | ./init-tests-after-clone.sh From 5d113942786b2cc86f5fd7bb228f9f75c8c78beb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Nov 2023 18:46:00 -0500 Subject: [PATCH 13/29] Fix and expand bash.exe xfail marks on hook tests 881456b (#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 - https://github.com/python/cpython/issues/91558#issuecomment-1100942950 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. --- test/test_index.py | 126 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 113 insertions(+), 13 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index 9fe35d42b..dfacb60db 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -3,12 +3,14 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ +import enum from io import BytesIO +import logging import os import os.path as osp from pathlib import Path from stat import S_ISLNK, ST_MODE -import shutil +import subprocess import tempfile import pytest @@ -34,19 +36,97 @@ HOOKS_SHEBANG = "#!/usr/bin/env sh\n" +log = logging.getLogger(__name__) -def _found_in(cmd, directory): - """Check if a command is resolved in a directory (without following symlinks).""" - path = shutil.which(cmd) - return path and Path(path).parent == Path(directory) +class _WinBashMeta(enum.EnumMeta): + """Metaclass allowing :class:`_WinBash` custom behavior when called.""" -is_win_without_bash = os.name == "nt" and not shutil.which("bash.exe") + def __call__(cls): + return cls._check() -is_win_with_wsl_bash = os.name == "nt" and _found_in( - cmd="bash.exe", - directory=Path(os.getenv("WINDIR")) / "System32", -) + +@enum.unique +class _WinBash(enum.Enum, metaclass=_WinBashMeta): + """Status of bash.exe for native Windows. Affects which commit hook tests can pass. + + Call ``_WinBash()`` to check the status. + + This can't be reliably discovered using :func:`shutil.which`, as that approximates + how a shell is expected to search for an executable. On Windows, there are major + differences between how executables are found by a shell and otherwise. (This is the + cmd.exe Windows shell and should not be confused with bash.exe.) Our run_commit_hook + function in GitPython uses subprocess.Popen, including to run bash.exe on Windows. + It does not pass shell=True (and should not). Popen calls CreateProcessW, which + searches several locations prior to using the PATH environment variable. It is + expected to search the System32 directory, even if another directory containing the + executable precedes it in PATH. (There are other differences, less relevant here.) + When WSL is installed, even if no WSL *systems* are installed, bash.exe exists in + System32, and Popen finds it even if another bash.exe precedes it in PATH, as + happens on CI. If WSL is absent, System32 may still have bash.exe, as Windows users + and administrators occasionally copy executables there in lieu of extending PATH. + """ + + INAPPLICABLE = enum.auto() + """This system is not native Windows: either not Windows at all, or Cygwin.""" + + ABSENT = enum.auto() + """No command for ``bash.exe`` is found on the system.""" + + NATIVE = enum.auto() + """Running ``bash.exe`` operates outside any WSL environment (as with Git Bash).""" + + WSL = enum.auto() + """Running ``bash.exe`` runs bash on a WSL system.""" + + WSL_NO_DISTRO = enum.auto() + """Running ``bash.exe` tries to run bash on a WSL system, but none exists.""" + + ERROR_WHILE_CHECKING = enum.auto() + """Could not determine the status. + + This should not trigger a skip or xfail, as it typically indicates either a fixable + problem on the test machine, such as an "Insufficient system resources exist to + complete the requested service" error starting WSL, or a bug in this detection code. + ``ERROR_WHILE_CHECKING.error_or_process`` has details about the most recent failure. + """ + + @classmethod + def _check(cls): + if os.name != "nt": + return cls.INAPPLICABLE + + try: + # Print rather than returning the test command's exit status so that if a + # failure occurs before we even get to this point, we will detect it. + script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"' + command = ["bash.exe", "-c", script] + proc = subprocess.run(command, capture_output=True, check=True, text=True) + except FileNotFoundError: + return cls.ABSENT + except OSError as error: + return cls._error(error) + except subprocess.CalledProcessError as error: + no_distro_message = "Windows Subsystem for Linux has no installed distributions." + if error.returncode == 1 and error.stdout.startswith(no_distro_message): + return cls.WSL_NO_DISTRO + return cls._error(error) + + status = proc.stdout.rstrip() + if status == "0": + return cls.WSL + if status == "1": + return cls.NATIVE + return cls._error(proc) + + @classmethod + def _error(cls, error_or_process): + if isinstance(error_or_process, subprocess.CompletedProcess): + log.error("Strange output checking WSL status: %s", error_or_process.stdout) + else: + log.error("Error running bash.exe to check WSL status: %s", error_or_process) + cls.ERROR_WHILE_CHECKING.error_or_process = error_or_process + return cls.ERROR_WHILE_CHECKING def _make_hook(git_dir, name, content, make_exec=True): @@ -917,12 +997,22 @@ class Mocked: rel = index._to_relative_path(path) self.assertEqual(rel, os.path.relpath(path, root)) + @pytest.mark.xfail( + _WinBash() is _WinBash.WSL_NO_DISTRO, + reason="Currently uses the bash.exe for WSL even with no WSL distro installed", + raises=HookExecutionError, + ) @with_rw_repo("HEAD", bare=True) def test_pre_commit_hook_success(self, rw_repo): index = rw_repo.index _make_hook(index.repo.git_dir, "pre-commit", "exit 0") index.commit("This should not fail") + @pytest.mark.xfail( + _WinBash() is _WinBash.WSL_NO_DISTRO, + reason="Currently uses the bash.exe for WSL even with no WSL distro installed", + raises=AssertionError, + ) @with_rw_repo("HEAD", bare=True) def test_pre_commit_hook_fail(self, rw_repo): index = rw_repo.index @@ -930,7 +1020,7 @@ def test_pre_commit_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if is_win_without_bash: + if _WinBash() is _WinBash.ABSENT: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") @@ -946,10 +1036,15 @@ def test_pre_commit_hook_fail(self, rw_repo): raise AssertionError("Should have caught a HookExecutionError") @pytest.mark.xfail( - is_win_without_bash or is_win_with_wsl_bash, + _WinBash() in {_WinBash.ABSENT, _WinBash.WSL}, reason="Specifically seems to fail on WSL bash (in spite of #1399)", raises=AssertionError, ) + @pytest.mark.xfail( + _WinBash() is _WinBash.WSL_NO_DISTRO, + reason="Currently uses the bash.exe for WSL even with no WSL distro installed", + raises=HookExecutionError, + ) @with_rw_repo("HEAD", bare=True) def test_commit_msg_hook_success(self, rw_repo): commit_message = "commit default head by Frèderic Çaufl€" @@ -963,6 +1058,11 @@ def test_commit_msg_hook_success(self, rw_repo): new_commit = index.commit(commit_message) self.assertEqual(new_commit.message, "{} {}".format(commit_message, from_hook_message)) + @pytest.mark.xfail( + _WinBash() is _WinBash.WSL_NO_DISTRO, + reason="Currently uses the bash.exe for WSL even with no WSL distro installed", + raises=AssertionError, + ) @with_rw_repo("HEAD", bare=True) def test_commit_msg_hook_fail(self, rw_repo): index = rw_repo.index @@ -970,7 +1070,7 @@ def test_commit_msg_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if is_win_without_bash: + if _WinBash() is _WinBash.ABSENT: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") From b21535729ca695e47c52086abe390ca4e6792ae3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Nov 2023 22:07:06 -0500 Subject: [PATCH 14/29] 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. --- test/test_index.py | 73 ++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index dfacb60db..9a520275c 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -39,32 +39,11 @@ log = logging.getLogger(__name__) -class _WinBashMeta(enum.EnumMeta): - """Metaclass allowing :class:`_WinBash` custom behavior when called.""" - - def __call__(cls): - return cls._check() - - @enum.unique -class _WinBash(enum.Enum, metaclass=_WinBashMeta): +class _WinBashStatus(enum.Enum): """Status of bash.exe for native Windows. Affects which commit hook tests can pass. - Call ``_WinBash()`` to check the status. - - This can't be reliably discovered using :func:`shutil.which`, as that approximates - how a shell is expected to search for an executable. On Windows, there are major - differences between how executables are found by a shell and otherwise. (This is the - cmd.exe Windows shell and should not be confused with bash.exe.) Our run_commit_hook - function in GitPython uses subprocess.Popen, including to run bash.exe on Windows. - It does not pass shell=True (and should not). Popen calls CreateProcessW, which - searches several locations prior to using the PATH environment variable. It is - expected to search the System32 directory, even if another directory containing the - executable precedes it in PATH. (There are other differences, less relevant here.) - When WSL is installed, even if no WSL *systems* are installed, bash.exe exists in - System32, and Popen finds it even if another bash.exe precedes it in PATH, as - happens on CI. If WSL is absent, System32 may still have bash.exe, as Windows users - and administrators occasionally copy executables there in lieu of extending PATH. + Call :meth:`check` to check the status. """ INAPPLICABLE = enum.auto() @@ -74,13 +53,13 @@ class _WinBash(enum.Enum, metaclass=_WinBashMeta): """No command for ``bash.exe`` is found on the system.""" NATIVE = enum.auto() - """Running ``bash.exe`` operates outside any WSL environment (as with Git Bash).""" + """Running ``bash.exe`` operates outside any WSL distribution (as with Git Bash).""" WSL = enum.auto() - """Running ``bash.exe`` runs bash on a WSL system.""" + """Running ``bash.exe`` calls ``bash`` in a WSL distribution.""" WSL_NO_DISTRO = enum.auto() - """Running ``bash.exe` tries to run bash on a WSL system, but none exists.""" + """Running ``bash.exe` tries to run bash on a WSL distribution, but none exists.""" ERROR_WHILE_CHECKING = enum.auto() """Could not determine the status. @@ -92,13 +71,34 @@ class _WinBash(enum.Enum, metaclass=_WinBashMeta): """ @classmethod - def _check(cls): + def check(cls): + """Check the status of the ``bash.exe`` :func:`index.fun.run_commit_hook` uses. + + This uses EAFP, attempting to run a command via ``bash.exe``. Which ``bash.exe`` + is used can't be reliably discovered by :func:`shutil.which`, which approximates + how a shell is expected to search for an executable. On Windows, there are major + differences between how executables are found by a shell and otherwise. (This is + the cmd.exe Windows shell, and shouldn't be confused with bash.exe itself. That + the command being looked up also happens to be an interpreter is not relevant.) + + :func:`index.fun.run_commit_hook` uses :class:`subprocess.Popen`, including when + it runs ``bash.exe`` on Windows. It doesn't pass ``shell=True`` (and shouldn't). + On Windows, `Popen` calls ``CreateProcessW``, which searches several locations + prior to using the ``PATH`` environment variable. It is expected to search the + ``System32`` directory, even if another directory containing the executable + precedes it in ``PATH``. (Other differences are less relevant here.) When WSL is + installed, even with no distributions, ``bash.exe`` exists in ``System32``, and + `Popen` finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. + If WSL is absent, ``System32`` may still have ``bash.exe``, as Windows users and + administrators occasionally put executables there in lieu of extending ``PATH``. + """ if os.name != "nt": return cls.INAPPLICABLE try: # Print rather than returning the test command's exit status so that if a - # failure occurs before we even get to this point, we will detect it. + # failure occurs before we even get to this point, we will detect it. See + # https://superuser.com/a/1749811 for information on ways to check for WSL. script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"' command = ["bash.exe", "-c", script] proc = subprocess.run(command, capture_output=True, check=True, text=True) @@ -129,6 +129,9 @@ def _error(cls, error_or_process): return cls.ERROR_WHILE_CHECKING +_win_bash_status = _WinBashStatus.check() + + def _make_hook(git_dir, name, content, make_exec=True): """A helper to create a hook""" hp = hook_path(name, git_dir) @@ -998,7 +1001,7 @@ class Mocked: self.assertEqual(rel, os.path.relpath(path, root)) @pytest.mark.xfail( - _WinBash() is _WinBash.WSL_NO_DISTRO, + _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=HookExecutionError, ) @@ -1009,7 +1012,7 @@ def test_pre_commit_hook_success(self, rw_repo): index.commit("This should not fail") @pytest.mark.xfail( - _WinBash() is _WinBash.WSL_NO_DISTRO, + _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=AssertionError, ) @@ -1020,7 +1023,7 @@ def test_pre_commit_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if _WinBash() is _WinBash.ABSENT: + if _win_bash_status is _WinBashStatus.ABSENT: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") @@ -1036,12 +1039,12 @@ def test_pre_commit_hook_fail(self, rw_repo): raise AssertionError("Should have caught a HookExecutionError") @pytest.mark.xfail( - _WinBash() in {_WinBash.ABSENT, _WinBash.WSL}, + _win_bash_status in {_WinBashStatus.ABSENT, _WinBashStatus.WSL}, reason="Specifically seems to fail on WSL bash (in spite of #1399)", raises=AssertionError, ) @pytest.mark.xfail( - _WinBash() is _WinBash.WSL_NO_DISTRO, + _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=HookExecutionError, ) @@ -1059,7 +1062,7 @@ def test_commit_msg_hook_success(self, rw_repo): self.assertEqual(new_commit.message, "{} {}".format(commit_message, from_hook_message)) @pytest.mark.xfail( - _WinBash() is _WinBash.WSL_NO_DISTRO, + _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=AssertionError, ) @@ -1070,7 +1073,7 @@ def test_commit_msg_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if _WinBash() is _WinBash.ABSENT: + if _win_bash_status is _WinBashStatus.ABSENT: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") From cabb5728e75aab3ed73376702e219bd1514ee615 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Nov 2023 22:24:24 -0500 Subject: [PATCH 15/29] Temporarily don't install WSL system to test xfail This reverts "Install WSL system on CI for hook tests", commit 9717b8d847b514f761ecaa423226054c73f2a325. 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. --- .github/workflows/pythonpackage.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index c2bbc1bbb..d21ea3949 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -35,12 +35,6 @@ jobs: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} - - name: Set up WSL - if: startsWith(matrix.os, 'windows') - uses: Vampire/setup-wsl@v2.0.2 - with: - distribution: Debian - - name: Prepare this repo for tests run: | ./init-tests-after-clone.sh From 2875ffa083b1e136cd647e6d087a4747aa85a4bc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Nov 2023 22:33:26 -0500 Subject: [PATCH 16/29] 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 https://github.com/Vampire/setup-wsl/issues/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. --- .github/workflows/cygwin-test.yml | 4 +--- .github/workflows/pythonpackage.yml | 28 ++++++++++------------------ 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 96728e51a..13a01dec4 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -70,9 +70,7 @@ jobs: command -v git python git version python --version - python -c 'import sys; print(sys.platform)' - python -c 'import os; print(os.name)' - python -c 'import git; print(git.compat.is_win)' # NOTE: Deprecated. Use os.name directly. + python -c 'import os, sys; print(f"sys.platform={sys.platform!r}, os.name={os.name!r}")' - name: Test with pytest run: | diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index d21ea3949..3915296ef 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -35,6 +35,12 @@ jobs: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} + - name: Set up WSL (Windows) + if: startsWith(matrix.os, 'windows') + uses: Vampire/setup-wsl@v2.0.2 + with: + distribution: Debian + - name: Prepare this repo for tests run: | ./init-tests-after-clone.sh @@ -62,29 +68,15 @@ jobs: command -v git python git version python --version - python -c 'import sys; print(sys.platform)' - python -c 'import os; print(os.name)' - python -c 'import git; print(git.compat.is_win)' # NOTE: Deprecated. Use os.name directly. + python -c 'import os, sys; print(f"sys.platform={sys.platform!r}, os.name={os.name!r}")' # For debugging hook tests on native Windows systems that may have WSL. - - name: Show where bash.exe may be found + - name: Show bash.exe candidates (Windows) if: startsWith(matrix.os, 'windows') run: | set +e - type -a bash.exe - python -c 'import shutil; print(shutil.which("bash.exe"))' - bash.exe --version - python -c 'import subprocess; p = subprocess.run(["bash.exe", "--version"]); print(f"result: {p!r}")' - bash.exe -c 'echo "$BASH"' - python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", """echo "$BASH" """]); print(f"result: {p!r}")' - bash.exe -c 'echo "$BASH_VERSION"' - python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", """echo "$BASH_VERSION" """]); print(f"result: {p!r}")' - bash.exe -c 'printenv WSL_DISTRO_NAME' - python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", "printenv WSL_DISTRO_NAME"]); print(f"result: {p!r}")' - bash.exe -c 'ls -l /proc/sys/fs/binfmt_misc/WSLInterop' - python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", "ls -l /proc/sys/fs/binfmt_misc/WSLInterop"]); print(f"result: {p!r}")' - bash.exe -c 'uname -a' - python -c 'import subprocess; p = subprocess.run(["bash.exe", "-c", "uname -a"]); print(f"result: {p!r}")' + bash.exe -c 'printenv WSL_DISTRO_NAME; uname -a' + python -c 'import subprocess; subprocess.run(["bash.exe", "-c", "printenv WSL_DISTRO_NAME; uname -a"])' continue-on-error: true - name: Check types with mypy From 0f8cd4ce4a7f942d793a8670257f8738ab18b757 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Nov 2023 23:52:27 -0500 Subject: [PATCH 17/29] 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 #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.) --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 7109389d7..5c3117096 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,6 +8,7 @@ filterwarnings = "ignore::DeprecationWarning" python_files = "test_*.py" tmp_path_retention_policy = "failed" testpaths = "test" # Space separated list of paths from root e.g test tests doc/testing. +xfail_strict = true # Treat the XPASS status as a test failure (unless strict=False is passed). # --cov coverage # --cov-report term # send report to terminal term-missing -> terminal with line numbers html xml # --cov-report term-missing # to terminal with line numbers From 82c361e7e1a20df96cc9e5c443b2c18fc20b18c9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 24 Nov 2023 02:05:29 -0500 Subject: [PATCH 18/29] 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. --- test/test_submodule.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 68dbcc491..1e39924dd 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -950,13 +950,14 @@ def test_remove_norefs(self, rwdir): assert not sm.exists() @pytest.mark.xfail( - os.name == "nt", + os.name == "nt" and (3, 12) <= sys.version_info < (3, 13), reason=( "The sm.move call fails. Submodule.move calls os.renames, which raises:\n" "PermissionError: [WinError 32] " "The process cannot access the file because it is being used by another process: " R"'C:\Users\ek\AppData\Local\Temp\test_renamekkbznwjp\parent\mymodules\myname' " R"-> 'C:\Users\ek\AppData\Local\Temp\test_renamekkbznwjp\parent\renamed\myname'" + "\nThis resembles other Windows errors, but seems only to affect Python 3.12 somehow." ), raises=PermissionError, ) From 0ae5dd19d5e7256b0876987948774b48e207f231 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 24 Nov 2023 02:19:56 -0500 Subject: [PATCH 19/29] 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 0f8cd4ce4a7f942d793a8670257f8738ab18b757. --- pyproject.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 5c3117096..7109389d7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,7 +8,6 @@ filterwarnings = "ignore::DeprecationWarning" python_files = "test_*.py" tmp_path_retention_policy = "failed" testpaths = "test" # Space separated list of paths from root e.g test tests doc/testing. -xfail_strict = true # Treat the XPASS status as a test failure (unless strict=False is passed). # --cov coverage # --cov-report term # send report to terminal term-missing -> terminal with line numbers html xml # --cov-report term-missing # to terminal with line numbers From 0b7ee17849a48ca691eaa3cc5d3eb81a4e6592b7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 24 Nov 2023 20:14:35 -0500 Subject: [PATCH 20/29] 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. --- test/test_submodule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 1e39924dd..7fbccf831 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -950,14 +950,14 @@ def test_remove_norefs(self, rwdir): assert not sm.exists() @pytest.mark.xfail( - os.name == "nt" and (3, 12) <= sys.version_info < (3, 13), + os.name == "nt" and sys.version_info >= (3, 12), reason=( "The sm.move call fails. Submodule.move calls os.renames, which raises:\n" "PermissionError: [WinError 32] " "The process cannot access the file because it is being used by another process: " R"'C:\Users\ek\AppData\Local\Temp\test_renamekkbznwjp\parent\mymodules\myname' " R"-> 'C:\Users\ek\AppData\Local\Temp\test_renamekkbznwjp\parent\renamed\myname'" - "\nThis resembles other Windows errors, but seems only to affect Python 3.12 somehow." + "\nThis resembles other Windows errors, but only occurs starting in Python 3.12." ), raises=PermissionError, ) From 8621e892ed5ed05b1a304c9ea25a460f376677bb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 25 Nov 2023 00:01:33 -0500 Subject: [PATCH 21/29] Reword comment in _WinBashStatus.check for clarity --- test/test_index.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index 9a520275c..e1bcd5b25 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -96,9 +96,9 @@ def check(cls): return cls.INAPPLICABLE try: - # Print rather than returning the test command's exit status so that if a - # failure occurs before we even get to this point, we will detect it. See - # https://superuser.com/a/1749811 for information on ways to check for WSL. + # Output rather than forwarding the test command's exit status so that if a + # failure occurs before we even get to this point, we will detect it. For + # information on ways to check for WSL, see https://superuser.com/a/1749811. script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"' command = ["bash.exe", "-c", script] proc = subprocess.run(command, capture_output=True, check=True, text=True) From 7ff3cee63c53520650db14bdf6e55551b8848567 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 25 Nov 2023 00:23:40 -0500 Subject: [PATCH 22/29] 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. --- test-requirements.txt | 1 + test/test_index.py | 66 ++++++++++++++++++++----------------------- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/test-requirements.txt b/test-requirements.txt index 7cfb977a1..fcdc93c1d 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -9,3 +9,4 @@ pytest-cov pytest-instafail pytest-mock pytest-sugar +sumtypes diff --git a/test/test_index.py b/test/test_index.py index e1bcd5b25..f60a86309 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -3,7 +3,6 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ -import enum from io import BytesIO import logging import os @@ -14,6 +13,7 @@ import tempfile import pytest +from sumtypes import constructor, sumtype from git import ( IndexFile, @@ -39,35 +39,34 @@ log = logging.getLogger(__name__) -@enum.unique -class _WinBashStatus(enum.Enum): +@sumtype +class _WinBashStatus: """Status of bash.exe for native Windows. Affects which commit hook tests can pass. Call :meth:`check` to check the status. """ - INAPPLICABLE = enum.auto() + Inapplicable = constructor() """This system is not native Windows: either not Windows at all, or Cygwin.""" - ABSENT = enum.auto() + Absent = constructor() """No command for ``bash.exe`` is found on the system.""" - NATIVE = enum.auto() + Native = constructor() """Running ``bash.exe`` operates outside any WSL distribution (as with Git Bash).""" - WSL = enum.auto() + Wsl = constructor() """Running ``bash.exe`` calls ``bash`` in a WSL distribution.""" - WSL_NO_DISTRO = enum.auto() + WslNoDistro = constructor() """Running ``bash.exe` tries to run bash on a WSL distribution, but none exists.""" - ERROR_WHILE_CHECKING = enum.auto() + ErrorWhileChecking = constructor("error_or_process") """Could not determine the status. This should not trigger a skip or xfail, as it typically indicates either a fixable problem on the test machine, such as an "Insufficient system resources exist to complete the requested service" error starting WSL, or a bug in this detection code. - ``ERROR_WHILE_CHECKING.error_or_process`` has details about the most recent failure. """ @classmethod @@ -93,7 +92,13 @@ def check(cls): administrators occasionally put executables there in lieu of extending ``PATH``. """ if os.name != "nt": - return cls.INAPPLICABLE + return cls.Inapplicable() + + no_distro_message = "Windows Subsystem for Linux has no installed distributions." + + def error_running_bash(error): + log.error("Error running bash.exe to check WSL status: %s", error) + return cls.ErrorWhileChecking(error) try: # Output rather than forwarding the test command's exit status so that if a @@ -103,30 +108,21 @@ def check(cls): command = ["bash.exe", "-c", script] proc = subprocess.run(command, capture_output=True, check=True, text=True) except FileNotFoundError: - return cls.ABSENT + return cls.Absent() except OSError as error: - return cls._error(error) + return error_running_bash(error) except subprocess.CalledProcessError as error: - no_distro_message = "Windows Subsystem for Linux has no installed distributions." if error.returncode == 1 and error.stdout.startswith(no_distro_message): - return cls.WSL_NO_DISTRO - return cls._error(error) + return cls.WslNoDistro() + return error_running_bash(error) status = proc.stdout.rstrip() if status == "0": - return cls.WSL + return cls.Wsl() if status == "1": - return cls.NATIVE - return cls._error(proc) - - @classmethod - def _error(cls, error_or_process): - if isinstance(error_or_process, subprocess.CompletedProcess): - log.error("Strange output checking WSL status: %s", error_or_process.stdout) - else: - log.error("Error running bash.exe to check WSL status: %s", error_or_process) - cls.ERROR_WHILE_CHECKING.error_or_process = error_or_process - return cls.ERROR_WHILE_CHECKING + return cls.Native() + log.error("Strange output checking WSL status: %s", proc.stdout) + return cls.ErrorWhileChecking(proc) _win_bash_status = _WinBashStatus.check() @@ -1001,7 +997,7 @@ class Mocked: self.assertEqual(rel, os.path.relpath(path, root)) @pytest.mark.xfail( - _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, + type(_win_bash_status) is _WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=HookExecutionError, ) @@ -1012,7 +1008,7 @@ def test_pre_commit_hook_success(self, rw_repo): index.commit("This should not fail") @pytest.mark.xfail( - _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, + type(_win_bash_status) is _WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=AssertionError, ) @@ -1023,7 +1019,7 @@ def test_pre_commit_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if _win_bash_status is _WinBashStatus.ABSENT: + if type(_win_bash_status) is _WinBashStatus.Absent: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") @@ -1039,12 +1035,12 @@ def test_pre_commit_hook_fail(self, rw_repo): raise AssertionError("Should have caught a HookExecutionError") @pytest.mark.xfail( - _win_bash_status in {_WinBashStatus.ABSENT, _WinBashStatus.WSL}, + type(_win_bash_status) in {_WinBashStatus.Absent, _WinBashStatus.Wsl}, reason="Specifically seems to fail on WSL bash (in spite of #1399)", raises=AssertionError, ) @pytest.mark.xfail( - _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, + type(_win_bash_status) is _WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=HookExecutionError, ) @@ -1062,7 +1058,7 @@ def test_commit_msg_hook_success(self, rw_repo): self.assertEqual(new_commit.message, "{} {}".format(commit_message, from_hook_message)) @pytest.mark.xfail( - _win_bash_status is _WinBashStatus.WSL_NO_DISTRO, + type(_win_bash_status) is _WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=AssertionError, ) @@ -1073,7 +1069,7 @@ def test_commit_msg_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if _win_bash_status is _WinBashStatus.ABSENT: + if type(_win_bash_status) is _WinBashStatus.Absent: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") From d5ed266f2208367e4300c29fa4cbda9cd2db0fb9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 25 Nov 2023 05:06:23 -0500 Subject: [PATCH 23/29] 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. --- .github/workflows/pythonpackage.yml | 6 ------ test/test_index.py | 13 +++++++------ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 3915296ef..cef24799e 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -35,12 +35,6 @@ jobs: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} - - name: Set up WSL (Windows) - if: startsWith(matrix.os, 'windows') - uses: Vampire/setup-wsl@v2.0.2 - with: - distribution: Debian - - name: Prepare this repo for tests run: | ./init-tests-after-clone.sh diff --git a/test/test_index.py b/test/test_index.py index f60a86309..8824d6648 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -94,10 +94,11 @@ def check(cls): if os.name != "nt": return cls.Inapplicable() - no_distro_message = "Windows Subsystem for Linux has no installed distributions." + # Use bytes because messages for different WSL errors use different encodings. + no_distro_message = b"Windows Subsystem for Linux has no installed distributions." def error_running_bash(error): - log.error("Error running bash.exe to check WSL status: %s", error) + log.error("Error running bash.exe to check WSL status: %r", error) return cls.ErrorWhileChecking(error) try: @@ -106,7 +107,7 @@ def error_running_bash(error): # information on ways to check for WSL, see https://superuser.com/a/1749811. script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"' command = ["bash.exe", "-c", script] - proc = subprocess.run(command, capture_output=True, check=True, text=True) + proc = subprocess.run(command, capture_output=True, check=True) except FileNotFoundError: return cls.Absent() except OSError as error: @@ -117,11 +118,11 @@ def error_running_bash(error): return error_running_bash(error) status = proc.stdout.rstrip() - if status == "0": + if status == b"0": return cls.Wsl() - if status == "1": + if status == b"1": return cls.Native() - log.error("Strange output checking WSL status: %s", proc.stdout) + log.error("Strange output checking WSL status: %r", proc.stdout) return cls.ErrorWhileChecking(proc) From 496acaac5d2cfcbe4edf8ace1bfae452f281ff15 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 26 Nov 2023 00:27:57 -0500 Subject: [PATCH 24/29] 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. --- test/test_index.py | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index 8824d6648..bd38ef3bb 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -61,13 +61,11 @@ class _WinBashStatus: WslNoDistro = constructor() """Running ``bash.exe` tries to run bash on a WSL distribution, but none exists.""" - ErrorWhileChecking = constructor("error_or_process") - """Could not determine the status. + CheckError = constructor("process", "message") + """Running ``bash.exe`` fails in an unexpected error or gives unexpected output.""" - This should not trigger a skip or xfail, as it typically indicates either a fixable - problem on the test machine, such as an "Insufficient system resources exist to - complete the requested service" error starting WSL, or a bug in this detection code. - """ + WinError = constructor("exception") + """``bash.exe`` may exist but can't run. ``CreateProcessW`` fails unexpectedly.""" @classmethod def check(cls): @@ -94,12 +92,7 @@ def check(cls): if os.name != "nt": return cls.Inapplicable() - # Use bytes because messages for different WSL errors use different encodings. - no_distro_message = b"Windows Subsystem for Linux has no installed distributions." - - def error_running_bash(error): - log.error("Error running bash.exe to check WSL status: %r", error) - return cls.ErrorWhileChecking(error) + no_distro_message = "Windows Subsystem for Linux has no installed distributions." try: # Output rather than forwarding the test command's exit status so that if a @@ -107,23 +100,26 @@ def error_running_bash(error): # information on ways to check for WSL, see https://superuser.com/a/1749811. script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"' command = ["bash.exe", "-c", script] - proc = subprocess.run(command, capture_output=True, check=True) + process = subprocess.run(command, capture_output=True) except FileNotFoundError: return cls.Absent() except OSError as error: - return error_running_bash(error) - except subprocess.CalledProcessError as error: - if error.returncode == 1 and error.stdout.startswith(no_distro_message): - return cls.WslNoDistro() - return error_running_bash(error) - - status = proc.stdout.rstrip() - if status == b"0": + return cls.WinError(error) + + encoding = "utf-16le" if b"\r\0\n\0" in process.stdout else "utf-8" + text = process.stdout.decode(encoding).rstrip() # stdout includes WSL errors. + + if process.returncode == 1 and text.startswith(no_distro_message): + return cls.WslNoDistro() + if process.returncode != 0: + log.error("Error running bash.exe to check WSL status: %s", text) + return cls.CheckError(process, text) + if text == "0": return cls.Wsl() - if status == b"1": + if text == "1": return cls.Native() - log.error("Strange output checking WSL status: %r", proc.stdout) - return cls.ErrorWhileChecking(proc) + log.error("Strange output checking WSL status: %s", text) + return cls.CheckError(process, text) _win_bash_status = _WinBashStatus.check() From d779a7546f4bf69783cf38be4679a0a229578ef9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 27 Nov 2023 04:35:02 -0500 Subject: [PATCH 25/29] 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). --- test/test_index.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index bd38ef3bb..a7ca2158b 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -8,6 +8,7 @@ import os import os.path as osp from pathlib import Path +import re from stat import S_ISLNK, ST_MODE import subprocess import tempfile @@ -58,7 +59,7 @@ class _WinBashStatus: Wsl = constructor() """Running ``bash.exe`` calls ``bash`` in a WSL distribution.""" - WslNoDistro = constructor() + WslNoDistro = constructor("process", "message") """Running ``bash.exe` tries to run bash on a WSL distribution, but none exists.""" CheckError = constructor("process", "message") @@ -80,20 +81,18 @@ def check(cls): :func:`index.fun.run_commit_hook` uses :class:`subprocess.Popen`, including when it runs ``bash.exe`` on Windows. It doesn't pass ``shell=True`` (and shouldn't). - On Windows, `Popen` calls ``CreateProcessW``, which searches several locations - prior to using the ``PATH`` environment variable. It is expected to search the - ``System32`` directory, even if another directory containing the executable - precedes it in ``PATH``. (Other differences are less relevant here.) When WSL is - installed, even with no distributions, ``bash.exe`` exists in ``System32``, and - `Popen` finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. - If WSL is absent, ``System32`` may still have ``bash.exe``, as Windows users and + On Windows, `Popen` calls ``CreateProcessW``, which checks some locations before + using the ``PATH`` environment variable. It is expected to try the ``System32`` + directory, even if another directory containing the executable precedes it in + ``PATH``. (Other differences are less relevant here.) When WSL is present, even + with no distributions, ``bash.exe`` usually exists in ``System32``, and `Popen` + finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. If WSL + is absent, ``System32`` may still have ``bash.exe``, as Windows users and administrators occasionally put executables there in lieu of extending ``PATH``. """ if os.name != "nt": return cls.Inapplicable() - no_distro_message = "Windows Subsystem for Linux has no installed distributions." - try: # Output rather than forwarding the test command's exit status so that if a # failure occurs before we even get to this point, we will detect it. For @@ -106,11 +105,12 @@ def check(cls): except OSError as error: return cls.WinError(error) + # FIXME: When not UTF-16LE: try local ANSI code page, then fall back to UTF-8. encoding = "utf-16le" if b"\r\0\n\0" in process.stdout else "utf-8" text = process.stdout.decode(encoding).rstrip() # stdout includes WSL errors. - if process.returncode == 1 and text.startswith(no_distro_message): - return cls.WslNoDistro() + if process.returncode == 1 and re.search(r"\bhttps://aka.ms/wslstore\b", text): + return cls.WslNoDistro(process, text) if process.returncode != 0: log.error("Error running bash.exe to check WSL status: %s", text) return cls.CheckError(process, text) From 9ac243884ebe2d614f03302104064ef22b2aee0f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 27 Nov 2023 17:09:31 -0500 Subject: [PATCH 26/29] 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. --- test/test_index.py | 70 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index a7ca2158b..39408e836 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -41,10 +41,13 @@ @sumtype -class _WinBashStatus: +class WinBashStatus: """Status of bash.exe for native Windows. Affects which commit hook tests can pass. Call :meth:`check` to check the status. + + The :class:`CheckError` and :class:`WinError` cases should not typically be used in + ``skip`` or ``xfail`` mark conditions, because they represent unexpected situations. """ Inapplicable = constructor() @@ -84,10 +87,10 @@ def check(cls): On Windows, `Popen` calls ``CreateProcessW``, which checks some locations before using the ``PATH`` environment variable. It is expected to try the ``System32`` directory, even if another directory containing the executable precedes it in - ``PATH``. (Other differences are less relevant here.) When WSL is present, even - with no distributions, ``bash.exe`` usually exists in ``System32``, and `Popen` - finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. If WSL - is absent, ``System32`` may still have ``bash.exe``, as Windows users and + ``PATH``. (The other differences are less relevant here.) When WSL is present, + even with no distributions, ``bash.exe`` usually exists in ``System32``, and + `Popen` finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. + If WSL is absent, ``System32`` may still have ``bash.exe``, as Windows users and administrators occasionally put executables there in lieu of extending ``PATH``. """ if os.name != "nt": @@ -105,9 +108,7 @@ def check(cls): except OSError as error: return cls.WinError(error) - # FIXME: When not UTF-16LE: try local ANSI code page, then fall back to UTF-8. - encoding = "utf-16le" if b"\r\0\n\0" in process.stdout else "utf-8" - text = process.stdout.decode(encoding).rstrip() # stdout includes WSL errors. + text = cls._decode(process.stdout).rstrip() # stdout includes WSL's own errors. if process.returncode == 1 and re.search(r"\bhttps://aka.ms/wslstore\b", text): return cls.WslNoDistro(process, text) @@ -121,8 +122,45 @@ def check(cls): log.error("Strange output checking WSL status: %s", text) return cls.CheckError(process, text) + @staticmethod + def _decode(stdout): + """Decode ``bash.exe`` output as best we can. (This is used only on Windows.)""" + # When bash.exe is the WSL wrapper but the output is from WSL itself rather than + # code running in a distribution, the output is often in UTF-16LE, which Windows + # uses internally. The UTF-16LE representation of a Windows-style line ending is + # rarely seen otherwise, so use it to detect this situation. + if b"\r\0\n\0" in stdout: + return stdout.decode("utf-16le") + + import winreg + + # At this point, the output is probably either empty or not UTF-16LE. It's often + # UTF-8 from inside a WSL distro or a non-WSL bash shell. But our test command + # only uses the ASCII subset, so it's safe to guess wrong for that command's + # output. Errors from inside a WSL distro or non-WSL bash.exe are arbitrary, but + # unlike WSL's own messages, go to stderr, not stdout. So we can try the system + # active code page first. (Although console programs usually use the OEM code + # page, the ACP seems more accurate here. For example, on en-US Windows set to + # fr-FR, the message, if not UTF-16LE, is windows-1252, same as the ACP, while + # the OEM code page on such a system defaults to 437, which can't decode it.) + hklm_path = R"SYSTEM\CurrentControlSet\Control\Nls\CodePage" + with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, hklm_path) as key: + value, _ = winreg.QueryValueEx(key, "ACP") + try: + return stdout.decode(f"cp{value}") + except UnicodeDecodeError: + pass + except LookupError as error: + log.warning("%s", str(error)) # Message already says "Unknown encoding:". + + # Assume UTF-8. If we don't have valid UTF-8, substitute Unicode replacement + # characters. (For example, on zh-CN Windows set to fr-FR, error messages from + # WSL itself, if not UTF-16LE, are in windows-1252, even though the ACP and OEM + # code pages are 936; decoding as code page 936 or as UTF-8 both have errors.) + return stdout.decode("utf-8", errors="replace") + -_win_bash_status = _WinBashStatus.check() +_win_bash_status = WinBashStatus.check() def _make_hook(git_dir, name, content, make_exec=True): @@ -994,7 +1032,7 @@ class Mocked: self.assertEqual(rel, os.path.relpath(path, root)) @pytest.mark.xfail( - type(_win_bash_status) is _WinBashStatus.WslNoDistro, + type(_win_bash_status) is WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=HookExecutionError, ) @@ -1005,7 +1043,7 @@ def test_pre_commit_hook_success(self, rw_repo): index.commit("This should not fail") @pytest.mark.xfail( - type(_win_bash_status) is _WinBashStatus.WslNoDistro, + type(_win_bash_status) is WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=AssertionError, ) @@ -1016,7 +1054,7 @@ def test_pre_commit_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if type(_win_bash_status) is _WinBashStatus.Absent: + if type(_win_bash_status) is WinBashStatus.Absent: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") @@ -1032,12 +1070,12 @@ def test_pre_commit_hook_fail(self, rw_repo): raise AssertionError("Should have caught a HookExecutionError") @pytest.mark.xfail( - type(_win_bash_status) in {_WinBashStatus.Absent, _WinBashStatus.Wsl}, + type(_win_bash_status) in {WinBashStatus.Absent, WinBashStatus.Wsl}, reason="Specifically seems to fail on WSL bash (in spite of #1399)", raises=AssertionError, ) @pytest.mark.xfail( - type(_win_bash_status) is _WinBashStatus.WslNoDistro, + type(_win_bash_status) is WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=HookExecutionError, ) @@ -1055,7 +1093,7 @@ def test_commit_msg_hook_success(self, rw_repo): self.assertEqual(new_commit.message, "{} {}".format(commit_message, from_hook_message)) @pytest.mark.xfail( - type(_win_bash_status) is _WinBashStatus.WslNoDistro, + type(_win_bash_status) is WinBashStatus.WslNoDistro, reason="Currently uses the bash.exe for WSL even with no WSL distro installed", raises=AssertionError, ) @@ -1066,7 +1104,7 @@ def test_commit_msg_hook_fail(self, rw_repo): try: index.commit("This should fail") except HookExecutionError as err: - if type(_win_bash_status) is _WinBashStatus.Absent: + if type(_win_bash_status) is WinBashStatus.Absent: self.assertIsInstance(err.status, OSError) self.assertEqual(err.command, [hp]) self.assertEqual(err.stdout, "") From b07e5c7d997b49d980bf3d2d749839726663b274 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 27 Nov 2023 19:58:08 -0500 Subject: [PATCH 27/29] 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.) --- .github/workflows/pythonpackage.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index cef24799e..3915296ef 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -35,6 +35,12 @@ jobs: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} + - name: Set up WSL (Windows) + if: startsWith(matrix.os, 'windows') + uses: Vampire/setup-wsl@v2.0.2 + with: + distribution: Debian + - name: Prepare this repo for tests run: | ./init-tests-after-clone.sh From 3303c740bd9aae3ec2fa6b0f1a750bba9ad2b60e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 28 Nov 2023 15:55:29 -0500 Subject: [PATCH 28/29] 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. --- test/test_index.py | 96 +++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 49 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index 39408e836..bfdbca867 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -40,58 +40,60 @@ log = logging.getLogger(__name__) +def _get_windows_ansi_encoding(): + """Get the encoding specified by the Windows system-wide ANSI active code page.""" + # locale.getencoding may work but is only in Python 3.11+. Use the registry instead. + import winreg + + hklm_path = R"SYSTEM\CurrentControlSet\Control\Nls\CodePage" + with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, hklm_path) as key: + value, _ = winreg.QueryValueEx(key, "ACP") + return f"cp{value}" + + @sumtype class WinBashStatus: """Status of bash.exe for native Windows. Affects which commit hook tests can pass. - Call :meth:`check` to check the status. - - The :class:`CheckError` and :class:`WinError` cases should not typically be used in - ``skip`` or ``xfail`` mark conditions, because they represent unexpected situations. + Call check() to check the status. (CheckError and WinError should not typically be + used to trigger skip or xfail, because they represent unexpected situations.) """ Inapplicable = constructor() """This system is not native Windows: either not Windows at all, or Cygwin.""" Absent = constructor() - """No command for ``bash.exe`` is found on the system.""" + """No command for bash.exe is found on the system.""" Native = constructor() - """Running ``bash.exe`` operates outside any WSL distribution (as with Git Bash).""" + """Running bash.exe operates outside any WSL distribution (as with Git Bash).""" Wsl = constructor() - """Running ``bash.exe`` calls ``bash`` in a WSL distribution.""" + """Running bash.exe calls bash in a WSL distribution.""" WslNoDistro = constructor("process", "message") - """Running ``bash.exe` tries to run bash on a WSL distribution, but none exists.""" + """Running bash.exe tries to run bash on a WSL distribution, but none exists.""" CheckError = constructor("process", "message") - """Running ``bash.exe`` fails in an unexpected error or gives unexpected output.""" + """Running bash.exe fails in an unexpected error or gives unexpected output.""" WinError = constructor("exception") - """``bash.exe`` may exist but can't run. ``CreateProcessW`` fails unexpectedly.""" + """bash.exe may exist but can't run. CreateProcessW fails unexpectedly.""" @classmethod def check(cls): - """Check the status of the ``bash.exe`` :func:`index.fun.run_commit_hook` uses. - - This uses EAFP, attempting to run a command via ``bash.exe``. Which ``bash.exe`` - is used can't be reliably discovered by :func:`shutil.which`, which approximates - how a shell is expected to search for an executable. On Windows, there are major - differences between how executables are found by a shell and otherwise. (This is - the cmd.exe Windows shell, and shouldn't be confused with bash.exe itself. That - the command being looked up also happens to be an interpreter is not relevant.) - - :func:`index.fun.run_commit_hook` uses :class:`subprocess.Popen`, including when - it runs ``bash.exe`` on Windows. It doesn't pass ``shell=True`` (and shouldn't). - On Windows, `Popen` calls ``CreateProcessW``, which checks some locations before - using the ``PATH`` environment variable. It is expected to try the ``System32`` - directory, even if another directory containing the executable precedes it in - ``PATH``. (The other differences are less relevant here.) When WSL is present, - even with no distributions, ``bash.exe`` usually exists in ``System32``, and - `Popen` finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. - If WSL is absent, ``System32`` may still have ``bash.exe``, as Windows users and - administrators occasionally put executables there in lieu of extending ``PATH``. + """Check the status of the bash.exe that run_commit_hook will try to use. + + This runs a command with bash.exe and checks the result. On Windows, shell and + non-shell executable search differ; shutil.which often finds the wrong bash.exe. + + run_commit_hook uses Popen, including to run bash.exe on Windows. It doesn't + pass shell=True (and shouldn't). On Windows, Popen calls CreateProcessW, which + checks some locations before using the PATH environment variable. It is expected + to try System32, even if another directory with the executable precedes it in + PATH. When WSL is present, even with no distributions, bash.exe usually exists + in System32; Popen finds it even if a shell would run another one, as on CI. + (Without WSL, System32 may still have bash.exe; users sometimes put it there.) """ if os.name != "nt": return cls.Inapplicable() @@ -124,7 +126,7 @@ def check(cls): @staticmethod def _decode(stdout): - """Decode ``bash.exe`` output as best we can. (This is used only on Windows.)""" + """Decode bash.exe output as best we can.""" # When bash.exe is the WSL wrapper but the output is from WSL itself rather than # code running in a distribution, the output is often in UTF-16LE, which Windows # uses internally. The UTF-16LE representation of a Windows-style line ending is @@ -132,31 +134,27 @@ def _decode(stdout): if b"\r\0\n\0" in stdout: return stdout.decode("utf-16le") - import winreg - - # At this point, the output is probably either empty or not UTF-16LE. It's often - # UTF-8 from inside a WSL distro or a non-WSL bash shell. But our test command - # only uses the ASCII subset, so it's safe to guess wrong for that command's - # output. Errors from inside a WSL distro or non-WSL bash.exe are arbitrary, but - # unlike WSL's own messages, go to stderr, not stdout. So we can try the system - # active code page first. (Although console programs usually use the OEM code - # page, the ACP seems more accurate here. For example, on en-US Windows set to - # fr-FR, the message, if not UTF-16LE, is windows-1252, same as the ACP, while - # the OEM code page on such a system defaults to 437, which can't decode it.) - hklm_path = R"SYSTEM\CurrentControlSet\Control\Nls\CodePage" - with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, hklm_path) as key: - value, _ = winreg.QueryValueEx(key, "ACP") + # At this point, the output is either blank or probably not UTF-16LE. It's often + # UTF-8 from inside a WSL distro or non-WSL bash shell. Our test command only + # uses the ASCII subset, so we can safely guess a wrong code page for it. Errors + # from such an environment can contain any text, but unlike WSL's own messages, + # they go to stderr, not stdout. So we can try the system ANSI code page first. + # (Console programs often use the OEM code page, but the ACP seems more accurate + # here. For example, on en-US Windows with the original system code page but the + # display language set to fr-FR, the message, if not UTF-16LE, is windows-1252, + # same as the ACP, while the OEMCP is 437, which can't decode its accents.) + acp = _get_windows_ansi_encoding() try: - return stdout.decode(f"cp{value}") + return stdout.decode(acp) except UnicodeDecodeError: pass except LookupError as error: log.warning("%s", str(error)) # Message already says "Unknown encoding:". - # Assume UTF-8. If we don't have valid UTF-8, substitute Unicode replacement - # characters. (For example, on zh-CN Windows set to fr-FR, error messages from - # WSL itself, if not UTF-16LE, are in windows-1252, even though the ACP and OEM - # code pages are 936; decoding as code page 936 or as UTF-8 both have errors.) + # Assume UTF-8. If invalid, substitute Unicode replacement characters. (For + # example, on zh-CN Windows set to display fr-FR, errors from WSL itself, if not + # UTF-16LE, are in windows-1252, even though the ANSI and OEM code pages both + # default to 936, and decoding as code page 936 or as UTF-8 both have errors.) return stdout.decode("utf-8", errors="replace") From e00fffc918da5cd6c3c749d1d2e59d8ae6835189 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 28 Nov 2023 16:51:10 -0500 Subject: [PATCH 29/29] Shorten comments on _decode steps This removes the parenthesized examples from the per-step comments in the WinBashStatus._decode helper. --- test/test_index.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index bfdbca867..cd1c37efc 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -139,10 +139,6 @@ def _decode(stdout): # uses the ASCII subset, so we can safely guess a wrong code page for it. Errors # from such an environment can contain any text, but unlike WSL's own messages, # they go to stderr, not stdout. So we can try the system ANSI code page first. - # (Console programs often use the OEM code page, but the ACP seems more accurate - # here. For example, on en-US Windows with the original system code page but the - # display language set to fr-FR, the message, if not UTF-16LE, is windows-1252, - # same as the ACP, while the OEMCP is 437, which can't decode its accents.) acp = _get_windows_ansi_encoding() try: return stdout.decode(acp) @@ -151,10 +147,7 @@ def _decode(stdout): except LookupError as error: log.warning("%s", str(error)) # Message already says "Unknown encoding:". - # Assume UTF-8. If invalid, substitute Unicode replacement characters. (For - # example, on zh-CN Windows set to display fr-FR, errors from WSL itself, if not - # UTF-16LE, are in windows-1252, even though the ANSI and OEM code pages both - # default to 936, and decoding as code page 936 or as UTF-8 both have errors.) + # Assume UTF-8. If invalid, substitute Unicode replacement characters. return stdout.decode("utf-8", errors="replace")