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

Better handling of build backend without editable support and add --exit-and-dump-after flag #2597

Merged
merged 1 commit into from
Dec 6, 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
2 changes: 2 additions & 0 deletions docs/changelog/2595.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix leaking backend processes when the build backend does not support editable wheels and fix failure when multiple
environments exist that have a build backend that does not support editable wheels - by :user:`gaborbernat`.
2 changes: 2 additions & 0 deletions docs/changelog/2595.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add ``--exit-and-dump-after`` flag that allows automatically killing tox if does not finish within the passed seconds,
and dump the thread stacks (useful to debug tox when it seemingly hangs) - by :user:`gaborbernat`.
4 changes: 2 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ dependencies = [
"pluggy>=1",
"pyproject-api>=1.2.1",
'tomli>=2.0.1; python_version < "3.11"',
"virtualenv>=20.17",
"filelock>=3.8.1",
"virtualenv>=20.17.1",
"filelock>=3.8.2",
'importlib-metadata>=5.1; python_version < "3.8"',
'typing-extensions>=4.4; python_version < "3.8"',
]
Expand Down
14 changes: 14 additions & 0 deletions src/tox/config/cli/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ def is_colored(self) -> bool:
""":return: flag indicating if the output is colored or not"""
return cast(bool, self.colored == "yes")

exit_and_dump_after: int


ArgumentArgs = Tuple[Tuple[str, ...], Optional[Type[Any]], Dict[str, Any]]

Expand Down Expand Up @@ -305,9 +307,21 @@ def add_color_flags(parser: ArgumentParser) -> None:
)


def add_exit_and_dump_after(parser: ArgumentParser) -> None:
parser.add_argument(
"--exit-and-dump-after",
dest="exit_and_dump_after",
metavar="seconds",
default=0,
type=int,
help="dump tox threads after n seconds and exit the app - useful to debug when tox hangs, 0 means disabled",
)


def add_core_arguments(parser: ArgumentParser) -> None:
add_color_flags(parser)
add_verbosity_flags(parser)
add_exit_and_dump_after(parser)
parser.add_argument(
"-c",
"--conf",
Expand Down
1 change: 1 addition & 0 deletions src/tox/execute/pep517_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ def close(self) -> None:
execute.process.wait(timeout=0.1) # pragma: no cover
except TimeoutExpired: # pragma: no cover
execute.process.terminate() # pragma: no cover # if does not stop on its own kill it
self.is_alive = False


class LocalSubProcessPep517ExecuteInstance(ExecuteInstance):
Expand Down
3 changes: 3 additions & 0 deletions src/tox/run.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Main entry point for tox."""
from __future__ import annotations

import faulthandler
import logging
import os
import sys
Expand Down Expand Up @@ -51,6 +52,8 @@ def setup_state(args: Sequence[str]) -> State:
# parse CLI arguments
options = get_options(*args)
options.parsed.start = start
if options.parsed.exit_and_dump_after:
faulthandler.dump_traceback_later(timeout=options.parsed.exit_and_dump_after, exit=True) # pragma: no cover
# build tox environment config objects
state = State(options, args)
return state
12 changes: 8 additions & 4 deletions src/tox/tox_env/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,21 @@ def __str__(self) -> str:
return str(self.path)


locked = False


def _lock_method(thread_lock: RLock, file_lock: FileLock | None, meth: Callable[..., Any]) -> Callable[..., Any]:
def _func(*args: Any, **kwargs: Any) -> Any:
with thread_lock:
file_locks = False
if file_lock is not None and file_lock.is_locked is False: # file_lock is to lock from other tox processes
file_lock.acquire()
file_locks = True
try:
return meth(*args, **kwargs)
finally:
if file_lock is not None:
file_lock.release()
if file_locks:
cast(FileLock, file_lock).release()

return _func

Expand Down Expand Up @@ -94,8 +99,7 @@ def mark_active_run_env(self, run_env: RunToxEnv) -> None:

def teardown_env(self, conf: EnvConfigSet) -> None:
self._envs.remove(conf.name)
has_envs = bool(self._envs)
if not has_envs:
if len(self._envs) == 0:
self._teardown()

@abstractmethod
Expand Down
31 changes: 16 additions & 15 deletions src/tox/tox_env/python/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ def extract_base_python(env_name: str) -> str | None:
candidates: list[str] = []
for factor in env_name.split("-"):
spec = PythonSpec.from_string_spec(factor)
if spec.implementation is not None:
if spec.implementation.lower() in INTERPRETER_SHORT_NAMES and env_name is not None:
candidates.append(factor)
impl = spec.implementation or "python"
Copy link

Choose a reason for hiding this comment

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

@gaborbernat This change has the unintended (I presume) effect of interpreting a numeric factor (e.g. 2105) as a candidate base python, breaking previously valid environment names (e.g. py37-foo-2105) by raising ValueError: conflicting factors py37, 2105 in py37-foo-2105.
Example GitHub workflow build: https://github.com/galaxyproject/planemo/actions/runs/3654111042/jobs/6174245903 for tox.ini https://github.com/galaxyproject/planemo/blob/nsoranzo-patch-1/tox.ini

Copy link
Member Author

Choose a reason for hiding this comment

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

That error is a bug 🤣 please fill an issue for it.

Copy link

Choose a reason for hiding this comment

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

if impl.lower() in INTERPRETER_SHORT_NAMES and env_name is not None and spec.path is None:
candidates.append(factor)
if candidates:
if len(candidates) > 1:
raise ValueError(f"conflicting factors {', '.join(candidates)} in {env_name}")
Expand All @@ -123,18 +123,19 @@ def _validate_base_python(env_name: str, base_pythons: list[str], ignore_base_py
elements.update(env_name.split("-")) # and also any factor
for candidate in elements:
spec_name = PythonSpec.from_string_spec(candidate)
if spec_name.implementation is not None and spec_name.implementation.lower() in ("pypy", "cpython"):
for base_python in base_pythons:
spec_base = PythonSpec.from_string_spec(base_python)
if any(
getattr(spec_base, key) != getattr(spec_name, key)
for key in ("implementation", "major", "minor", "micro", "architecture")
if getattr(spec_base, key) is not None and getattr(spec_name, key) is not None
):
msg = f"env name {env_name} conflicting with base python {base_python}"
if ignore_base_python_conflict:
return [env_name] # ignore the base python settings
raise Fail(msg)
if spec_name.implementation and spec_name.implementation.lower() not in INTERPRETER_SHORT_NAMES:
continue
for base_python in base_pythons:
spec_base = PythonSpec.from_string_spec(base_python)
if any(
getattr(spec_base, key) != getattr(spec_name, key)
for key in ("implementation", "major", "minor", "micro", "architecture")
if getattr(spec_base, key) is not None and getattr(spec_name, key) is not None
):
msg = f"env name {env_name} conflicting with base python {base_python}"
if ignore_base_python_conflict:
return [env_name] # ignore the base python settings
raise Fail(msg)
return base_pythons

@abstractmethod
Expand Down
7 changes: 7 additions & 0 deletions src/tox/tox_env/python/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,10 @@ def child_pkg_envs(self, run_conf: EnvConfigSet) -> Iterator[PackageToxEnv]:
env = self._wheel_build_envs.get(run_conf["wheel_build_env"])
if env is not None and env.name != self.name:
yield env

def _teardown(self) -> None:
for env in self._wheel_build_envs.values():
if env is not self:
with env.display_context(self._has_display_suspended):
env.teardown()
super()._teardown()
16 changes: 10 additions & 6 deletions src/tox/tox_env/python/virtual_env/package/pyproject.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
import os
import sys
from collections import defaultdict
from contextlib import contextmanager
from pathlib import Path
from threading import RLock
Expand Down Expand Up @@ -92,7 +93,7 @@ class Pep517VirtualEnvPackager(PythonPackageToxEnv, VirtualEnv):
def __init__(self, create_args: ToxEnvCreateArgs) -> None:
super().__init__(create_args)
self._frontend_: Pep517VirtualEnvFrontend | None = None
self.builds: set[str] = set()
self.builds: defaultdict[str, list[EnvConfigSet]] = defaultdict(list)
self._distribution_meta: PathDistribution | None = None
self._package_dependencies: list[Requirement] | None = None
self._package_name: str | None = None
Expand Down Expand Up @@ -136,7 +137,8 @@ def meta_folder(self) -> Path:

def register_run_env(self, run_env: RunToxEnv) -> Generator[tuple[str, str], PackageToxEnv, None]:
yield from super().register_run_env(run_env)
self.builds.add(run_env.conf["package"])
build_type = run_env.conf["package"]
self.builds[build_type].append(run_env.conf)

def _setup_env(self) -> None:
super()._setup_env()
Expand Down Expand Up @@ -169,13 +171,15 @@ def perform_packaging(self, for_env: EnvConfigSet) -> list[Package]:
try:
deps = self._load_deps(for_env)
except BuildEditableNotSupported:
targets = [e for e in self.builds.pop("editable") if e["package"] == "editable"]
names = ", ".join(sorted({t.env_name for t in targets if t.env_name}))
logging.error(
f"package config for {for_env.env_name} is editable, however the build backend {self._frontend.backend}"
f"package config for {names} is editable, however the build backend {self._frontend.backend}"
f" does not support PEP-660, falling back to editable-legacy - change your configuration to it",
)
self.builds.remove("editable")
self.builds.add("editable-legacy")
for_env._defined["package"].value = "editable-legacy" # type: ignore
for env in targets:
env._defined["package"].value = "editable-legacy" # type: ignore
self.builds["editable-legacy"].append(env)
deps = self._load_deps(for_env)
of_type: str = for_env["package"]
if of_type == "editable-legacy":
Expand Down
2 changes: 2 additions & 0 deletions tests/config/cli/test_cli_env_var.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def test_verbose_no_test() -> None:
"list_envs": False,
"devenv_path": None,
"env": CliEnv(),
"exit_and_dump_after": 0,
"skip_missing_interpreters": "config",
"skip_pkg_install": False,
"recreate": False,
Expand Down Expand Up @@ -118,6 +119,7 @@ def test_env_var_exhaustive_parallel_values(
"config_file": None,
"factors": [],
"labels": [],
"exit_and_dump_after": 0,
}
assert options.parsed.verbosity == 4
assert options.cmd_handlers == core_handlers
Expand Down
2 changes: 2 additions & 0 deletions tests/config/cli/test_cli_ini.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ def default_options(tmp_path: Path) -> dict[str, Any]:
"config_file": (tmp_path / "tox.ini").absolute(),
"factors": [],
"labels": [],
"exit_and_dump_after": 0,
}


Expand Down Expand Up @@ -132,6 +133,7 @@ def test_ini_exhaustive_parallel_values(exhaustive_ini: Path, core_handlers: dic
"config_file": exhaustive_ini,
"factors": [],
"labels": [],
"exit_and_dump_after": 0,
}
assert options.parsed.verbosity == 4
assert options.cmd_handlers == core_handlers
Expand Down
2 changes: 0 additions & 2 deletions tests/tox_env/python/test_python_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ def test_diff_msg_no_diff() -> None:
[
("magic", ["pypy"]),
("magic", ["py39"]),
(".pkg", ["py"]),
],
ids=lambda a: "|".join(a) if isinstance(a, list) else str(a),
)
Expand All @@ -90,7 +89,6 @@ def test_base_python_env_no_conflict(env: str, base_python: list[str], ignore_co
@pytest.mark.parametrize(
("env", "base_python", "conflict"),
[
("py", ["pypy"], ["pypy"]),
("cpython", ["pypy"], ["pypy"]),
("pypy", ["cpython"], ["cpython"]),
("pypy2", ["pypy3"], ["pypy3"]),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,10 @@ def test_pyproject_deps_static_with_dynamic(
def test_pyproject_no_build_editable_fallback(tox_project: ToxProjectCreator, demo_pkg_inline: Path) -> None:
proj = tox_project({"tox.ini": ""}, base=demo_pkg_inline)
execute_calls = proj.patch_execute(lambda r: 0 if "install" in r.run_id else None)
result = proj.run("r", "--notest", "--develop")
result = proj.run("r", "-e", "a,b", "--notest", "--develop")
result.assert_success()
warning = (
".pkg: package config for py is editable, however the build backend build does not support PEP-660, "
".pkg: package config for a, b is editable, however the build backend build does not support PEP-660, "
"falling back to editable-legacy - change your configuration to it"
)
assert warning in result.out.splitlines()
Expand All @@ -187,7 +187,9 @@ def test_pyproject_no_build_editable_fallback(tox_project: ToxProjectCreator, de
(".pkg", "_optional_hooks"),
(".pkg", "build_wheel"),
(".pkg", "get_requires_for_build_sdist"),
("py", "install_package"),
("a", "install_package"),
(".pkg", "get_requires_for_build_sdist"),
("b", "install_package"),
(".pkg", "_exit"),
]
found_calls = [(i[0][0].conf.name, i[0][3].run_id) for i in execute_calls.call_args_list]
Expand Down
1 change: 1 addition & 0 deletions whitelist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ exe
executables
extlinks
extractall
faulthandler
favicon
filelock
fixup
Expand Down