gh-120522: Apply App Store compliance patch to installed products, not source #121830
Conversation
freakboy3742
added
OS-mac
3.12
|
!buildbot iOS |
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 0ce487d 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
I don't know what's going on with the failing thread sanitizer patch - I can't see how it could be related to this PR. The iOS buildbot is also failing, but this is because of #121832, rather than this patch. You can see that the patch is being applied on L4792-4794 of the "Install host Python" build step. |
|
The thread sanitizer CI task appears to have passed on a re-run. |
| # known modifications to the source tree before building. The patch will be | ||
| # applied in a dry-run mode (validating, but not applying the patch) on builds | ||
| # that *have* a compliance patch, but where compliance has not been enabled. |
There was a problem hiding this comment.
You'll have to amend this comment to align it with the new behaviour :)
I may also be worth mentioning that this target is added to INSTALLTARGETS by the configure script.
There was a problem hiding this comment.
Thanks for addressing the issues brought up in the first review. Unfortunately, there are some issues with the revised approach. The pyc generation should be straight-forward to fix as suggested. The problem of having a single .patch file that may include a patch for an optionally installed file should be able to be worked around by ignoring errors but it would be nice to find a slightly cleaner solution while trying to strike a balance between ease-of-use and implementation complexity.
In any case, we are facing the deadline for the last 3.13.0 beta. If we can't get this fixed in time for the release, I think we should consider reverting the original PR for the beta and shoot for rc1 if @Yhg1s is agreeable once we have a fully tested PR.
Opinions?
| echo "$(APP_STORE_COMPLIANCE_PATCH)" > build/app-store-compliant ; \ | ||
| fi | ||
| .PHONY: app-store-compliant | ||
| app-store-compliant: libinstall |
There was a problem hiding this comment.
Continuing to have a separate Makefile rule to do the patch seems like a good idea at first, but unfortunately it doesn't play well with how the libinstall rule works. libinstall does both the install of the lib files and then runs compileall.py to produce the various byte-compiled __pycache__/*.pyc files in the installed lib directory, including those for urllib/parse.py and test/test_urlparse.py. As it stands, they will now get patched after the pycs have been generated, thus invalidating them. (As a side note, when those files are first imported after installation and if the executing process has write access to the installed location, importlib may try to recompile them but, for many reasons, we shouldn't count on that!)
I think we can simplify this by just moving the patch step directly into the libinstall rule after the installs and before the compilealls while testing for APP_STORE_COMPLIANCE_PATCH and avoid adding the changes here and in configure to modify INSTALLTARGETS. But see additional comments below.
There was a problem hiding this comment.
🤦 I completely forgot about the PYC compilation. It's obvious when I think about it.
| fi | ||
| .PHONY: app-store-compliant | ||
| app-store-compliant: libinstall | ||
| patch @APP_STORE_COMPLIANCE_PATCH_FLAGS@ --forward --strip=2 --directory="$(LIBDEST)" --input "$(abs_srcdir)/$(APP_STORE_COMPLIANCE_PATCH)" |
There was a problem hiding this comment.
The install location needs to include DESTDIR:
--directory=""$(DESTDIR)$(LIBDEST)"
Also, should include the --batch option to patch, otherwise patch may pause and ask for input if a file is missing.
| AC_MSG_RESULT([applying default app store compliance patch]) | ||
| ;; | ||
| Darwin) | ||
| # Always *check* the compliance patch on macOS | ||
| APP_STORE_COMPLIANCE_PATCH="Mac/Resources/app-store-compliance.patch" | ||
| APP_STORE_COMPLIANCE_PATCH_TARGET="build/app-store-compliant" | ||
| APP_STORE_COMPLIANCE_PATCH_FLAGS="--dry-run" |
There was a problem hiding this comment.
While reviewing the Makefile changes, I was reminded of the configure --disable-test-modules option. Specifying that causes the Makefile to be generated to not install test directories and files, something which seems like an option that developers of stand-alone apps with an embedded Python might be likely to use. Unfortunately, that causes problems with the current approach of having a single unified default patch file for iOS and macOS builds (i.e. app-store-compliance.patch) since it includes patches for both a non-test and a test file. If a file to be patched is not present, patch fails (somewhat more gracefully if the --batch option is included):
No file to patch. Skipping...
1 out of 1 hunks ignored--saving rejects to 'test/test_urlparse.py.rej'
Can't create '/var/folders/88/pzbbx0t17b58mtgk9hp9v4jh0000gn/T/patchrkhI75QBojY', output is in '/var/folders/88/pzbbx0t17b58mtgk9hp9v4jh0000gn/T/patchrkhI75QBojY': No such file or directory
patching file 'urllib/parse.py'
make: *** [app-store-compliant] Error 1
So the net effect for this patch is kinda OK, since urlib/parse.py was successfully patched and the test/test_urlparse.py.rej cannot be created since there is no test directory. Of course, the user can always specify a different (i.e. edited) patch file that doesn't include the test file patch. As a somewhat ugly hack, we could ignore patch errors and continue on.
It appears that patch --dry-run also fails if the file to be patched does not exist, which means that make install will fail for macOS builds if --disable-test-modules is included on configure and the default patch file is not overridden with --with-app-store-compliance=.... So even with other changes, it might be best to not try to run patch --dry-run by default on macOS builds.
There was a problem hiding this comment.
I didn't know about --disable-test-modules - good to know.
Ignoring patch errors seems like a bad idea to me - we want the patch to be noisy if it can't be applied, so that we know the patch needs to be updated.
Maybe we could split the difference - always do a --dry-run on the app sources directory (so we can verify that the patch is up to date), but then only actually apply the patch if it is required, ignoring patch errors and missing files. Does that make sense?
There was a problem hiding this comment.
I suppose that could work assuming we can trust patch to not try to modify anything and to have the required options. It's a bit worrying that there are various flavors of patch out there including a POSIX standard. Also, the vast majority of builders of Python are not going to be using the patch.
| @@ -0,0 +1,2 @@ | |||
| The app store compliance patch is now applied to the installed products, | |||
| rather than to the original source tree. | |||
There was a problem hiding this comment.
Since the original PR hasn't been in a release yet, there's no need to add a blurb for this change.
|
When you're done making the requested changes, leave the comment: |
|
@Yhg1s, I've marked this as a release-blocker due to the schedule constraints. With holiday time, we just got a first cut (this PR) at addressing the review issues for #120984 which was checked in since the last beta. With time differences et al, we may not have enough time to get this PR ready to go before you cut the upcoming final beta. If so, I think we should consider reverting #120984 (which has not yet been in a release) for the beta as it has the potential to disrupt some downstream builders as it stands. @freakboy3742, @erlend-aasland, what do you think? |
Reverting and deferring a fix for this until after the beta makes sense to me. There's no runtime behavior change, as it's purely a tweak to the build system that helps to make a "review compliant" version of the source. As such, the potential risk of landing this in the RC timeframe seems limited. |
|
I agree. Do you have time to do a revert for main and 3.13 (and close the pending 3.12 PR)? |
I'm about to wrap up after a very long day, but I will have time first thing tomorrow (so... just over 12 hours from now... ). The 3.12 PR (#121174) has already been closed, on the basis that a completely different backport was going to be required. |
Thanks! If you don't mind, I should have time to do the revert now so you don't have to rush and the release can get going earlier. |
No problem at all - revert away! |
|
I agree reverting is the best option for now; thanks for chiming in with your "install target" insights, Ned. There's a lot of stuff I didn't think about here. |
This PR modifies the App Store compliance handling added in #120984 (backported to 3.13 as #121173, 3.12 as #121174).
@ned-deily pointed out in a review after the original PRs were merged that patching the original source location could be problematic as it prevented a single source directory being used for multiple builds. It was also a potential foot-gun, as it would be trivial to accidentally command and submit the patched version of the source files as part of an unrelated PR.
This PR takes a slightly different approach - instead of patching the original source location, it patches the installed location. On macOS, this means the patched version of the file won't be tested (at least, not until we add an explicit installed framework test); but on iOS, the patch is applied to every build, and the
installtarget is a pre-requisite for thetestiostarget, so we will have some validation that the patch remains valid over time.This also allows us to simplify the Makefile.pre.in configuration, as we can use the existing
INSTALLTARGETSvariable rather than adding a variable just for including theapp-store-compliancetarget.As with #120984, I've marked this for backport to 3.12 as the underlying problem exists on 3.12; however, it isn't a security issue in the traditional sense, and addressing the problem requires adding a
configureflag.