Skip to content
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

ENH: make the Meson executable user-configurable #495

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

rgommers
Copy link
Contributor

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 of pyproject.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 from numpy, as tested in this branch with these changes:

 [build-system]
 build-backend = "mesonpy"
-backend-path = ['./vendored-meson/meson-python']
 requires = [
     "Cython>=3.0",
-    # All dependencies of the vendored meson-python (except for meson, because
-    # we've got that vendored too - that's the point of this exercise).
-    'pyproject-metadata >= 0.7.1',
-    'tomli >= 1.0.0; python_version < "3.11"',
-    'setuptools >= 60.0; python_version >= "3.12"',
-    'colorama; os_name == "nt"',
-    # Note that `ninja` and (on Linux) `patchelf` are added dynamically by
-    # meson-python if those tools are not already present on the system. No
-    # need to worry about those unless one does a non-isolated build - in that
-    # case they must already be installed on the system.
+    "meson-python @ git+https://github.com/rgommers/meson-python.git@meson-cli-configurable"
 ]
 
+[tool.meson-python]
+cli = "vendored-meson/meson/meson.py"

If this looks good, it will still need docs before it's ready.

@rgommers rgommers added the enhancement New feature or request label Sep 10, 2023
@rgommers rgommers marked this pull request as draft September 10, 2023 20:54
Copy link
Member

@dnicolodi dnicolodi left a comment

Choose a reason for hiding this comment

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

Thanks @rgommers This implements what we discussed already and it is a good feature to have. The only important comment I have is regarding the option name. I think cli is not the most descriptive name and meson would be better.

In the future we could think about extending this to allow to specify the path to ninja and patchelf too. In the case of patchelf we could even allow the path to be false to disable requiring patchelf.

In this hypothetical future,

[tool.meson-python]
meson = 'whatever/meson.py'
patchelf = false

looks better than the version with cli in it.

The other comments are just variable names nitpicking. Sorry for being pedantic about these, but I find code much easier to read if variables are uniformly named.

mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
mesonpy/__init__.py Outdated Show resolved Hide resolved
tests/packages/vendored-meson/meson.build Show resolved Hide resolved
@rgommers
Copy link
Contributor Author

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.

@dnicolodi
Copy link
Member

@rgommers Thinking about it, there is one problem with this patch: meson is listed as a dependency of meson-python

'meson >= 0.63.3',

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?

@rgommers
Copy link
Contributor Author

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.

@dnicolodi
Copy link
Member

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.

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 of pyproject.toml

Closes mesonbuildgh-458
@rgommers rgommers added this to the v0.15.0 milestone Sep 12, 2023
@rgommers rgommers marked this pull request as ready for review September 12, 2023 10:06
@rgommers
Copy link
Contributor Author

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.

@rgommers
Copy link
Contributor Author

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?

@dnicolodi
Copy link
Member

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

select = [
'B', # flake8-bugbear
'C4', # flake8-comprehensions
'C9', # mccabe
'E', # pycodestyle
'F', # pyflakes
'W', # pycodestyle
'RUF100', # ruff
]
then I think ruff will complain that some local disables are unneeded and will need to be removed.

@dnicolodi
Copy link
Member

LGTM! Thanks for the revision @rgommers. I have just a couple of non critical comments.

It has given issues a couple of times that take time to resolve,
and the complaints it emits seem unjustified at least in some
cases. So remove it - relying on code review is fine here.
@rgommers
Copy link
Contributor Author

I would be inclined in disabling the check all together.

Agreed, done in the last commit.

@rgommers
Copy link
Contributor Author

All other changes adopted, I'll resolve those comments.

@dnicolodi
Copy link
Member

Thanks @rgommers

@dnicolodi dnicolodi merged commit 1e79b26 into mesonbuild:main Sep 12, 2023
36 checks passed
@rgommers rgommers deleted the meson-cli-configurable branch January 18, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants