Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem with Win10 shell #560

Merged
merged 7 commits into from Jan 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions src/pyscaffold/file_system.py
Expand Up @@ -11,14 +11,14 @@
import os
import shutil
import stat
import sys
from contextlib import contextmanager
from functools import partial
from pathlib import Path
from tempfile import mkstemp
from typing import Callable, Optional, Union

from .log import logger
from .shell import IS_WINDOWS

PathLike = Union[str, os.PathLike]

Expand Down Expand Up @@ -199,11 +199,7 @@ def is_pathname_valid(pathname: str) -> bool:
# Directory guaranteed to exist. If the current OS is Windows, this is
# the drive to which Windows was installed (e.g., the "%HOMEDRIVE%"
# environment variable); else, the typical root directory.
root_dirname = (
os.environ.get("HOMEDRIVE", "C:")
if sys.platform == "win32"
else os.path.sep
)
root_dirname = os.environ.get("HOMEDRIVE", "C:") if IS_WINDOWS else os.path.sep
assert os.path.isdir(root_dirname) # ...Murphy and her ironclad Law

# Append a path separator to this directory if needed.
Expand Down
35 changes: 26 additions & 9 deletions src/pyscaffold/shell.py
Expand Up @@ -18,6 +18,7 @@
PathLike = Union[str, os.PathLike]

IS_POSIX = os.name == "posix"
IS_WINDOWS = sys.platform == "win32"

# The following default flags were borrowed from Github's docs:
# https://docs.github.com/en/github/getting-started-with-github/associating-text-editors-with-git
Expand Down Expand Up @@ -61,7 +62,12 @@ class ShellCommand:
character, ``command`` needs to be properly quoted.
"""

def __init__(self, command: str, shell: bool = True, cwd: Optional[str] = None):
def __init__(
self,
command: str,
shell: bool = True,
cwd: Optional[str] = None,
):
self._command = command
self._shell = shell
self._cwd = cwd
Expand All @@ -81,6 +87,11 @@ def run(self, *args, **kwargs) -> subprocess.CompletedProcess:
"stdout": subprocess.PIPE,
"stderr": subprocess.STDOUT,
"universal_newlines": True,
"env": {
**os.environ,
# try to disable i18n
**dict(LC_ALL="C", LANGUAGE=""),
},
**kwargs, # allow overwriting defaults
}
if self._shell:
Expand All @@ -91,16 +102,22 @@ def run(self, *args, **kwargs) -> subprocess.CompletedProcess:

def __call__(self, *args, **kwargs) -> Iterator[str]:
"""Execute the command, returning an iterator for the resulting text output"""
completed = self.run(*args, **kwargs)
try:
completed = self.run(*args, **kwargs)
except FileNotFoundError as e:
msg = f"{e.strerror}: {e.filename}"
logger.debug(f'last command failed with "{msg}"')
raise ShellCommandException(msg) from e

try:
completed.check_returncode()
except subprocess.CalledProcessError as ex:
except subprocess.CalledProcessError as e:
stdout, stderr = (e or "" for e in (completed.stdout, completed.stderr))
stdout, stderr = (e.strip() for e in (stdout, stderr))
sep = " :: " if stdout and stderr else ""
sep = "; " if stdout and stderr else ""
msg = sep.join([stdout, stderr])
logger.debug(f'last command failed with "{msg}"')
raise ShellCommandException(msg) from ex
raise ShellCommandException(msg) from e

return (line for line in (completed.stdout or "").splitlines())

Expand Down Expand Up @@ -132,10 +149,10 @@ def get_git_cmd(**args):
Args:
**args: additional keyword arguments to :obj:`~.ShellCommand`
"""
if sys.platform == "win32": # pragma: no cover
if IS_WINDOWS: # pragma: no cover
# ^ CI setup does not aggregate Windows coverage
for cmd in ["git.cmd", "git.exe"]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand from this removal that "git.cmd" is no longer necessary, and that we expect any distribution of git for Windows to add a .exe to the PATH, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was actually trying to find git.cmd by googling but had no luck. There is a "git CMD" but no "git.cmd" so maybe it was a mistake? Also setuptools-scm doesn't seem to be doing this, so at least we would fail both.

git = ShellCommand(cmd, **args)
for shell in (True, False):
git = ShellCommand("git.exe", shell=shell, **args)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very curious to understand what is happening for the command with shell=True to be failing...

Would be a problem with file names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I am also very curious about this. We only now that one of @ksteptoe's two Windows machines seem to have a problem with the shell setup. Somehow an & is added but I have no good explanation for that. I think it will be impossible to figure this out without access to the machine. Also I am no Windows (Power)shell expert.

try:
git("--version")
except ShellCommandException:
Expand Down Expand Up @@ -238,7 +255,7 @@ def edit(file: PathLike, *args, **kwargs) -> Path:

def join(parts: Iterable[Union[str, PathLike]]) -> str:
"""Join different parts of a shell command into a string, quoting whitespaces."""
if sys.platform == "win32": # pragma: no cover
if IS_WINDOWS: # pragma: no cover
# ^ CI setup does not aggregate Windows coverage
return subprocess.list2cmdline(map(str, parts))

Expand Down
6 changes: 6 additions & 0 deletions tests/test_shell.py
Expand Up @@ -162,3 +162,9 @@ def test_join():
# Join should be the opposite of split
args = ["/my path/to exec", '"other args"', "asdf", "42", "'a b c'"]
assert shlex.split(shell.join(args)) == args


def test_non_existent_file_exception():
cmd = shell.ShellCommand("non_existent_cmd", shell=False)
with pytest.raises(shell.ShellCommandException):
cmd()