GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-114099 - Add iOS testbed, plus Makefile target to invoke it. by freakboy3742 · Pull Request #115930 · python/cpython Conversation
|
| .PHONY: testiOS | ||
| testiOS: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| .PHONY: testiOS | |
| testiOS: | |
| .PHONY: test-iOS | |
| test-iOS: |
?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
My thinking here was to be consistent with testuniversal, cleantest, buildbottest et al. However, I'm happy to change this name if there's a general preference for test-iOS over testiOS.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
All existing makefile targets are lower-case, even acronyms like multissltest.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I guess that's true... although I suspect I'd twitch every time I need to spell it with capitalization that breaks the branding guide 😀
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to testios
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| // Set the stdlib location for the Python interpreter | ||
| path = [NSString stringWithFormat:@"%@/python/lib/python%@", resourcePath, py_version_string, nil]; | ||
| NSLog(@"Stdlib dir: %@", path); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this entire block is redundant, because this is already the default location that will be derived from config.home. In which case, the whole chain that passes the Python version from the configure script can be removed as well.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Agreed this block isn't needed. The testbed project still needs a version number, but I guess it's not critical that this version number be the same as the Python version, as it's not going to be a released app, and there are other details in the logs that identify the Python version, so I guess we can drop the configuration portion as well.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // will be handled as if it were an argument to `python -m test` | ||
| const char *argv[] = { | ||
| "iOSTestbed", // argv[0] is the process that is running. | ||
| "-uall,-gui,-curses", // Enable most resources; GUI and curses tests won't work on iOS |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The curses and tkinter tests will be skipped automatically due to their stdlib modules not being built, so there's no need to exclude them here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks pretty good to me. I like the adding of the default tool stub selections into configure. I might have more comments after the follow-on PRs to get a fully working import system and the test suite can run.
One thing that I would like you to give some thought to, if you haven't already, is to make it possible to run make testiOS when the build directory is not the source directory. As a perhaps unwritten goal, we try to support multiple builds with different configure args from one source directory with the ultimate goal of a read-only source directory. Requiring the writable testbed directory to be in the source directory hinders those goals. Even something as simple as having the make testiOS rule copy the iOS/testbed directory from the source directory to the build directory would be fine. If that presents formidable obstacles, we wouldn't necessarily need to solve it immediately but I'd like your opinion on it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
That definitely makes sense as a goal; I think I've addressed that goal with my most recent commit. To make this work, I've slightly modified the configure script. Previously, Then, the This means each testbed run is completely isolated from past runs; it also means there will be multiple copies of the testbed project and the full "installed" iOS framework - but most of them will be in the build folder, so they're easily cleaned up. I've retained the instructions to purge the "source" version of the testbed as part of the clean target, in case anyone wants to use the older approach of "in tree" testbed builds - my feeling is that an in-tree build will be easier to manage for day-to-day development than the copied version in the build folder. Does that approach makes sense? Is there an alternative approach that I haven't considered? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the previous comments. It's clear that, primarily due to the constraints of using an Xcode-provided simulator and of the necessary iOS framework structure, we are doing a bit of shoehorning here with a mixture of a Unix-style autoconf build and an Xcode project. But, if we can resolve the noted issues, I think this should be a reasonable compromise between the two styles.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| // iOS doesn't like uncaught signals. | ||
| signal(SIGPIPE, SIG_IGN); | ||
| signal(SIGXFSZ, SIG_IGN); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I encountered this on Android as well. It turns out Python normally ignores these signals itself, but this is disabled by default with an isolated config. This can be fixed by adding the following line to the initialization code:
config.install_signal_handlers = 1;
Then it should be possible to remove these manual ignores.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@ned-deily I've pushed an update that I think addresses all the issues you've flagged with the There's one additional piece - in #115774, @pablogsal added a new test module ( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
You can skip it in iOS if you want or add something to configure so it's not compiled in iOS |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for resolving the comments nicely! It looks good to me now, with the caveat that, as advertised, the tests don't yet run under the simulator pending the additional changes to make extension module loading work. Unless someone has an objection, I think we can merge this PR and move on to the next.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The good news is that the next PR in the chain adds the extension module loader, which means the test suite will run (although there are 4 failures for platform/sysconfig-related reasons). That PR is ready to go - I can push it as soon as this one lands. The PR after that one will resolve those 4 test failures. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Hrm... after rebasing with main, I'm getting a weird error I wasn't getting a couple of days ago. Investigating... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Is this the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I did get that one - fixed it by rebuilding the host python. This one is a test failure the importlib loader tests that I wasn't seeing before... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ok - found the problem. It was a weird import cache issue that I hadn't seen previously for some reason. PR incoming. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.