Python dependency refactor #11392
Python dependency refactor #11392
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11392 +/- ##
==========================================
+ Coverage 66.53% 68.90% +2.36%
==========================================
Files 414 209 -205
Lines 90562 45301 -45261
Branches 20100 9367 -10733
==========================================
- Hits 60258 31215 -29043
+ Misses 25720 11692 -14028
+ Partials 4584 2394 -2190 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
mesonbuild/modules/python.py
Outdated
| @@ -428,6 +431,7 @@ def __init__(self, name: str, command: T.Optional[T.List[str]] = None, | |||
| 'sysconfig_paths': {}, | |||
| 'paths': {}, | |||
| 'platform': 'sentinal', | |||
There was a problem hiding this comment.
Not part of the patch, but this looks like a typo.
mesonbuild/dependencies/python.py
Outdated
|
|
||
|
|
||
| class Python3DependencySystem(SystemDependency): | ||
| def __init__(self, name: str, environment: 'Environment', kwargs: T.Dict[str, T.Any]) -> None: |
There was a problem hiding this comment.
I see that you are just moving code around and I don't know what the the Meson coding style says about this, but if you have from __futute__ import annotations using string for the annotations is redundant as they are not resolved when not type checking anyway. Namely, this could be environment: Environment.
| # https://sourceforge.net/p/mingw-w64/mailman/message/30504611/ | ||
| if mesonlib.is_windows() and self.get_windows_python_arch() == '64' and self.major_version == 2: | ||
| self.compile_args += ['-DMS_WIN64'] | ||
|
|
There was a problem hiding this comment.
Again, not related to the refactoring, but compiling Cython extension modules with GCC on Windows requires to add this to meson.build also for Python 3:
if host_machine.system() == 'windows' and meson.get_compiler('c').get_id() == 'gcc'
add_project_arguments('-DMS_WIN64', language: ['c', 'cpp'])
endiftherefore I think the and self.major_version == 2 guard should be dropped.
There was a problem hiding this comment.
I really have no clue how that works, but I guess that can be done... in a separate commit. ;)
@MathieuDuponchelle, do you happen to remember why you originally guarded it by version 2.x, way back in commit e1b138a?
There was a problem hiding this comment.
Hey, I should have documented that part specifically but only added the thread that justified adding -DMS_WIN64, I have absolutely no clue at this point why I added the python 2 restriction, sorry about that.
There was a problem hiding this comment.
No big deal, looks like it may have just been due to unclear wording in the linked message...
I bet @rgommers knows a lot about the topic anyway and can confirm the correctness of such a change.
There was a problem hiding this comment.
The SciPy code comment points at python/cpython#72454. There's a bunch of MinGW patches that are needed but were never accepted by CPython (they basically never cared much about anything but MSVC). It looks like it was very recently resubmitted and merged (python/cpython#100137), so it should be fixed for Python 3.12.0. So the define is needed for Python 3 up to today, and hence dropping the == 2 sounds good to me.
And, just to be sure: if -DMS_WIN64 is added twice (once by Meson, once by e.g. SciPy), that won't hurt will it?
There was a problem hiding this comment.
Thanks for the context -- that's wonderfully actionable and gives me a lot of confidence about what to do here.
And yes, double-defining it should be fine, the definitions are compatible so it's a 10-byte no-op.
|
A couple of base commits are shared in common with both PRs, yes. As you can see, there's a bunch of cleanups here I'm trying to do. :D As far as merging it goes, I still have both my PRs to CPython open, and I was sort of hoping to get confirmation that that's what we're supposed to do before merging that same logic but ported to our introspection script. I haven't heard back yet, though. |
mesonbuild/dependencies/python.py
Outdated
| @@ -13,18 +13,95 @@ | |||
| # limitations under the License. | |||
| from __future__ import annotations | |||
|
|
|||
| import json, sysconfig | |||
There was a problem hiding this comment.
Can we please keep these split into:
import json
import sysconfig| import typing as T | ||
|
|
||
| from .. import mlog | ||
| from .. import mesonlib, mlog |
There was a problem hiding this comment.
The thing is... immediately below, we do exactly the same thing with e.g.
+from .base import process_method_kw, DependencyMethods, DependencyTypeName, ExternalDependency, SystemDependencyI really find it more natural and idiomatic to do this, and I don't see the point in spreading imports across one bound name per line. That just becomes difficult to read and difficult to scroll past.
There was a problem hiding this comment.
Idiomatically modules are imported one per line, objects from modules are imported on a single line:
import os
import sys
from os.path import join, dirname, basenamehttps://peps.python.org/pep-0008/#imports
I get it, style guides always have something in them that annoys people. But having a single styl, makes the whole codebase much more readable.
There was a problem hiding this comment.
And this is from foo import bar, baz...
I'm really unhappy that people keep campaigning to make ugly code, the same people who defend "modules are imported one per line" are defending "objects from modules are imported one per line" and for the same reason.
This is nothing to do with style. I've already stated my strong objections to "add a lint rule to force objects from modules to be imported one per line because I think it's better for rebasing". What I actually want is a lint rule that forbids that.
eli-schwartz
force-pushed
the
python-dependency-refactor
branch
from
5966787 to
aec6e3f
Compare
Of particular note: We technically cannot pass a NotFoundDependency out of a factory lookup, because it doesn't support External interfaces.
…ion objects We pass around a PythonInstallation into the depths of the dependency factory, solely so that we can get at is_pypy in one particular branch. We don't need the DSL functions, we don't need access to the interpreter, let's just use the enhanced ExternalProgram object on its own. Add is_pypy to the object, and modify other references to get information from .info['...'] instead of direct access.
We write this out as an embedded string to a tempfile in order to run it, which is pretty awkward. And usually Meson's files are already files on disk, not packed into a zip, so we can simply run it directly. Since python 3.7, which is our new minimum, we can handle this well via the stdlib. (There's also mesonbuild.mesondata, but we do not need persistence in the builddir.) This also solves the problem that has always been there, of giant python programs inside strings occasionally confusing syntax highlighters. Or even, it would be nice if we had syntax highlighting for this introspection program. :D
It can go directly inside the function which immediately uses it. There's no purpose in looking it up exactly once and using it exactly once, but looking it up outside the function and complicating the function signature in order to pass it as a function argument.
In the commit that originally added this import, it wasn't even used. Now the implementation is being moved, so it will fail to work. I do not know why I originally added it, but it needs to go. :)
In preparation for handling more work inside dependencies.*, we need to be able to run a PythonExternalProgram from the python dependency. Move most of the definition -- but only the parts that have no interest in a ModuleState -- and subclass a bit of sanity checking that we need to handle specially when used in the module.
In preparation for wholly merging the dependency handling from the python module into dependencies.*, move the unique class definitions from there into their new home in dependencies.python, which is semantically convenient.
We have two copies of this code, and the python module one is vastly superior, not just because it allows choosing which python executable to base itself on. Unify this. Fixes various issues including non-Windows support for sysconfig, and pypy edge cases.
We can set it once instead of tangling it inside the guts of the overly specialized library lookups. It is mostly identical anyway.
…config We may or may not care that the library can be found. If link_libpython is false, we allow it to be missing and still find the dependency.
Only search for and provide linkage to libpython, if the dependency expects to be linked to it. Fixes overlinking on Linux / macOS when pkg-config isn't installed and the sysconfig lookup is used instead. This was correctly handled for pkg-config rather than deferring it until use, since commit bf83274 -- but that handling neglected to cover sysconfig dependencies. And sysconfig would always try to link to libpython, it just respected the dependency configuration barely enough to allow falling back to "don't link" if both link_libpython=False and the library wasn't found.
This workaround was never exclusive to python2, and in fact only just got fixed in the upcoming python 3.12 release. Extend the version comparison to cover all those other cases.
eli-schwartz
force-pushed
the
python-dependency-refactor
branch
from
aec6e3f to
b4e792f
Compare
This is now done by Meson itself, after mesonbuild/meson#11392.
This moves the python dependency factory out of modules/python.py and into dependencies/python.py. As a result, we get some consistency -- particularly, bugfixes! for the sysconfig resolver for
dependency('python3'). It's also a lot cleaner, and easier to read modules/.Finally, fix bad overlinking in the sysconfig dependency.