-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow users to target Py_LIMITED_API via python's extension_module #11745
Conversation
Fixed CI test issue by removing project's meson_version kwarg. |
For any future reviewers, there is a discussion here about this, which might be useful for context. |
Thanks for working on this, it is very welcome addition. However, I think that requiring to manually specify the dependency to the Python shared library is a step backward. Since quite a few meson versions (at least since 0.63.3) there is no need to add the dependency explicitly: Meson inspect the Python interpreter and determines if and how the Python shared library should be linked. I think the support for |
I share your opinion that this feels odd to have to manually specify the dependency. I wasn't sure how else to do it without having the logic spill into the lower levels of Meson. I had considered adding that logic in but figured I would try this solution first. I will work to remove this requirement to specify the dependency directly. Thank you for your feedback. |
mesonbuild/modules/python.py
Outdated
|
||
major = int(decimal_match.group(1)) | ||
minor = int(decimal_match.group(2)) | ||
decimal_version = f'{major}.{minor}' |
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.
Isn't the decimal version the initial value we require? We don't need to recalculate this or return it.
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.
(That is to say, assuming we actually validate it fully.)
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 has been removed. I was returned decimal_version
here as a cleaned up version of the input string. If a user supplied " 03.4 "
then decimal_version
would be '3.4'
. This was then used in an error message. This logic has been simplified.
mesonbuild/modules/python.py
Outdated
|
||
if 'gnu_symbol_visibility' not in kwargs and \ | ||
(self.is_pypy or mesonlib.version_compare(self.version, '>=3.9')): | ||
kwargs['gnu_symbol_visibility'] = 'inlineshidden' | ||
|
||
return self.interpreter.func_shared_module(None, args, kwargs) | ||
|
||
def _parse_limited_api_version(self, limited_api: str) -> (str, str): | ||
|
||
decimal_match = limited_api_decimal_format.match(limited_api) |
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.
AFAICT there is no convenient way to handle this without regex, oh well...
But we should still use fullmatch here. Without that, it's valid to pass limited_api: '3.9 all the way'
.
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 uses fullmatch
now.
mesonbuild/modules/python.py
Outdated
@@ -59,8 +61,11 @@ class FindInstallationKw(ExtractRequired): | |||
modules: T.List[str] | |||
pure: T.Optional[bool] | |||
|
|||
limited_api_decimal_format = re.compile(r'\b0*(\d)\.(\d+)\b') |
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.
And if we use fullmatch, we don't need to bother with \b
either.
Also while we're here, please avoid using \d
.
>>> re.match(r'\d', '𑶢')
<re.Match object; span=(0, 1), match='𑶢'>
>>> '𑶢'.isdigit()
True
>>> int('𑶢')
2
And also,
>>> _parse_limited_api_version('𑶣.𑶨')
('3.8', '0x03080000')
Use exactly what we mean and want -- [0-9]
, no more, no less.
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 updated the regex to be more explicit (avoiding \d
) and getting rid of \b
.
I have removed the I am currently working on a solution but I would like to get a sense of if my intended solution is a good one or if there is a simpler way of doing things: my solution is to make a copy of the supplied Python dependency object, modify the link_args of this copy to instead link to the limited api lib and to then add this copy to the link deps of the extension module. This seems like a hack to me, but I'm not sure how else to go about it? @eli-schwartz mentioned during the earlier discussion that this functionality could be a module option. I wasn't sure what that was until recently. If I were to make this functionality a module option would that mean that all Python installation/dependency objects found via a module with this option enabled would instead link to the limited API? Would that prevent users from declaring two different It's fun hacking on this but I hope all these questions are not a burden. I apologize if so. |
789c15c
to
8729b43
Compare
Any updates on this? I've rebased this PR against upstream master and will rework this functionality however the maintainers wish in order to get it ready. Feedback very welcome. |
See #11851 |
@eli-schwartz do you have time to come back and review this? |
Getting back to this:
I wonder if maybe instead, we can add the limited_api kwarg to We don't have to handle the case where users manually passed their own dependency object to extension_module -- because if they have done so, they are probably trying to be very exact with what they want, or else they should simply not do that for new functionality. We don't worry about the possibility that they get the |
My thought here is that we may want a module option to suppress use of the limited_api, that is to say, modules which don't say they use the limited API would continue to not use it -- but modules which can use the limited API, default to using it and allow disabling this by setting a meson option like We can't opt modules in to using the limited API if they don't support it. We can opt modules out if they do support it, and this can result in more optimized code at the cost of, well, using more than just the limited API. :) ... Anyways, this idea can be ignored for an MVP and added later on, if it's too complicated. |
@bruchar1 you may be interested in testing this PR |
Apologies for not responding sooner, work has been busy recently. I admit that it's been a while since I looked at this PR, so I will re-acquaint myself with its changes and give my response to recent comments and feedback as soon as possible.
Forgive me if I'm wrong here but would this mean that, if not explicitly disabled via this module option, the limited API kwarg would always be set, either to some default value or to whatever the user supplied, and therefore Py_LIMITED_API would always be defined? Is that right? |
mesonbuild/modules/python.py
Outdated
|
||
if limited_api_version != '': | ||
|
||
has_cython_files = any(source_file.endswith('.pyx') for source_file in args) |
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.
source_file
is not necessarily a str
. It could be a File
, a CustomTarget
, etc. You cannot use .endswith
directly on it.
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.
Thanks for finding this!
I'm prototyping a fix to this and I am wondering if something like this would get the job done:
def check_for_cython_files(args):
for arg in args:
if isinstance(arg, mesonlib.File) and arg.endswith('.pyx'):
return True
elif isinstance(arg, build.BuildTarget) and check_for_cython_files(arg.get_sources()):
return True
return False
Although maybe there exists functionality already for doing something like this that I'm not aware of?
I admit that the Cython support for this is considerably less well tested currently than raw C/C++. Cython's support for the Python Limited API is currently limited to the upcoming 3.0 release, which I wasn't able to find packaged anywhere that allowed easy testing. It might therefore be a good idea to just remove this functionality until this version of Cython is more widely available. I have no strong feelings either way, and will do whatever is amenable to maintainers.
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 think you want get_outputs, actually. A BuildTarget won't be supported at all here -- those are executable() or library() return values, and get_sources would be the inputs to the executable, rather than the inputs to py.extension_module. The concern here is CustomTarget instead, which might be used to produce .c or .pyx files at build time. You do still have to check for str.endswith as well.
It may of course not be worth making this check at all -- what's the outcome of a failure here?
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 may of course not be worth making this check at all -- what's the outcome of a failure here?
The check exists to detect if we have any cython files as input so that we can then check to see if the version of Cython supports the limited API. If it doesn't we can then emit a warning. So the outcome of failure here (defined as failing to correctly detect the presence of cython files in the input to this extension module) would mean that a warning is not emitted when it should be.
Is the value of this warning worth the complexity of this task?
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.
Let's just say that I would in no way consider this any kind of blocker, especially if it's possible to get a false positive.
Pseudocode: kwargval = get_function_kwarg('limited_api')
moduleval = get_cli_option('allow_limited_api')
if moduleval is True and kwargval is not None:
finalval: str = kwargval
else:
finalval = None
So the default value would be "do not define Py_LIMITED_API" |
Beside the bug I mentionned with cython detection, it seems to work with my cffi project on Windows. Should It would be great if limited api could be automatically detected, at least for cffi extensions... |
Is that not what limited_api_suffix already does? |
No. This would be very complicated and then only work for source files checked into git which also trigger full meson reconfiguration whenever the source file was edited.
How would this work? Usually get_define tries to |
Oh. The limited api suffix on Windows is |
@@ -43,6 +43,7 @@ class PythonIntrospectionDict(TypedDict): | |||
paths: T.Dict[str, str] | |||
platform: str | |||
suffix: str | |||
limited_api_suffix: str |
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.
The documentation calls this limited_api
, here it is limited_api_suffix
. They should be aligned.
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 is part of the introspection data gathered from the interpreter the extension is being built for, it does not indicate whether the extension is built for the limited API or not. I think it is fine as it is.
mesonbuild/dependencies/python.py
Outdated
@@ -196,7 +198,7 @@ def __init__(self, name: str, environment: 'Environment', | |||
if self.link_libpython: | |||
# link args | |||
if mesonlib.is_windows(): | |||
self.find_libpy_windows(environment) | |||
self.find_libpy_windows(environment, False) |
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.
can you use the keyword variant for clarity: limited_api=False
mesonbuild/modules/python.py
Outdated
# https://doc.pypy.org/en/latest/cpython_differences.html#permitted-abi-tags-in-extensions | ||
if self.is_pypy: | ||
self.limited_api_suffix = self.suffix | ||
|
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! I came here to mention this and it was already done.
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 looks good and solves some thorny problems. Thanks! A couple of meaningless comments, mostly about why meson needs to deal with import libs at all when using MSVC.
@@ -277,6 +279,8 @@ def get_windows_link_args(self) -> T.Optional[T.List[str]]: | |||
else: | |||
libpath = Path(f'python{vernum}.dll') | |||
else: | |||
if limited_api: | |||
vernum = vernum[0] |
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 will work until the major version goes above 9. Nit: you could use verdot.split(".")[0]
instead, but probably other things will break before this becomes a problem.
# When compiled under MSVC, Python's PC/pyconfig.h forcibly inserts pythonMAJOR.MINOR.lib | ||
# into the linker path when not running in debug mode via a series #pragma comment(lib, "") | ||
# directives. We manually override these here as this interferes with the intended | ||
# use of the 'limited_api' kwarg |
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 is a shame you need this. I think the intention of the header was deal with linking the correct import lib, and meson should ideally not specify any link arguments to deal with the import lib at all. But it seems the header does not properly link to the "limited api + debug" import lib. I opened python/cpython#107585 to clarify 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.
Ideally meson would read a sysconfig setting specifying which import lib to link to, and use that. Doing this via MSVC specific pragmas is painful, weird, and bad, for a variety of reasons but perhaps the simplest and most persuasive one is that it cripples meson's ability to notice a bug in CPython and add a quirk like this to work around the problem.
You should see the crimes against computing that pybind11 does to allow building a python extension with debug symbols for user code without forcing to link against the debug version of python311.dll -- meson simply spawns a scary warning if you try to mix them.
Hot take: the computing industry would be better off if that MSVC pragma had not been invented.
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 also regret that this was necessary but it does seem that CPython's pyconfig.h
is missing a bit of logic here.
Back when I was starting this work I recall finding a PR in CPython which aimed to fix the logic in PC/pyconfig.h
, allowing a correct and peaceful coexistence between the Py_LIMITED_API
and _DEBUG
macros, but after searching for it again just now I can't seem to find it. That's frustrating: if I'm unable to find it again then I will work on a fix for CPython.
That being said, even if a fix makes it into CPython, I think Meson should probably still aim to support users with older versions of CPython that are already in the wild, right?
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.
Precisely.
if sys.version_info >= (3, 2): | ||
try: | ||
from importlib.machinery import EXTENSION_SUFFIXES | ||
limited_api_suffix = EXTENSION_SUFFIXES[1] |
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.
There is a new API design for sysconfig, I commented on the compile-specific issue
Oof, I was looking at what else needs to be addressed and realised that I haven't yet addressed this. I am working on this now and have two questions:
|
|
0099efd
to
5928a51
Compare
Thanks for your feedback. This has been implemented and I've pushed the change up to this PR. Let me know what you think. |
mesonbuild/modules/python.py
Outdated
# into the linker path when not running in debug mode via a series #pragma comment(lib, "") | ||
# directives. We manually override these here as this interferes with the intended | ||
# use of the 'limited_api' kwarg | ||
c_compiler = detect_c_compiler(self.interpreter.environment, MachineChoice.BUILD) |
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.
Instead of detect_c_compiler we should assume the user has specified one via the project languages or add_languages()
, and look it up in environment.coredata.compilers[for_machine]
.
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.
Implemented, thanks for this feedback in particular. I remember being unsure if this was a reliable way to get this information or not.
mesonbuild/modules/python.py
Outdated
new_c_args = mesonlib.extract_as_list(kwargs, 'c_args') | ||
new_c_args.append(f'-DPy_LIMITED_API={limited_api_version_hex}') | ||
kwargs['c_args'] = new_c_args |
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.
We should try appending it for cpp_args 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.
Implemented, thanks for the feedback.
mesonbuild/modules/python.py
Outdated
if mesonlib.version_compare(limited_api_version, '<3.2'): | ||
raise InvalidArguments(f'Python Limited API version invalid: {limited_api_version} (must be greater than 3.2)') | ||
if mesonlib.version_compare(limited_api_version, '>' + pydep.version): | ||
raise InvalidArguments(f'Python Limited API version too high: {limited_api_version} (detected {pydep.version})') |
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'd probably move this into the converter function.
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.
Implemented, thanks again for the feedback.
fb9780d
to
93dccfd
Compare
mesonbuild/modules/python.py
Outdated
# use of the 'limited_api' kwarg | ||
for_machine = self.interpreter.machine_from_native_kwarg(kwargs) | ||
compilers = self.interpreter.environment.coredata.compilers[for_machine] | ||
if any(compiler.get_id() == 'msvc' for _, compiler in compilers.items()): |
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.
if any(compiler.get_id() == 'msvc' for _, compiler in compilers.items()): | |
if any(compiler.get_id() == 'msvc' for compiler in compilers.values()): |
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.
Thanks, implemented!
93dccfd
to
a3ae2a6
Compare
@@ -62,8 +62,7 @@ class ExtensionModuleKw(SharedModuleKw): | |||
|
|||
subdir: NotRequired[T.Optional[str]] | |||
|
|||
|
|||
mod_kwargs = {'subdir'} | |||
mod_kwargs = {'subdir', 'limited_api'} |
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.
Can you leave the empty line in there?
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.
Sure, done.
if any(compiler.get_id() == 'msvc' for compiler in compilers.values()): | ||
pydep_copy = copy.copy(pydep) | ||
pydep_copy.find_libpy_windows(self.env, limited_api=True) | ||
if not pydep_copy.found(): | ||
raise mesonlib.MesonException('Python dependency supporting limited API not found') | ||
|
||
new_deps.remove(pydep) | ||
new_deps.append(pydep_copy) |
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 was trying to think whether there was value to making this part of py_inst.dependency(limited: ...)
but after thinking about it a bit more I think the way you've done it is fine.
I think it's fairly unlikely that anyone will ever want to touch the limited API for reasons other than building an extension_module, so we do not need to make this directly exposed.
docs/markdown/Python-module.md
Outdated
@@ -101,6 +101,11 @@ the addition of the following: | |||
`/usr/lib/site-packages`. When subdir is passed to this method, | |||
it will be appended to that location. This keyword argument is | |||
mutually exclusive with `install_dir` | |||
- `limited_api`: *since 1.3.0* A string containing the Python version | |||
of the [Py_LIMITED_API](C-https://docs.python.org/3/c-api/stable.html) 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.
This link looks odd, is the C-
part a typo?
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.
Ah yes, this is definitely a typo on my part. I've fixed this.
mesonbuild/scripts/python_info.py
Outdated
@@ -76,4 +90,5 @@ def links_against_libpython(): | |||
'is_venv': sys.prefix != variables['base_prefix'], | |||
'link_libpython': links_against_libpython(), | |||
'suffix': suffix, | |||
'limited_api_suffix': limited_api_suffix |
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.
Please end this with a comma :D that way the next person to update this doesn't have to change two lines.
Although we are dumping json, this file isn't actually json, which means we can do sensible things like end an entry with a comma even when commas aren't mandatory -- biggest gripe against json, a trailing comma is outright illegal. :(
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.
Done!
58bc6d1
to
88ad193
Compare
I had a bit of a nightmare last night about this Windows linking hack that I'm proposing in this PR and so have added an extra bit to the test case to ensure that the hack actually does what I intend: linking to the right version of the library on Windows and also not impacting future invocations of Let me know if this is just paranoia on my part. |
@@ -0,0 +1,10 @@ | |||
project('Python limited api disabled', 'c', | |||
default_options : ['buildtype=release', 'werror=true', 'python.allow_limited_api=False']) |
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.
If you have to do another push based on other comments. Don't use a capital F for false.
How do the Meson maintainers feel about the current state of this PR? Is there a consensus that it's ready or that it needs a bit more work before it's ready? Apologies if I have missed any feedback that has been left so far. |
I'm feeling pretty good about it and will do a final review today. Can you fix the minor typo pointed out above while you wait? |
This commit adds a new keyword arg to extension_module() that enables a user to target the Python Limited API, declaring the version of the limited API that they wish to target. Two new unittests have been added to test this functionality.
88ad193
to
86049de
Compare
Done. |
Missed reviewing this over the weekend, sorry. :D Thanks for this fantastic work. |
Many thanks to all reviewers for their feedback! I aim to follow up on the Cython stuff when a version of Cython that supports the limited API is no longer in beta. |
FWIW, the limited API in cython is triggered by setting |
Thanks for the nudge, I've started to implement this now. I will push a PR soon and invite feedback. |
What does this PR do?
This PR adds the ability for a user to target the [Python Limited API] directly in the build system by supplying a new, optional keyword argument to the
extension_module
function on thepython_installation
object. For example:Under the hood, Meson takes the
'3.6'
argument and converts it to a hex value corresponding to the equivalentPY_VERSION_HEX
. It then sets the value of thePy_LIMITED_API
preprocessor directive to this value.There is an extra complexity on Windows: when targeting Python's limited API on Windows one must link to
python3.dll
and notpython3.X.dll
. In order to allow enable this, thedependency
function onpython_installation
has gained thelimited_api
boolean keyword argument. When supplied, this causes the dependency to link topython3.lib
andpython3.dll
on Windows instead of the unstable API versions. This new kwarg only has an effect on Windows.So a full example, including Windows:
Motivation
Targeting the Python limited API allows an extension to be loaded in any version of the Python interpreter that supports that API, simplifying compatibility.
Notes
I had to teach the testsuite how to match for installed python modules which target the limited API. The testsuite file matching works by checking the file extension and limited API modules have a different extension.
Let me know what you think.