Bug 74472

Summary: [chromium] Remove redundant third_party entries from chromium DEPS
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: Tools / TestsAssignee: Tony Chang <tony>
Severity: Normal CC: abarth, dglazkov, dpranke, jamesr, jochen, maruel, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch for landing none

Description Kenneth Russell 2011-12-13 17:57:56 PST
Currently Source/WebKit/chromium/DEPS contains an entry for 'third_party' as well as several directories under there. The entry is currently needed in order to create the third_party directory, but the redundancy is causing two sets of problems. First, it is causing intermittent gclient failures on the Windows builders due to gclient's parallelization and the fact that the top-level directory is locked by Subversion while gclient is attempting to populate subdirectories. See http://build.webkit.org/builders/Chromium%20Win%20Release/builds/36771 for an example of such a failure. Second, all of Chromium's directories in third_party are being pulled in via this rule. This makes the explicit listing of the dependencies under that directory meaningless -- and, in fact, it seems that several additional dependencies have crept in unnoticed.

update-webkit-chromium should be updated to manually create the third_party directory before running gclient, and all of the needed third_party dependencies should be enumerated in the DEPS file.
Comment 1 Tony Chang 2011-12-13 18:29:51 PST
The entry isn't needed to create the directory.  I think gclient is smart enough to create the necessary directories.  It was added because every week or two, someone would add something to src/third_party that webkit needed.  It mades rolling a lot harder when you need to add an entry to DEPS.

I thought gclient was smart enough to not start on a subdir until directories above it were finished.

We could also just turn parallel syncs off on the bot.
Comment 2 Marc-Antoine Ruel 2011-12-13 18:33:41 PST
- gclient won't sync a subdirectory before the parent directory is done. e.g.
if you have dependencies src and src/third_party/foo, src/third_party/foo won't be synced until src is completely done.
- gclient will create the directories itself up to the parent directory of the solution. So if you have src but no src/third_party and sync src/third_party/foo, gclient will first mkdir src/third_party, once src is completely synced.
Comment 3 Kenneth Russell 2011-12-13 18:37:07 PST
I see. As I'm struggling to get this change to actually work and adding in the required dependencies one by one I see what you mean.

Another solution, I'm pretty sure, would be to just remove the explicit checkouts of the third_party directories coming from Var('chromium_svn')+'/third_party/...'. They're useless given that we're already checking out the parent directory. Any objection to my making this change?
Comment 4 Kenneth Russell 2011-12-13 19:25:37 PST
Created attachment 119138 [details]
Comment 5 Kenneth Russell 2011-12-13 19:26:16 PST
What do you think about this? It seems safe, correct and should clear up these intermittent Windows gclient failures.
Comment 6 WebKit Review Bot 2011-12-13 20:21:46 PST
Comment on attachment 119138 [details]

Attachment 119138 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10873072
Comment 7 Kenneth Russell 2011-12-13 20:29:52 PST
Hmm. That built locallly.
Comment 8 Tony Chang 2011-12-14 10:04:12 PST
This patch looks correct to me.  I think gclient is getting confused because it thinks that third_party/libwebp is safe to delete.

Maybe there's some way to get it to update without having to clobber the bots.  I'll try a few things locally.
Comment 9 Tony Chang 2011-12-14 16:41:59 PST
Created attachment 119328 [details]
Comment 10 Tony Chang 2011-12-14 16:42:35 PST
After the bots cycle, we can revert the change to update-webkit-chromium.
Comment 11 Kenneth Russell 2011-12-14 16:50:20 PST
Comment on attachment 119328 [details]

Thanks, looks good; r=me. Do you want to file a follow-on bug about the cleanup and reference it in the comment?
Comment 12 Tony Chang 2011-12-14 17:00:13 PST
Created attachment 119336 [details]
Patch for landing
Comment 13 Kenneth Russell 2011-12-14 18:59:25 PST
Another instance of the same problem, this time on Linux:

Comment 14 WebKit Review Bot 2011-12-14 22:34:39 PST
Comment on attachment 119336 [details]
Patch for landing

Clearing flags on attachment: 119336

Committed r102891: <http://trac.webkit.org/changeset/102891>
Comment 15 WebKit Review Bot 2011-12-14 22:34:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 jochen 2012-01-02 07:29:15 PST
With this patch, update-webkit --chromium asks you to remove those directories, and if you fall for this trap, your checkout is broken :-/
Comment 17 Tony Chang 2012-01-03 10:15:58 PST
(In reply to comment #16)
> With this patch, update-webkit --chromium asks you to remove those directories, and if you fall for this trap, your checkout is broken :-/

That's true, we should have sent a note to chromium-dev about this.