Skip to content

Automatic vendoring #4093

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

Merged
merged 4 commits into from
Nov 6, 2016
Merged

Automatic vendoring #4093

merged 4 commits into from
Nov 6, 2016

Conversation

xavfernandez
Copy link
Member

@xavfernandez xavfernandez force-pushed the automatic_vendoring branch 3 times, most recently from adb3303 to 990fece Compare November 6, 2016 13:24
@xavfernandez
Copy link
Member Author

And cc @pfmoore since you were the original author of 1c4f2bb

@xavfernandez xavfernandez reopened this Nov 6, 2016
@pfmoore
Copy link
Member

pfmoore commented Nov 6, 2016

Only a couple of comments, as I don't have time for a full review at the moment, sorry.

  1. Are the edits to vendored files meant to be in there? Seems odd for a patch that fully automates vendoring to me also adding manual changes to the vendored files.
  2. As long as the new vendoring task works on Windows, I'm OK with it. I originally wrote the vendoring script to replace some Unix-only shell scripts, so that I could re-vendor myself if needed. As invoke now works on Windows, I'm happy to move the functionality to the invoke tasks.
  3. Although actually, you don't reference invoke in the new script, or make it a task. Are you aware that the contents of the tasks directory are run via invoke? (It wouldn't be surprising if you weren't, it's not documented anywhere. AFAIK, it's just the tool @dstufft prefers to use for some of the release jobs.)

@xavfernandez
Copy link
Member Author

Are the edits to vendored files meant to be in there? Seems odd for a patch that fully automates vendoring to me also adding manual changes to the vendored files.

Well, the special cases editions are there to stay and are likely to be done at each revendoring, so I'd say automating this part also makes sense.

Although actually, you don't reference invoke in the new script, or make it a task. Are you aware that the contents of the tasks directory are run via invoke? (It wouldn't be surprising if you weren't, it's not documented anywhere. AFAIK, it's just the tool @dstufft prefers to use for some of the release jobs.)

Yes, I started my first implementation with invoke (and btw, for invoke 0.13.0, the generate.authors task needs to be updated) but I did not see the added value of invoke in our case so I went back to full python instead. I'm also tempted to drop the invoke part for authors...

@dstufft
Copy link
Member

dstufft commented Nov 6, 2016

I'd prefer it this was done using an invoke task. My goal with those is to put all of the automation in that so that a release can be as simple as invoke release.

@xavfernandez
Copy link
Member Author

I'd prefer it this was done using an invoke task. My goal with those is to put all of the automation in that so that a release can be as simple as invoke release.

@dstufft : I understand the final goal but what is the added value of invoke vs a python script release.py that would call the different step functions ?

@dstufft
Copy link
Member

dstufft commented Nov 6, 2016

@xavfernandez A better UX for calling it with different options. Invoke is obviously nothing you can't replace with argparse, subprocess, etc etc... but why reinvent the wheel when invoke exists already?

@pfmoore
Copy link
Member

pfmoore commented Nov 6, 2016

Well, the special cases editions are there to stay and are likely to be done at each revendoring, so I'd say automating this part also makes sense.

Absolutely, any modifications we make to the vendored code should be automated if possible. But for example, the change to pip/_vendor/html5lib/__init__.py - should that not be done by the vendoring script, and so having it as an edit in this PR seems contrary to what we'd want. But I may be completely misunderstanding - it seems to me that this particular chunk is an independent fix for an error in the current revendoring.

A better UX for calling it with different options.

+1 on this, but at the moment we don't have a noticeably better UX. It's the choice between python tasks/re-vendor.py and (first install invoke) inv re-vendor. So while an improved UX is a good thing, we're not really using the capability at the moment.

OTOH, it's only really you (@dstufft) who does any of the release work anyway, so to an extent I'm OK with "whatever tools you prefer".But it would be nice if the release process was documented and automated so it didn't need to be just you doing it. If invoke makes that easier to achieve, then that's great.

@xavfernandez
Copy link
Member Author

xavfernandez commented Nov 6, 2016

But for example, the change to pip/_vendor/html5lib/init.py - should that not be done by the vendoring script, and so having it as an edit in this PR seems contrary to what we'd want. But I may be completely misunderstanding

It is indeed done by the vendoring script !
Cf commit 990fece which contains all the modifications performed (automatically) by the script.
Maybe I should not have put it in the same PR :-/

Edited: I removed 990fece from the PR

@dstufft
Copy link
Member

dstufft commented Nov 6, 2016

it would be nice if the release process was documented and automated so it didn't need to be just you doing it. If invoke makes that easier to achieve, then that's great.

My end goal here is to make it so invoke release is all you need to do to execute a release, and ideally we can even move that to be run on some automation host so all that is needed to do is push some tag.

@xavfernandez xavfernandez force-pushed the automatic_vendoring branch 3 times, most recently from 42bb1b5 to 6051433 Compare November 6, 2016 18:46
And update README.rst with the setuptools/pkg_resources "modification".
)


SPECIAL_CASES = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about turning these into actual diff files stored alongside this (tasks/vendoring/__init__.py and tasks/vendoring/patches/*.diff) and restoring using difflib.restore()? That seems like it'd be easier to manage then baking them into the file itself.

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, with git apply.

@pfmoore
Copy link
Member

pfmoore commented Nov 6, 2016

It is indeed done by the vendoring script !

Ah, got it now! Sorry for being dense :-)

@dstufft dstufft merged commit 2c97354 into pypa:master Nov 6, 2016
@xavfernandez xavfernandez deleted the automatic_vendoring branch November 6, 2016 23:23
@xavfernandez xavfernandez mentioned this pull request Nov 7, 2016
@xavfernandez xavfernandez added this to the Automated Release Process milestone Nov 7, 2016
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants