Skip to content

feat: Add a setting to disallow hard-coding some setting keys in the pyproject.toml #1078

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ minimum-version = "0.11" # current version
build-dir = ""

# Immediately fail the build. This is only useful in overrides.
fail = false
fail = ""

```

Expand Down
1 change: 0 additions & 1 deletion docs/reference/configs.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ print(mk_skbuild_docs())
```{eval-rst}
.. confval:: fail
:type: ``bool``
:default: false

Immediately fail the build. This is only useful in overrides.
```
Expand Down
1 change: 0 additions & 1 deletion src/scikit_build_core/resources/scikit-build.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,6 @@
},
"fail": {
"type": "boolean",
"default": false,
"description": "Immediately fail the build. This is only useful in overrides."
},
"overrides": {
Expand Down
9 changes: 8 additions & 1 deletion src/scikit_build_core/settings/skbuild_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -414,7 +416,12 @@ class ScikitBuildSettings:
The build directory. Defaults to a temporary directory, but can be set.
"""

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.
"""
99 changes: 96 additions & 3 deletions src/scikit_build_core/settings/skbuild_overrides.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import dataclasses
import os
import platform
import re
Expand All @@ -18,7 +19,7 @@
from ..errors import CMakeNotFoundError
from ..resources import resources

__all__ = ["process_overides", "regex_match"]
__all__ = ["OverrideRecord", "process_overides", "regex_match"]


def __dir__() -> list[str]:
Expand All @@ -29,6 +30,30 @@
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.
Expand Down Expand Up @@ -257,20 +282,72 @@
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)

Check warning on line 317 in src/scikit_build_core/settings/skbuild_overrides.py

View check run for this annotation

Codecov / codecov/patch

src/scikit_build_core/settings/skbuild_overrides.py#L316-L317

Added lines #L316 - L317 were not covered by tests
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_overides(
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
Expand Down Expand Up @@ -354,17 +431,33 @@
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(
value2, inner.get(key2, None), inherit2
)
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
56 changes: 55 additions & 1 deletion src/scikit_build_core/settings/skbuild_read_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import os
from collections.abc import Generator, Mapping

from .skbuild_overrides import OverrideRecord


__all__ = ["SettingsReader"]

Expand Down Expand Up @@ -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,
Expand All @@ -151,7 +204,7 @@ def __init__(

# Handle overrides
pyproject = copy.deepcopy(pyproject)
self.overrides = process_overides(
self.overrides, self.overriden_items = process_overides(
pyproject.get("tool", {}).get("scikit-build", {}),
state=state,
env=env,
Expand Down Expand Up @@ -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:
Expand Down
1 change: 0 additions & 1 deletion tests/test_settings_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ def test_skbuild_docs() -> None:
in docs
)
assert "DEPRECATED in 0.10, use build.verbose instead." in docs
assert "fail = false" in docs


def test_mk_docs() -> None:
Expand Down
48 changes: 48 additions & 0 deletions tests/test_settings_overrides.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import logging
import sysconfig
import typing
from pathlib import Path
Expand All @@ -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
Expand Down
Loading