Skip to content
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

Merged
merged 11 commits into from Mar 19, 2024

Conversation

freakboy3742
Copy link
Contributor

@freakboy3742 freakboy3742 commented Mar 7, 2024

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.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/machinery.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Python/dynload_shlib.c Outdated Show resolved Hide resolved
Comment on lines 1771 to 1775
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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 and name for modules inside a package should be tested somehow as well.

Copy link
Contributor Author

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.

@mhsmith
Copy link
Member

mhsmith commented Mar 7, 2024

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

@freakboy3742
Copy link
Contributor Author

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.

@mhsmith
Copy link
Member

mhsmith commented Mar 8, 2024

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

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``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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``

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Doc/library/importlib.rst Outdated Show resolved Hide resolved
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄)

Comment on lines 1736 to 1741
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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved
Comment on lines 1760 to 1765
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.
"""
Copy link
Member

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 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. 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

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.

Copy link
Member

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 46 to 50
# if defined(__APPLE__) && TARGET_OS_IPHONE
# define SHLIB_SUFFIX ".dylib"
# else
# define SHLIB_SUFFIX ".so"
# endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

bedevere-app bot commented Mar 8, 2024

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.

freakboy3742 and others added 2 commits March 11, 2024 13:09
Co-authored-by: Malcolm Smith <smith@chaquo.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its modules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1746 to 1749
if dylib_exists:
origfile = None
if paths:
for parent_path in paths:
Copy link
Member

@mhsmith mhsmith Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 127 to 129
assert module.__file__ == os.path.join(
os.path.dirname(__file__),
os.path.split(util.EXTENSIONS.file_path)[-1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use unittest assertion methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@brettcannon brettcannon removed their request for review March 13, 2024 19:21
@freakboy3742
Copy link
Contributor Author

@ericsnowcurrently I think I need to take you up on your offer for some pointers on a FileFinder-based approach.

I've just pushed an implementation of a FileFinder-based approach that almost works. It hinges on doing a little trick during extension module processing - in addition to moving the binary file to the Frameworks location, it also drops a .fwork file in the original location. The .fwork file acts as a "really soft" symlink - it contains the location (relative to the app bundle) where the library has been moved. It's not importable as-is; and it doesn't fall foul of Apple's validation requirements (which prevent using an actual symlink), but it's discoverable (using a normal FileFinder registered against the .fwork extension), and with a little fancy footwork, the importer can be modified to use the content of the .fwork file to load from the "real" location.

In the implementation I've pushed here, the ModuleSpec is created with an origin pointing at the .fwork file (as found by the finder). When the module is created, the spec's origin is temporarily updated to point at the actual binary, then switched back after the binary is loaded. I've taken this approach because there's a lot of logic in the tests that creates copies of a module using the spec, and this causes some round-trip issues because the second module reports a location based on the binary location, not the .fwork location.

However, I suspect this temporary origin change on the spec is where I'm hitting a problem. With this PR in its current state, many of thetest_import.SinglephaseInitTests tests are failing because when those tests load the same module a second time, the second copy isn't initialised. I suspect this is because the second copy is being loaded fresh, rather than hitting the import cache, because the cache lookup is being based on the "restored" location of the spec, rather than the actual binary location.

So:

  1. Does that analysis sound correct? If so, can you point me at where this cache is being implemented?
  2. Does the "temporary origin swap" approach sound like an acceptable approach at all? I will admit I'm not completely satisfied with it - it's a little too much like trying to trick the importer, rather than working with it as intended. I'd be a lot happier if the spec origin was always the binary file; but that requires a way to reverse the binary location back to the .fwork location... which I guess might be possible by adding some metadata in the binary location to essentially provide the reverse of the .fwork file - but before I try to implement that, I wanted to feel out whether I'm on the right track at all.

@freakboy3742
Copy link
Contributor Author

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.

FYI: I've made this change in the most recent update.

@freakboy3742
Copy link
Contributor Author

@ericsnowcurrently I've managed to work it out - as I suspected, the problem was the discrepancy between module.__file__ and spec.origin. Once I got those straight, all the pieces fell into place.

So - presented for review is an updated version of iOS dynamic loading. It uses a FileFinder-based Loader, and doesn't require any metapath handling.

In this iteration: spec.origin always points at the binary file; module.__file__ always points at the original module location. On every other platform, these two are always the same; so this change requires a lot more tweaks (mostly to tests) to use the right one in the right place. The loader can be instantiated with either location; when the module is created, spec.origin and module.__file__ are updated as appropriate.

To assist the process of converting between the two locations, two tracking files are created - a .fwork file in the original location of the binary; and a .origin file next to the framework binary. Both are text files that contain the path to the other, relative to the app bundle. Whenever these files are loaded, I haven't done any error handling for FileNotFound etc; if these files don't exist (or can't be loaded), importing won't work at all, so we might as well report that immediately as a file problem.

Two additional notes:

  • I looked into adding the reverse location to the existing Info.plist, rather than creating the new .origin file. This would have been my preferred approach, as it's an existing metadata file; but when you sign frameworks, the plist is compiled into binary format - so unless we're going to freeze plistutil into the bootstrap, the extra file seemed the easier approach.
  • The logic around parsing .fwork and .origin files is repeated in the ctypes module (so that you can CDLL(module.__file__), and a couple of times in the tests. I contemplated factoring this out into a utility method, but I wasn't sure what the right place for such a utility would be; plus, it's 2 lines of code, with only 1 usage in the runtime library that is outside the bootstrap, so it didn't seem worth it. I also contemplated adding another attribute to either spec or module (or both), but I wasn't sure expanding the API surface of modules or spec was desirable.

Happy to revisit either of these issue (or anything else) if required.

For Bevedere's benefit:

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Mar 15, 2024

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

@freakboy3742
Copy link
Contributor Author

Hrm... not sure what the CIFuzz test failure is about...

@freakboy3742
Copy link
Contributor Author

freakboy3742 commented Mar 16, 2024

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.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

It definitely feels simpler using the ExtensionFileLoader subclass.

Lib/importlib/_bootstrap_external.py Show resolved Hide resolved
@freakboy3742
Copy link
Contributor Author

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.

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@ned-deily ned-deily merged commit 408e127 into python:main Mar 19, 2024
35 of 36 checks passed
@freakboy3742 freakboy3742 deleted the ios-framework-loader branch March 19, 2024 22:01
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
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
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
Co-authored-by: Malcolm Smith <smith@chaquo.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants