Skip to content

Commit

Permalink
Don't assume WSL-related bash.exe error is English
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
EliahKagan committed Nov 27, 2023
1 parent 496acaa commit d779a75
Showing 1 changed file with 12 additions and 12 deletions.
24 changes: 12 additions & 12 deletions test/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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
Expand All @@ -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)
Expand Down

0 comments on commit d779a75

Please sign in to comment.