Skip to content
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

Merged
merged 7 commits into from
Dec 21, 2020
Merged

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Dec 20, 2020

Add a workflow to update dependencies.

Manual dispatch.

@mayeut
Copy link
Member Author

mayeut commented Dec 20, 2020

The workflow does not appear in "Actions"... I must be doing something wrong but I have no clue at the moment.

@henryiii
Copy link
Contributor

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).
@mayeut
Copy link
Member Author

mayeut commented Dec 20, 2020

It may need to be in master - or it may have had to run once.

From what I've read around, probably needs to be in master. Adding on push for now (to trigger a sample PR)

@mayeut
Copy link
Member Author

mayeut commented Dec 20, 2020

And here is the sample PR: #491

@joerick
Copy link
Contributor

joerick commented Dec 20, 2020

Nice!

@joerick
Copy link
Contributor

joerick commented Dec 20, 2020

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

@mayeut mayeut marked this pull request as ready for review December 20, 2020 17:26
@mayeut
Copy link
Member Author

mayeut commented Dec 20, 2020

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

I removed the push trigger and enforce PR creation to be done only on master (I can revert this last one if that's not wanted).

Copy link
Contributor

@joerick joerick left a 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.

.github/workflows/update-dependencies.yml Outdated Show resolved Hide resolved
bin/update_dependencies.py Outdated Show resolved Hide resolved
@@ -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([
Copy link
Contributor

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.

Copy link
Member Author

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 ?

Copy link
Contributor

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!

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member Author

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.

mayeut and others added 2 commits December 20, 2020 20:31
Co-authored-by: Joe Rickerby <joerick@mac.com>
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')
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Member

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 :-) )

Copy link
Member Author

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.

Copy link
Member

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 :-)

Copy link
Contributor

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.

Copy link
Member

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!

@mayeut mayeut merged commit 40ddede into master Dec 21, 2020
@mayeut
Copy link
Member Author

mayeut commented Dec 21, 2020

I merged as-is.
We can still update the bit about constraints.txt later if needed.

Thanks for your reviews.

@mayeut mayeut deleted the update-dependencies-workflow branch December 21, 2020 18:58
@mayeut
Copy link
Member Author

mayeut commented Dec 21, 2020

I can, now that it's in master, dispatch the workflow. #495 created (can probably be closed though).
@joerick expressed at some point 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.

@henryiii
Copy link
Contributor

Dependabot (referring to .github/dependabot.yml) is configurable - you can have it update once a week if you like. There have been a few other things under the name dependabot, including a built-in security updater. But the configurable one is quite nice.

@joerick
Copy link
Contributor

joerick commented Dec 23, 2020 via email

@YannickJadoul
Copy link
Member

As noted by @henryiii (somewhere else; I forgot why), it might also be nice to update the Python versions?
The main question is: is there a clean way of getting a list of Python versions?

@mayeut
Copy link
Member Author

mayeut commented Dec 23, 2020

@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 manylinux. I expended the experiment for cibuildwheel and I will post a draft PR showing it can work. I'm not sure I will have time to improve on it though but it might give some ideas/pointers to others.

@YannickJadoul
Copy link
Member

@mayeut That would be amazing :-) Anything that can help us get started could help, anyway.

@joerick
Copy link
Contributor

joerick commented Dec 23, 2020

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/ ...

@YannickJadoul
Copy link
Member

YannickJadoul commented Dec 23, 2020

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.

@mayeut
Copy link
Member Author

mayeut commented Dec 23, 2020

Draft PR #496 created.
@YannickJadoul, the ideas implemented are "clean" per your criteria, the implementation is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants