Checkout Web Page Replay in src/third_party (was chrome/tools/build/third_party).

Checkout Web Page Replay in src/third_party (was chrome/tools/build/third_party).


BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134380

[slamm_google, 2012-04-18 20:25:57]

When I run "gclient sync" with this change, it does not pull in WPR. 

Is that because I have a safesync_url? Or, did not I specify the dependency
correctly?

[cmp, 2012-04-18 20:35:38]

On 2012/04/18 20:25:57, slamm wrote:
> When I run "gclient sync" with this change, it does not pull in WPR. 
> 
> Is that because I have a safesync_url? Or, did not I specify the dependency
> correctly?

safesync_url should override the checkout to use LKGR, so I would expect your
local changes to be ignored.  I haven't tested that myself, so I'm not entirely
sure.  Can you try without the safesync_url?

https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS#newcode339
DEPS:339: "http://web-page-replay.googlecode.com/svn/trunk@447",
Use (Var("googlecode_url") % "web-page-replay") here instead of
"http://web-page-replay.googlecode.com/svn".

File a ticket to request adding this repo to our local SVN mirror in case it
doesn't already exist.

https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS#newcode341
DEPS:341: "http://ipaddr-py.googlecode.com/svn/trunk@244",
Use (Var("googlecode_url") % "ipaddr-py") here, too.

In the same ticket as above, request adding this repo, too.

(file the ticket at new.crbug.com using the "Build Infrastructure" template.)

[slamm_google, 2012-04-18 21:14:46]

I uploaded a new patch and filed a ticket:

http://code.google.com/p/chromium/issues/detail?id=124073

https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS#newcode339
DEPS:339: "http://web-page-replay.googlecode.com/svn/trunk@447",
On 2012/04/18 20:35:38, cmp wrote:
> Use (Var("googlecode_url") % "web-page-replay") here instead of
> "http://web-page-replay.googlecode.com/svn".
> 
> File a ticket to request adding this repo to our local SVN mirror in case it
> doesn't already exist.

Done.

https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS#newcode341
DEPS:341: "http://ipaddr-py.googlecode.com/svn/trunk@244",
On 2012/04/18 20:35:38, cmp wrote:
> Use (Var("googlecode_url") % "ipaddr-py") here, too.
> 
> In the same ticket as above, request adding this repo, too.
> 
> (file the ticket at http://new.crbug.com using the "Build Infrastructure"
template.)

Done.

[slamm_google, 2012-04-18 21:25:27]

On 2012/04/18 20:35:38, cmp wrote:
> On 2012/04/18 20:25:57, slamm wrote:
> > When I run "gclient sync" with this change, it does not pull in WPR. 
> > 
> > Is that because I have a safesync_url? Or, did not I specify the dependency
> > correctly?
> 
> safesync_url should override the checkout to use LKGR, so I would expect your
> local changes to be ignored.  I haven't tested that myself, so I'm not
entirely
> sure.  Can you try without the safesync_url?

I kicked off a "gclient sync" without the safesync_url and it hangs.

The git instructions talk about setting up LKGR, but not removing it.
http://code.google.com/p/chromium/wiki/UsingNewGit#Initial_checkout

There are also some DEPS instructions I do not quite understand:

http://code.google.com/p/chromium/wiki/UsingNewGit#Rolling_DEPS

-slamm

> https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS
> File DEPS (right):
> 
> https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS#newcode339
> DEPS:339: "http://web-page-replay.googlecode.com/svn/trunk@447",
> Use (Var("googlecode_url") % "web-page-replay") here instead of
> "http://web-page-replay.googlecode.com/svn".
> 
> File a ticket to request adding this repo to our local SVN mirror in case it
> doesn't already exist.
> 
> https://chromiumcodereview.appspot.com/10020064/diff/1/DEPS#newcode341
> DEPS:341: "http://ipaddr-py.googlecode.com/svn/trunk@244",
> Use (Var("googlecode_url") % "ipaddr-py") here, too.
> 
> In the same ticket as above, request adding this repo, too.
> 
> (file the ticket at http://new.crbug.com using the "Build Infrastructure"
template.)

[cmp, 2012-04-19 00:28:43]

https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS#newcode339
DEPS:339: (Var("googlecode_url") % "web-page-replay") + "/trunk@447",
hmm, are there other examples of a dep-within-a-dep in this file?  have you
tested this and verified it works?

i worry that there's not an example of this today, and it may throw us off in
the future if we add it now.

[cmp, 2012-04-19 00:30:09]

https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS#newcode339
DEPS:339: (Var("googlecode_url") % "web-page-replay") + "/trunk@447",
On 2012/04/19 00:28:43, cmp wrote:
> hmm, are there other examples of a dep-within-a-dep in this file?  have you
> tested this and verified it works?
> 
> i worry that there's not an example of this today, and it may throw us off in
> the future if we add it now.

what i'm thinking about is that gclient sync has a -j arg which allows it to
checkout multiple deps in parallel.  if it starts processing the second dep
before the first, then that will probably get confused.  +maruel for insight.

[Marc-Antoine Ruel (Google), 2012-04-19 00:54:08]

On 2012/04/19 00:30:09, cmp wrote:
> what i'm thinking about is that gclient sync has a -j arg which allows it to
> checkout multiple deps in parallel.  if it starts processing the second dep
> before the first, then that will probably get confused.  +maruel for insight.

And for a git checkout, you'd need to have .DEPS.git updated too.

-j1 helps figuring out issues with sync since it serializes the operations.

[slamm_google, 2012-04-19 17:51:51]

Is .DEPS.git updated automatically? Seems like I cannot test that until
web-page-replay and ipaddr get added.

Here is the issue to add the repositories:
http://code.google.com/p/chromium/issues/detail?id=124073

Another comment inline...

https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS#newcode339
DEPS:339: (Var("googlecode_url") % "web-page-replay") + "/trunk@447",
On 2012/04/19 00:28:43, cmp wrote:
> hmm, are there other examples of a dep-within-a-dep in this file?  have you
> tested this and verified it works?
> 
> i worry that there's not an example of this today, and it may throw us off in
> the future if we add it now.

I had not thought of that. There is one other case in this file:
   src/third_party/WebKit
That has various sub directories:
   src/third_party/WebKit/LayoutTests
   src/third_party/WebKit/Source
   src/third_party/WebKit/Tools/DumpRenderTree
   src/third_party/WebKit/Tools/Scripts
   src/third_party/WebKit/Tools/TestWebKitAPI

In you look in gclient_utils.py, someone has written code to handle this issue:

def safe_makedirs(tree):
  """Creates the directory in a safe manner.                                    
                                                                      
                                                                                
                                                                      
  Because multiple threads can create these directories concurently, trap the   
                                                                      
  exception and pass on.                                                        
                                                                      
  """
  count = 0
  while not os.path.exists(tree):
    count += 1
    try:
      os.makedirs(tree)
    except OSError, e:
      # 17 POSIX, 183 Windows                                                   
                                                                      
      if e.errno not in (17, 183):
        raise
      if count > 40:
        # Give up.                                                              
                                                                      
        raise

[cmp, 2012-04-19 18:06:04]

On 2012/04/19 17:51:51, slamm wrote:
> Is .DEPS.git updated automatically? Seems like I cannot test that until
> web-page-replay and ipaddr get added.

.DEPS.git is automatically updated.  I think M-A's comment was just M-A speak
for make sure that the right Git repos are created for you.  That was covered by
my first comment above and the ticket you filed.  So I think this comment is
safe to ignore.

> 
> Here is the issue to add the repositories:
> http://code.google.com/p/chromium/issues/detail?id=124073
> 
> Another comment inline...
> 
> https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS
> File DEPS (right):
> 
> https://chromiumcodereview.appspot.com/10020064/diff/6001/DEPS#newcode339
> DEPS:339: (Var("googlecode_url") % "web-page-replay") + "/trunk@447",
> On 2012/04/19 00:28:43, cmp wrote:
> > hmm, are there other examples of a dep-within-a-dep in this file?  have you
> > tested this and verified it works?
> > 
> > i worry that there's not an example of this today, and it may throw us off
in
> > the future if we add it now.
> 
> I had not thought of that. There is one other case in this file:
>    src/third_party/WebKit
> That has various sub directories:
>    src/third_party/WebKit/LayoutTests
>    src/third_party/WebKit/Source
>    src/third_party/WebKit/Tools/DumpRenderTree
>    src/third_party/WebKit/Tools/Scripts
>    src/third_party/WebKit/Tools/TestWebKitAPI
> 
> In you look in gclient_utils.py, someone has written code to handle this
issue:
> 
> def safe_makedirs(tree):
>   """Creates the directory in a safe manner.                                  
 
>                                                                       
>                                                                               
 
>                                                                       
>   Because multiple threads can create these directories concurently, trap the 
 
>                                                                       
>   exception and pass on.                                                      
 
>                                                                       
>   """
>   count = 0
>   while not os.path.exists(tree):
>     count += 1
>     try:
>       os.makedirs(tree)
>     except OSError, e:
>       # 17 POSIX, 183 Windows                                                 
 
>                                                                       
>       if e.errno not in (17, 183):
>         raise
>       if count > 40:
>         # Give up.                                                            
 
>                                                                       
>         raise

Good find.  This method appears to protect the caller from the 'file exists'
condition.  And that's important to at least avoid failing since you might be
creating a directory that another thread just created.

Where I'm confused is I just tested and found that cloning only works when the
directory doesn't exist or if it's empty:

cmp@terakrom:/work/depot_tools$ mkdir v8-i18n
cmp@terakrom:/work/depot_tools$ git clone
http://git.chromium.org/external/v8-i18n.git
Cloning into 'v8-i18n'...
remote: Counting objects: 56, done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 56 (delta 25), reused 56 (delta 25)
Unpacking objects: 100% (56/56), done.
cmp@terakrom:/work/depot_tools$ rm -rf v8-i18n
cmp@terakrom:/work/depot_tools$ mkdir v8-i18n
cmp@terakrom:/work/depot_tools$ mkdir v8-i18n/foo
cmp@terakrom:/work/depot_tools$ git clone
http://git.chromium.org/external/v8-i18n.git
fatal: destination path 'v8-i18n' already exists and is not an empty directory.

M-A, does gclient order dep directories so only a single thread handles those
directories with deps inside each other, and then that single thread ensures it
checks out the parent's deps first, then the child's deps?   In gclient_scm.py,
where safe_makedirs() is first called (in _Clone) to create the parent dir,
there's no safeguard before the git clone command is called.  Maybe there's code
elsewhere in gclient to avoid this issue?

[cmp, 2012-04-19 18:16:45]

M-A tells me that gclient has magic to ensure that
deps-directories-within-deps-directories are ordered correctly.  And across
solutions, too.  So ancestors are always checked out before descendants, and
this should not be an issue.  If it ever comes up as an issue in the field
later, we can revisit then.

[cmp, 2012-04-19 18:19:30]

lgtm once mirror repos are created/available

[slamm_google, 2012-04-24 22:12:08]

On 2012/04/19 18:19:30, cmp wrote:
> lgtm once mirror repos are created/available

The mirror bug has been sitting untouched for 6 days. Did it get dropped, or is
it expected to take a while?

http://code.google.com/p/chromium/issues/detail?id=124073

[cmp, 2012-04-26 22:02:33]

Hi Steve, that bug should be fixed now.  Looks like you'll need to add ipaddr to
third_party (easiest to do it in this CL), and then you'll be ready to land
here.

[slamm_google, 2012-04-26 22:57:27]

The DEPS change includes putting ipaddr under WPRs third_party directory:

+ "src/third_party/webpagereplay/third_party/ipaddr": +
(Var("googlecode_url") % "ipaddr") + "/trunk@244",

Are you thinking that is not the way to go?

-slamm

On Thu, Apr 26, 2012 at 3:02 PM, <cmp@chromium.org> wrote:

> Hi Steve, that bug should be fixed now.  Looks like you'll need to add
> ipaddr to
> third_party (easiest to do it in this CL), and then you'll be ready to land
> here.
>
>
https://chromiumcodereview.**appspot.com/10020064/<https://chromiumcodereview...
>

[cmp, 2012-04-26 23:50:04]

slamm and I spoke over IM and he plans to investigate adding ipaddr into
web-page-replay directly to work around not being able to have a mirror of
ipaddr on Chrome's SVN systems

[slamm_google, 2012-04-27 18:18:48]

WPR now has third_party/ipaddr similar the to other 3rd party sources it uses. I
took the opportunity to double check the licenses for all the code there and
added README.web-page-replay files for each project too.

DEPS diff only adds WPR now.

PTAL

[cmp, 2012-04-27 18:22:52]

lgtm

[James Simonsen, 2012-04-27 18:29:07]

https://chromiumcodereview.appspot.com/10020064/diff/17001/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/10020064/diff/17001/DEPS#newcode339
DEPS:339: (Var("googlecode_url") % "web-page-replay") + "/trunk@447",
I think you need to update this. r454 has the new third_party resources.

[cmp, 2012-04-27 18:29:57]

Good catch, I did not verify the revision was correct/updated in DEPS.  Thanks
James.

[slamm_google, 2012-04-27 19:37:06]

On 2012/04/27 18:29:57, cmp wrote:
> Good catch, I did not verify the revision was correct/updated in DEPS.  Thanks
> James.

Thanks, James. I bumped the revision to 454. If every revision was a gram, we
would have a pound of them now.

[commit-bot: I haz the power, 2012-04-27 21:01:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/slamm@google.com/10020064/21002

[commit-bot: I haz the power, 2012-04-27 22:57:06]

Change committed as 134380