Skip to content

Commit d779a75

Browse files
committed
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).
1 parent 496acaa commit d779a75

File tree

1 file changed

+12
-12
lines changed

1 file changed

+12
-12
lines changed

‎test/test_index.py

+12-12
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import os
99
import os.path as osp
1010
from pathlib import Path
11+
import re
1112
from stat import S_ISLNK, ST_MODE
1213
import subprocess
1314
import tempfile
@@ -58,7 +59,7 @@ class _WinBashStatus:
5859
Wsl = constructor()
5960
"""Running ``bash.exe`` calls ``bash`` in a WSL distribution."""
6061

61-
WslNoDistro = constructor()
62+
WslNoDistro = constructor("process", "message")
6263
"""Running ``bash.exe` tries to run bash on a WSL distribution, but none exists."""
6364

6465
CheckError = constructor("process", "message")
@@ -80,20 +81,18 @@ def check(cls):
8081
8182
:func:`index.fun.run_commit_hook` uses :class:`subprocess.Popen`, including when
8283
it runs ``bash.exe`` on Windows. It doesn't pass ``shell=True`` (and shouldn't).
83-
On Windows, `Popen` calls ``CreateProcessW``, which searches several locations
84-
prior to using the ``PATH`` environment variable. It is expected to search the
85-
``System32`` directory, even if another directory containing the executable
86-
precedes it in ``PATH``. (Other differences are less relevant here.) When WSL is
87-
installed, even with no distributions, ``bash.exe`` exists in ``System32``, and
88-
`Popen` finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI.
89-
If WSL is absent, ``System32`` may still have ``bash.exe``, as Windows users and
84+
On Windows, `Popen` calls ``CreateProcessW``, which checks some locations before
85+
using the ``PATH`` environment variable. It is expected to try the ``System32``
86+
directory, even if another directory containing the executable precedes it in
87+
``PATH``. (Other differences are less relevant here.) When WSL is present, even
88+
with no distributions, ``bash.exe`` usually exists in ``System32``, and `Popen`
89+
finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI. If WSL
90+
is absent, ``System32`` may still have ``bash.exe``, as Windows users and
9091
administrators occasionally put executables there in lieu of extending ``PATH``.
9192
"""
9293
if os.name != "nt":
9394
return cls.Inapplicable()
9495

95-
no_distro_message = "Windows Subsystem for Linux has no installed distributions."
96-
9796
try:
9897
# Output rather than forwarding the test command's exit status so that if a
9998
# failure occurs before we even get to this point, we will detect it. For
@@ -106,11 +105,12 @@ def check(cls):
106105
except OSError as error:
107106
return cls.WinError(error)
108107

108+
# FIXME: When not UTF-16LE: try local ANSI code page, then fall back to UTF-8.
109109
encoding = "utf-16le" if b"\r\0\n\0" in process.stdout else "utf-8"
110110
text = process.stdout.decode(encoding).rstrip() # stdout includes WSL errors.
111111

112-
if process.returncode == 1 and text.startswith(no_distro_message):
113-
return cls.WslNoDistro()
112+
if process.returncode == 1 and re.search(r"\bhttps://aka.ms/wslstore\b", text):
113+
return cls.WslNoDistro(process, text)
114114
if process.returncode != 0:
115115
log.error("Error running bash.exe to check WSL status: %s", text)
116116
return cls.CheckError(process, text)

0 commit comments

Comments
 (0)