-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Prototype support for PEP 739 (continued) #14657
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
Conversation
f02c282
to
6c6ad29
Compare
88fa2cc
to
e716a1f
Compare
a587516
to
0559b4a
Compare
Okay, I think this is as far as I can go on my own. Few final points:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see it's ready for testing!
You may want to rewrite history already, since it's required in Meson development to have self-contained orthogonal commits before merging.
A few initial comments; I'll try to find time to test this asap!
mesonbuild/dependencies/python.py
Outdated
class PythonBuildConfig: | ||
"""PEP 739 build-details.json config file.""" | ||
|
||
"""Schema version currently implemented.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be a regular code comment (#
) it looks like. Same comment for 2 lines lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it was meant as a docstring, but according to PEP 258, attribute docstrings go below the assignment, not above. While it was rejected, it seems that tools support this convention and I see it used e.g. in mesonbuild/cargo/manifest.py
, so I'll just fix the location.
self.variables = python_holder.info['variables'] | ||
self.build_config = python_holder.build_config | ||
|
||
if self.build_config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I look at the multiple if self.build_config ... else
I wondered if it isn't better to combined build_config
and info
into the same structure in a single place. I bet you considered it though and decided against doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I haven't given it much thought, but they certainly do trigger different code paths, and contain slightly different information. I don't think trying to combine them would make the code more readable.
Do you happen to have a reusable recipe for this? |
This seems to work as expected, here is a set of CI jobs that cross-compiles SciPy from x86-64 to aarch64 in the first job, and then uses an aarch64 runner in the second job to download the built wheel and run the test suite:
I think there was a script to generate the |
Not really reusable, no. I've used Gentoo's crossdev to get the needed dependencies, i.e.:
As a result, I've been able to test on a `build-details.json`{
"schema_version": "1.0",
"base_prefix": "/usr",
"base_interpreter": "/usr/bin/python3.14",
"platform": "linux-aarch64",
"language": {
"version": "3.14",
"version_info": {
"major": 3,
"minor": 14,
"micro": 0,
"releaselevel": "beta",
"serial": 3
}
},
"implementation": {
"name": "cpython",
"cache_tag": "cpython-314",
"version": {
"major": 3,
"minor": 14,
"micro": 0,
"releaselevel": "beta",
"serial": 3
},
"hexversion": 51249331,
"_multiarch": "aarch64-linux-gnu"
},
"abi": {
"flags": [],
"extension_suffix": ".cpython-314-aarch64-linux-gnu.so",
"stable_abi_suffix": ".abi3.so"
},
"suffixes": {
"source": [
".py"
],
"bytecode": [
".pyc"
],
"extensions": [
".cpython-314-x86_64-linux-gnu.so",
".abi3.so",
".so"
]
},
"libpython": {
"dynamic": "/usr/lib/libpython3.14.so",
"dynamic_stableabi": "/usr/lib/libpython3.so",
"link_extensions": false
},
"c_api": {
"headers": "/usr/include/python3.14",
"pkgconfig_path": "/usr/lib/pkgconfig"
}
} Created a cross file: meson --cross-file[binaries]
c = 'aarch64-unknown-linux-gnu-gcc'
cpp = 'aarch64-unknown-linux-gnu-g++'
pkg-config = 'aarch64-unknown-linux-gnu-pkg-config'
[properties]
sys_root = '/usr/aarch64-unknown-linux-gnu'
pkg_config_libdir = '/usr/aarch64-unknown-linux-gnu/usr/lib/pkgconfig'
needs_exe_wrapper = true
longdouble_format = 'IEEE_QUAD_LE'
[host_machine]
system = 'linux'
cpu_family = 'aarch64'
cpu = 'aarch64'
endian = 'little' Merged this branch with #14773, copied
|
15c0394
to
a8bbbb1
Compare
I opened mesonbuild/meson-python#779 for the needed |
a8bbbb1
to
9071816
Compare
Signed-off-by: Filipe Laíns <[email protected]> Signed-off-by: Michał Górny <[email protected]>
9071816
to
1232ec1
Compare
Gentle ping. Should I do something else here? |
@@ -3055,6 +3056,121 @@ def test_pkg_config_libdir(self): | |||
self.wipe() | |||
self.init(testdir, extra_args=['-Dstart_native=true'], override_envvars=env) | |||
|
|||
@skipIf(is_osx(), 'Not implemented for Darwin yet') | |||
@skipIf(is_windows(), 'POSIX only') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there estimates on how much work would supporting Windows and Mac be? As in would it need architectural changes or is it merely busywork that someone would just need to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have neither a Windows nor macOS setup to work on that.
The Windows limitation comes from the original PR, and I don't really know the details — particularly, whether it's a limitation at spec level, code level or just test case.
I've added macOS skip due to CI failures. Apparently, Meson's test setup is pretty nonstandard, so I'm not even sure if it's actually a problem with the code or with the CI setup. There's also the following code:
meson/mesonbuild/dependencies/python.py
Lines 330 to 338 in e38545b
# But not Apple, because it's a framework | |
if self.env.machines.host.is_darwin() and 'PYTHONFRAMEWORKPREFIX' in self.variables: | |
framework_prefix = self.variables['PYTHONFRAMEWORKPREFIX'] | |
# Add rpath, will be de-duplicated if necessary | |
if framework_prefix.startswith('/Applications/Xcode.app/'): | |
self.link_args += ['-Wl,-rpath,' + framework_prefix] | |
if self.raw_link_args is not None: | |
# When None, self.link_args is used | |
self.raw_link_args += ['-Wl,-rpath,' + framework_prefix] |
I don't know how that is supposed to interact with PEP 739, and whether we shouldn't actually be fixing Python not to require these hacks (presuming that is even possible, I don't know how Frameworks work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just busywork, and mostly in the test case. The config file keys/values make sense on all platforms, the annoying thing is to construct a correct config file for an unknown interpreter. This test case uses /usr
, .so
, etc. which isn't correct for macOS and Windows. From Python 3.14, CPython starts shipping this json file with the interpreter itself, which will make testing in the future much easier.
In practice this is primarily useful for cross compiling though, so the main use cases are for Linux et al; cross compiling on macOS and Windows isn't much of a thing in comparison.
and whether we shouldn't actually be fixing Python not to require these hacks (presuming that is even possible, I don't know how Frameworks work).
That is a workaround for the python.pc
file shipped by Apple not containing -Wl,-rpath
and framework installs are not on the loader search path. The latter is on purpose, for security. The former seems like a bug in the .pc
file. It's bad practice to use the macOS system Python for anything as a user or application author, so it's mostly a don't-care I think.
I'd prefer for this to be merged, since it's easier to add to it later. But I can look into the macOS thing if you think that is needed now.
Thanks a lot! |
@@ -41,12 +41,13 @@ def set_program_override(pkg_bin: ExternalProgram, for_machine: MachineChoice) - | |||
PkgConfigInterface.pkg_bin_per_machine[for_machine] = pkg_bin | |||
|
|||
@staticmethod | |||
def instance(env: Environment, for_machine: MachineChoice, silent: bool) -> T.Optional[PkgConfigInterface]: | |||
def instance(env: Environment, for_machine: MachineChoice, silent: bool, | |||
extra_paths: T.Optional[T.List[str]] = None) -> T.Optional[PkgConfigInterface]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change of moving extra_paths
into the instantiation breaks detection of Python on platforms where we need to search in a custom pkg-config path, such as on macOS.
You're caching the instances in the class_impl
class variable, which means if you change extra_paths
it gets ignored. The instances should probably be keyed with a tuple of extra_paths too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project('testpy', 'c')
# Comment this out and the Python dependency below will be found
dependency('glib-2.0', required: false)
pymod = import('python')
python = pymod.find_installation('python3')
python_dep = python.dependency(embed: true, include_type: 'system')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a macOS machine to reproduce. I'm going to try to build an artificial environment to reproduce this — but if you feel like writing a patch, that will probably be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I didn't really reproduce a failure but I've reproduced the extra_paths
mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented your suggestion in #15001. I'm not particularly proud of that solution, so if anyone has a better idea…
Add `extra_paths` to cache keys for `PkgConfigInterface` and `PkgConfigCLI` instances, to avoid incorrectly reusing an instance with a different `extra_paths` value, see: mesonbuild#14657 (comment) Signed-off-by: Michał Górny <[email protected]>
Add `extra_paths` to cache keys for `PkgConfigInterface` and `PkgConfigCLI` instances, to avoid incorrectly reusing an instance with a different `extra_paths` value, see: mesonbuild#14657 (comment) Signed-off-by: Michał Górny <[email protected]>
This is a continuation of work from #13945, see #13945 (comment). For a start, just rebased it against fresh git head.