unix: build test extensions as shared libraries; enable _testcapi#1025
unix: build test extensions as shared libraries; enable _testcapi#1025
Conversation
9cd0276 to
1c77491
Compare
895a59c to
15c329a
Compare
|
I'm going to continue iterating on this to get musl shared builds to behave better. |
fb5f7c3 to
77b14cd
Compare
geofft
left a comment
There was a problem hiding this comment.
Seems good. The only thing I'm wondering is whether we want to ship these test extensions in the install-only-stripped build or drop them—most users will not need them.
| sources: | ||
| - _testcapimodule.c | ||
| sources-conditional: | ||
| - source: _testcapi/abstract.c |
There was a problem hiding this comment.
Do we want to support _testcapi/*.c? I feel like we're going to forget to update this in newer verisions. (But maybe the real answer is that we should figure out how to use upstream's build system / what to fix in upstream's build system in newer versions.)
There was a problem hiding this comment.
Yes. This missing extension fails dozens of tests and causes a lot of chaos in the stdlib test suite. We want to keep it around.
If we forget to update the source files list, the extension fails to load on at least macOS due to missing symbol resolution at load time. So failures will be pretty obvious once we enable the stdlib test harness.
There was a problem hiding this comment.
Sorry, I meant, would it be preferable to add syntax support for wildcards, so we could write literally - source: _testcapi/*.c instead of having to keep the expanded list in the YAML file? But the point that we'll see either build or test failures quickly is a good one.
There was a problem hiding this comment.
Oh, morning brain didn't grok your initial comment :)
I think this should be doable. Nice quality of life win to make ongoing Python version updates simpler.
But I'd prefer to do it as a follow-up since it is scope creep.
04d7b41 to
3bc54d4
Compare
These extensions are only used by the stdlib tests AFAICT. So it makes sense to strip them from the install-only artifacts. |
| sources: | ||
| - _testcapimodule.c | ||
| sources-conditional: | ||
| - source: _testcapi/abstract.c |
There was a problem hiding this comment.
Sorry, I meant, would it be preferable to add syntax support for wildcards, so we could write literally - source: _testcapi/*.c instead of having to keep the expanded list in the YAML file? But the point that we'll see either build or test failures quickly is a good one.
3bc54d4 to
fcdc23e
Compare
Way back when we only supported statically linked extension modules. And we had to disable `_testcapi` because it wouldn't build statically. Now that we support building extension modules as shared libraries, it makes sense for extension modules in support of tests to not be part of libpython and to be shared libraries. This commit makes that change. And since `_testcapi` is buildable as a shared library, we enable it. In doing so, we eliminate the largest single source of test failures.
fcdc23e to
d5de681
Compare
These extensions are only needed to support the stdlib tests, which we strip from these archives. These extensions offer no utility. So don't ship them. This is a follow-up to #1025. Implementing as its own PR since we remove previously existing extension modules. This is backwards incompatible and deserves to be discussed on its own merits.
Way back when we only supported statically linked extension modules. And we had to disable
_testcapiand_testlimitedcapibecause it wouldn't build statically.Now that we support building extension modules as shared libraries, we can enable
_testcapiand_testlimitedcapias shared libraries.This commit makes that change.
By making this change, we eliminate the largest single source of test failures. But this impact is not captured in code or CI since we currently don't run the stdlib tests in CI.
Enabling the new extensions allows tests - including PGO training tests - to exercise new code paths. This resulted in a new segfault appearing in test_bytes. Why, I'm not sure. (My best guess is it has something to do with calling variadic functions via ctypes.) This particular crash was likely a latent issue. We ignore this crashing test during PGO training as a workaround.
In order to get the extensions enabled on musl shared builds - but not static - we had to introduce some new logic to conditionally enable a shared extension on supported targets while automatically disabling on static targets. There may be a cleaner way to express this semantic intent. But the implemented solution does work.