-
Notifications
You must be signed in to change notification settings - Fork 234
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 CPython 3.10 support #675
Conversation
c9c2846
to
cdfc7c3
Compare
I've added a basic implementation of a |
961551c
to
cbef481
Compare
Should we un-draft this? It shouldn't build Python 3.10 wheels unless someone explicitly passes |
The dependency update script might need to be updated first, It filters out pre-releases and beta2.../rc1/... would not be picked up. I might just quote you on this:
|
4d385a3
to
139e240
Compare
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.
Nice. My only comment is that I think we should keep to the convention and have an environment variable to configure this, rather than just a command line flag. I'd suggest CIBW_PRERELEASE_PYTHONS
. Then we can add a little more documentation about ABI stability etc. in that option's documentation.
That then suggests the command-line option should be --prerelease-pythons
. I know somebody was keen on matching pip's --pre
flag, but I think our consistency is more important (CIBW_PLATFORM
<-> --platform
, CIBW_ARCHS
<-> --archs
, CIBW_OUTPUT_DIR
<-> --output-dir
).
I'm not sure we should have an environment variable for this. It's a special testing flag, and not something that users should do without understanding that the wheels generated should not be uploaded to PyPI; if it's a general listed option, it might be more likely to be misused? However, having the OS variants could be useful, so I'm not against it either. |
It could be useful in a matrix to run a test, so I'm +0.1. |
I'd prefer it was a sibling to CIBW_BUILD, CIBW_SKIP, CIBW_REQUIRES_PYTHON, as it's doing a very similar job to those options - build selection. |
I've added CIBW_PRERELEASE_PYTHONS. One argument against it is that I think it's the first and only boolean option in our environment variables. It's also the only one that explicitly makes invalid-to-upload wheels. And it can be completely controlled via BUILD/SKIP if you enable it. One argument for it is that then we'd have to add a way to use it with the GitHub action if there's no env-var (though it could just be unavailable with the action, as it's a special setting for testing only, I guess). |
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.
Nice job. Just a few minor changes to go I think.
rebased on master to get CI working. |
cibuildwheel/__main__.py
Outdated
@@ -206,6 +212,9 @@ def main() -> None: | |||
build_verbosity_str = get_option_from_environment( | |||
"CIBW_BUILD_VERBOSITY", platform=platform, default="" | |||
) | |||
prerelease_pythons = args.prerelease_pythons or cibuildwheel.util.strtobool( | |||
get_option_from_environment("CIBW_PRERELEASE_PYTHONS", platform=platform, default="0") |
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.
Grrr, this will either require it be added to the pyproject.toml (yuck, not at all a good idea for a flag like this), or removing the platform specific choices, or adding custom code back to handle platform choices but no pyproject.toml support for #684. Does it have to support platform specific variants?
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 guess no platform specific variant is not an issue for this one (and shall definitely not be in pyproject.toml
)
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 hadn't fully considered the pyproject.toml angle, but I think you're right, it would be kinda crazy to write this into a pyproject.toml
! I could imagine that perhaps this option would only be specifiable at runtime from flag or env var, if that makes you feel better henryiii? I could imagine in the docs something like-
Only configurable at runtime via
--prerelease-pythons
orCIBW_PRERELEASE_PYTHONS
. Cannot be set inpyproject.toml
.
It's interesting that CIBW_OUTPUT_DIR
is another one of these runtime-only options? (maybe you've discussed this in #547, apologies, I'm behind!) Makes me wonder about a runtime-only section of the options docs, perhaps.
I'm not so worried about the platform-specific thing here, it seems pretty harmless. I wouldn't miss it either, if anyone has a stronger opinion.
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.
Okay, I'll remove the variants for now.
CIBW_OUTPUT_DIR
This is undocumented currently, actually. It's discussed in #547, though it's a bit of an open question. I had it in the toml then I took it out, but could put it back in.
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.
Apply suggestions from code review Co-authored-by: Matthieu Darbois <mayeut@users.noreply.github.com>
I'd love to get #684 into an alpha release too, so it's easier to start testing it in some projects. Though using GHA you can pin a commit, so can still test if there's no alpha for it. And I guess there could be an alpha 1 and an alpha 2, there's no cost for alphas... |
Just checking if we should expect any surprise with this.
No reflection on the
--pre
proposal in #657 yet