Skip to content

patch for xrOS #6

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

Merged
merged 35 commits into from
Apr 23, 2025
Merged

Conversation

johnzhou721
Copy link

@johnzhou721 johnzhou721 commented Apr 13, 2025

Patch for xrOS in Git commit format as mentioned in beeware/Python-Apple-support#269.

EDIT -- new PR for PAS is at beeware/Python-Apple-support#270

Context for more visitors: PAS = beeware/Python-Apple-Support, xrOS is internal codename in Apple for visionOS. This patch adds support to an already-patched version of CPython to support for visionOS.

Copy link
Owner

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The broad strokes of this PR look right; but there's a few details to sort out.

  • There's definitely an aclocal version issue; there shouldn't be a need for any changes to aclocal.m4. If you use the Tools/build/regen-configure.sh script mentioned at the top of configure.ac, that will guarantee all the right tool versions are in use (using Docker to provide a clean development environment).

  • The xros vs visionOS naming convention flagged in your other visionOS PR. There are definitely some places here where xros will need to be used (most notably, as XROS_DEPLOYMENT_TARGET - but ac_sys_system should definitely be resolving to visionOS so that sys.platform returns visionos; and the modifications in platform should also be using visionOS naming.

  • There are some consistency issues with flagging the minimum visionOS version as 4.0, and the question of how that version is to be passed into the compiler - the compiler shims may need to be modified to include XROS_DEPLOYMENT_TARGET as part of the -target definition.

  • There's no need to provide arm64_32 or x86_64 variants of the compiler shims, as those platforms don't exist for visionOS - and there's no need for the pyconfig.h shim either, as there's only 1 architecture for vision OS.

Co-authored-by: Russell Keith-Magee <[email protected]>
@johnzhou721
Copy link
Author

Again, sorry for 4.0; I should've had more diligence when copy + pasting

@johnzhou721
Copy link
Author

In addition -- I have some stuff I want to clarify --- I copy-pasted a lot of stuff from watchOS -- there's some stuff unavailable only for TV and Watch -- in that case, I think we should test those tv- and watchOS only stuff on visionOS and see if they're actually unavailable.

@johnzhou721
Copy link
Author

OK I have configure running locally at this point, I changed some more stuff that I need to push

@johnzhou721
Copy link
Author

johnzhou721 commented Apr 13, 2025

visionos deployment target -- that variable i think is fine

Besides the SDK everything else could use visionOS; I kept the xros target triples just in case someone needs to compile them using xros

@johnzhou721
Copy link
Author

OK somehow make clean does not remove the generated info.plist. Haven't tried make install yet but configure and make could run (idk if correctly)

@johnzhou721
Copy link
Author

Not very sure about the latest commit-- no way to test at this moment -- I will need to generate a patch and feed it into py-apple-support

@johnzhou721
Copy link
Author

@freakboy3742 Ping

@freakboy3742
Copy link
Owner

@freakboy3742 Ping

As I mentioned yesterday - I get notified on every change to every BeeWare repo; there's no need to ping me directly.

Also - timezones are a thing. I've literally just woken up :-)

@johnzhou721
Copy link
Author

Oh sorry! Yeah I realized 5 seconds after I sent the ping comment that you were from australia

I was from China UTC+8, moved a few years (<5) ago, and yes, time zones are relatable

@johnzhou721
Copy link
Author

Something along these lines: visionOS 3D content must be done with SwiftUI and UIKit cannot be used for visionOS-specific or 3D content: https://developer.apple.com/documentation/visionos/bringing-your-app-to-visionos/

So... should we start writing rubicon-swift?

Copy link
Owner

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've flagged a bunch of consistency issues - mostly related to where the build scripts should be accepting visionOS, and when they should be accepting xros. However, there's also a bunch of 4.0 version references that I flagged in my last pass.

@freakboy3742
Copy link
Owner

Something along these lines: visionOS 3D content must be done with SwiftUI and UIKit cannot be used for visionOS-specific or 3D content: https://developer.apple.com/documentation/visionos/bringing-your-app-to-visionos/

So... should we start writing rubicon-swift?

See beeware/rubicon-objc#563.

The same problem/limitation exists for watchOS.

@johnzhou721
Copy link
Author

Just finished changing a bunch of stuff; sorry for the delay; will push once build finishes
Also thanks for doing some final touchups and merging the cpython-apple-source-deps stuff.

@johnzhou721
Copy link
Author

Also I got this while compiling:

./Modules/_hacl/Lib_Memzero0.c:52:6: warning: "Your platform does not support any safe implementation of memzero -- consider a pull request!" [-W#warnings]
52 | #warning "Your platform does not support any safe implementation of memzero -- consider a pull request!"
| ^

@johnzhou721
Copy link
Author

Still I pushed a commit to address the code style.

@freakboy3742
Copy link
Owner

I've just tried the laster version of this branch - and it's launching fine for me, on both iOS and visionOS, under Xcode.

It's possible the cause of the problem you're seeing is that you need a "build" version of Python that must be the exact same version that is being used for testing. So - you need to build a macOS version of CPython off the patched branch, and that needs to be the version of CPython that the visionOS build finds - essentially your "build" python needs to be on the path before the version in /Library/Frameworks/Python.framework. If this isn't the case, the frozen modules may not be compatible, which would lead to the sort of error you're reporting on startup.

If you're running the test suite through Xcode, you'll also need to turn on automated breakpoints, same as you do for iOS; without that, the test suite will halt about 5 tests in when then first SIGINT is raised

With those two caveats, most of the test suite is running; however:

  • When the test suite is building, I'm seeing a warning about compiling InterfaceBuilder products. I suspect this is occurring because you've taken the iOS test project verbatim; some cleanup is needed here.
  • When the test suite runs, I'm seeing at least 4 test failures due to basic platform identification issues (issues where a platform tag is coming back as visionos-2.0-visionsimulator not visionos-2.0-xrsimulator, or similar).
  • I'm seeing a hard crash near the end of the test suite. I haven't dug into detail to find the cause of this one.
  • The run target on a testbed project (i.e., path/to/testbed run), and by extension, make testvisionos, isn't working for me. It's timing out after 5 minutes; it looks to me like it is starting a simulator, but the mechanism in the testbed that tries to identify the booted simulator isn't working.

@johnzhou721
Copy link
Author

johnzhou721 commented Apr 21, 2025

  1. Can confirm stuff related to using the official alpha 7 build gives the error.

  2. What are we supposed to do with this thing? Just continued for now but it says Message from debugger: Terminated due to signal 9. Is this the hard crash you are talking about?

Screenshot 2025-04-21 at 9 42 10 AM
  1. You said "needs to be on the path before the version in /Library/Frameworks/Python.framework ", but it wasn't on the path at all, I just passed it to --with-build-python (wasn't using Python-Apple-Support), is that fine?

  2. Just pushed a commit addressing item 1.

  3. 4 seems to be caused by the devices being listed using xrOS and not visionOS in simctl, fixed.

I'm testing this right now... I hope messages like this counts as a small amount of large messages.

@johnzhou721
Copy link
Author

johnzhou721 commented Apr 21, 2025

Caught where the issue for #2 is

Current commands for build --

export PATH=./visionOS/Resources/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Library/Apple/usr/bin
./configure --host=arm64-apple-xros2.0-simulator --build=arm64-apple-darwin --enable-framework --with-build-python=/Users/johnzhou/cpythonxrmac/bin/python3.14 && make -j16 && make install -j16 && make testvisionos

@freakboy3742
Copy link
Owner

freakboy3742 commented Apr 21, 2025

  1. Can confirm stuff related to using the official alpha 7 build gives the error.

That would be consistent. Any new feature that alters CPython's bytecode definition will be a source of problems, and a7 included the addition of t-strings. Once beta 1 lands, these problems become less prevalent because new features are no longer allowed.

  1. What are we supposed to do with this thing? Just continued for now but it says Message from debugger: Terminated due to signal 9. Is this the hard crash you are talking about?

Yes, that's the hard crash I was talking about.

As for how to handle it... it depends what is causing it. EXC_GUARD is an exception that indicates access of a protected file system resource, which seems highly probable given the test that seems to be failing is in test_zipfile. The question is which resource is causing the problem.

So - the next step is to narrow down on which subtest of test_zipfile is the specific cause of the failure. If you've got the project loaded in Xcode, you can edit the visionOSTestbed-Info.plist file; that file contains a TestArgs key which are the arguments that will be passed to the test suite. Add test_zipfile -v as 2 additional arguments, and only test_zipfile will run, and it will run in "verbose" mode, showing each test as it runs. That should help you to narrow down to a specific test that is causing the error. Then you can dig into the code for test_zipfile for that test, and work out what is going on.

Beyond that, it depends what the test is doing to cause a problem. It might be as simple as a test that is trying to access a file that isn't available in the visionOS sandbox. At the very least, we can skip that specific test. However, ideally we won't just skip the test, we'd disable the fix the underlying problem. If the issue is the specific filename, then we can find a better filename; if it's using an underlying feature, we can disable that feature. That's what these checks are controlling - it's identifying system calls that exist, but if you actually try to invoke them, an error is raised.

If you want to test a theory about whether disabling a specific test will fix the problem, you can manually edit the contents of test_zipfile in the package Framework, then do a clean build of the app - that will run your patched code in the test suite.

  1. You said "needs to be on the path before the version in /Library/Frameworks/Python.framework ", but it wasn't on the path at all, I just passed it to --with-build-python (wasn't using Python-Apple-Support), is that fine?

Yes, that's fine - --with-build-python is the other way (and more reliably way) to guarantee the interpreter that is used.

  1. 4 seems to be caused by the devices being listed using xrOS and not visionOS in simctl, fixed.

That's about what I expected; glad to see it was an easy one.

I'm testing this right now... I hope messages like this counts as a small amount of large messages.

Yes, this is a lot better - thanks for taking that feedback on board.

Comment on lines 18 to 22
<array>
<string>iPhoneOS</string>
</array>
<key>MinimumOSVersion</key>
<string>12.0</string>
Copy link
Owner

Choose a reason for hiding this comment

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

Just noticed that these keys don't seem right

Copy link
Author

Choose a reason for hiding this comment

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

Not sure about canoncial input for this, and don't know how to look it up (Google is failing me), but by searching up random stuff I managed to find that the tvOS value is AppleTVOS, so I used VisionOS as of right now.

@johnzhou721
Copy link
Author

Also it's funny #2 was referencing issue 2. Anyway I just pushed a change to Python-Apple-Support doing a macOS build in the CI, so we could test these stuff on CI before Beta 1 comes out. If we happen to merge after (or before and you don't like it, I'm guessing so) Beta 1, we will revert.

@johnzhou721
Copy link
Author

Let's go iOS passing for the first time in forever on PAS...

Will go investigate issue about zipfile now. Doing what you said; however, FYI your comment has a formatting issue, but don't worry i've had tons during translation.

@johnzhou721
Copy link
Author

@freakboy3742 Sorry for sending a premature message 14 minutes ago.

I set verbose + test_zipfile but it crashes without printing anything verbose (aka just the test name). Any ideas?
Screenshot 2025-04-21 at 6 50 39 PM
Screenshot 2025-04-21 at 6 52 18 PM
Any ideas of what to do now?

Also never mind test_platform is failing, a reference to the fact that I was writing in C++ 5 minutes ago.

@freakboy3742
Copy link
Owner

Any ideas of what to do now?

My apologies - I forgot that -W and -v interact. You'll also need to delete the -W option. You should get output that looks like:

...
test_write (test.test_zipfile.test_core.UnseekableTests.test_write) ... ok
test_writestr (test.test_zipfile.test_core.UnseekableTests.test_writestr) ... ok
test_compresslevel_property (test.test_zipfile.test_core.ZipInfoTests.test_compresslevel_property) ... ok
test_from_dir (test.test_zipfile.test_core.ZipInfoTests.test_from_dir) ... ok
test_from_file (test.test_zipfile.test_core.ZipInfoTests.test_from_file) ... ok
test_from_file_bytes (test.test_zipfile.test_core.ZipInfoTests.test_from_file_bytes) ... ok
test_from_file_fileno (test.test_zipfile.test_core.ZipInfoTests.test_from_file_fileno) ... ok
test_from_file_pathlike (test.test_zipfile.test_core.ZipInfoTests.test_from_file_pathlike) ... ok

----------------------------------------------------------------------
Ran 333 tests in 7.470s

OK (skipped=3)

== Tests result: SUCCESS ==

1 test OK.

Total duration: 7.6 sec
Total tests: run=333 skipped=3
Total test files: run=1/1
Result: SUCCESS

@johnzhou721
Copy link
Author

Hi @freakboy3742, apologies for the slow response time (and the many other times where I had to wake up before I could respond, even though I wake up as early as 5:30 in some days within the past month or so).

The crashing test is test.test_zipfile.test_core.TestsWithMultipleOpens.test_many_opens, whose job is to verify

        # Verify that read() and open() promptly close the file descriptor,
        # and don't rely on the garbage collector to free resources.

Now, the code is pretty short that I can post it here (if GitHub ever relicenses user-submitted contents under dirty stuff we can argue fair use here).

    def test_many_opens(self):
        # Verify that read() and open() promptly close the file descriptor,
        # and don't rely on the garbage collector to free resources.
        startcount = fd_count()
        self.make_test_archive(TESTFN2)
        with zipfile.ZipFile(TESTFN2, mode="r") as zipf:
            for x in range(100):
                zipf.read('ones')
                with zipf.open('ones') as zopen1:
                    pass
        self.assertEqual(startcount, fd_count())

fd_count seems sussy... any insights other than "disable the test"?

@freakboy3742
Copy link
Owner

fd_count seems sussy... any insights other than "disable the test"?

I guess my follow up question is to ask what line is actually throwing the error? Is it fd_count()? That seems unlikely, as fd_count() is a test utility method used elsewhere in the test suite. Or is it one of the other calls? In particular - is make_test_archive() trying to create a file somewhere it's not allowed to? Is it rejecting the request to read the specific zipfile entry?

@johnzhou721
Copy link
Author

By monkey patching (changing the produced XCFramework) and putting print("=======") (a technique from doing competitive programming (not-that-successfully)), I found that the culprit is fd_count on the line following where it's inserted

    def test_many_opens(self):
        # Verify that read() and open() promptly close the file descriptor,
        # and don't rely on the garbage collector to free resources.
        print("=======")
        startcount = fd_count()
        self.make_test_archive(TESTFN2)
        with zipfile.ZipFile(TESTFN2, mode="r") as zipf:
            for x in range(100):
                zipf.read('ones')
                with zipf.open('ones') as zopen1:
                    pass
        self.assertEqual(startcount, fd_count())

Will stop working today at GMT-5 (daylight saving time already applied into the time zone), and sorry for the brisk comment.

@freakboy3742
Copy link
Owner

By monkey patching (changing the produced XCFramework) and putting print("=======") (a technique from doing competitive programming (not-that-successfully)), I found that the culprit is fd_count on the line following where it's inserted

Looks like I spoke too soon about fd_count() not being a likely culprit :-)

I've done a poke around, and all the exiting uses are being skipped - including, in one case, this skip that identifies the test as "unstable".

I'd like to have a little better understanding of exactly why it's failing on visionOS, so if you're able to dig into the internals of fd_count() to see where it's failing that might point at a cause; but I'm getting close to just saying "skip it".

@johnzhou721
Copy link
Author

johnzhou721 commented Apr 22, 2025

By monkey patching (changing the produced XCFramework) and putting print("=======") (a technique from doing competitive programming (not-that-successfully)), I found that the culprit is fd_count on the line following where it's inserted

Looks like I spoke too soon about fd_count() not being a likely culprit :-)

Just went ahead and re-checked this using flush=True on print and yes I can confirm :)

I've done a poke around, and all the exiting uses are being skipped - including, in one case, this skip that identifies the test as "unstable".

Yes, but that being skipped because it's unstable is because it produces different output because of system stuff tempering w/ the file descriptor count (which I have no idea about), not because it crashes AFAIK (by looking at the comments).

I'd like to have a little better understanding of exactly why it's failing on visionOS, so if you're able to dig into the internals of fd_count() to see where it's failing that might point at a cause; but I'm getting close to just saying "skip it".

Crashing, not failing. In my own opinion I would not skip it. After extensive debugging, it is crashing on https://github.com/johnzhou721/cpython/blob/379c3d965724046bffcde63199893fe79dfdded7/Lib/test/support/os_helper.py#L689 for the 8th time it executes (fd=7, but 0-indexing :) )

If you got confused on my stuff above and would like to see concrete evidence

Relevant definition (see the lines #ADD) I added:

def fd_count():
    """Count the number of open file descriptors.
    """
    if sys.platform.startswith(('linux', 'android', 'freebsd', 'emscripten')):
        fd_path = "/proc/self/fd"
    elif sys.platform == "darwin":
        fd_path = "/dev/fd"
    else:
        fd_path = None

    if fd_path is not None:
        try:
            names = os.listdir(fd_path)
            # Subtract one because listdir() internally opens a file
            # descriptor to list the content of the directory.
            return len(names) - 1
        except FileNotFoundError:
            pass

    MAXFD = 256
    if hasattr(os, 'sysconf'):
        try:
            MAXFD = os.sysconf("SC_OPEN_MAX")
        except OSError:
            pass

    old_modes = None
    if sys.platform == 'win32':
        # bpo-25306, bpo-31009: Call CrtSetReportMode() to not kill the process
        # on invalid file descriptor if Python is compiled in debug mode
        try:
            import msvcrt
            msvcrt.CrtSetReportMode
        except (AttributeError, ImportError):
            # no msvcrt or a release build
            pass
        else:
            old_modes = {}
            for report_type in (msvcrt.CRT_WARN,
                                msvcrt.CRT_ERROR,
                                msvcrt.CRT_ASSERT):
                old_modes[report_type] = msvcrt.CrtSetReportMode(report_type,
                                                                 0)

    try:
        count = 0
        for fd in range(MAXFD):
            try:
                # Prefer dup() over fstat(). fstat() can require input/output
                # whereas dup() doesn't.
                print("========", fd, flush=True)             # ADDED BY ME
                fd2 = os.dup(fd)             # 689 IN ORIGINAL
                print("++++++++", fd, flush=True)             # ADDED BY ME
            except OSError as e:
                if e.errno != errno.EBADF:
                    raise
            else:
                os.close(fd2)
                count += 1
    finally:
        if old_modes is not None:
            for report_type in (msvcrt.CRT_WARN,
                                msvcrt.CRT_ERROR,
                                msvcrt.CRT_ASSERT):
                msvcrt.CrtSetReportMode(report_type, old_modes[report_type])

    return count

Log just before it crashed:
Screenshot 2025-04-22 at 6 46 00 PM

@freakboy3742
Copy link
Owner

I'd like to have a little better understanding of exactly why it's failing on visionOS, so if you're able to dig into the internals of fd_count() to see where it's failing that might point at a cause; but I'm getting close to just saying "skip it".

Crashing, not failing. In my own opinion I would not skip it. After extensive debugging, it is crashing on https://github.com/johnzhou721/cpython/blob/379c3d965724046bffcde63199893fe79dfdded7/Lib/test/support/os_helper.py#L689 for the 8th time it executes (fd=7, but 0-indexing :) )

My apologies - that was loose language on my part. I'm aware it's a crash, not a soft fail.

That testing definitely confirms it's os.dup() that is the problem; and from some poking around, it's not entirely surprising that EXC_GUARD is a problem for some file descriptors. Unfortunately, Apple doesn't seem to want to document how these file descriptors should be handled.

The good news, though, is that the fix is staring us in the face. CPython macOS has already hit this problem, and applied a fix. At the top of fd_count() is a short-cut mechanism for getting a descriptor count on darwin... but iOS, tvOS, watchOS and visionOS are also Darwin kernels, so the same approach works there too.

So - if you modify the if sys.platform == 'darwin' check to be if support.is_apple, you should get a pass on the test_zipimport test. That fix should probably be applied upstream as well, as it will be an improvement for iOS as well - so if you want to earn your CPython contribution wings, submit that fix upstream (as well as making it on this PR) and tag me and I'll get it merged in.

macOS Sonoma seems to have issues with support.fd_count, at
pythongh-109981. However, after experimenting using macOS Sonoma
on an attempt to support visionOS unofficially through another
project, the simulator is hard-crashing, leading to the con-
clusion that this probably applies on all Apple platforms.
Hence this patch.
@johnzhou721
Copy link
Author

@freakboy3742 Done.
python#132823

Copy link
Owner

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've just pushed a couple of minor updates to your patch - but with those changes, I'm getting a clean test run! Thanks for your work on this patch.

@freakboy3742 freakboy3742 merged commit da97b72 into freakboy3742:3.14-patched Apr 23, 2025
Copy link
Author

@johnzhou721 johnzhou721 left a comment

Choose a reason for hiding this comment

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

sorry, finishing up a pending review from forever ago

Comment on lines 18 to 22
<array>
<string>iPhoneOS</string>
</array>
<key>MinimumOSVersion</key>
<string>12.0</string>
Copy link
Author

Choose a reason for hiding this comment

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

Not sure about canoncial input for this, and don't know how to look it up (Google is failing me), but by searching up random stuff I managed to find that the tvOS value is AppleTVOS, so I used VisionOS as of right now.

@johnzhou721
Copy link
Author

Like I flagged those style things and forgot to click ``finish your review''

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.

2 participants