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.

Sign up for GitHub

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

Jump to bottom

gh-120522: Apply App Store compliance patch to installed products, not source #121830

Closed
wants to merge 2 commits into from
Closed

gh-120522: Apply App Store compliance patch to installed products, not source #121830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b348a61
Apply the app store compliance patch to the installed product, not th…
freakboy3742 Jul 16, 2024
0ce487d
Add blurb.
freakboy3742 Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions Makefile.pre.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ list-targets:
@grep -E '^[A-Za-z][-A-Za-z0-9]+:' Makefile | awk -F : '{print $$1}'

.PHONY: build_all
build_all: check-clean-src @APP_STORE_COMPLIANCE_PATCH_TARGET@ $(BUILDPYTHON) platform sharedmods \
build_all: check-clean-src $(BUILDPYTHON) platform sharedmods \
gdbhooks Programs/_testembed scripts checksharedmods rundsymutil

.PHONY: build_wasm
Expand Down Expand Up @@ -936,12 +936,9 @@ $(BUILDPYTHON)-gdb.py: $(SRC_GDB_HOOKS)
# 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.
Comment on lines 936 to 938
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.

build/app-store-compliant:
patch @APP_STORE_COMPLIANCE_PATCH_FLAGS@ --forward --strip=1 --directory="$(srcdir)" --input "$(APP_STORE_COMPLIANCE_PATCH)"
@if test "@APP_STORE_COMPLIANCE_PATCH_FLAGS@" == ""; then \
mkdir -p build ; \
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.

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.


# This rule is here for OPENSTEP/Rhapsody/MacOSX. It builds a temporary
# minimal framework (not including the Lib directory and such) in the current
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Build/2024-07-16-11-18-43.gh-issue-120522.exdb3D.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

11 changes: 4 additions & 7 deletions configure
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 4 additions & 6 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,17 +710,17 @@ AC_ARG_WITH(
Darwin|iOS)
# iOS is able to share the macOS patch
APP_STORE_COMPLIANCE_PATCH="Mac/Resources/app-store-compliance.patch"
APP_STORE_COMPLIANCE_PATCH_TARGET="build/app-store-compliant"
APP_STORE_COMPLIANCE_PATCH_FLAGS=
INSTALLTARGETS="$INSTALLTARGETS app-store-compliant"
;;
*) AC_MSG_ERROR([no default app store compliance patch available for $ac_sys_system]) ;;
esac
AC_MSG_RESULT([applying default app store compliance patch])
;;
*)
APP_STORE_COMPLIANCE_PATCH="${withval}"
APP_STORE_COMPLIANCE_PATCH_TARGET="build/app-store-compliant"
APP_STORE_COMPLIANCE_PATCH_FLAGS=
INSTALLTARGETS="$INSTALLTARGETS app-store-compliant"
AC_MSG_RESULT([applying custom app store compliance patch])
;;
esac
Expand All @@ -729,28 +729,26 @@ AC_ARG_WITH(
iOS)
# Always apply the compliance patch on iOS; we can use the macOS patch
APP_STORE_COMPLIANCE_PATCH="Mac/Resources/app-store-compliance.patch"
APP_STORE_COMPLIANCE_PATCH_TARGET="build/app-store-compliant"
APP_STORE_COMPLIANCE_PATCH_FLAGS=
INSTALLTARGETS="$INSTALLTARGETS app-store-compliant"
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.

INSTALLTARGETS="$INSTALLTARGETS app-store-compliant"
AC_MSG_RESULT([checking (not applying) default app store compliance patch])
;;
*)
# No app compliance patching on any other platform
APP_STORE_COMPLIANCE_PATCH=
APP_STORE_COMPLIANCE_PATCH_TARGET=
APP_STORE_COMPLIANCE_PATCH_FLAGS=
AC_MSG_RESULT([not patching for app store compliance])
;;
esac
])
AC_SUBST([APP_STORE_COMPLIANCE_PATCH])
AC_SUBST([APP_STORE_COMPLIANCE_PATCH_TARGET])
AC_SUBST([APP_STORE_COMPLIANCE_PATCH_FLAGS])

AC_SUBST([_PYTHON_HOST_PLATFORM])
Expand Down
Loading