Copy Crashpad into the Chrome tree instead of importing it via DEPS

Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
R=primiano@chromium.org, rsesek@chromium.org, scottmg@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/b212e219b061f342502635af586ff4fc0e14a4d5

[Mark Mentovai, 2015-12-08 23:34:04]

Description was changed from

==========
Copy crashpad into the Chrome tree instead of importing it via DEPS

BUG=472900
==========

to

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

BUG=472900
==========

[Mark Mentovai, 2015-12-08 23:35:15]

Description was changed from

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

BUG=472900
==========

to

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing at all in this commit.

BUG=472900
==========

[Mark Mentovai, 2015-12-08 23:35:31]

Description was changed from

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing at all in this commit.

BUG=472900
==========

to

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
==========

[Mark Mentovai, 2015-12-08 23:35:44]

Description was changed from

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
==========

to

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
==========

[Mark Mentovai, 2015-12-08 23:35:45]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org, scottmg@chromium.org

[Mark Mentovai, 2015-12-08 23:36:24]

https://codereview.chromium.org/1505213004/diff/10001/DEPS
File DEPS (left):

https://codereview.chromium.org/1505213004/diff/10001/DEPS#oldcode129
DEPS:129: 'src/third_party/crashpad/crashpad':
The new third_party/crashpad/crashpad was produced by running

git remote add crashpad https://chromium.googlesource.com/crashpad/crashpad
git read-tree --prefix=third_party/crashpad/crashpad \
    -u 6bebb10829332dee5c7315abafb0a8bf32840c15
git remote remove crashpad

and patching in 7efdc94f5982a1f9654c53fd4bf5663e5efa66ce.

https://codereview.chromium.org/1505213004/diff/10001/DEPS#oldcode130
DEPS:130: Var('chromium_git') + '/crashpad/crashpad.git' + '@' +
'6bebb10829332dee5c7315abafb0a8bf32840c15',
On sync, gclient will take care of moving the old crashpad out of the way and
prompting the user to remove it if it’s unneeded.

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/DEPS
File third_party/crashpad/DEPS (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/DE...
third_party/crashpad/DEPS:5: # Crashpad has its own #include structure not
rooted at Chromium’s root.
Noting that this file needed a UTF-8 BOM for checkdeps to process it.

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:29: git pull --strategy=subtree --squash
crashpad master_or_hash
I want to replace this sequence of steps with a script that also formats the
commit message nicely, updates the revision in this file, and maybe drops the
“local modifications” section if there are no diffs between upstream and the
import. But before I do that, I wanted to ask some questions about this process.

I had expected to tell the merge both the “from” and “to” commit-ishes from
Crashpad to merge into the current branch. This only has a “to”, and presumably
git does something to figure out where to start merging from. This is a little
more scary to me, since we won’t have merge commits to identify this
definitively to git. We do have the “revision” field in this file, and I was
hoping to use that. Do either of you know of a way to make this work?

The other bit of git magic is that the subtree merge magically finds
third_party/crashpad/crashpad without being told. That’s scary too, and I want
to tell it the path explicitly. I thought that “--strategy=recursive
-Xsubtree=third_party/crashpad/crashpad” would work, but it doesn’t. Ideas?

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/cr...
File third_party/crashpad/crashpad/.clang-format (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/cr...
third_party/crashpad/crashpad/.clang-format:1: # Copyright 2014 The Crashpad
Authors. All rights reserved.
Stop reviewing here.

[Mark Mentovai, 2015-12-08 23:47:08]

https://codereview.chromium.org/1505213004/diff/10001/DEPS
File DEPS (left):

https://codereview.chromium.org/1505213004/diff/10001/DEPS#oldcode131
DEPS:131: 
Also, this brings in these PRESUBMIT warnings that we don’t care about, but it’s
worth mentioning:

** Presubmit Warnings **
New code should not use wstrings.  If you are calling a cross-platform API that
accepts a wstring, fix the API.
    third_party/crashpad/crashpad/client/crashpad_client.h:122
    third_party/crashpad/crashpad/client/crashpad_client.h:137
    third_party/crashpad/crashpad/client/crashpad_client.h:207
    third_party/crashpad/crashpad/handler/handler_main.cc:346
    third_party/crashpad/crashpad/test/multiprocess_exec.h:69

[scottmg, 2015-12-08 23:47:18]

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/DEPS
File third_party/crashpad/DEPS (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/DE...
third_party/crashpad/DEPS:5: # Crashpad has its own #include structure not
rooted at Chromium’s root.
On 2015/12/08 23:36:24, Mark Mentovai wrote:
> Noting that this file needed a UTF-8 BOM for checkdeps to process it.

I don't see one in
https://codereview.chromium.org/1505213004/patch/10001/5678957424803840 is it
just missing in the UI?

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:29: git pull --strategy=subtree --squash
crashpad master_or_hash
On 2015/12/08 23:36:24, Mark Mentovai wrote:
> I want to replace this sequence of steps with a script that also formats the
> commit message nicely, updates the revision in this file, and maybe drops the
> “local modifications” section if there are no diffs between upstream and
the
> import. But before I do that, I wanted to ask some questions about this
process.
> 
> I had expected to tell the merge both the “from” and “to” commit-ishes
from
> Crashpad to merge into the current branch. This only has a “to”, and
presumably
> git does something to figure out where to start merging from. This is a little
> more scary to me, since we won’t have merge commits to identify this
> definitively to git. We do have the “revision” field in this file, and I was
> hoping to use that. Do either of you know of a way to make this work?
> 
> The other bit of git magic is that the subtree merge magically finds
> third_party/crashpad/crashpad without being told. That’s scary too, and I want
> to tell it the path explicitly. I thought that “--strategy=recursive
> -Xsubtree=third_party/crashpad/crashpad” would work, but it doesn’t. Ideas?

Sorry, I don't know. If I was doing it, I have made the steps something like rm
-rf, cp -R, git add, git commit, as it's going to land as a squashed patch
anyway.

I would probably summon primiano@ as he seems to know a lot about all of git's
funny business.

[scottmg, 2015-12-08 23:50:39]

https://codereview.chromium.org/1505213004/diff/10001/DEPS
File DEPS (left):

https://codereview.chromium.org/1505213004/diff/10001/DEPS#oldcode131
DEPS:131: 
On 2015/12/08 23:47:08, Mark Mentovai wrote:
> Also, this brings in these PRESUBMIT warnings that we don’t care about, but
it’s
> worth mentioning:
> 
> ** Presubmit Warnings **
> New code should not use wstrings.  If you are calling a cross-platform API
that
> accepts a wstring, fix the API.
>     third_party/crashpad/crashpad/client/crashpad_client.h:122
>     third_party/crashpad/crashpad/client/crashpad_client.h:137
>     third_party/crashpad/crashpad/client/crashpad_client.h:207
>     third_party/crashpad/crashpad/handler/handler_main.cc:346
>     third_party/crashpad/crashpad/test/multiprocess_exec.h:69

We could avoid those by using a _win file somehow, rather than #ifdef'ing, but
it's probably not worth the torture.

... oh, I see we could annotate with "// presubmit: allow wstring" too if the
clutter is less annoying than the warning.

[Mark Mentovai, 2015-12-09 01:35:26]

https://codereview.chromium.org/1505213004/diff/10001/DEPS
File DEPS (left):

https://codereview.chromium.org/1505213004/diff/10001/DEPS#oldcode131
DEPS:131: 
scottmg wrote:
> We could avoid those by using a _win file somehow, rather than #ifdef'ing, but
> it's probably not worth the torture.
> 
> ... oh, I see we could annotate with "// presubmit: allow wstring" too if the
> clutter is less annoying than the warning.

Yeah, I think it’s easiest to just deal with these. It’s bogus that this
PRESUBMIT check is enabled for arbitrary third_party code. If anything, that
should be fixed in preference to torturing our own code.

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:29: git pull --strategy=subtree --squash
crashpad master_or_hash
scottmg wrote:
> Sorry, I don't know. If I was doing it, I have made the steps something like
rm
> -rf, cp -R, git add, git commit, as it's going to land as a squashed patch
> anyway.

I don’t want to trash local modifications that were made for base interface
changes if those haven’t had a chance to migrate back up to the authoritative
home, though. That’s why I went with a merge-based approach.

> I would probably summon primiano@ as he seems to know a lot about all of git's
> funny business.

Good call, I sent out a cry for help.

[Robert Sesek, 2015-12-09 16:26:13]

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/OW...
File third_party/crashpad/OWNERS (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/OW...
third_party/crashpad/OWNERS:7: # Aside from work necessary to adhere to changes
in Chromium interfaces used by
This is complicated enough that I'd maybe consider a "set noparent".

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:29: git pull --strategy=subtree --squash
crashpad master_or_hash
On 2015/12/08 23:36:24, Mark Mentovai wrote:
> I want to replace this sequence of steps with a script that also formats the
> commit message nicely, updates the revision in this file, and maybe drops the
> “local modifications” section if there are no diffs between upstream and
the
> import. But before I do that, I wanted to ask some questions about this
process.
> 
> I had expected to tell the merge both the “from” and “to” commit-ishes
from
> Crashpad to merge into the current branch. This only has a “to”, and
presumably
> git does something to figure out where to start merging from. This is a little
> more scary to me, since we won’t have merge commits to identify this
> definitively to git. We do have the “revision” field in this file, and I was
> hoping to use that. Do either of you know of a way to make this work?
> 
> The other bit of git magic is that the subtree merge magically finds
> third_party/crashpad/crashpad without being told. That’s scary too, and I want
> to tell it the path explicitly. I thought that “--strategy=recursive
> -Xsubtree=third_party/crashpad/crashpad” would work, but it doesn’t. Ideas?

Try looking at https://git-scm.com/docs/git-merge-base. It's generally for
three-way/octopus merges, but you can potentially use it to identify the
divergent point and make sure it matches the hash in the README. I don't think
there's a way to manually specify the mergebase (that's basically a rebase
--onto then, isn't it?).

[Primiano Tucci (use gerrit), 2015-12-09 16:48:47]

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

[Primiano Tucci (use gerrit), 2015-12-09 16:48:49]

The deps to chromium move LGTM.
Maybe the license checker will bark. See what happened here
https://codereview.chromium.org/1410853008
Some comments about the merge / workflow inline

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:29: git pull --strategy=subtree --squash
crashpad master_or_hash
On 2015/12/09 16:26:13, Robert Sesek wrote:
> On 2015/12/08 23:36:24, Mark Mentovai wrote:
> > I want to replace this sequence of steps with a script that also formats the
> > commit message nicely, updates the revision in this file, and maybe drops
the
> > “local modifications” section if there are no diffs between upstream
and
> the
> > import. But before I do that, I wanted to ask some questions about this
> process.
> > 
> > I had expected to tell the merge both the “from” and “to” commit-ishes
> from
> > Crashpad to merge into the current branch. This only has a “to”, and
> presumably
> > git does something to figure out where to start merging from. This is a
little
> > more scary to me, since we won’t have merge commits to identify this
> > definitively to git. We do have the “revision” field in this file, and I was
> > hoping to use that. Do either of you know of a way to make this work?
> > 
> > The other bit of git magic is that the subtree merge magically finds
> > third_party/crashpad/crashpad without being told. That’s scary too, and I
want
> > to tell it the path explicitly. I thought that “--strategy=recursive
> > -Xsubtree=third_party/crashpad/crashpad” would work, but it doesn’t. Ideas?
> 
> Try looking at https://git-scm.com/docs/git-merge-base. It's generally for
> three-way/octopus merges, but you can potentially use it to identify the
> divergent point and make sure it matches the hash in the README. I don't think
> there's a way to manually specify the mergebase (that's basically a rebase
> --onto then, isn't it?).

I had an internal thread with Mark and scott.
I think this cannot be just one git command. You need a script here.
by squashing (and you cannot do otherwise, you can't do merges in the chromium
repo) git does not have enough context on what CLs to merge.
It's really a ~10 lines bash script you want here.

copy-pasting from the internal thread with Mark:

To port a change chromium -> crashpad
In the chromium repo:
git format-patch -o /tmp/ -1 --binary $SHA1 --add-header="Message-Id: Merged
from chromium $SHA1" -- third_party/crashpad/ # (mind the trailing slash)
with SHA1 being the SHA1 of the change you want to port.

In the crashpad repo:
git am -m -3 -p3 /tmp/0001-whatever-name.patch
git cl upload

(I love how -1 in the format-patch command means "just export the 1st commit"
while -3 in am means "three way merge")

To sync-up changes crashpad -> chromium 
well here you need a script which just figures out the last thing you merged
from chromium and takes the rest.
Logically the script should do something like:
 - Find the last commit in the history of chromium -- third_party/crashpad/ that
was merged (use some annotation in the commit message)
 - from there you should get the SHA1 (in $true_crashpad_repo) of the last
crashpad CL merged
 - at this point you want to get the revlist of crashpad for all commits in the
range SHA1..HEAD excluding the chromium one (which have the "Merged from
chromium" annotation) and cherry-pick +squash them

Let me know if you need help on the latter (should be a short-ish bash script)

[Mark Mentovai, 2015-12-09 22:09:45]

Thanks, everyone. Updated.

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/DEPS
File third_party/crashpad/DEPS (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/DE...
third_party/crashpad/DEPS:5: # Crashpad has its own #include structure not
rooted at Chromium’s root.
scottmg wrote:
> On 2015/12/08 23:36:24, Mark Mentovai wrote:
> > Noting that this file needed a UTF-8 BOM for checkdeps to process it.
> 
> I don't see one in
> https://codereview.chromium.org/1505213004/patch/10001/5678957424803840 is it
> just missing in the UI?

Yeah, the UI isn’t showing it at all, but if you download
https://codereview.chromium.org/download/issue1505213004_10001_56789574248038...,
you’ll see it.

I wonder what other characters we don’t indicate. :/

I wanted to fix checkdeps to make it assume UTF-8, but it uses execfile() to
read in DEPS files, and you can’t give that an assumed encoding.

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/OW...
File third_party/crashpad/OWNERS (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/OW...
third_party/crashpad/OWNERS:7: # Aside from work necessary to adhere to changes
in Chromium interfaces used by
Robert Sesek wrote:
> This is complicated enough that I'd maybe consider a "set noparent".

Done.

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/10001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:29: git pull --strategy=subtree --squash
crashpad master_or_hash
After some discussion, this turned into the script at
https://codereview.chromium.org/1513843002/.

[Mark Mentovai, 2015-12-09 22:36:58]

https://codereview.chromium.org/1505213004/diff/20001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/20001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:28: To carry changes made in Chromium to
Crashpad, run:
I don’t know if we really need to explain this here, it seems kind of out of
scope.

[scottmg, 2015-12-09 22:40:30]

lgtm

[Robert Sesek, 2015-12-09 22:42:45]

LGTM

https://codereview.chromium.org/1505213004/diff/20001/third_party/crashpad/RE...
File third_party/crashpad/README.chromium (right):

https://codereview.chromium.org/1505213004/diff/20001/third_party/crashpad/RE...
third_party/crashpad/README.chromium:28: To carry changes made in Chromium to
Crashpad, run:
On 2015/12/09 22:36:57, Mark Mentovai wrote:
> I don’t know if we really need to explain this here, it seems kind of out of
> scope.

It should be documented somewhere, but yes, maybe here isn't the best place.

[commit-bot: I haz the power, 2015-12-10 14:34:56]

Description was changed from

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
==========

to

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
R=primiano@chromium.org, rsesek@chromium.org, scottmg@chromium.org

Committed: https://crrev.com/b212e219b061f342502635af586ff4fc0e14a4d5
Cr-Commit-Position: refs/heads/master@{#364355}
==========

[commit-bot: I haz the power, 2015-12-10 14:34:58]

Patchset 3 (id:??) landed as
https://crrev.com/b212e219b061f342502635af586ff4fc0e14a4d5
Cr-Commit-Position: refs/heads/master@{#364355}

[Mark Mentovai, 2015-12-10 14:35:24]

Description was changed from

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
R=primiano@chromium.org, rsesek@chromium.org, scottmg@chromium.org

Committed: https://crrev.com/b212e219b061f342502635af586ff4fc0e14a4d5
Cr-Commit-Position: refs/heads/master@{#364355}
==========

to

==========
Copy Crashpad into the Chrome tree instead of importing it via DEPS

Crashpad’s revision, 6bebb10829332dee5c7315abafb0a8bf32840c15, is not
changing in this commit.

BUG=472900
R=primiano@chromium.org, rsesek@chromium.org, scottmg@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/b212e219b061f342502635af586f...
==========

[Mark Mentovai, 2015-12-10 14:35:26]

Committed patchset #3 (id:20001) manually as
b212e219b061f342502635af586ff4fc0e14a4d5 (presubmit successful).