BUG: improve validation of pyproject.toml meson-python configuration #304
Conversation
dnicolodi
force-pushed
the
pyproject-toml
branch
from
aaaba53 to
feccd5a
Compare
dnicolodi
force-pushed
the
pyproject-toml
branch
from
feccd5a to
cf6cf99
Compare
dnicolodi
force-pushed
the
pyproject-toml
branch
10 times, most recently
from
fe7904b to
e0363cc
Compare
dnicolodi
force-pushed
the
pyproject-toml
branch
3 times, most recently
from
e1962a5 to
1947798
Compare
|
We need this (or something similar) to fix #293, ideally before the next release. |
dnicolodi
force-pushed
the
pyproject-toml
branch
from
1947798 to
517f4e8
Compare
| assert isinstance(name, str) | ||
| return name.replace('-', '_') | ||
| """Project name.""" | ||
| return str(self._metadata.name).replace('-', '_') |
There was a problem hiding this comment.
str is not needed here. The assert was because mypy was complaining because of the import IIRC.
There was a problem hiding this comment.
mypy in CI disagrees with you https://github.com/mesonbuild/meson-python/actions/runs/4359109213/jobs/7620540144#step:5:13
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's the version of mypy used in the CI, later versions of mypy do not complain
mesonpy/__init__.py
Outdated
|
|
||
| def _string(value: Any, name: str) -> str: | ||
| if not isinstance(value, str): | ||
| raise ConfigError(f'only one value for "{name}" can be specified') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No, it can be only a string or a list of strings. It is written in the PEP
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_settingis 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
dnicolodi
force-pushed
the
pyproject-toml
branch
4 times, most recently
from
91f3766 to
025e022
Compare
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.
dnicolodi
force-pushed
the
pyproject-toml
branch
from
025e022 to
da119d1
Compare
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
da119d1 to
b5aeea9
Compare
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.