BUG: improve validation of pyproject.toml meson-python configuration #304

dnicolodi · 2023-02-09T00:13:25Z

Switch from an incomplete (an bugged) ad hoc validation to a scheme based validation strategy. The scheme is defined in the function _validate_pyproject_config() as nested dictionaries where the keys are configuration field names and valued are validation functions. Unknown fields result in an error.

Fixes #293.

mesonpy/__init__.py

dnicolodi · 2023-03-01T12:26:19Z

We need this (or something similar) to fix #293, ideally before the next release.

mesonpy/__init__.py

FFY00

Just some minor comments, then I think it is ready for merging. Though, as mentioned in the other PR, we will probably move this code to pyproject-metadata when it implements the unknown keys check functionality anyway.

tests/test_pep517.py

FFY00 · 2023-03-07T21:08:24Z

mesonpy/__init__.py

-        assert isinstance(name, str)
-        return name.replace('-', '_')
+        """Project name."""
+        return str(self._metadata.name).replace('-', '_')


str is not needed here. The assert was because mypy was complaining because of the import IIRC.

mypy in CI disagrees with you https://github.com/mesonbuild/meson-python/actions/runs/4359109213/jobs/7620540144#step:5:13

That field can only ever be a string.

https://pep621.readthedocs.io/en/latest/#pyproject_metadata.StandardMetadata.name

I don't think mypy is loading the type hint correctly, hence the assert.

It's the version of mypy used in the CI, later versions of mypy do not complain

mesonpy/__init__.py

FFY00 · 2023-03-07T21:15:44Z

mesonpy/__init__.py

+
+    def _string(value: Any, name: str) -> str:
+        if not isinstance(value, str):
+            raise ConfigError(f'only one value for "{name}" can be specified')


This error messages assumes it can only be a container, while in fact it can be anything. For most PEP 517 frontends, it will be a container of some sort, but that is not guaranteed.

No, it can be only a string or a list of strings. It is written in the PEP

The PEP only makes a recommendation when frontends are building the config settings from a user provided string mapping 😅

In the PEP, you can see 3 words capitalized, "MUST", "SHOULD", and "MAY", they indicate obligation, recommendation, and optionality, respectively. So while the PEP does recommend for that to be the case, it does not mandate it. It also explicitly mentions that fronts can have an alternative mechanism to build the dictionary.

Build frontends MAY also provide arbitrary other mechanisms for users to place entries in this dictionary.

It's a simple enough check anyway.

Please, if you need to quote from the PEP, do no quote only the part that makes it look like it supports your claims:

This argument, which is passed to all hooks, is an arbitrary dictionary provided as an “escape hatch” for users to pass ad-hoc configuration into individual package builds. Build backends MAY assign any semantics they like to this dictionary. Build frontends SHOULD provide some mechanism for users to specify arbitrary string-key/string-value pairs to be placed in this dictionary. For example, they might support some syntax like --package-config CC=gcc. In case a user provides duplicate string-keys, build frontends SHOULD combine the corresponding string-values into a list of strings. Build frontends MAY also provide arbitrary other mechanisms for users to place entries in this dictionary.

The dictionary is string-key, string-value. You can use arbitrary mechanism to add entries to the dictionary, but the dictionary has string keys and string values.

I did not quote the part we were talking about because you were already referring to it, so I assumed you knew where it was. I only quoted the arbitrary mechanism bit to make it easy for you to ctrl+f it and see exactly what I was referring to.

The dictionary is string-key, string-value.

It doesn't say that anywhere, the first sentence even says it is an arbitrary dictionary.

In fact, I have considered adding support for loading config_settings from a Python file in pypa/build.

You can use arbitrary mechanism to add entries to the dictionary

Yes.

but the dictionary has string keys and string values.

No, the user options provided via that mechanism are.

My takeaways for that paragraph are:

config_setting is an arbitrary dictionary

It is intended for backends to assign semantics to it if they want

The PEP recommends that frontends to implement a mechanism to allow users to add settings to the dictionary

If a user provide duplicated keys via this mechanism, they should be combined into a list

Anyway, I don't think this is being productive. Feel free to merge this PR whenever you think it's good enough to merge.

Can you please take your time and read all the text?

Build frontends SHOULD provide some mechanism for users to specify arbitrary string-key/string-value pairs to be placed in this dictionary.

https://github.com/pypa/setuptools/blob/main/setuptools/build_meta.py#L142

Switch from an incomplete (an bugged) ad hoc validation to a scheme based validation strategy. The scheme is defined in the function _validate_pyproject_config() as nested dictionaries where the keys are configuration field names and valued are validation functions. Unknown fields result in an error. Fixes mesonbuild#293.

Code can be simplified using pyproject_metadata.StandardMetadata also when we infer the package metadata from meson.build.

PEP 517 specifies that the options are received as a dictionary with string keys and string values, thus there is no need to verify that.

dnicolodi force-pushed the pyproject-toml branch from aaaba53 to feccd5a Compare February 9, 2023 00:22

dnicolodi mentioned this pull request Feb 9, 2023

All configuration keys must be set if any are set #305

Closed

dnicolodi force-pushed the pyproject-toml branch from feccd5a to cf6cf99 Compare February 9, 2023 19:20

FFY00 reviewed Feb 9, 2023

View reviewed changes

mesonpy/__init__.py Show resolved Hide resolved

dnicolodi force-pushed the pyproject-toml branch 10 times, most recently from fe7904b to e0363cc Compare February 9, 2023 23:08

FFY00 added the enhancement New feature or request label Mar 1, 2023

dnicolodi force-pushed the pyproject-toml branch 3 times, most recently from e1962a5 to 1947798 Compare March 1, 2023 11:30

dnicolodi added this to the v0.13.0 milestone Mar 1, 2023

dnicolodi added the bug Something isn't working label Mar 1, 2023

dnicolodi mentioned this pull request Mar 4, 2023

ENH: add support for dynamic dependencies computation #319

Draft

FFY00 suggested changes Mar 7, 2023

View reviewed changes

mesonpy/__init__.py Show resolved Hide resolved

mesonpy/__init__.py Show resolved Hide resolved

dnicolodi mentioned this pull request Mar 7, 2023

MAINT: refactor config parsing and validation code #334

Closed

dnicolodi force-pushed the pyproject-toml branch from 1947798 to 517f4e8 Compare March 7, 2023 20:33

FFY00 suggested changes Mar 7, 2023

View reviewed changes

dnicolodi force-pushed the pyproject-toml branch 4 times, most recently from 91f3766 to 025e022 Compare March 7, 2023 22:23

dnicolodi force-pushed the pyproject-toml branch from 025e022 to da119d1 Compare March 7, 2023 22:24

dnicolodi added 2 commits March 7, 2023 23:28

MAINT: simplify package metadata handling

9a76f09

Code can be simplified using pyproject_metadata.StandardMetadata also when we infer the package metadata from meson.build.

MAINT: consolidate validation of options from build frontend

b5aeea9

PEP 517 specifies that the options are received as a dictionary with string keys and string values, thus there is no need to verify that.

dnicolodi force-pushed the pyproject-toml branch from da119d1 to b5aeea9 Compare March 7, 2023 22:30

dnicolodi merged commit 6d96b3e into mesonbuild:main Mar 7, 2023

peter-urban mentioned this pull request Mar 10, 2023

DOC: add documentation for Meson arguments #291

Merged

dnicolodi mentioned this pull request Mar 12, 2023

DOC: add details on default build options used by meson-python #338

Merged

FFY00 mentioned this pull request Apr 4, 2023

Build directory for editable installations #370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: improve validation of pyproject.toml meson-python configuration #304

BUG: improve validation of pyproject.toml meson-python configuration #304

dnicolodi commented Feb 9, 2023

dnicolodi commented Mar 1, 2023

FFY00 left a comment

FFY00 Mar 7, 2023

dnicolodi Mar 7, 2023

FFY00 Mar 7, 2023

dnicolodi Mar 7, 2023

FFY00 Mar 7, 2023

dnicolodi Mar 7, 2023

FFY00 Mar 7, 2023

dnicolodi Mar 7, 2023

FFY00 Mar 7, 2023

dnicolodi Mar 7, 2023

dnicolodi Mar 7, 2023

BUG: improve validation of pyproject.toml meson-python configuration #304

BUG: improve validation of pyproject.toml meson-python configuration #304

Conversation

dnicolodi commented Feb 9, 2023

dnicolodi commented Mar 1, 2023

FFY00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment