Conversation
| runner: macos-14 | ||
| py: 'cpython-3.8' | ||
| optimizations: 'debug' | ||
| options: 'debug' |
There was a problem hiding this comment.
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.
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.
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.
| if [ -n "${CPYTHON_FREETHREADED}" ]; then | ||
| CONFIGURE_FLAGS="${CONFIGURE_FLAGS} --disable-gil" | ||
| fi |
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.
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.
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.
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.
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.
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.
Ah thanks for clarifying. It'd be options = optimizations.union(...) and I'm hesitant to introduce another local variable (basically just personal preference).
| openssl_entry: str, | ||
| ) -> pathlib.Path: | ||
| pgo = profile == "pgo" | ||
| parsed_build_options = set(build_options.split("+")) |
There was a problem hiding this comment.
Was this wrong before? Or was it just never called with pgo+lto?
There was a problem hiding this comment.
Ah there are only "pgo" builds on Windows. I just changed this for consistency.
There was a problem hiding this comment.
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.
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.
(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.
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: