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-120522: Apply App Store compliance patch to installed products, not source #121830

Closed
wants to merge 2 commits into from

Conversation

freakboy3742
Copy link
Contributor

@freakboy3742 freakboy3742 commented Jul 16, 2024

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 install target is a pre-requisite for the testios target, 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 INSTALLTARGETS variable rather than adding a variable just for including the app-store-compliance target.

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 configure flag.

@freakboy3742
Copy link
Contributor Author

!buildbot iOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 0ce487d 🤖

The command will test the builders whose names match following regular expression: iOS

The builders matched are:

  • iOS ARM64 Simulator PR

@freakboy3742
Copy link
Contributor Author

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.

@freakboy3742
Copy link
Contributor Author

The thread sanitizer CI task appears to have passed on a re-run.

Comment on lines 936 to 938
# 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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.

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

Choose a reason for hiding this comment

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

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.

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 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)"
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Since the original PR hasn't been in a release yet, there's no need to add a blurb for this change.

@bedevere-app
Copy link

bedevere-app bot commented Jul 16, 2024

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@ned-deily
Copy link
Member

@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?

@freakboy3742
Copy link
Contributor Author

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

@ned-deily
Copy link
Member

I agree. Do you have time to do a revert for main and 3.13 (and close the pending 3.12 PR)?

@freakboy3742
Copy link
Contributor Author

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.

@ned-deily
Copy link
Member

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

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.

@freakboy3742
Copy link
Contributor Author

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!

@erlend-aasland
Copy link
Contributor

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.

@ned-deily
Copy link
Member

OK, thanks! The original change has now been reverted from both main (#121844) and 3.13 (#121845) so 3.13.0b4 can proceed.

@freakboy3742
Copy link
Contributor Author

Given this no longer applies after the revert of #120984, I've opted to start from scratch with a clean implementation, rather than navigate a messy merge/rebase. See #121947 for the new version.

@freakboy3742 freakboy3742 deleted the defer-appstore-patch branch July 18, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes awaiting changes DO-NOT-MERGE needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes OS-ios OS-mac
Projects
Development

Successfully merging this pull request may close these issues.

4 participants