-
-
Notifications
You must be signed in to change notification settings - Fork 143
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
Add support for freethreaded builds of Python 3.13 #326
Conversation
optimizations: 'debug' | ||
options: 'debug' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I added a separate freethreaded: true
flag but defining all the build options at once makes passing them around much easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be variant
? Or flavor
, as they're called in the docs? Don't feel strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the documentation, this isn't the "flavor" it's the "build configuration". The "flavors" are install_only
/ full
. These are options to configure the build.
Instead of --options
and build_options
we could do --config
and build_config
but I don't think that's clearer.
cpython-unix/build-cpython.sh
Outdated
if [ -n "${CPYTHON_FREETHREADED}" ]; then | ||
CONFIGURE_FLAGS="${CONFIGURE_FLAGS} --disable-gil" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "key" change.
This comment was marked as outdated.
This comment was marked as outdated.
Notes on failures Resolved issuesbuild-313 (x86_64_v4-unknown-linux-musl, cpython-3.13, lto)
build-313 (x86_64_v4-unknown-linux-gnu, cpython-3.13, freethreaded+lto)
build-313 (ppc64le-unknown-linux-gnu, cpython-3.13, freethreaded+lto)
All the LTO failures are addressed by 0a53b06 which was a mistake in the options refactor. build-313 (x86_64_v4-unknown-linux-musl, cpython-3.13, freethreaded+debug
The musl builds are still on LLVM 14 which doesn't have C11 support as far as I can tell — we need to upgrade it to unblock. However, this is relatively low-priority since the musl builds are generally unusable (they don't allow loading Python extension modules). I disabled them in a01c634 |
We will work on these separately
if [ -n "${CPYTHON_FREETHREADED}" ]; then | ||
PYTHON_BINARY_SUFFIX=t | ||
PYTHON_LIB_SUFFIX=t | ||
else | ||
PYTHON_BINARY_SUFFIX= | ||
PYTHON_LIB_SUFFIX= | ||
fi | ||
if [ -n "${CPYTHON_DEBUG}" ]; then | ||
PYTHON_BINARY_SUFFIX="${PYTHON_BINARY_SUFFIX}d" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do flag in lexicographic order, at least i remember them being in this order, even if the PEP doesn't talk about order. I tried to to look in cpython but didn't find anything about order either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh sys.abiflags
say td
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ordering matches the order that is produced by CPython; we're not choosing the order, we're just constructing these variables to pull artifacts from the builds.
"--optimizations", | ||
choices={"debug", "noopt", "pgo", "lto", "pgo+lto"}, | ||
"--options", | ||
choices=optimizations.union({f"freethreaded+{o}" for o in optimizations}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I might just construct this inline on line 56, rather than splitting the construction between there and here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit annoying to construct in one line. Are you suggesting I just enumerate them all manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting:
optimizations = {"debug", "noopt", "pgo", "lto", "pgo+lto"}
optimizations = optimizations.union({f"freethreaded+{o}" for o in optimizations})
parser.add_argument(..., choices=optimizations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for clarifying. It'd be options = optimizations.union(...)
and I'm hesitant to introduce another local variable (basically just personal preference).
msvc_version: str, | ||
windows_sdk_version: str, | ||
openssl_archive, | ||
libffi_archive, | ||
openssl_entry: str, | ||
) -> pathlib.Path: | ||
pgo = profile == "pgo" | ||
parsed_build_options = set(build_options.split("+")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this wrong before? Or was it just never called with pgo+lto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah there are only "pgo" builds on Windows. I just changed this for consistency.
See https://github.com/indygreg/python-build-standalone/pull/326/files#r1777582385
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this seems like a good change.
# TODO: Rename this to `--options` to match the Unix build script | ||
optimizations = {"debug", "noopt", "pgo", "lto", "pgo+lto"} | ||
parser.add_argument( | ||
"--profile", | ||
choices={"noopt", "pgo"}, | ||
"--options", | ||
choices=optimizations.union({f"freethreaded+{o}" for o in optimizations}), | ||
default="noopt", | ||
help="How to compile Python", | ||
help="Build options to apply when compiling Python", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things to follow up on here
- I renamed this, I should remove the stale comment.
- I added more optimization types here, that's wrong I should remove the other options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Hesitant to do that in this pull request unless there are other changes since it takes so long to do builds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff --git a/cpython-windows/build.py b/cpython-windows/build.py
index 3425a3b..92bca7d 100644
--- a/cpython-windows/build.py
+++ b/cpython-windows/build.py
@@ -1863,8 +1863,7 @@ def main() -> None:
default="cpython-3.11",
help="Python distribution to build",
)
- # TODO: Rename this to `--options` to match the Unix build script
- optimizations = {"debug", "noopt", "pgo", "lto", "pgo+lto"}
+ optimizations = {"noopt", "pgo"}
parser.add_argument(
"--options",
choices=optimizations.union({f"freethreaded+{o}" for o in optimizations}),
These were unintentionally expanded in #326
These were unintentionally expanded in #326
Closes #320
There are a couple important notes here: