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 fresh subprocesses and allow duplicate register config calls for the core set only #3237

Merged
merged 1 commit into from
Mar 6, 2024
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/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)