for discussion: add tools/roll_pdfium.py

for discussion: add tools/roll_pdfium.py

demo: produced https://codereview.chromium.org/1285343003/

BUG=

[Nico, 2015-08-17 04:33:06]

thakis@chromium.org changed reviewers:
+ kjellander@chromium.org

[Nico, 2015-08-17 04:33:06]

kjellander, when you're back I'd like to get your opinion on how to reduce
copypasta among all these roll scripts. My idea is to add a tools/roll_dep.py
that gets local path and name, change roll_angle.py to shell out to that with
"--path tools/angle --name ANGLE", and leave roll_webrtc.py as-is for now as
it's different in that it does several rolls at once. Other ideas?

[kjellander_chromium, 2015-09-01 08:47:43]

kjellander@chromium.org changed reviewers:
+ agable@chromium.org

[kjellander_chromium, 2015-09-01 08:50:37]

On 2015/08/17 04:33:06, Nico wrote:
> kjellander, when you're back I'd like to get your opinion on how to reduce
> copypasta among all these roll scripts. 

Hi, yeah let's not continue down this path of code duplication even further.
I think I recall the Chrome infra team wanted logic like this to live in depot
tools and I admit I kind of sneaked our script in here below their radar since I
didn't have time to spend on reworking it. Then angle forked our copy and now
copies line up :)
+agable for input on this and advice on how we should proceed.

> My idea is to add a tools/roll_dep.py
> that gets local path and name, change roll_angle.py to shell out to that with
> "--path tools/angle --name ANGLE"

That sounds like a step in the right direction, but the name will be confusing
since there's
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/roll...
already.
I suggest roll_lib.py or something like this, if we cannot move more logic into
depot tools instead and then use that (which I realize is much more work).

> , and leave roll_webrtc.py as-is for now as
> it's different in that it does several rolls at once. Other ideas?

I'd prefer to un-duplicate our script if progress is made on the other ends.
I'll be happy to do that work if you don't want to mess around with our script
in your CL. The only difference with our script is that we roll two deps in the
same CL, so ideally it would be just to invoke a script/function twice instead
of once.

[agable, 2015-09-01 16:49:40]

As Henrik pointed out, depot_tools already has roll_dep.py.

Why is it insufficient? What extra logic is this script doing? Why is this
script anything other than "shell out to roll_dep.py with the path set to
third_party/pdfium"? If it is insufficient, then it should be fixed, rather than
having this script keep all the logic. Angle should do the same. It wouldn't be
hard to add "do multiple rolls simultaneously" logic to roll_dep.py, and then
WebRTC could also do the same. Or it could be given a "don't do git cl upload
yet" flag on all but the last roll; either way works.

[Nico, 2015-09-01 18:01:54]

On 2015/09/01 16:49:40, agable wrote:
> As Henrik pointed out, depot_tools already has roll_dep.py.
> 
> Why is it insufficient? What extra logic is this script doing? Why is this
> script anything other than "shell out to roll_dep.py with the path set to
> third_party/pdfium"? If it is insufficient, then it should be fixed, rather
than
> having this script keep all the logic. Angle should do the same. It wouldn't
be
> hard to add "do multiple rolls simultaneously" logic to roll_dep.py, and then
> WebRTC could also do the same. Or it could be given a "don't do git cl upload
> yet" flag on all but the last roll; either way works.

Cool, I didn't know about this script. It doesn't seem to work, though:

$ roll-dep third_party/angle/
error: Ensure /Users/thakis/src/chrome/src is clean first.
Nicos-MacBook-Pro-10:src thakis$ git diff

Apparently the script wants me to have no untracked files in my checkout (I
always have a bunch of "test.cc" files in the root dir and whatnot). roll_angle
has a `--ignore-checks` flag for this; could roll-dep have something like that
too?

[Nico, 2015-09-01 18:04:30]

On 2015/09/01 18:01:54, Nico wrote:
> On 2015/09/01 16:49:40, agable wrote:
> > As Henrik pointed out, depot_tools already has roll_dep.py.
> > 
> > Why is it insufficient? What extra logic is this script doing? Why is this
> > script anything other than "shell out to roll_dep.py with the path set to
> > third_party/pdfium"? If it is insufficient, then it should be fixed, rather
> than
> > having this script keep all the logic. Angle should do the same. It wouldn't
> be
> > hard to add "do multiple rolls simultaneously" logic to roll_dep.py, and
then
> > WebRTC could also do the same. Or it could be given a "don't do git cl
upload
> > yet" flag on all but the last roll; either way works.
> 
> Cool, I didn't know about this script. It doesn't seem to work, though:
> 
> $ roll-dep third_party/angle/
> error: Ensure /Users/thakis/src/chrome/src is clean first.
> Nicos-MacBook-Pro-10:src thakis$ git diff
> 
> Apparently the script wants me to have no untracked files in my checkout (I
> always have a bunch of "test.cc" files in the root dir and whatnot).
roll_angle
> has a `--ignore-checks` flag for this; could roll-dep have something like that
> too?

Oh, nevermind, I just wasn't on the master branch. And apparently it's `roll-dep
src/third_party/angle`, and it doesn't create a roll branch and doesn't
automatically upload the change. But the existing roll_angle script should use
roll-dep for sure. Thanks for the pointer.

[Nico, 2015-09-01 18:06:41]

On 2015/09/01 18:04:30, Nico wrote:
> On 2015/09/01 18:01:54, Nico wrote:
> > On 2015/09/01 16:49:40, agable wrote:
> > > As Henrik pointed out, depot_tools already has roll_dep.py.
> > > 
> > > Why is it insufficient? What extra logic is this script doing? Why is this
> > > script anything other than "shell out to roll_dep.py with the path set to
> > > third_party/pdfium"? If it is insufficient, then it should be fixed,
rather
> > than
> > > having this script keep all the logic. Angle should do the same. It
wouldn't
> > be
> > > hard to add "do multiple rolls simultaneously" logic to roll_dep.py, and
> then
> > > WebRTC could also do the same. Or it could be given a "don't do git cl
> upload
> > > yet" flag on all but the last roll; either way works.
> > 
> > Cool, I didn't know about this script. It doesn't seem to work, though:
> > 
> > $ roll-dep third_party/angle/
> > error: Ensure /Users/thakis/src/chrome/src is clean first.
> > Nicos-MacBook-Pro-10:src thakis$ git diff
> > 
> > Apparently the script wants me to have no untracked files in my checkout (I
> > always have a bunch of "test.cc" files in the root dir and whatnot).
> roll_angle
> > has a `--ignore-checks` flag for this; could roll-dep have something like
that
> > too?
> 
> Oh, nevermind, I just wasn't on the master branch. And apparently it's
`roll-dep
> src/third_party/angle`, and it doesn't create a roll branch and doesn't
> automatically upload the change. But the existing roll_angle script should use
> roll-dep for sure. Thanks for the pointer.

roll-dep also doesn't do the BUG= line collection. So it does almost none of the
things the existing roll scripts do.

[agable, 2015-09-01 18:11:56]

We'd be more than happy to have a list of features that roll_dep.py needs, or
even to mostly replace it by one of the current roll_angle/roll_pdfium/whatever
scripts if those have all the logic that is needed. I just think we shouldn't be
replicating that logic between multiple places in src/ and depot_tools.

[Lei Zhang, 2015-11-05 23:58:57]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-11-05 23:58:57]

So do we have a bug for going forward? I just did some search and replace to
produce roll_breakpad.py locally...

[Lei Zhang, 2016-09-26 22:10:27]

On 2015/11/05 23:58:57, Lei Zhang wrote:
> So do we have a bug for going forward? I just did some search and replace to
> produce roll_breakpad.py locally...

And PDFium has moved to Skia's auto roller, so we don't need this script
anymore.

[kjellander_chromium, 2016-09-29 06:09:10]

On 2016/09/26 22:10:27, Lei Zhang wrote:
> On 2015/11/05 23:58:57, Lei Zhang wrote:
> > So do we have a bug for going forward? I just did some search and replace to
> > produce roll_breakpad.py locally...
> 
> And PDFium has moved to Skia's auto roller, so we don't need this script
> anymore.

Great. I'm closing this CL then, to get if off my review list.