Skip to content

Commit

Permalink
Fix fresh subprocesses and allow duplicate register config calls for …
Browse files Browse the repository at this point in the history
…the core set only (#3237)
  • Loading branch information
gaborbernat authored Mar 6, 2024
1 parent d37cb08 commit 969fbec
Show file tree
Hide file tree
Showing 13 changed files with 62 additions and 40 deletions.
2 changes: 2 additions & 0 deletions docs/changelog/3235.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix crash with fresh subprocess, if the build backend is setuptools automatically enable fresh subprocesses for
build backend calls - by :user:`gaborbernat`.
2 changes: 1 addition & 1 deletion docs/config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ Python virtual environment packaging
.. conf::
:keys: fresh_subprocess
:version_added: 4.14.0
:default: False
:default: True if build backend is setuptools otherwise False

A flag controlling if each call to the build backend should be done in a fresh subprocess or not (especially older
build backends such as ``setuptools`` might require this to discover newly provisioned dependencies).
Expand Down
8 changes: 4 additions & 4 deletions src/tox/config/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def __init__( # noqa: PLR0913
self._overrides[override.namespace].append(override)

self._src = config_source
self._key_to_conf_set: dict[tuple[str, str], ConfigSet] = OrderedDict()
self._key_to_conf_set: dict[tuple[str, str, str], ConfigSet] = OrderedDict()
self._core_set: CoreConfigSet | None = None
self.memory_seed_loaders: defaultdict[str, list[MemoryLoader]] = defaultdict(list)

Expand Down Expand Up @@ -131,7 +131,7 @@ def get_section_config( # noqa: PLR0913
for_env: str | None,
loaders: Sequence[Loader[Any]] | None = None,
) -> T:
key = section.key, for_env or ""
key = section.key, for_env or "", "-".join(base or [])
try:
return self._key_to_conf_set[key] # type: ignore[return-value] # expected T but found ConfigSet
except KeyError:
Expand All @@ -154,7 +154,7 @@ def get_env(
"""
Return the configuration for a given tox environment (will create if not exist yet).
:param item: the name of the environment
:param item: the name of the environment is
:param package: a flag indicating if the environment is of type packaging or not (only used for creation)
:param loaders: loaders to use for this configuration (only used for creation)
:return: the tox environments config
Expand All @@ -170,7 +170,7 @@ def get_env(

def clear_env(self, name: str) -> None:
section, _, __ = self._src.get_tox_env_section(name)
del self._key_to_conf_set[(section.key, name)]
self._key_to_conf_set = {k: v for k, v in self._key_to_conf_set.items() if k[0] == section.key and k[1] == name}


___all__ = [
Expand Down
6 changes: 5 additions & 1 deletion src/tox/config/of_type.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ def __call__(
def __eq__(self, o: object) -> bool:
return type(self) == type(o) and super().__eq__(o) and self.value == o.value # type: ignore[attr-defined]

def __repr__(self) -> str:
values = ((k, v) for k, v in vars(self).items() if v is not None)
return f"{type(self).__name__}({', '.join(f'{k}={v}' for k, v in values)})"


_PLACE_HOLDER = object()

Expand Down Expand Up @@ -111,7 +115,7 @@ def __call__(
return cast(T, self._cache)

def __repr__(self) -> str:
values = ((k, v) for k, v in vars(self).items() if k != "post_process" and v is not None)
values = ((k, v) for k, v in vars(self).items() if k not in {"post_process", "_cache"} and v is not None)
return f"{type(self).__name__}({', '.join(f'{k}={v}' for k, v in values)})"

def __eq__(self, o: object) -> bool:
Expand Down
18 changes: 8 additions & 10 deletions src/tox/config/sets.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,17 @@ def _add_conf(self, keys: Sequence[str], definition: ConfigDefinition[V]) -> Con
key = keys[0]
if key in self._defined:
self._on_duplicate_conf(key, definition)
else:
self._keys[key] = None
for item in keys:
self._alias[item] = key
for key in keys:
self._defined[key] = definition

self._keys[key] = None
for item in keys:
self._alias[item] = key
for key in keys:
self._defined[key] = definition
return definition

def _on_duplicate_conf(self, key: str, definition: ConfigDefinition[V]) -> None:
earlier = self._defined[key]
if definition != earlier: # pragma: no branch
msg = f"config {key} already defined"
raise ValueError(msg)
msg = f"duplicate configuration definition for {self.name}:\nhas: {self._defined[key]}\nnew: {definition}"
raise ValueError(msg)

def __getitem__(self, item: str) -> Any:
"""
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 @@ -91,6 +91,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._local_execute = None
self.is_alive = False


Expand Down
16 changes: 9 additions & 7 deletions src/tox/session/env_select.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,12 @@ def _env_name_to_active(self) -> dict[str, bool]:
def _defined_envs(self) -> dict[str, _ToxEnvInfo]: # noqa: C901, PLR0912
# The problem of classifying run/package environments:
# There can be two type of tox environments: run or package. Given a tox environment name there's no easy way to
# find out which it is. Intuitively a run environment is any environment that's not used for packaging by
# another run environment. To find out what are the packaging environments for a run environment you have to
# first construct it. This implies a two phase solution: construct all environments and query their packaging
# environments. The run environments are the ones not marked as of packaging type. This requires being able
# to change tox environments type, if it was earlier discovered as a run environment and is marked as packaging
# we need to redefine it, e.g. when it shows up in config as [testenv:.package] and afterwards by a run env is
# find out which it is. Intuitively, a run environment is any environment not used for packaging by another run
# environment. To find out what are the packaging environments for a run environment, you have to first
# construct it. This implies a two-phase solution: construct all environments and query their packaging
# environments. The run environments are the ones not marked as of packaging type. This requires being able to
# change tox environments types, if it was earlier discovered as a run environment and is marked as packaging,
# we need to redefine it. E.g., when it shows up in config as [testenv:.package] and afterward by a run env is
# marked as package_env.

if self._defined_envs_ is None: # noqa: PLR1702
Expand All @@ -267,7 +267,7 @@ def _defined_envs(self) -> dict[str, _ToxEnvInfo]: # noqa: C901, PLR0912
try:
run_env.package_env = self._build_pkg_env(pkg_name_type, name, env_name_to_active)
except Exception as exception: # noqa: BLE001
# if it's not a run environment, wait to see if ends up being a packaging one -> rollback
# if it's not a run environment, wait to see if ends up being a packaging one -> rollback
failed[name] = exception
for key in self._pkg_env_counter - start_package_env_use_counter:
del self._defined_envs_[key]
Expand Down Expand Up @@ -320,6 +320,7 @@ def _build_run_env(self, name: str) -> RunToxEnv | None:
journal = self._journal.get_env_journal(name)
args = ToxEnvCreateArgs(env_conf, self._state.conf.core, self._state.conf.options, journal, self._log_handler)
run_env = runner(args)
run_env.register_config()
self._manager.tox_add_env_config(env_conf, self._state)
return run_env

Expand Down Expand Up @@ -363,6 +364,7 @@ def _get_package_env(self, packager: str, name: str, is_active: bool) -> Package
journal = self._journal.get_env_journal(name)
args = ToxEnvCreateArgs(pkg_conf, self._state.conf.core, self._state.conf.options, journal, self._log_handler)
pkg_env: PackageToxEnv = package_type(args)
pkg_env.register_config()
self._defined_envs_[name] = _ToxEnvInfo(pkg_env, is_active)
self._manager.tox_add_env_config(pkg_conf, self._state)
return pkg_env
Expand Down
2 changes: 0 additions & 2 deletions src/tox/tox_env/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@ def __init__(self, create_args: ToxEnvCreateArgs) -> None:
self._interrupted = False
self._log_id = 0

self.register_config()

@property
def cache(self) -> Info:
return Info(self.env_dir)
Expand Down
23 changes: 16 additions & 7 deletions src/tox/tox_env/python/virtual_env/package/pyproject.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,19 @@ def __init__(self, create_args: ToxEnvCreateArgs) -> None:
self._package_dependencies: list[Requirement] | None = None
self._package_name: str | None = None
self._pkg_lock = RLock() # can build only one package at a time
self.root = self.conf["package_root"]
self._package_paths: set[Path] = set()
self._root: Path | None = None

@property
def root(self) -> Path:
if self._root is None:
self._root = self.conf["package_root"]
return self._root

@root.setter
def root(self, value: Path) -> None:
self._root = value
self._frontend_ = None # force recreating the frontend with new root

@staticmethod
def id() -> str:
Expand All @@ -117,8 +128,7 @@ def id() -> str:
@property
def _frontend(self) -> Pep517VirtualEnvFrontend:
if self._frontend_ is None:
fresh = cast(bool, self.conf["fresh_subprocess"])
self._frontend_ = Pep517VirtualEnvFrontend(self.root, self, fresh_subprocess=fresh)
self._frontend_ = Pep517VirtualEnvFrontend(self.root, self)
return self._frontend_

def register_config(self) -> None:
Expand All @@ -140,7 +150,7 @@ def register_config(self) -> None:
self.conf.add_config(
keys=["fresh_subprocess"],
of_type=bool,
default=False,
default=self._frontend.backend.split(".")[0] == "setuptools",
desc="create a fresh subprocess for every backend request",
)

Expand Down Expand Up @@ -377,10 +387,9 @@ def id() -> str:


class Pep517VirtualEnvFrontend(Frontend):
def __init__(self, root: Path, env: Pep517VenvPackager, *, fresh_subprocess: bool) -> None:
def __init__(self, root: Path, env: Pep517VenvPackager) -> None:
super().__init__(*Frontend.create_args_from_folder(root))
self._tox_env = env
self._fresh_subprocess = fresh_subprocess
self._backend_executor_: LocalSubProcessPep517Executor | None = None
into: dict[str, Any] = {}

Expand Down Expand Up @@ -435,7 +444,7 @@ def _send_msg(
if outcome is not None: # pragma: no branch
outcome.assert_success()
finally:
if self._fresh_subprocess:
if self._tox_env.conf["fresh_subprocess"]:
self.backend_executor.close()

def _unexpected_response( # noqa: PLR0913
Expand Down
15 changes: 13 additions & 2 deletions tests/config/test_sets.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import re
from collections import OrderedDict
from pathlib import Path
from typing import TYPE_CHECKING, Callable, Dict, Optional, Set, TypeVar
Expand Down Expand Up @@ -125,14 +126,24 @@ def test_config_dynamic_repr(conf_builder: ConfBuilder) -> None:
def test_config_redefine_constant_fail(conf_builder: ConfBuilder) -> None:
config_set = conf_builder("path = path")
config_set.add_constant(keys="path", desc="desc", value="value")
with pytest.raises(ValueError, match="config path already defined"):
msg = (
"duplicate configuration definition for py39:\n"
"has: ConfigConstantDefinition(keys=('path',), desc=desc, value=value)\n"
"new: ConfigConstantDefinition(keys=('path',), desc=desc2, value=value)"
)
with pytest.raises(ValueError, match=re.escape(msg)):
config_set.add_constant(keys="path", desc="desc2", value="value")


def test_config_redefine_dynamic_fail(conf_builder: ConfBuilder) -> None:
config_set = conf_builder("path = path")
config_set.add_config(keys="path", of_type=str, default="default_1", desc="path")
with pytest.raises(ValueError, match="config path already defined"):
msg = (
"duplicate configuration definition for py39:\n"
"has: ConfigDynamicDefinition(keys=('path',), desc=path, of_type=<class 'str'>, default=default_1)\n"
"new: ConfigDynamicDefinition(keys=('path',), desc=path, of_type=<class 'str'>, default=default_2)"
)
with pytest.raises(ValueError, match=re.escape(msg)):
config_set.add_config(keys="path", of_type=str, default="default_2", desc="path")


Expand Down
4 changes: 1 addition & 3 deletions tests/session/cmd/test_sequential.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ def test_result_json_sequential(
assert "result" not in log_report["testenvs"][".pkg"]

assert packaging_setup[-1][0] in {0, None}
assert packaging_setup[-1][1] == "_exit"
assert packaging_setup[:-1] == [
assert packaging_setup == [
(0, "install_requires"),
(None, "_optional_hooks"),
(None, "get_requires_for_build_wheel"),
Expand Down Expand Up @@ -303,7 +302,6 @@ def test_skip_develop_mode(tox_project: ToxProjectCreator, demo_pkg_setuptools:
(".pkg", "install_requires_for_build_editable"),
(".pkg", "build_editable"),
("py", "install_package"),
(".pkg", "_exit"),
]
assert calls == expected

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ def test_tox_install_pkg_sdist(tox_project: ToxProjectCreator, pkg_with_extras_p
(".pkg_external_sdist_meta", "prepare_metadata_for_build_wheel", []),
("py", "install_package_deps", deps),
("py", "install_package", ["--force-reinstall", "--no-deps", str(pkg_with_extras_project_sdist)]),
(".pkg_external_sdist_meta", "_exit", []),
]


Expand Down
4 changes: 2 additions & 2 deletions tests/tox_env/python/virtual_env/test_setuptools.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,5 @@ def test_setuptools_package(
assert len(py_messages) == 5, "\n".join(py_messages) # 1 install wheel + 3 command + 1 final report

package_messages = [i for i in result if ".pkg: " in i]
# 1 optional hooks + 1 install requires + 1 build requires + 1 build meta + 1 build isolated + 1 exit
assert len(package_messages) == 6, "\n".join(package_messages)
# 1 optional hooks + 1 install requires + 1 build requires + 1 build meta + 1 build isolated
assert len(package_messages) == 5, "\n".join(package_messages)

0 comments on commit 969fbec

Please sign in to comment.