Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gh-114099 - Add iOS framework loading machinery. #116454
gh-114099 - Add iOS framework loading machinery. #116454
Changes from 2 commits
e907723
1359bc8
5e0659e
d5fda7e
284e225
9f42faa
8308611
fa3ffbb
dbf818d
e66aee1
04d1c79
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 see this in the importlib docs. It would probably make sense to have it 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.
Makes sense; I'll add it in.
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.
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 tosys.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 onsys.path
. It would also mean that it would be checked (unnecessarily) for any othersys.path
entries and that any other path-entry finders would check the framework-specificsys.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 onsys.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. 😄
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.
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 theExtensionFileLoader
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 insys.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?FYI, the import machinery already makes this (almost) a non-issue with path hooks and
sys.path_importer_cache
.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'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.
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.
Sounds good. Thanks for the detailed explanation!
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 me know if you have any questions on this.
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 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.
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 actually found a way to test this by faking the request; see
test_file_origin
in the loader tests.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 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
andname
for modules inside a package should be tested somehow as well.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 code no longer exists, so I guess extra tests aren't needed either.