Closed rgommers closed 1 year ago
Thanks for the review @dnicolodi. All you comments seem good, and I like the meson
instead of cli
name and potential future extension to ninja
/patchelf
. I'll update the code and will add some docs.
@rgommers Thinking about it, there is one problem with this patch: meson
is listed as a dependency of meson-python
https://github.com/mesonbuild/meson-python/blob/b4d42400b06f0f23420048dfe46e71a66f7fe54d/pyproject.toml#L36
thus, even when this mechanism is used to point to an alternative meson implementation, the meson dependency is installed or required to be installed when not using build isolation. Should the meson dependency removed from the static dependencies and returned from get_requires_for_build_wheel()
if meson
is not specified in tool.meson.python
or the env var is not set?
Hmm, good question. I think it'd be better to leave it as a static build dependency. Overriding it should be quite rare, and in those cases there's still little downside to having meson
in the build requirements, it just doesn't get used. Not having it there will have more downsides, like making it impossible for recipe generators and for dependency analysis tools like libraries.io / GitHub dependencies to understand the dependency. And also it will then not be possible to have a tool install dependencies from pyproject.toml
to set up a build/dev env. I'm still hoping pip
will gain that ability at some point - but if not, there's already hacky versions out there that can do this.
I think ninja
is different, because it's not a Python dependency but a system one. Plus there are two good alternatives, samurai is a great drop-in replacement.
Keeping the meson
dependency is fine for me. I just wanted to make sure that we were not forgetting about it and keeping it by accident instead than by design.
Hmm, not sure what to think of this pre-commit complaint:
mesonpy/__init__.py:
509:5 C901 `_validate_pyproject_config` is too complex (13 > 12)
It looks pretty clean to me.
There's a max-complexity
setting for ruff
in pyproject.toml
. I'm inclined to jump change it from 12 to either 13 or a higher number. @dnicolodi WDYT?
We hit this questionable linting error also elsewhere. In that case it is disabled locally with a comment. I would be inclined in disabling the check all together. Just remove C9
from the list here https://github.com/mesonbuild/meson-python/blob/b4d42400b06f0f23420048dfe46e71a66f7fe54d/pyproject.toml#L82-L90 then I think ruff
will complain that some local disables are unneeded and will need to be removed.
LGTM! Thanks for the revision @rgommers. I have just a couple of non critical comments.
I would be inclined in disabling the check all together.
Agreed, done in the last commit.
All other changes adopted, I'll resolve those comments.
Thanks @rgommers
Allow using a MESON environment variable, analogous to the NINJA one, or a string to an executable or Python script in the
[tool.meson-python]
section ofpyproject.toml
.Addresses gh-458.
Package authors are likely going to need/prefer the
tool.meson-python
setting, as discussed in gh-458. For local development or user overrides the environment variable will be useful; it is implemented analogous to the NINJA one.This will allow me to unvendor
meson-python
fromnumpy
, as tested in this branch with these changes:If this looks good, it will still need docs before it's ready.