gh-114099 - Add iOS framework loading machinery. #116454

freakboy3742 · 2024-03-07T08:44:12Z

Part of the PEP 730 work to add iOS support.

This PR adds an extension module Finder and Loader to accommodate the unusual requirements of iOS app packaging.

The iOS App Store requires that all binary modules must be .dylib objects, contained in a framework, stored in the Frameworks folder of the packaged app. There can be only a single binary object per framework, and there can be no executable binary material the app outside the Frameworks folder (other than the app executable itself). The testbed project includes a build step to do the post-processing required to move binary modules from the lib-dynload folder into the Frameworks folder, adding the appropriate metadata and signing the modules; this PR modifies the Python interpreter so that it can find the modules in the new location.

With this PR, the testbed project added in #115930 will now run, with all but 4 tests passing (test_concurrent_futures.test_thread_pool, test_marshal, test_platform, and test_sysconfig); it takes about 12 minutes to run the test suite on a 2021 M1 MacBook Pro, with a couple more minutes required on the first run to prepare and boot the iOS simulator image. Fixing these last couple of tests will be the subject of the next PR.

There's one potentially controversial part of this PR that requires an decision to be made. In addition to the Finder looking for the .dylib file in the new location, the Loader also rewrites the __file__ attribute of the binary module so that it reports the original location of the module, not the location in the Frameworks folder. For example, if a third party has written a foo.bar._whiz module, _whiz.abi3.dylib will be moved to the Frameworks folder, but _whiz.__file__ will report a location that is <somewhere on python path>/foo/bar/_whiz.abi3.dylib, rather than <sys.executable>/Frameworks/foo.bar._whiz.framework/_whiz.abi3.dylib, (which is where the file actually is).

This has been done because there is an implied assumption in the broader Python ecosystem that binary modules will be in a directory hierarchy, and we've occasionally seen examples in the wild of modules that assume that the __file__ attribute of a binary module will be in the filesystem, and can be used to anchor requests for "adjacent" data files. This is somewhat analogous the the issues that are seen with the zipimporter and eggs - code breaks because it assumes __file__ is a reliable way to find files adjacent to the module in question.

There's a reasonable argument to be made that this should be considered a bug in user code, and part of adapting a project to iOS is to fix code that makes this assumption. If the decision is made to drop the __file__ rewriting, the custom Loader isn't required.

The implementation of the loader also requires the creation of 2 additional files for each module - a .fwork file, which acts as a replacement of the extension module in the original location, and a .origin file that is put next to the extension module in it's new home so that you can find where it came from. Filesystem symlinks aren't an option because they're prohibited by Apple's guidelines; these are essentially "text based" symlinks.

Issue: Add support for iOS as a target platform #114099

mhsmith

there is an implied assumption in the broader Python ecosystem that binary modules will be in a directory hierarchy, and we've occasionally seen examples in the wild of modules that assume that the __file__ attribute of a binary module will be in the filesystem, and can be used to anchor requests for "adjacent" data files.
[...]
There's a reasonable argument to be made that this should be considered a bug in user code, and part of adapting a project to iOS is to fix code that makes this assumption. If the decision is made to drop the __file__ rewriting, the custom Loader isn't required.

Based on my experience maintaining another custom finder, I strongly recommend keeping the __file__ rewriting. This kind of assumption is extremely pervasive – there are over 100 instances of dirname(__file__) in the CPython test suite alone.

On the other hand, whether the __file__ exists on the filesystem almost never matters, because it's being used to find other files in the same directory, not the module's own file.

Lib/importlib/_bootstrap_external.py

Lib/importlib/machinery.py

Lib/importlib/_bootstrap_external.py

Python/dynload_shlib.c

mhsmith · 2024-03-07T22:14:31Z

Lib/importlib/_bootstrap_external.py

+        for extension in EXTENSION_SUFFIXES:
+            dylib_file = _path_join(self.frameworks_path, f"{fullname}.framework", f"{name}{extension}")
+            _bootstrap._verbose_message("Looking for Apple Framework dylib {}", dylib_file)
+            try:
+                dylib_exists = _path_isfile(dylib_file)


You mentioned yesterday that this logic is difficult to test completely when the stdlib doesn't have any extension modules inside packages. But couldn't it be tested with any directory containing files with the correct suffixes? They don't actually have to be dylibs, and the test doesn't actually have to load them, it just needs to verify that the correct spec is returned.

I actually found a way to test this by faking the request; see test_file_origin in the loader tests.

That tests the loader, and I see extension/test_finder.py gives some coverage of the finder for top-level modules, but the distinction between fullname and name for modules inside a package should be tested somehow as well.

This code no longer exists, so I guess extra tests aren't needed either.

mhsmith · 2024-03-07T22:49:46Z

Also, there are quite a few lines which go significantly over the 80-column limit.

freakboy3742 · 2024-03-07T23:12:22Z

Also, there are quite a few lines which go significantly over the 80-column limit.

Yes - but so do the original files. I've optimised for minimum diffs, rather than enforcing a new code style; happy to reformat if the core team wants.

…ed public docs.

mhsmith · 2024-03-08T14:49:13Z

There are still a couple of long lines in the new code, specifically the verbose_message calls.

mhsmith · 2024-03-08T15:32:27Z

Doc/library/importlib.rst

+   and replacing ``/`` with ``.``).
+
+   However, this loader will re-write the ``__file__`` attribute of the
+   ``_whiz`` module will report as the original location inside the ``foo/bar``


Suggested change

``_whiz`` module will report as the original location inside the ``foo/bar``

``_whiz`` module to report the original location inside the ``foo/bar``

I've reworked this sentence, so the suggestion no longer applies.

Doc/library/importlib.rst

ericsnowcurrently

Overall I think what you've done is great. I really appreciate the explanation in the docs all about the new loader and why iOS apps are a special case.

There are a few gaps in my knowledge that I'd like clarified and a few potential adjustments I'd like you to consider.

(FYI, I tend to dig deep in code review and tend toward verbosity. Please don't take the physical size of my review on your screen as an indictment of your PR. It's not you, it's me. 😄)

ericsnowcurrently · 2024-03-08T19:39:09Z

Lib/importlib/_bootstrap_external.py

+    The Xcode project building the app is responsible for converting any
+    ``.dylib`` files from wherever they exist in the ``PYTHONPATH`` into
+    frameworks in the ``Frameworks`` folder (including the addition of
+    framework metadata, and signing the resulting framework). This will usually
+    be done with a build step in the Xcode project; see the iOS documentation
+    for details on how to construct this build step.


I don't see this in the importlib docs. It would probably make sense to have it there.

Makes sense; I'll add it in.

Lib/importlib/_bootstrap_external.py

ericsnowcurrently · 2024-03-08T20:50:29Z

Lib/importlib/_bootstrap_external.py

+class AppleFrameworkFinder:
+    """A finder for modules that have been packaged as Apple Frameworks
+    for compatibility with Apple's App Store policies.
+
+    See AppleFrameworkLoader for details.
+    """


Why is this a meta-path finder and not a path-entry finder? In other words, why don't you subclass FileFinder and then plug this in to sys.path_hooks? This is certainly a file-focused finder.

FWIW, I think I know the answer already. 😄 Correct me if I'm wrong, but that would require FRAMEWORKS/FULLNAME.framework to be on sys.path. It would also mean that it would be checked (unnecessarily) for any other sys.path entries and that any other path-entry finders would check the framework-specific sys.path entries unnecessarily.

That said, would it be feasible to write a custom FileFinder subclass that operated relative to the app's frameworks directory. Then you'd put that on sys.path and wouldn't need a custom __init__(). I'm not saying you need to do this (or even that it is doable, though my gut says it is). Mostly I'm asking if you had considered this and, if not, recommend that you take a look.

One reason I bring it up is because, any time we step outside the normal flow of things, we run the risk of disrupting users. For example, in this case a user might expect extension modules to be found via a path-entry finder rather than a meta-path finder. They might mess with things under that assumption and then get grumpy when things break (most likely mysteriously). Of course the risk here is supremely small, but users have a habit of surprising us. 😄

It would also mean that it would be checked (unnecessarily) for any other sys.path entries

Out of curiosity, what should one expect sys.path to look like on iOS?

Also, if I've understood correctly then extension modules can only be found under the app's frameworks folder and never on any other sys.path entry. If that's the case then should we remove the ExtensionFileLoader entry from _get_supported_file_loaders() in Lib/importlib/_bootstrap_external.py? Also, are the restrictions on extension modules (location, suffix) technical restrictions or strictly a consequence of the fact that apps cannot ship with non-conforming binary files, as enforced by the app store police? Is there a possibility that one could find an extension file out of place somehow, like in some system directory?

IIUC, .py files may still be imported using the normal path-entry finder (FileFinder), right? I'm not familiar with iOS apps to even imagine when the finder might be looking (i.e. what's in sys.path).

Relatedly, what happens if an app maintainer (or, somehow, a user) registers additional importers or messes with sys.path? Could that cause problems for this finder? Could it circumvent the app store rules?

and that any other path-entry finders would check the framework-specific sys.path entries unnecessarily.

FYI, the import machinery already makes this (almost) a non-issue with path hooks and sys.path_importer_cache.

You're correct that the reason for the metapath finder is the structure of the Frameworks folder. I hadn't considered using a custom FileFinder; I agree we should do anything we can to avoid breaking user expectations (however eccentric or unexpected those might be), so I'll take a look and see if I can make this work.

As for ExtensionFileLoader - that's an interesting case. iOS apps are entirely capable of loading binary modules from the PYTHONPATH; it's entirely an issue of the resulting app being acceptable to the App Store. If you've got a project that doesn't have the "move dylibs to the Frameworks folder" build step, the app will still run - it will just be rejected when you try to submit it to the App store (and it will be rejected by an automated check at time of submission, not hours/days/weeks later as a result of the review process). I've also had a couple of bugs (including one I'm still chasing with cryptography) where it's useful to be able to confirm if the problem you're seeing is because the .dylib has been moved, or because the .dylib isn't working. I therefore opted to retain the ExtensionFileLoader, just in case it was useful.

The SourceFileLoader and SourcelessFileLoader both work exactly the same, however. Loading .py files and .pyc files from anywhere in the app bundle isn't an issue; it's only binary modules that the App Store restricts. sys.path for a running app will contain {dirname(sys.executable)}/python/lib/python3.X at a minimum (i.e., prefix=={dirname(sys.executable)}/python, but the bin/include/man/share folders aren't installed). BeeWare apps usually end up with {dirname(sys.executable)}/app and {dirname(sys.executable)}/app_packages as well as the location of app code and app dependencies, but that's entirely up to the app itself.

If a user installs an additional importer... I guess that depends on what the importer is doing. The entire contents of the app bundle is fair game for reading; so a novel mechanism for finding .py files shouldn't be an issue. The only place I could force a problem is if the importer is expecting to find an executable binary in a specific location outside the Frameworks folder - but the app won't be accepted into the App Store in the first place if they try to do this. As for circumventing the App Store rules - it might be a lack of imagination on my part, but I'm having difficulty imaging what you'd be able to do here. Normal filesystem write protections should prevent even a malicious user from doing anything damaging, and you won't be able to use anything binary that isn't in the app bundle and signed on that basis.

Sounds good. Thanks for the detailed explanation!

I hadn't considered using a custom FileFinder; I agree we should do anything we can to avoid breaking user expectations (however eccentric or unexpected those might be), so I'll take a look and see if I can make this work.

Let me know if you have any questions on this.

ericsnowcurrently · 2024-03-08T21:07:46Z

Python/dynload_shlib.c

+#  if defined(__APPLE__) && TARGET_OS_IPHONE
+#    define SHLIB_SUFFIX ".dylib"
+#  else
+#    define SHLIB_SUFFIX ".so"
+#  endif


Out of curiosity, is there really strictly no way any extension module could have a .so suffix? What about in places the app wouldn't be expected to look but might still have access to (if that's even possible)?

(To be clear, I'm not opposed to restricting the suffix here if that satisfies conforming to the iOS app rules.)

That's a good question - I don't think I've actually tried. iOS App Store policies definitely restrict the type of binary you can use in a framework, and I assumed that would extend to using the default extension as well - but this might not be true. It's definitely worth confirming, though - I agree it would be a lot nicer if we could use the .so extension consistently.

So... I feel like a bit of an idiot. It doesn't matter what file extension is used when the module is compiled - we can rename it when it's moved to the final Frameworks location. The extension referenced in this list is only used for finder purposes... but the FileFinder won't ever find a .so or .dylib file in the normal PYTHONPATH, because the App Store prohibits that.

For bonus facepalm points: the default extension used by an Apple framework is... no extension at all - so when the binary is moved, we can strip off any extension, and rename it to the fully-qualified Python module name.

bedevere-app · 2024-03-08T21:20:16Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

ned-deily

At this point, I don't have any major comments on the PR especially after the thorough review by @ericsnowcurrently and @mhsmith of the importlib changes. I did build and test with iOS simulators with the latest Xcode and macOS releases on both Apple Silicon and Intel Macs. I am ignoring test case errors at this point. But, in passing, I think we should change the default Python test suite options in iOSTestbedTests.m from -v to -W which is what we typically use for CI tests; IMO, the output from -v is overwhelming and makes it difficult to find problems.

ned-deily · 2024-03-11T21:59:55Z

Lib/test/test_importlib/extension/test_loader.py

+        # packages assume that __file__ will return somewhere in the
+        # PYTHONPATH; this won't be true on Apple mobile, so the
+        # AppleFrameworkLoader rewrites __file__ to point at the original path.
+        # However, the standard library puts all it's modules in lib-dynload,


its modules

This comment no longer exists, as there isn't a need for a separate finder.

mhsmith · 2024-03-12T01:23:33Z

Lib/importlib/_bootstrap_external.py

+            if dylib_exists:
+                origfile = None
+                if paths:
+                    for parent_path in paths:


If I understand correctly, when importing a top-level module, paths is None, so the __file__ attribute will not be rewritten.

In fact, I don't think it could be rewritten with the information available. If an app has both the standard library and some other source of binary modules on sys.path (whether it's app_packages, site_packages or anything else), then the current framework naming convention makes it impossible to know which path entry the "original" location was.

Maybe the framework name needs some additional dotted segments at the start, so it effectively represents the "original" path relative to dirname(sys.executable)? This would also ensure there are no name clashes with any other frameworks that happen to be in the app.

The new loader implementation I've just pushed should resolve this. It leaves a .fwork breadcrumb which is used to anchor the discovery of the module, which allows us to canonically know where a binary came from, without any guesswork, even if it's in lib-dynload or somewhere else at a root of a sys.path entry.

mhsmith · 2024-03-12T01:39:19Z

Lib/test/test_importlib/extension/test_loader.py

+        assert module.__file__ == os.path.join(
+            os.path.dirname(__file__),
+            os.path.split(util.EXTENSIONS.file_path)[-1]


Should use unittest assertion methods.

Good note; but this test no longer exist in the new implementation.

freakboy3742 · 2024-03-16T01:32:43Z

It appears the CIFuzz failure isn't related to this PR specifically - there's a lot of recent CI passes (e.g., 1 2 3) that are failing on that step.

Edit: The CI problem has now been logged as an issue #116886.

ericsnowcurrently

LGTM

It definitely feels simpler using the ExtensionFileLoader subclass.

Lib/importlib/_bootstrap_external.py

freakboy3742 · 2024-03-19T00:25:50Z

I've just pushed a small additional related fix: the iOS App Store requires that the CFBundleShortVersionString be numbers and periods only, so 3.13.0a5+ fails validation. I've opted to use 3.13 for the short string; CFBundleLongVersionString still has the full 3.13.0a5+ string. I didn't pick this up earlier because the test harness I had evidently manually tweaked the version string on the project I was using to test validation, but when I ran a clean project (while working on the next PR in the chain), I got a validation error.

ned-deily

LGTM. I tested building all variants on both Intel and Apple Silicon Macs and running the simulator variants on their native Mac platforms with essentially the same results: several known failing test cases which will be addressed in followup PR(s). One other possible followup issue is that, when I run make testios, the final test results appears to be displayed 3 times (?). Otherwise, say the "m" word, @freakboy3742, and we'll move on :)

freakboy3742 · 2024-03-19T06:16:11Z

One other possible followup issue is that, when I run make testios, the final test results appears to be displayed 3 times (?).

Hrm - I hadn't noticed that; I'm usually running the test suite inside Xcode (because it's faster, and you get immediate feedback)... I'll try and reproduce and see what I can make of it.

Otherwise, say the "m" word, @freakboy3742, and we'll move on :)

@ned-deily Lets do it!

freakboy3742 · 2024-03-19T07:25:45Z

One other possible followup issue is that, when I run make testios, the final test results appears to be displayed 3 times (?).

Hrm - I hadn't noticed that; I'm usually running the test suite inside Xcode (because it's faster, and you get immediate feedback)... I'll try and reproduce and see what I can make of it.

Looks like the query selector I'm using to extract results in the case of a failure isn't quite right - it's matching a group that contains multiple copies of the log output (although it's not clear to me why there are multiple copies). I'll need to poke around a bit to clean that up, but it should be fixable.

Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…GH-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…H-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…GH-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…H-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…GH-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…H-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…GH-116454) Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

Add iOS framework loading machinery.

e907723

freakboy3742 requested review from ericsnowcurrently, brettcannon, ncoghlan and warsaw as code owners March 7, 2024 08:44

bedevere-app bot added the awaiting review label Mar 7, 2024

bedevere-app bot mentioned this pull request Mar 7, 2024

Add support for iOS as a target platform #114099

Closed

freakboy3742 requested a review from ned-deily March 7, 2024 08:44

Removed a stray character.

1359bc8

mhsmith reviewed Mar 7, 2024

View reviewed changes

freakboy3742 added 3 commits March 8, 2024 07:16

Simplify logic for dylib extensions.

5e0659e

Clarified docstrings and implementation on iOS Loader/Finder, and add…

d5fda7e

…ed public docs.

Add iOS to the list of platforms known by documentation.

284e225

mhsmith reviewed Mar 8, 2024

View reviewed changes

ericsnowcurrently requested changes Mar 8, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Mar 8, 2024

freakboy3742 and others added 2 commits March 11, 2024 13:09

Improvements to documentation.

9f42faa

Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

Modify loader to do more processing in the finder.

8308611

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

ned-deily reviewed Mar 12, 2024

View reviewed changes

mhsmith reviewed Mar 12, 2024

View reviewed changes

brettcannon removed their request for review March 13, 2024 19:21

Switch to a FileLoader-based approach for Apple framework loading.

fa3ffbb

freakboy3742 requested review from markshannon and gvanrossum as code owners March 14, 2024 06:26

ericsnowcurrently approved these changes Mar 18, 2024

View reviewed changes

Lib/importlib/_bootstrap_external.py Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting change review labels Mar 18, 2024

Ensure CFBundleShortVersionString meets App Store guidelines.

04d1c79

ned-deily approved these changes Mar 19, 2024

View reviewed changes

ned-deily merged commit 408e127 into python:main Mar 19, 2024
35 of 36 checks passed

bedevere-app bot removed the awaiting merge label Mar 19, 2024

freakboy3742 deleted the ios-framework-loader branch March 19, 2024 22:01

freakboy3742 mentioned this pull request Mar 20, 2024

gh-114099: Additions to standard library to support iOS #117052

Merged

vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024

pythongh-114099 - Add iOS framework loading machinery. (pythonGH-116454)

57b5e07

Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024

pythongh-114099 - Add iOS framework loading machinery. (pythonGH-116454)

85fea22

Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-114099 - Add iOS framework loading machinery. (pythonGH-116454)

743a5c6

Co-authored-by: Malcolm Smith <smith@chaquo.com> Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

	``_whiz`` module will report as the original location inside the ``foo/bar``
	``_whiz`` module to report the original location inside the ``foo/bar``

gh-114099 - Add iOS framework loading machinery. #116454

gh-114099 - Add iOS framework loading machinery. #116454

Conversation

freakboy3742 commented Mar 7, 2024 • edited Loading

mhsmith left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented Mar 7, 2024

freakboy3742 commented Mar 7, 2024

mhsmith commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsnowcurrently left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Mar 8, 2024

ned-deily left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

mhsmith Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 commented Mar 16, 2024 • edited Loading

ericsnowcurrently left a comment

Choose a reason for hiding this comment

freakboy3742 commented Mar 19, 2024

ned-deily left a comment

Choose a reason for hiding this comment

freakboy3742 commented Mar 19, 2024

freakboy3742 commented Mar 19, 2024

freakboy3742 commented Mar 7, 2024 •

edited

Loading

mhsmith left a comment •

edited

Loading

mhsmith commented Mar 8, 2024 •

edited

Loading

ericsnowcurrently left a comment •

edited

Loading

ned-deily left a comment •

edited

Loading

freakboy3742 Mar 14, 2024 •

edited

Loading

mhsmith Mar 12, 2024 •

edited

Loading

freakboy3742 commented Mar 16, 2024 •

edited

Loading