-
Notifications
You must be signed in to change notification settings - Fork 247
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 a workflow to update dependencies #490
Conversation
The workflow does not appear in "Actions"... I must be doing something wrong but I have no clue at the moment. |
It may need to be in master - or it may have had to run once. |
Run the update with the right file. Use current repo in the created link. Run on push for now (not in master yet to get the workflow_dispatch actually working).
From what I've read around, probably needs to be in master. Adding on push for now (to trigger a sample PR) |
And here is the sample PR: #491 |
Nice! |
Default method is to use docker
It's pretty cool that it updates the existing PR if there's already one open! I think once we remove the 'push' trigger this looks good, to me |
PR are only created for master
I removed the |
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.
Apologies, I just realised the implicit Python that's generating the versionless constraints.txt
should probably be controlled too.
bin/update_dependencies.py
Outdated
@@ -12,21 +13,39 @@ | |||
|
|||
# CUSTOM_COMPILE_COMMAND is a pip-compile option that tells users how to | |||
# regenerate the constraints files | |||
os.environ['CUSTOM_COMPILE_COMMAND'] = "bin/update_constraints.py" | |||
os.environ['CUSTOM_COMPILE_COMMAND'] = "bin/update_dependencies.py" | |||
subprocess.check_call([ |
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 just realised... this command is implicitly run in python3.8, because that's what my venv is in on my mac >.< (maybe a little sloppy, sorry!). Could it be made explicit? Perhaps 38
and 39
should be added to the list of versions and the default constraints.txt uses the final entry of this list? Then it should also be generated inside the docker container, so we're sure which version of Python is running it.
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.
Shall we use a default constraints.txt
at all ?
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.
That's a great point. Maybe we don't need it at all, in fact!
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.
Oh, well actually our code does assert that it exists, currently
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's also maybe useful as a starting point if somebody wanted to customise their constraints. But just copying the latest Python we support should do
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.
Done. I had to modify a test so I updated all dependencies. Anyway, I think I'll have to rebase on #489 anyway.
Co-authored-by: Joe Rickerby <[email protected]>
This also updates constraints in order to get something consistent given that one test will require to be updated.
@@ -10,7 +10,8 @@ def test_defaults(): | |||
resources_dir = project_root / 'cibuildwheel' / 'resources' | |||
|
|||
assert dependency_constraints.base_file_path.samefile(resources_dir / 'constraints.txt') | |||
assert dependency_constraints.get_for_python_version('3.8').samefile(resources_dir / 'constraints.txt') | |||
assert dependency_constraints.get_for_python_version('3.99').samefile(resources_dir / 'constraints.txt') |
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.
Is there a reason to still have a default constraints.txt
file?
I thought the reason it's there is because 3.8 and 3.9 currently have the exact same version file?
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.
see @joerick comments #490 (comment) on his review.
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, I saw, but to start customizing your own constraints, you could also start from constraints-python39.txt
? constraints.txt
feels like a historical remainder, somehow. But ofc, it's not wrong; I was just thinking if it's not better to get a reminder (in the form of a failing CI), when we add 3.10, and future versions to cibuildwheel
?
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 it's simpler just to have one per version. "But just copying the latest Python we support should do" I think is exactly what @YannickJadoul is referring to 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.
Yeah, we're coming from a situation at the start where we only had one constraints.txt
. Then 2.7 started acting up, and we had constraints-python2.7.txt
, then 3.5, then 3.6, ... and now, there seems to be the realization that just having a separate file for all is better.
So why not just ditch the "default", then, if falling back to the default is a mistake?
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.
Not that crucial, though. Just wondering if there's a good reason. (There's nothing else to nitpick about, so I need to find something? I very much like the idea and rest of the PR :-) )
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.
@YannickJadoul, @henryiii, I agree and proposed to do so. Final decision left to @joerick.
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.
Right. Sorry; should have added my comment to that thread, then :-)
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'm happy to go with that - that we don't bundle a constraints.txt. I think as @YannickJadoul says, the version-specific ones appeared to solve specific problems, but version-specific is probably a more robust solution. We'll have to remove this line but that's not a huge deal, I think.
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.
Hmmm, looking at that code, I'm now seeing it's because we want to keep supporting a default constraints.txt
for users.
I'm fine either way, then! As you see fit, @mayeut. Sorry about the confusion!
I merged as-is. Thanks for your reviews. |
I can, now that it's in master, dispatch the workflow. #495 created (can probably be closed though). |
Dependabot (referring to |
I'd be happy for this to run regularly, perhaps weekly? It's always just one PR, as opposed to the dependabot 'sea', and we can dial it back if it becomes irritating.
… On 21 Dec 2020, at 19:07, Matthieu Darbois ***@***.***> wrote:
I can, now that it's in master, dispatch the workflow. #495 <#495> created (can probably be closed though).
@joerick <https://github.com/joerick> expressed at some point <#256 (comment)> that he was not a big fan of dependabot because of the "sea of PR" it was creating. If need be and if this is not seen as "nuisance" PR, the workflow could also be run as a cron job.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#490 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJPZE4QVQX4KEHD26257DTSV6MGNANCNFSM4VDGAGAA>.
|
As noted by @henryiii (somewhere else; I forgot why), it might also be nice to update the Python versions? |
@YannickJadoul, I don't know about "clean" but I have been experimenting with this because it's also something I'd like to have in |
@mayeut That would be amazing :-) Anything that can help us get started could help, anyway. |
I agree, it would be nice to automate the Python upgrades as well. Sounds good @mayeut , I look forward to seeing it! The only way I can think of to do it would be to parse https://www.python.org/ftp/python/ ... |
Yes, by "clean", I was referring to something like PyPy's https://downloads.python.org/pypy/versions.json, and not having to parse things, or check if trying to download e.g. https://www.python.org/ftp/python/3.9.2/python-3.9.2-macos11.0.pkg doesn't fail. |
Draft PR #496 created. |
Add a workflow to update dependencies.
Manual dispatch.