Fix: Dont skip optimizations

[RobertDeRose]

Fixes #979

[RobertDeRose]

Looks like 3.13 Makefile removed the || true in the profile task line:

3.12

$(LLVM_PROF_FILE) $(RUNSHARED) ./$(BUILDPYTHON) $(PROFILE_TASK) || true

3.13

$(LLVM_PROF_FILE) $(RUNSHARED) ./$(BUILDPYTHON) $(PROFILE_TASK)

Thus, sadly, the mistake of having optimizations wrongfully disabled has hidden the fact that profiling does not work for 3.13.
The thing that I am curious about is, when I run 3.12.7 build, the tests fail at the same spot as they do for 3.13, but because of the || true the build continues. So, if the tests fail, was it wrong that before 3.13 they called || true or is it wrong they removed it from 3.13?

Anyone here a Python contributor and know the answer?

[edmorley]

So, if the tests fail, was it wrong that before 3.13 they called || true or is it wrong they removed it from 3.13?

I ran into this for our Ubuntu 22.04 builds. There's some context in:
python/cpython#125067

[RobertDeRose]

So, if the tests fail, was it wrong that before 3.13 they called || true or is it wrong they removed it from 3.13?

I ran into this for our Ubuntu 22.04 builds. There's some context in: python/cpython#125067

I have a workaround, just testing it out now. Seems to work. I too found python/cpython#125067 and so I feel the solution is to just restore the profile settings in the Makefile for the time being. I've run the newly generated Dockerfile for 3.13 bullseye and it works, just trying a few more to ensure I didn't miss anything else.

[RobertDeRose]

@yosifkit I've figured out how to re-enable the optimization and work-around the fact that 3.13 changed the Makefile to no longer ignore the failing tests for the system expat (xml) and re-added the timeout.

I also made a few other minor changes, I can remove them if you wish, but the one is just a minor thing that Docker no longer likes ENV thing value and now wants ENV thing=value. The other thing I noticed while testing my changes is that we don't actually remove the __pyache__ directories or the .so modules that are only built for the test suite. If you run configure with --disable-test-modules and without --enable-optimizations the build will not include the now removed files.

 diff so.files.no-tests.txt so.files.optimization.txt
14a15
> _ctypes_test.cpython-312-aarch64-linux-gnu.so
17a19
> _dbm.cpython-312-aarch64-linux-gnu.so
19a22
> _gdbm.cpython-312-aarch64-linux-gnu.so
41a45,52
> _testbuffer.cpython-312-aarch64-linux-gnu.so
> _testcapi.cpython-312-aarch64-linux-gnu.so
> _testclinic.cpython-312-aarch64-linux-gnu.so
> _testimportmultiple.cpython-312-aarch64-linux-gnu.so
> _testinternalcapi.cpython-312-aarch64-linux-gnu.so
> _testmultiphase.cpython-312-aarch64-linux-gnu.so
> _testsinglephase.cpython-312-aarch64-linux-gnu.so
> _tkinter.cpython-312-aarch64-linux-gnu.so
44a56
> _xxtestfuzz.cpython-312-aarch64-linux-gnu.so
65a78
> xxsubtype.cpython-312-aarch64-linux-gnu.so

The only other change I would think might be helpful would be to add:

find /usr -type d -name __pycache__  -exec rm -rf '{}' +;

To also remove the __pycache__ files from the distro python that is used by the build script to generate some of the files.

[tianon]

Yes, please drop 8f2e9c5 (moby/buildkit#5130). ❤️

Also, I'd prefer to see 2d4d8c5 keep the single-find-invocation so we continue to only scan that directory tree once. 👀

As for working around python/cpython#125067, I think I'd rather apply Ed's patch from https://github.com/heroku/heroku-buildpack-python/raw/99684a6ffdb23e50a9a2ee0b59ddf3c5e865ea5d/builds/python-3.13-ubuntu-22.04-libexpat-workaround.patch directly (with sha256 verification) than a sed that works today but might stop and we not notice (which is exactly the kind of oversight we're fixing here, and probably the exact justification used upstream for removing the || true 😅 ❤️).

On a separate note, I'd suggest avoiding issue/PR references in commit messages, because they tend to lead to "references spam" on the issue as the commit gets updated over time; see #979 (reference) for an example of the effect of this. 😬

[RobertDeRose]

@tianon I see you are deeply against the ENV change, so I will drop it.

As for the single line find statement, do you not care that there are still __pycache__ directories or test module .so files?
Or do you just want me to modify the find statement to do it all in one go? To be fair, the tree depth is rather shallow so the added clarity I thought was a win. But I'll make the change however you request.

I'll also remove the commit reference.

[RobertDeRose]

@tianon Oh, forgot, so for the preferred method of using the patch, how would you prefer this be done?

Including an inline heredoc or a conditional COPY statement in the Dockerfile to include the patch and apply it?

[RobertDeRose]

@tianon I never heard back, so I assumed an inline patch was the more acceptable solution. I did not follow what you wanted done with the sha check, I assume you meant, to check the test suite for file to see if the file had been changed since last time the patch was made, but that seemed really fragile. Instead, if the patch applies, it applies, if it doesn't, it just skips it and the build will either fail again or perhaps upstream corrected the test to make them pass on older systems (unlikely).

I'm not sure this solution is any better than my proposed sed command replacement, as this still leaves the timeout value at "" vs mine reset it to 1200 again.

Both solutions will break as soon as upstream changes either file, but the sed is more likely be less fragile as it would only modify the file if the issue is still in the same state.

I've also removed the issue reference, dropped the ENV change and returned to the one line find but just included the extra parameters in there.

I hope this PR now meets your requirements for inclusion. Thanks

[RobertDeRose]

after all the testing of the sed and the patch... I just realized I could have just specifically set the PROFILE_TASK to the same value that is used in 3.12....

Oh, well, if one of the maintainers would just let me know what they want me to do, if anything.

[edmorley]

Fixes #393

Should this be #979? :-)

[RobertDeRose]

Fixes #393

Should this be #979? :-)

Yes, sorry, GitHub autocomplete got me and I didn't notice.

[RobertDeRose]

This was the recommended change (to the be of my understanding of @tianon comment) to my previous attempt at using a sed to update the Makefile.

However, I realized after all that, I could just conditionally set PROFILE_TASK to be the same value used in Python 3.12

PROFILE_TASK=./python -m test --pgo --timeout=1200 || true

So I am just waiting for one of the maintainer to let me know if that would be a better solution than relying on a sed or patch that could eventually fail, where as setting the PROFILE_TASK explicitly just for 3.13 for these distros that currently have the failing expat test, would be the simpler, smaller, and thus cleaner solution.

[tianon]

Oh, well, if one of the maintainers would just let me know what they want me to do, if anything.

Please, be patient -- this isn't the only thing we work on/maintain, and I promise this issue is/you are in the queue 🙇 ❤️

I did not follow what you wanted done with the sha check

What I meant was actually using wget to download Ed's patch as-is and use a sha256sum to verify the integrity of our download before we apply the patch.

However, what I realized while looking into this later is that Ed's patch doesn't completely fix this for us as-is (only for our Bullseye failures) -- the Alpine failures are test_math and test_re, not the same XML-related failures as Bullseye.

I don't love hard-coding us back to the || true upstream used to use, because failures in these test runs are concerning and something we should be either dealing with and resolving or evaluating whether we accept the risk of the failing test (in the case of the XML-related failures we're seeing, I think that's an upstream bug in the tests, but I think that's likely the more rare case and the Alpine failures are probably indicative of deeper problems, especially given that Alpine/musl is not an officially supported platform for running Python 😞).

All of this has me thinking that we should probably be running the full upstream test suite somewhere/somehow (at least the "default" full set, perhaps not including the "tests [which] use special kinds of resources" referenced in https://devguide.python.org/testing/run-write-tests/), which is only tangentially related to this PR, but this PR is what exposes the oversight so IMO it's very relevant/on-topic and something we need to resolve at least with a plan as part of this PR (especially if we decide to continue ignoring some or all failures in the upstream tests again).

I was hoping that python -m test --pgo would play nicely with --exclude -- I'm not familiar with the code at all, but https://github.com/python/cpython/blob/a5a7f5e16d8c3938d266703ea8fba8ffee3e3ae5/Lib/test/libregrtest/main.py#L186-L200 seemed like it might, and that might be an easier way for us to configure this sanely with different sets of tests for Bullseye vs Alpine such that we're only ignoring the tests we know are failing, and future failures will be something we notice and catch. However, in testing, when I added the necessary excludes, our list of tests being run exploded from 44 to 476, so clearly --pgo and --exclude are exclusive, and the latter swaps the behavior to the "explicit list of tests have been specified" mode. 😞

My other thought as a small workaround that's going to be easy to maintain/sustain and not likely to have unexpected results in the future was to lean on the fact that https://github.com/python/cpython/blob/a5a7f5e16d8c3938d266703ea8fba8ffee3e3ae5/Lib/test/libregrtest/pgo.py#L34 is pretty reasonably maintained and line-based, so we could do something like sed -i -e '/test_math/d' to remove the specific tests we're concerned about from the list.

I don't think this is the 100% final version of what I'd like to see, but for full context here's what I've been testing with (successfully) locally:

Diff:

diff --git a/Dockerfile-linux.template b/Dockerfile-linux.template
index a0b86ca..18f3619 100644
--- a/Dockerfile-linux.template
+++ b/Dockerfile-linux.template
@@ -165,6 +165,22 @@ RUN set -eux; \
 	mkdir -p /usr/src/python; \
 	tar --extract --directory /usr/src/python --strip-components=1 --file python.tar.xz; \
 	rm python.tar.xz; \
+{{ if is_alpine or (env.variant | contains("bullseye")) then ( -}}
+	\
+# https://github.com/docker-library/python/pull/980
+# https://github.com/python/cpython/blob/a5a7f5e16d8c3938d266703ea8fba8ffee3e3ae5/Lib/test/libregrtest/pgo.py
+	sed -i {{
+		if is_alpine then
+			# TODO ideally we would also add a comment justifying these (and ideally our "justification" comments would be part of the generated Dockerfile)
+			[ "test_math", "test_re" ]
+		else
+			# https://github.com/python/cpython/issues/125067
+			[ "test_xml_etree", "test_xml_etree_c" ]
+		end
+		| map("-e " + ("/[^a-z]\(.)[^a-z]/d" | @sh))
+		| join(" ")
+	}} /usr/src/python/Lib/test/libregrtest/pgo.py; \
+{{ ) else "" end -}}
 	\
 	cd /usr/src/python; \
 	gnuArch="$(dpkg-architecture --query DEB_BUILD_GNU_TYPE)"; \
@@ -206,7 +222,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:-}" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 	; \
 # https://github.com/docker-library/python/issues/784
 # prevent accidental usage of a system installed libpython of the same version
@@ -214,7 +229,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:--Wl},-rpath='\$\$ORIGIN/../lib'" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 		python \
 	; \
 	make install; \

(To be very clear, I am not asking for further changes to the PR yet; I think this is still in the documenting/discussing/experimentation phase as we continue to work out how to best navigate this. ❤️)

[tianon]

I was hoping that python -m test --pgo would play nicely with --exclude -- I'm not familiar with the code at all, but https://github.com/python/cpython/blob/a5a7f5e16d8c3938d266703ea8fba8ffee3e3ae5/Lib/test/libregrtest/main.py#L186-L200 seemed like it might, and that might be an easier way for us to configure this sanely with different sets of tests for Bullseye vs Alpine such that we're only ignoring the tests we know are failing, and future failures will be something we notice and catch. However, in testing, when I added the necessary excludes, our list of tests being run exploded from 44 to 476, so clearly --pgo and --exclude are exclusive, and the latter swaps the behavior to the "explicit list of tests have been specified" mode. 😞

Applying the following patch to Python upstream would probably fix this particular edge case and allow --pgo and --exclude to co-exist (and --tsan which appears to be something similar to --pgo in that it's a pre-canned set of tests to run for some specific purpose), although I haven't tested it (and am still debating whether it's worth trying to propose upstream):

Diff:

diff --git a/Lib/test/libregrtest/main.py b/Lib/test/libregrtest/main.py
index 2ef4349552b..574acfd1194 100644
--- a/Lib/test/libregrtest/main.py
+++ b/Lib/test/libregrtest/main.py
@@ -183,6 +183,12 @@ def find_tests(self, tests: TestList | None = None) -> tuple[TestTuple, TestList
 
         strip_py_suffix(tests)
 
+        exclude_tests = set()
+        if self.exclude:
+            for arg in self.cmdline_args:
+                exclude_tests.add(arg)
+            self.cmdline_args = []
+
         if self.pgo:
             # add default PGO tests if no tests are specified
             setup_pgo_tests(self.cmdline_args, self.pgo_extended)
@@ -190,12 +196,6 @@ def find_tests(self, tests: TestList | None = None) -> tuple[TestTuple, TestList
         if self.tsan:
             setup_tsan_tests(self.cmdline_args)
 
-        exclude_tests = set()
-        if self.exclude:
-            for arg in self.cmdline_args:
-                exclude_tests.add(arg)
-            self.cmdline_args = []
-
         alltests = findtests(testdir=self.test_dir,
                              exclude=exclude_tests)
 

[tianon]

I was going down this road further and making the changes more defensive, explicit, and commented and came up with the following:

Diff:

diff --git a/Dockerfile-linux.template b/Dockerfile-linux.template
index a0b86ca..1fa2f77 100644
--- a/Dockerfile-linux.template
+++ b/Dockerfile-linux.template
@@ -165,6 +165,43 @@ RUN set -eux; \
 	mkdir -p /usr/src/python; \
 	tar --extract --directory /usr/src/python --strip-components=1 --file python.tar.xz; \
 	rm python.tar.xz; \
+{{ if is_alpine or (env.variant | contains("bullseye")) then ( -}}
+	\
+# https://github.com/docker-library/python/pull/980
+# https://github.com/python/cpython/blob/a5a7f5e16d8c3938d266703ea8fba8ffee3e3ae5/Lib/test/libregrtest/pgo.py
+{{
+	[
+		if is_alpine then
+			"# https://github.com/python/cpython/issues/90548",
+			"test_re",
+			"test_math",
+
+			empty # trailing comma
+		else
+			"# https://github.com/python/cpython/issues/125067",
+			"test_xml_etree",
+			"test_xml_etree_c",
+
+			empty # trailing comma
+		end
+		| (
+			if startswith("#") then
+				# preserve comments as-is
+				. += "\n"
+			else
+				"\\b\(.)\\b"
+				| (
+-}}
+	grep -nE {{ @sh }} /usr/src/python/Lib/test/libregrtest/pgo.py; \
+	sed -i -e {{ "/\(.)/d" | @sh }} /usr/src/python/Lib/test/libregrtest/pgo.py; \
+	if grep -nE {{ @sh }} /usr/src/python/Lib/test/libregrtest/pgo.py; then exit 1; fi; \
+{{
+				)
+			end
+		)
+	] | add
+-}}
+{{ ) else "" end -}}
 	\
 	cd /usr/src/python; \
 	gnuArch="$(dpkg-architecture --query DEB_BUILD_GNU_TYPE)"; \
@@ -206,7 +243,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:-}" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 	; \
 # https://github.com/docker-library/python/issues/784
 # prevent accidental usage of a system installed libpython of the same version
@@ -214,7 +250,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:--Wl},-rpath='\$\$ORIGIN/../lib'" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 		python \
 	; \
 	make install; \

However, what I really like about Ed's patch vs this is that it's explicitly skipping only the 3-6 tests that fail, not the entire test_xxx group of tests. I thought about ways we could maybe automate generating patches, maintaining them, and distributing them in a way that doesn't have icky downstream effects for us, but didn't come up with anything good. After doing all that, I realized that there's an easier answer for the purposes of this PR: we can simply not enable PGO for the failing builds (for now).

At some point, upstream might fix the Bullseye failures (that's python/cpython#125067), but the Alpine failures are likely here to stay for the long-term (python/cpython#90548). Alpine is also not an officially supported upstream target (they've had a worker running the tests for a while now, but every single build fails quite a few tests that include the ones we see failing here in the PGO subset, test_re and test_math).

Arguably, we probably should deprecate and remove our Alpine builds accordingly, but that's a completely orthogonal proposal to this one (and one that's likely a lot more controversial).

So, here's what I'm actually proposing, more concretely:

Diff:

diff --git a/Dockerfile-linux.template b/Dockerfile-linux.template
index a0b86ca..05f38b5 100644
--- a/Dockerfile-linux.template
+++ b/Dockerfile-linux.template
@@ -172,13 +172,12 @@ RUN set -eux; \
 		--build="$gnuArch" \
 		--enable-loadable-sqlite-extensions \
 {{
-	# skip optimizations on alpine on riscv64 (except python 3.9)
-	# only 3.9 completes building on riscv64 with optimizations, 3.10-3.13 all hit the 3 hour limit
-	if (is_alpine | not) or rcVersion == "3.9" then (
+	# https://github.com/docker-library/python/pull/980 (fixing PGO runs tests that fail, but shouldn't)
+	# https://github.com/python/cpython/issues/125067 (bullseye failures; might be fixed in future upstream releases)
+	# https://github.com/python/cpython/issues/90548 (alpine failures; not likely to be fixed any time soon)
+	if (env.variant | contains("bullseye")) or is_alpine then "" else (
 -}}
 		--enable-optimizations \
-{{ ) else ( -}}
-		$(test "$gnuArch" != 'riscv64-linux-musl' && echo '--enable-optimizations') \
 {{ ) end -}}
 		--enable-option-checking=fatal \
 		--enable-shared \
@@ -206,7 +205,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:-}" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 	; \
 # https://github.com/docker-library/python/issues/784
 # prevent accidental usage of a system installed libpython of the same version
@@ -214,7 +212,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:--Wl},-rpath='\$\$ORIGIN/../lib'" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 		python \
 	; \
 	make install; \

@yosifkit thoughts? I considered making the conditional for Bullseye more particular (so it would only match > 3.12), but in the end I don't know that it's actually worth it, as our current builds aren't optimized anyhow and users should really be looking to get off Bullseye ASAP, but if the tests get fixed upstream (or we figure out that it's something we're doing and have control over and fix it), it's easy to re-enable.

[tianon]

Regarding the follow-on changes here (__pycache__ and *test*.so), I think I need to back up a little bit to understand better what's happening.

For the former (__pycache__ directories), I believe they exist, but are empty, which is fine, right? Does fixing PGO fill them with contents for some reason in spite of our setting PYTHONDONTWRITEBYTECODE=1? (I'd be surprised if so, and would think that's an upstream bug, but perhaps there's a rationale for it if so.)

For the latter (*test*.so files), is there anything we can do to make this less prone to matching unrelated files in the future that happen to contain test for some reason? Looking at the diff you shared above, I don't see a single more reliable pattern, but I think we could maybe create a set of patterns? Maybe there's an upstream Makefile target that will clean them up for us, or some way we can ask that it put those in a different place so that they don't become part of what's installed with make install? For them to show up as a result of this fix, they've got to be generated during the tests, which seems weird, but seems like something that we might be able to control in some way. 🤞

[yosifkit]

thoughts?

I think disabling optimizations where the tests to create them fail is better than trying to use patch or sed to skip them. With it affecting just Alpine* (i.e. musl and thus unsupported by Python upstream), then the maintenance of the patch isn't something I want to add here.

* I was able to successfully build with optimizations on arm64v8 by dropping --with-system-expat, so we can keep the optimizations on Bullseye. It was added way back in #205 and seems to be fine without it now (but could possibly break some arches on Bookworm? 😞). I am currently running a build test for 3.13-bookworm on s390x (user-mode qemu) to see if there are immediate red flags with removing --with-system-expat.

Diff:

diff --git a/Dockerfile-linux.template b/Dockerfile-linux.template
index a0b86ca9..ecb9c541 100644
--- a/Dockerfile-linux.template
+++ b/Dockerfile-linux.template
@@ -99,7 +99,6 @@ RUN set -eux; \
 		bluez-dev \
 		bzip2-dev \
 		dpkg-dev dpkg \
-		expat-dev \
 		findutils \
 		gcc \
 		gdbm-dev \
@@ -133,7 +132,6 @@ RUN set -eux; \
 		libbz2-dev \
 		libc6-dev \
 		libdb-dev \
-		libexpat1-dev \
 		libffi-dev \
 		libgdbm-dev \
 		liblzma-dev \
@@ -172,13 +170,11 @@ RUN set -eux; \
 		--build="$gnuArch" \
 		--enable-loadable-sqlite-extensions \
 {{
-	# skip optimizations on alpine on riscv64 (except python 3.9)
-	# only 3.9 completes building on riscv64 with optimizations, 3.10-3.13 all hit the 3 hour limit
-	if (is_alpine | not) or rcVersion == "3.9" then (
+	# https://github.com/docker-library/python/pull/980 (fixing PGO runs tests that fail, but shouldn't)
+	# https://github.com/python/cpython/issues/90548 (alpine failures; not likely to be fixed any time soon)
+	if is_alpine then "" else (
 -}}
 		--enable-optimizations \
-{{ ) else ( -}}
-		$(test "$gnuArch" != 'riscv64-linux-musl' && echo '--enable-optimizations') \
 {{ ) end -}}
 		--enable-option-checking=fatal \
 		--enable-shared \
@@ -188,7 +184,6 @@ RUN set -eux; \
 -}}
 		--with-lto \
 {{ ) end -}}
-		--with-system-expat \
 		--with-ensurepip \
 	; \
 	nproc="$(nproc)"; \
@@ -206,7 +201,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:-}" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 	; \
 # https://github.com/docker-library/python/issues/784
 # prevent accidental usage of a system installed libpython of the same version
@@ -214,7 +208,6 @@ RUN set -eux; \
 	make -j "$nproc" \
 		"EXTRA_CFLAGS=${EXTRA_CFLAGS:-}" \
 		"LDFLAGS=${LDFLAGS:--Wl},-rpath='\$\$ORIGIN/../lib'" \
-		"PROFILE_TASK=${PROFILE_TASK:-}" \
 		python \
 	; \
 	make install; \

[tianon]

Maybe there's an upstream Makefile target that will clean them up for us

There is a make cleantest that does something, but it's described like this:

# Remove "test_python_*" directories of previous failed test jobs.

... which doesn't sound like what we're seeing, but maybe it is?

edit: more fun targets, but probably not super relevant for us:

# Sanitation targets -- clean leaves libraries, executables and tags
# files, which clobber removes as well
.PHONY: pycremoval
pycremoval:
	-find $(srcdir) -depth -name '__pycache__' -exec rm -rf {} ';'
	-find $(srcdir) -name '*.py[co]' -exec rm -f {} ';'

.PHONY: rmtestturds
rmtestturds:
	-rm -f *BAD *GOOD *SKIPPED
	-rm -rf OUT
	-rm -f *.TXT
	-rm -f *.txt
	-rm -f gb-18030-2000.xml

[yosifkit]

I am currently running a build test for 3.13-bookworm on s390x (user-mode qemu) to see if there are immediate red flags with removing --with-system-expat.

The end result was a successful build (and it seems to run fine via qemu).

[tianon]

Alpine is also not an officially supported upstream target (they've had a worker running the tests for a while now, but every single build fails quite a few tests that include the ones we see failing here in the PGO subset, test_re and test_math).

Arguably, we probably should deprecate and remove our Alpine builds accordingly, but that's a completely orthogonal proposal to this one (and one that's likely a lot more controversial).

I've gone through the upstream Alpine buildbot logs and read through every FAIL: and almost all of them are locale-related, which is expected (https://wiki.musl-libc.org/open-issues#Locale-limitations). There are two failures that do not appear to be locale-related; one is test_fma_zero_result (test.test_math.FMATests.test_fma_zero_result) ... FAIL (AssertionError: False is not true : Expected a negative zero, got 0.0) and the other is FAIL: test_fpathconf (test.test_os.TestInvalidFD.test_fpathconf) (AssertionError: <built-in function pathconf> didn't raise an OSError with a bad file descriptor).

While I agree these are red flags and should be fixed, the situation is not as dire as https://gitlab.alpinelinux.org/alpine/aports/-/blob/be58be55d2b544e38696bd918b2051afb7b25b1b/main/python3/APKBUILD#L161-175 had me thinking it might be, and our Alpine builds do "mostly" work (to the degree you might reasonably expect from musl), so I don't think it's unreasonable for us to keep maintaining/providing them (edit: noted upstream in python/cpython#90548 (comment) now).

However, I do still think it's very reasonable to exclude profile optimizations from our Alpine builds -- they're intended to be small in disk size, not necessarily fast.

[tianon]

Oh, I was working on an incorrect assumption WRT *test*.so -- I thought that us successfully running python -m test by fixing our PROFILE_TASK was what was creating those, but those are created as part of the build as-is, and in fact exist in our current built/published images:

$ docker run --rm --pull=always python find /usr/local -name '*test*.so'
latest: Pulling from library/python
Digest: sha256:0a301600e451618e1c0a94c28b5d83f875f6f3c07820a71d6dd2565a000f7408
Status: Image is up to date for python:latest
/usr/local/lib/python3.13/lib-dynload/_testmultiphase.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_ctypes_test.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testclinic.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testinternalcapi.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testbuffer.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testclinic_limited.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testsinglephase.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testexternalinspection.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testcapi.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testlimitedcapi.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_testimportmultiple.cpython-313-x86_64-linux-gnu.so
/usr/local/lib/python3.13/lib-dynload/_xxtestfuzz.cpython-313-x86_64-linux-gnu.so

$ docker run --rm --pull=always python find /usr/local -name '*test*.so' -exec du -hc '{}' + | tail -1
latest: Pulling from library/python
Digest: sha256:0a301600e451618e1c0a94c28b5d83f875f6f3c07820a71d6dd2565a000f7408
Status: Image is up to date for python:latest
3.0M	total

As seen in this second command, they're also only 3MiB, so I think I'd prefer if we open a new PR/issue to discuss those (as they're fully orthogonal to fixing our PROFILE_TASK oversight). 🙇

So, I think @yosifkit's patch in #980 (comment) is all we need (and I'll plan to push to this PR branch shortly with that change so we can get it in). 👍 ❤️

[tianon]

(small premature force-push; https://github.com/docker-library/python/compare/caad86d8f1879bfa700f9074ee09d573e70a41ae..37a6827e0b7a9ef099cfdec5de305e3d4cea7331 is the best comparison link that combines those two force pushes, which also includes a rebase especially to pull in #981)

[RobertDeRose]

@tianon Sorry, wasn't trying to be impatient. Thanks for all the thoughtful comments.