Skip to content
Draft
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
40 changes: 30 additions & 10 deletions src/scikit_build_core/settings/skbuild_read_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,22 +137,28 @@ def _handle_move(

def _validate_overrides(
settings: ScikitBuildSettings,
static_settings: ScikitBuildSettings,
overrides: dict[str, OverrideRecord],
config_setting_keys: set[str],
) -> None:
"""Validate all fields with any override information."""

def validate_field(
field: dataclasses.Field[Any],
value: Any,
static_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("override_only", False):
original_value = record.original_value if record else value
if original_value is not None:
full_key = f"{prefix}{conf_key}"
original_value = record.original_value if record else static_value
if original_value is not None or (
value is not None and full_key not in config_setting_keys
):
Comment on lines +159 to +161
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think the logic here is right, but can't tell exactly what it should be. I think it fails if the user provided if from env-variable.

msg = f"{prefix}{conf_key} is not allowed to be hard-coded in the pyproject.toml file"
if settings.strict_config:
sys.stdout.flush()
Expand All @@ -162,6 +168,7 @@ def validate_field(

def validate_field_recursive(
obj: Any,
static_obj: Any,
record: OverrideRecord | None = None,
prefix: str = "",
) -> None:
Expand All @@ -170,20 +177,25 @@ def validate_field_recursive(
conf_key = field.name.replace("_", "-")
closest_record = overrides.get(f"{prefix}{conf_key}", record)
value = getattr(obj, field.name)
static_value = getattr(static_obj, field.name)
# Do the validation of the current field
validate_field(
field=field,
value=value,
static_value=static_value,
prefix=prefix,
record=closest_record,
)
if dataclasses.is_dataclass(value):
validate_field_recursive(
obj=value, record=closest_record, prefix=f"{prefix}{conf_key}."
obj=value,
static_obj=static_value,
record=closest_record,
prefix=f"{prefix}{conf_key}.",
)

# Navigate all fields starting from the top-level
validate_field_recursive(obj=settings)
validate_field_recursive(obj=settings, static_obj=static_settings)


class SettingsReader:
Expand Down Expand Up @@ -250,6 +262,9 @@ def __init__(
remaining = {
k: v for k, v in config_settings.items() if not k.startswith("skbuild.")
}
self.config_setting_keys = {
k[8:] if k.startswith("skbuild.") else k for k in config_settings
}
self.sources = SourceChain(
EnvSource("SKBUILD", env=env),
ConfSource("skbuild", settings=prefixed, verify=verify_conf),
Expand All @@ -259,7 +274,7 @@ def __init__(
)
self.settings = self.sources.convert_target(ScikitBuildSettings)

static_settings = SourceChain(
self.static_settings = SourceChain(
Comment on lines -262 to +277
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe what we need here is to break down the self.settings from above to get the EnvSource/ConfSource separate from the *toml_srcs and in the other part check explicitly if the key is in the dynamic_settings.

*toml_srcs, prefixes=["tool", "scikit-build"]
).convert_target(ScikitBuildSettings)

Expand Down Expand Up @@ -350,8 +365,8 @@ def __init__(
self.settings.build.verbose,
self.settings.minimum_version,
Version("0.10"),
static=static_settings.cmake.verbose == self.settings.cmake.verbose
and static_settings.build.verbose == self.settings.build.verbose,
static=self.static_settings.cmake.verbose == self.settings.cmake.verbose
and self.static_settings.build.verbose == self.settings.build.verbose,
)
self.settings.build.targets = _handle_move(
"cmake.targets",
Expand All @@ -360,8 +375,8 @@ def __init__(
self.settings.build.targets,
self.settings.minimum_version,
Version("0.10"),
static=static_settings.cmake.targets == self.settings.cmake.targets
and static_settings.build.targets == self.settings.build.targets,
static=self.static_settings.cmake.targets == self.settings.cmake.targets
and self.static_settings.build.targets == self.settings.build.targets,
)

if self.settings.sdist.inclusion_mode is not None:
Expand Down Expand Up @@ -421,7 +436,12 @@ def validate_may_exit(self) -> None:
self.print_suggestions()
raise SystemExit(7)
logger.warning("Unrecognized options: {}", ", ".join(unrecognized))
_validate_overrides(self.settings, self.overridden_items)
_validate_overrides(
self.settings,
self.static_settings,
self.overridden_items,
self.config_setting_keys,
)

for key, value in self.settings.metadata.items():
if "provider" not in value:
Expand Down
31 changes: 31 additions & 0 deletions tests/test_settings_overrides.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,37 @@ def test_disallow_hardcoded(
assert "is not allowed to be hard-coded in the pyproject.toml file" in out


@pytest.mark.parametrize("prefix", [True, False], ids=["skbuild", "noprefix"])
def test_allow_override_only_in_config_settings(
tmp_path: Path,
caplog: pytest.LogCaptureFixture,
prefix: bool,
):
pyproject_toml = tmp_path / "pyproject.toml"
pyproject_toml.write_text("", encoding="utf-8")

config_settings: dict[str, str] = {
"cmake.toolchain-file": "foo.cmake",
"wheel.tags": "cp312-abi3-win_amd64",
}
if prefix:
config_settings = {f"skbuild.{k}": v for k, v in config_settings.items()}

caplog.set_level(logging.WARNING)

settings_reader = SettingsReader.from_file(pyproject_toml, config_settings)
settings_reader.validate_may_exit()

assert settings_reader.settings.cmake.toolchain_file == Path("foo.cmake")
assert settings_reader.settings.wheel.tags == ["cp312-abi3-win_amd64"]
assert not [
record
for record in caplog.records
if "is not allowed to be hard-coded in the pyproject.toml file"
in str(record.msg)
]


@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