-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat: update python versions as part of update_dependencies #496
Conversation
If this helps or gives other ideas, I started to rework a bit the "CPython from python.org" part for source packages in
It prints:
|
Thanks @mayeut ! I didn't know about the python.org downloads API, that certainly simplifies things! The code is here for the curious- https://github.com/python/pythondotorg/blob/master/downloads/api.py , it's a simple Django Rest Framework API. As for integrating these into the code, I think a human/machine readable TOML config file would be best, similar to the manylinux one at cibuildwheel/resources/pinned_docker_images.cfg, instead of modifying the code in this script. @YannickJadoul I remember you had a similar idea in #431. |
I did not saw (or remember seeing) #431 before but that's certainly one option which matches one of the few comment in the commit ( |
I already completely forgot about #431. For the record, that was just a loose thought, and nothing concrete; so at any rate, we'd have to discuss a bit on how to best approach that, but yes, I agree the combination of these two ideas could be interesting! |
1e381e7
to
df23396
Compare
A few comments after I rewrote it a bit:
Should we have a way to opt-in to dev versions and include them in the list? Just like |
cafa348
to
051c684
Compare
@joerick GitLab CI seems to be both required, and stuck. :/ |
I've 'fixed' it by removing that CI from being required. The way Github does the auto merge is pretty odd. I have to choose the CI checks that are 'required' in settings, which are matched by display name to the checks being run. So, as we see in #484, because I changed the name of the macos job, it's forever waiting for the old job to report. Sigh. Maybe something 3rd party would fit better... really I think we just want 'anything that's started, wait for that to be successful' rather than choosing individual checks. |
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.
Wow, quite an impressive bit of work @henryiii !
general points
- I'm not sure how the 'prereleases' flag works, at the moment... should it even be a global flag? Generally we don't want them, but sometimes we want to release a beta version of cibuildwheel with a new version of Python e.g. 3.10 support - then we'd want prerelease support but only on 3.10.x versions.
- There might be a simpler way to write this script, by starting with a read of the
build-platforms.toml
, and then iterating through each configuration, trying to find a patch release that's newer than the one listed. This would allow us to manually edit thebuild-platforms.toml
to add e.g. 3.10 support, or something else, and would avoid the hard-coding of 2.7 versions here. What do you think?
e4b0aa8
to
230538c
Compare
Redesigned based on the idea of having the input file drive the search. |
Hope you don't mind, I had a little play with this @henryiii, to give nice diff outputs. Just an excuse to play with rich, really! Anyway, so long as you're happy with my changes, LGTM! |
I'm making one last style change (removing a comma), and looking to see if there's anything else I missed or any comments that might be useful. Then I'll push and trigger a pending merge! |
bin/update_pythons.py
Outdated
self.macos_u2 = CPythonVersions( | ||
plat_arch="macosx_universal2", | ||
file_ident="macos11.0.pkg", | ||
) | ||
self.macos_u2 = CPythonVersions(plat_arch="macosx_universal2", file_ident="macos11.0.pkg") |
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 "magic" comma in Black, which lets you force multiline. I didn't want to do that here, so I removed it.
PS: I originally tried to write this without Black, but the first time a ran I had 20 or so Flake8 errors; I had no interest in waisting time trying to fix them by hand, so I ran black bin/update_python.py
, and they all went away. So this is our one Black'ed file. :)
@@ -227,6 +228,7 @@ def update_config(self, config: Dict[str, str]) -> None: | |||
orig_config = copy.copy(config) | |||
config_update: Optional[AnyConfig] | |||
|
|||
# We need to use ** in update due to MyPy (probably a bug) |
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.
MyPy's handling of TypedDict's is pretty buggy. I had to do this, and I also don't know how to type narrow on a Union of TypedDicts, as assert isinstance
is not allowed, so this takes a normal Dict for now. 🤷
As mentioned in #490, a method to update CPython & PyPy on Windows/macOS.
Per @YannickJadoul criteria, the idea behind it is "clean" however, the implementation here is far from being clean and thus is opened as draft to give ideas/pointers to others as I probably won't have time to work on this.