-
Notifications
You must be signed in to change notification settings - Fork 917
Modify the handing of Frameworks on Apple platforms #5606
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
|
@davidhewitt I did some digging looking for the appropriate place to document this, but it's not entirely clear to me what that would be. The only references to linking libPython that I could find were in the macOS section of I'm not sure if that was what you were referring to about being able to deprecate regarding macOS? That includes reference to Any pointers or assistance here would be appreciated. It also took a couple of tries to fix some odd edge cases that didn't show up in local testing - an empty framework definition wasn't being handled consistently as "no framework" AFAICT, plus a linking error on Linux that I think I've handled correctly. |
At present, when targeting an `abi3` build, Maturin will use a dummy interpreter to provide build configuration details, unless the user specifies an interpreter with `--interpreter`, or the target is cross-compiling, or Windows is being targeted. The use of a dummy interpreter when a real interpreter is available can lead to the interpreter details that are available and defined in `sysconfigdata` not being used as part of the build process. This was discovered in the process of working on iOS support (see #2827 and #2828); although the iOS build environment *does* provide build configuration details, those details were not being passed down to the PyO3 build configuration. With those two patches (and some patches to PyO3 - see PyO3/pyo3#5605 and PyO3/pyo3#5606), it was possible to build a non-abi3 wheel; but this patch was needed to build an abi3 wheel.
davidhewitt
left a comment
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 this, overall looks good to me, just a few small thoughts.
I'm not sure if that was what you were referring to about being able to deprecate regarding macOS? That includes reference to
add_python_framework_link_args()- but in the context of macOS builds, I'm not sure how this would be turned on, since linking against the framework will be disabled on macOS.
add_python_framework_link_args() was indeed what I was thinking of, but having tested locally I think it's still necessary even with this PR.
| /// The name of the Python framework (if available) | ||
| /// | ||
| /// Serialized to `framework`. |
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 we should note that if this is set, this will be used for linking instead of shared and lib_name.
pyo3-build-config/src/impl_.rs
Outdated
| let python_framework_prefix = sysconfigdata | ||
| .get_value("PYTHONFRAMEWORKPREFIX") | ||
| .map(str::to_string); |
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 wonder if we should do something similar to filter out cases where the prefix is an empty string? Maybe if that is semantically no different from None, we should consider removing the Option outer layer from these variables and just treat the empty string as unset?
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 I'd lean to filtering out empty PYTHONFRAMEWORKPREFIX examples; I can't speak with much Rust experience, but as a Python user, an explicit "None" makes more sense to me than ignoring an empty string. I'll push an update to that effect.
davidhewitt
left a comment
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.
Great, thanks for this!
| "cargo:rustc-link-lib={link_model}{alias}{lib_name}", | ||
| link_model = if interpreter_config.shared { | ||
| link_model = if interpreter_config.framework.is_some() { | ||
| "framework=" |
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 the 3.11 CI failure has found a problem with this approach.
Seems like framework=Python does not specify the version, and nor does the search prefix which is just /Library/Frameworks (for the official distributions).
$ /Library/Frameworks/Python.framework/Versions/3.13/bin/python3 -m sysconfig | grep -i PYTHONFRAMEWORK
PYTHONFRAMEWORK = "Python"
PYTHONFRAMEWORKDIR = "Python.framework"
PYTHONFRAMEWORKINSTALLDIR = "/Library/Frameworks/Python.framework"
PYTHONFRAMEWORKINSTALLNAMEPREFIX = "/Library/Frameworks/Python.framework/Versions/3.13"
PYTHONFRAMEWORKPREFIX = "/Library/Frameworks"
Not sure what implications this has for the iOS build either? Seems like it's fundamentally impossible to use frameworks and link the right version.
I wonder, maybe we don't need this PR? If cross-compile env has the correct lib_dir, potentially PyO3 main is now already doing the correct thing to link against iOS via the versioned .dylib directly with the other patches placed into PyO3 and maturin?
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 the 3.11 CI failure has found a problem with this approach.
Which failure is this? I'm seeing 11 skips, 32 successful tests in the most recent CI pass.
Seems like
framework=Pythondoes not specify the version, and nor does the search prefix which is just/Library/Frameworks(for the official distributions).$ /Library/Frameworks/Python.framework/Versions/3.13/bin/python3 -m sysconfig | grep -i PYTHONFRAMEWORK PYTHONFRAMEWORK = "Python" PYTHONFRAMEWORKDIR = "Python.framework" PYTHONFRAMEWORKINSTALLDIR = "/Library/Frameworks/Python.framework" PYTHONFRAMEWORKINSTALLNAMEPREFIX = "/Library/Frameworks/Python.framework/Versions/3.13" PYTHONFRAMEWORKPREFIX = "/Library/Frameworks"Not sure what implications this has for the iOS build either? Seems like it's fundamentally impossible to use frameworks and link the right version.
So - macOS frameworks and iOS frameworks (and every other non-macOS framework) are structured slightly differently.
macOS frameworks are versioned - there's a single top-level framework, with a "Versions" subfolder; each version of the framework is then contained in the top level framework. A Current symlink exists to point at one specific version, and the top level framework links the content of the current version so that /Library/Frameworks/Python.framework can be used for linking. However, that doesn't give you control over the specific version. If you want a specific version, you need to use -F /Library/Frameworks/Python.framework/Versions/3.13 as your framework search path.
I wonder, maybe we don't need this PR? If cross-compile env has the correct
lib_dir, potentially PyO3mainis now already doing the correct thing to link against iOS with the other patches placed into PyO3 and maturin?
So... that doesn't currently work, because iOS doesn't ship a copy of libPython*.dylib in the lib folder. There's a range of reasons why that is the case... but with some other accomodations, that might indeed be an easier way to solve the problem. I'll experiment with this and report back.
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.
In the merge queue https://github.com/PyO3/pyo3/actions/runs/19310395011/job/55228919449
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.
you need to use -F /Library/Frameworks/Python.framework/Versions/3.13 as your framework search path.
I think I attempted this yesterday and had no luck. Possibly because Python.Framework only exists in /Library/Frameworks and not in any of the versioned subdirectories?
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 added CI-build-full label to this PR so any further pushes will run the full suite of versions we run in the merge queue.
|
I've experimented, and you're correct - if we put a I've submitted this fix upstream; it will take a while to propegate that change to all the places where it needs to exist, but on a similar sort of timeframe that you suggested for a new PyO3 release. On that basis, I'm going to close this PR. |
|
Ok, that suits me! 😄 |
On Apple platforms, Python is often provided as a framework build, rather than a dynamic library. This requires different linking commands if libPython is to be linked with a binary module.
With this change, if linking against libPython is enabled, and the
sysconfigdatafor the platform provides a value forPYTHONFRAMEWORK, the linking step will now performed against the framework (the equivalent of passing-frameworkto the C compiler, or therustc-link-lib=framework=$PYTHONFRAMEWORK), with a framework search path defined byPYTHONFRAMEWORKPREFIX(the equivalent of-F, orrustc-link-search=framework=$PYTHONFRAMEWORKPREFIX).