From 95ff7f73f80300f7f45c805bde116520f139ab02 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 14 May 2025 11:32:06 +0200 Subject: [PATCH 1/5] Save a record of the override actions Used for further validations Signed-off-by: Cristian Le --- .../settings/skbuild_overrides.py | 100 +++++++++++++++++- .../settings/skbuild_read_settings.py | 2 +- 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/scikit_build_core/settings/skbuild_overrides.py b/src/scikit_build_core/settings/skbuild_overrides.py index 53ec87edb..953fa0c02 100644 --- a/src/scikit_build_core/settings/skbuild_overrides.py +++ b/src/scikit_build_core/settings/skbuild_overrides.py @@ -1,5 +1,6 @@ from __future__ import annotations +import dataclasses import os import platform import re @@ -18,7 +19,8 @@ from ..errors import CMakeNotFoundError from ..resources import resources -__all__ = ["process_overrides", "regex_match"] + +__all__ = ["OverrideRecord", "process_overrides", "regex_match"] def __dir__() -> list[str]: @@ -29,6 +31,30 @@ def __dir__() -> list[str]: from collections.abc import Mapping +@dataclasses.dataclass +class OverrideRecord: + """ + Record of the override action. + + Saves the original and final values, and the override reasons. + """ + + key: str + """Settings key that is overridden.""" + original_value: Any | None + """ + Original value in the pyproject table. + + If the pyproject table did not have the key, this is a ``None``. + """ + value: Any + """Final value.""" + passed_all: dict[str, str] | None + """All if statements that passed (except the effective ``match_any``).""" + passed_any: dict[str, str] | None + """All if.any statements that passed.""" + + def strtobool(value: str) -> bool: """ Converts a environment variable string into a boolean value. @@ -257,20 +283,72 @@ def inherit_join( raise TypeError(msg) +def record_override( + *keys: str, + value: Any, + tool_skb: dict[str, Any], + overriden_items: dict[str, OverrideRecord], + passed_all: dict[str, str] | None, + passed_any: dict[str, str] | None, +) -> None: + full_key = ".".join(keys) + # Get the original_value to construct the record + if full_key in overriden_items: + # We found the original value from a previous override record + original_value = overriden_items[full_key].original_value + else: + # Otherwise navigate the original pyproject table until we resolved all keys + _dict_or_value = tool_skb + keys_list = [*keys] + while keys_list: + k = keys_list.pop(0) + if k not in _dict_or_value: + # We hit a dead end so we imply the original_value was not set (`None`) + original_value = None + break + _dict_or_value = _dict_or_value[k] + if isinstance(_dict_or_value, dict): + # If the value is a dict it is either the final value or we continue + # to navigate it + continue + # Otherwise it should be the final value + original_value = _dict_or_value + if keys_list: + msg = f"Could not navigate to the key {full_key} because {k} is a {type(_dict_or_value)}" + raise TypeError(msg) + break + else: + # We exhausted all keys so the current value should be the table key we are + # interested in + original_value = _dict_or_value + # Now save the override record + overriden_items[full_key] = OverrideRecord( + key=keys[-1], + original_value=original_value, + value=value, + passed_any=passed_any, + passed_all=passed_all, + ) + + def process_overrides( tool_skb: dict[str, Any], *, state: Literal["sdist", "wheel", "editable", "metadata_wheel", "metadata_editable"], retry: bool, env: Mapping[str, str] | None = None, -) -> set[str]: +) -> tuple[set[str], dict[str, OverrideRecord]]: """ Process overrides into the main dictionary if they match. Modifies the input dictionary. Must be run from the package directory. + + :return: A tuple of the set of matching overrides and a dict of changed keys and + override record """ has_dist_info = Path("PKG-INFO").is_file() global_matched: set[str] = set() + overriden_items: dict[str, OverrideRecord] = {} for override in tool_skb.pop("overrides", []): passed_any: dict[str, str] | None = None passed_all: dict[str, str] | None = None @@ -354,6 +432,14 @@ def process_overrides( inherit1 = inherit_override.get(key, {}) if isinstance(value, dict): for key2, value2 in value.items(): + record_override( + *[key, key2], + value=value, + tool_skb=tool_skb, + overriden_items=overriden_items, + passed_all=passed_all, + passed_any=passed_any, + ) inherit2 = inherit1.get(key2, "none") inner = tool_skb.get(key, {}) inner[key2] = inherit_join( @@ -361,10 +447,18 @@ def process_overrides( ) tool_skb[key] = inner else: + record_override( + key, + value=value, + tool_skb=tool_skb, + overriden_items=overriden_items, + passed_all=passed_all, + passed_any=passed_any, + ) inherit_override_tmp = inherit_override or "none" if isinstance(inherit_override_tmp, dict): assert not inherit_override_tmp tool_skb[key] = inherit_join( value, tool_skb.get(key), inherit_override_tmp ) - return global_matched + return global_matched, overriden_items diff --git a/src/scikit_build_core/settings/skbuild_read_settings.py b/src/scikit_build_core/settings/skbuild_read_settings.py index 33e4dfba4..af2f0ea43 100644 --- a/src/scikit_build_core/settings/skbuild_read_settings.py +++ b/src/scikit_build_core/settings/skbuild_read_settings.py @@ -151,7 +151,7 @@ def __init__( # Handle overrides pyproject = copy.deepcopy(pyproject) - self.overrides = process_overrides( + self.overrides, self.overriden_items = process_overrides( pyproject.get("tool", {}).get("scikit-build", {}), state=state, env=env, From 3472dc58d8fc9b9fff4432bf7036f0684f6d9497 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 14 May 2025 14:48:35 +0200 Subject: [PATCH 2/5] Add a metadata to not allow hardcoded options Signed-off-by: Cristian Le --- .../settings/skbuild_model.py | 2 + .../settings/skbuild_read_settings.py | 54 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/scikit_build_core/settings/skbuild_model.py b/src/scikit_build_core/settings/skbuild_model.py index a83abd2f8..8c2622f81 100644 --- a/src/scikit_build_core/settings/skbuild_model.py +++ b/src/scikit_build_core/settings/skbuild_model.py @@ -33,6 +33,8 @@ def __dir__() -> List[str]: class SettingsFieldMetadata(TypedDict, total=False): display_default: Optional[str] deprecated: bool + disallow_hard_code: bool + """Do not allow the field to be hard-coded in the pyproject table.""" class CMakeSettingsDefine(str): diff --git a/src/scikit_build_core/settings/skbuild_read_settings.py b/src/scikit_build_core/settings/skbuild_read_settings.py index af2f0ea43..fcd07f121 100644 --- a/src/scikit_build_core/settings/skbuild_read_settings.py +++ b/src/scikit_build_core/settings/skbuild_read_settings.py @@ -24,6 +24,8 @@ import os from collections.abc import Generator, Mapping + from .skbuild_overrides import OverrideRecord + __all__ = ["SettingsReader"] @@ -133,6 +135,57 @@ def _handle_move( return before +def _validate_overrides( + settings: ScikitBuildSettings, + overrides: dict[str, OverrideRecord], +) -> None: + """Validate all fields with any override information.""" + + def validate_field( + field: dataclasses.Field[Any], + value: Any, + prefix: str = "", + record: OverrideRecord | None = None, + ) -> None: + """Do the actual validation.""" + # Check if we had a hard-coded value in the record + conf_key = field.name.replace("_", "-") + if field.metadata.get("disallow_hard_code", False): + original_value = record.original_value if record else value + if original_value is not None: + msg = f"{prefix}{conf_key} is not allowed to be hard-coded in the pyproject.toml file" + if settings.strict_config: + sys.stdout.flush() + rich_print(f"{{bold.red}}ERROR:{{normal}} {msg}") + raise SystemExit(7) + logger.warning(msg) + + def validate_field_recursive( + obj: Any, + record: OverrideRecord | None = None, + prefix: str = "", + ) -> None: + """Navigate through all the keys and validate each field.""" + for field in dataclasses.fields(obj): + conf_key = field.name.replace("_", "-") + closest_record = overrides.get(f"{prefix}{conf_key}", record) + value = getattr(obj, field.name) + # Do the validation of the current field + validate_field( + field=field, + value=value, + prefix=prefix, + record=closest_record, + ) + if dataclasses.is_dataclass(value): + validate_field_recursive( + obj=value, record=closest_record, prefix=f"{prefix}{conf_key}." + ) + + # Navigate all fields starting from the top-level + validate_field_recursive(obj=settings) + + class SettingsReader: def __init__( self, @@ -352,6 +405,7 @@ def validate_may_exit(self) -> None: self.print_suggestions() raise SystemExit(7) logger.warning("Unrecognized options: {}", ", ".join(unrecognized)) + _validate_overrides(self.settings, self.overriden_items) for key, value in self.settings.metadata.items(): if "provider" not in value: From 2ef008e08cfe09ed5a801c4e9a0a3fd9bfa89a73 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 14 May 2025 16:40:56 +0200 Subject: [PATCH 3/5] Make `fail` a disallow_hard_code Add `test_disallow_hardcoded` to cover these type of settings Signed-off-by: Cristian Le --- README.md | 2 +- docs/reference/configs.md | 1 - .../resources/scikit-build.schema.json | 1 - .../settings/skbuild_model.py | 7 ++- tests/test_settings_overrides.py | 48 +++++++++++++++++++ 5 files changed, 55 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ba1626045..0767e9b80 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,7 @@ minimum-version = "0.11" # current version build-dir = "" # Immediately fail the build. This is only useful in overrides. -fail = false +fail = "" ``` diff --git a/docs/reference/configs.md b/docs/reference/configs.md index 8c6e8b857..fcaa30bd9 100644 --- a/docs/reference/configs.md +++ b/docs/reference/configs.md @@ -78,7 +78,6 @@ print(mk_skbuild_docs()) ```{eval-rst} .. confval:: fail :type: ``bool`` - :default: false Immediately fail the build. This is only useful in overrides. ``` diff --git a/src/scikit_build_core/resources/scikit-build.schema.json b/src/scikit_build_core/resources/scikit-build.schema.json index 5065b4153..322a9ea0d 100644 --- a/src/scikit_build_core/resources/scikit-build.schema.json +++ b/src/scikit_build_core/resources/scikit-build.schema.json @@ -491,7 +491,6 @@ }, "fail": { "type": "boolean", - "default": false, "description": "Immediately fail the build. This is only useful in overrides." }, "overrides": { diff --git a/src/scikit_build_core/settings/skbuild_model.py b/src/scikit_build_core/settings/skbuild_model.py index 8c2622f81..0de8d02af 100644 --- a/src/scikit_build_core/settings/skbuild_model.py +++ b/src/scikit_build_core/settings/skbuild_model.py @@ -507,7 +507,12 @@ class ScikitBuildSettings: This can be set to reuse the build directory from previous runs. """ - fail: bool = False + fail: Optional[bool] = dataclasses.field( + default=None, + metadata=SettingsFieldMetadata( + disallow_hard_code=True, + ), + ) """ Immediately fail the build. This is only useful in overrides. """ diff --git a/tests/test_settings_overrides.py b/tests/test_settings_overrides.py index 098414a52..c4d3cec14 100644 --- a/tests/test_settings_overrides.py +++ b/tests/test_settings_overrides.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging import sysconfig import typing from pathlib import Path @@ -22,6 +23,53 @@ class VersionInfo(typing.NamedTuple): releaselevel: str = "final" +def test_disallow_hardcoded( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + capsys: pytest.CaptureFixture[str], +): + caplog.set_level(logging.WARNING) + pyproject_toml = tmp_path / "pyproject.toml" + template = dedent( + """\ + [tool.scikit-build] + strict-config = {strict_config} + fail = false + """ + ) + + # First check without strict-config to make sure all fields are disallowed + strict_config = "false" + pyproject_toml.write_text( + template.format(strict_config=strict_config), + encoding="utf-8", + ) + + settings_reader = SettingsReader.from_file(pyproject_toml) + settings_reader.validate_may_exit() + assert caplog.records + for idx, key in enumerate(["fail"]): + assert ( + f"{key} is not allowed to be hard-coded in the pyproject.toml file" + in str(caplog.records[idx].msg) + ) + + # Next check that this exits if string-config is set + strict_config = "true" + pyproject_toml.write_text( + template.format(strict_config=strict_config), + encoding="utf-8", + ) + # Flush the capsys just in case + capsys.readouterr() + settings_reader = SettingsReader.from_file(pyproject_toml) + with pytest.raises(SystemExit) as exc: + settings_reader.validate_may_exit() + assert exc.value.code == 7 + out, _ = capsys.readouterr() + assert "is not allowed to be hard-coded in the pyproject.toml file" in out + + @pytest.mark.parametrize("python_version", ["3.9", "3.10"]) def test_skbuild_overrides_pyver( python_version: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch From 00703bae58462b9fc0eed730ef4d1da078743996 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Tue, 1 Jul 2025 15:59:39 -0400 Subject: [PATCH 4/5] chore: style fixes after rebase Signed-off-by: Henry Schreiner --- src/scikit_build_core/settings/skbuild_overrides.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/scikit_build_core/settings/skbuild_overrides.py b/src/scikit_build_core/settings/skbuild_overrides.py index 953fa0c02..3510d3253 100644 --- a/src/scikit_build_core/settings/skbuild_overrides.py +++ b/src/scikit_build_core/settings/skbuild_overrides.py @@ -19,8 +19,7 @@ from ..errors import CMakeNotFoundError from ..resources import resources - -__all__ = ["OverrideRecord", "process_overrides", "regex_match"] +__all__ = ["OverrideRecord", "process_overrides", "regex_match"] def __dir__() -> list[str]: From 4c114dfb1a45f284ecd49b792a09b67037300c9b Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 3 Jul 2025 16:55:06 -0400 Subject: [PATCH 5/5] fix: change name and remove from readme Signed-off-by: Henry Schreiner --- README.md | 3 --- src/scikit_build_core/settings/documentation.py | 2 ++ src/scikit_build_core/settings/skbuild_docs_readme.py | 6 +++++- src/scikit_build_core/settings/skbuild_model.py | 6 +++--- src/scikit_build_core/settings/skbuild_overrides.py | 4 ++++ src/scikit_build_core/settings/skbuild_read_settings.py | 2 +- tests/test_settings_docs.py | 2 +- 7 files changed, 16 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 0767e9b80..43c44aa92 100644 --- a/README.md +++ b/README.md @@ -286,9 +286,6 @@ minimum-version = "0.11" # current version # The CMake build directory. Defaults to a unique temporary directory. build-dir = "" -# Immediately fail the build. This is only useful in overrides. -fail = "" - ``` diff --git a/src/scikit_build_core/settings/documentation.py b/src/scikit_build_core/settings/documentation.py index 2d7ea0dac..67a736e2e 100644 --- a/src/scikit_build_core/settings/documentation.py +++ b/src/scikit_build_core/settings/documentation.py @@ -56,6 +56,7 @@ class DCDoc: docs: str field: dataclasses.Field[typing.Any] deprecated: bool = False + override_only: bool = False def sanitize_default_field(text: str) -> str: @@ -134,4 +135,5 @@ def mk_docs(dc: type[object], prefix: str = "") -> Generator[DCDoc, None, None]: docs=docs[field.name], field=field, deprecated=field.metadata.get("deprecated", False), + override_only=field.metadata.get("override_only", False), ) diff --git a/src/scikit_build_core/settings/skbuild_docs_readme.py b/src/scikit_build_core/settings/skbuild_docs_readme.py index 4bf45c65e..b731cdf90 100644 --- a/src/scikit_build_core/settings/skbuild_docs_readme.py +++ b/src/scikit_build_core/settings/skbuild_docs_readme.py @@ -46,7 +46,11 @@ def mk_skbuild_docs() -> str: Makes documentation for the skbuild model. """ doc = Document( - [Item(item) for item in mk_docs(ScikitBuildSettings) if not item.deprecated] + [ + Item(item) + for item in mk_docs(ScikitBuildSettings) + if not item.deprecated and not item.override_only + ] ) return doc.format() diff --git a/src/scikit_build_core/settings/skbuild_model.py b/src/scikit_build_core/settings/skbuild_model.py index 0de8d02af..953c18e31 100644 --- a/src/scikit_build_core/settings/skbuild_model.py +++ b/src/scikit_build_core/settings/skbuild_model.py @@ -33,8 +33,8 @@ def __dir__() -> List[str]: class SettingsFieldMetadata(TypedDict, total=False): display_default: Optional[str] deprecated: bool - disallow_hard_code: bool - """Do not allow the field to be hard-coded in the pyproject table.""" + override_only: bool + """Do not allow the field to be a top-level table.""" class CMakeSettingsDefine(str): @@ -510,7 +510,7 @@ class ScikitBuildSettings: fail: Optional[bool] = dataclasses.field( default=None, metadata=SettingsFieldMetadata( - disallow_hard_code=True, + override_only=True, ), ) """ diff --git a/src/scikit_build_core/settings/skbuild_overrides.py b/src/scikit_build_core/settings/skbuild_overrides.py index 3510d3253..cea43e771 100644 --- a/src/scikit_build_core/settings/skbuild_overrides.py +++ b/src/scikit_build_core/settings/skbuild_overrides.py @@ -40,16 +40,20 @@ class OverrideRecord: key: str """Settings key that is overridden.""" + original_value: Any | None """ Original value in the pyproject table. If the pyproject table did not have the key, this is a ``None``. """ + value: Any """Final value.""" + passed_all: dict[str, str] | None """All if statements that passed (except the effective ``match_any``).""" + passed_any: dict[str, str] | None """All if.any statements that passed.""" diff --git a/src/scikit_build_core/settings/skbuild_read_settings.py b/src/scikit_build_core/settings/skbuild_read_settings.py index fcd07f121..e0b5ede87 100644 --- a/src/scikit_build_core/settings/skbuild_read_settings.py +++ b/src/scikit_build_core/settings/skbuild_read_settings.py @@ -150,7 +150,7 @@ def validate_field( """Do the actual validation.""" # Check if we had a hard-coded value in the record conf_key = field.name.replace("_", "-") - if field.metadata.get("disallow_hard_code", False): + if field.metadata.get("override_only", False): original_value = record.original_value if record else value if original_value is not None: msg = f"{prefix}{conf_key} is not allowed to be hard-coded in the pyproject.toml file" diff --git a/tests/test_settings_docs.py b/tests/test_settings_docs.py index 8c8da9613..8ae98ad18 100644 --- a/tests/test_settings_docs.py +++ b/tests/test_settings_docs.py @@ -18,7 +18,7 @@ def test_skbuild_docs_readme() -> None: "A table of defines to pass to CMake when configuring the project. Additive." in docs ) - assert "fail = false" in docs + assert "fail = " not in docs # Deprecated items are not included here assert "ninja.minimum-version" not in docs