Add an AOSP builder recipe.

Add an AOSP builder recipe.

This adds a recipe for building the android_webview Chromium project
within the Android tree.

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

[mkosiba (inactive), 2013-05-03 17:45:59]

Hey Aaron! PTAL. This seems to run fine locally (testing this without a master
is soooo much better! the recipe stuff is real nice!)

The recipe depends on scripts not yet checked into Chromium
(https://codereview.chromium.org/14669003/). If you think any of these (like
with_lunch or generate_local_manifest.py) would be better off living in the
buildbot repository let me know and I'll move 'em.

[mkosiba (inactive), 2013-05-04 19:30:14]

ping?

[agable, 2013-05-06 17:53:11]

Sorry, was OOO on friday. Further comments on compile.py and the recipe
forthcoming. In the mean time, +iannucci to look at the recipe too.

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipe_util.py#...
scripts/slave/recipe_util.py:257: ret.update({'cwd': cwd})
This is already handled by **kwargs

[mkosiba (inactive), 2013-05-07 13:30:18]

Thanks Aaron! sorry about the ping, I didn't know you were OOO and assumed the
review got lost in email.

I've uploaded a new version. I moved the deps whitelist logic out to a script in
the Chromium tree (https://codereview.chromium.org/14669003). I also made the
recipe use a copy of repo from Android if it's available (this lets us safely
re-init the Android checkout and properly support changing the branch we're
tracking).

PTAL

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipe_util.py#...
scripts/slave/recipe_util.py:188: class DelayedEvalParameter(object):
don't need this

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipe_util.py#...
scripts/slave/recipe_util.py:257: ret.update({'cwd': cwd})
On 2013/05/06 17:53:11, Aaron Gable wrote:
> This is already handled by **kwargs

Done.

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipes/android...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://codereview.chromium.org/14602020/diff/1/scripts/slave/recipes/android...
scripts/slave/recipes/android_webview_aosp.py:95: class TrimmedDepsSpec(object):
would like feedback on this. Is it OK to implement "lazy" eval of arguments like
this?

https://codereview.chromium.org/14602020/diff/5001/scripts/slave/recipes/andr...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://codereview.chromium.org/14602020/diff/5001/scripts/slave/recipes/andr...
scripts/slave/recipes/android_webview_aosp.py:118: steps.step('repo init',
['echo', '\n\ny\n'], pipes=[[
is this the best way to supply the repo init command with input?

[agable, 2013-05-07 23:10:18]

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipe_util....
scripts/slave/recipe_util.py:187: 
Can you restore the whitespace?

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipes/andr...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipes/andr...
scripts/slave/recipes/android_webview_aosp.py:93: repo_init_step = []
repo_init_steps

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipes/andr...
scripts/slave/recipes/android_webview_aosp.py:116: steps.git_step('config',
'--global', 'user.email', 'chrome-bot@google.com'),
These are a bad idea. Ideally, recipes can and will be run by developers on
their local machines to replicate what the bots are doing. In addition, you have
no guarantee that the bot running this builder will never run any other builder.
We should avoid doing anything global.

[mkosiba (inactive), 2013-05-08 10:07:00]

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipe_util....
scripts/slave/recipe_util.py:187: 
On 2013/05/07 23:10:18, Aaron Gable wrote:
> Can you restore the whitespace?

Done.

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipes/andr...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://codereview.chromium.org/14602020/diff/6006/scripts/slave/recipes/andr...
scripts/slave/recipes/android_webview_aosp.py:116: steps.git_step('config',
'--global', 'user.email', 'chrome-bot@google.com'),
On 2013/05/07 23:10:18, Aaron Gable wrote:
> These are a bad idea. Ideally, recipes can and will be run by developers on
> their local machines to replicate what the bots are doing. In addition, you
have
> no guarantee that the bot running this builder will never run any other
builder.
> We should avoid doing anything global.

ok. I assumed this would be better than piping "\n\ny\n" into repo but I agree
that mucking with the global settings is a bad idea.

[mkosiba (inactive), 2013-05-09 08:45:02]

@iannucci - I'm guessing Aaron added you for compile.py - could you PTAL.

Also, CL is ready for another round of review.

[iannucci, 2013-05-09 21:13:37]

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
scripts/slave/recipes/android_webview_aosp.py:43: # TODO: better way?
*wistles*.  Cool hax bro :D.

Unfortunately, this will be (completely?) untestable (yes, I know we don't have
tests yet, but they're definitely planned).

I'll cook up a CL which will be a more scalable/testable way to support dynamic
step reconfiguration :)

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
scripts/slave/recipes/android_webview_aosp.py:121: pipes=[['cat']],
Ick. I think I'd (much) rather force ALL stdout for annotator steps to be
non-interactive.

[mkosiba (inactive), 2013-05-10 15:08:52]

so I rebased and I removed piping repo sync through 'cat'.

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
scripts/slave/recipes/android_webview_aosp.py:43: # TODO: better way?
On 2013/05/09 21:13:38, iannucci wrote:
> *wistles*.  Cool hax bro :D.

I was expecting more of a "omg, this is terrible" reaction, so thanks! :)
 
> Unfortunately, this will be (completely?) untestable (yes, I know we don't
have
> tests yet, but they're definitely planned).
>
> I'll cook up a CL which will be a more scalable/testable way to support
dynamic
> step reconfiguration :)

sweet! looking forward to it. Could you CC me on the review?

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
scripts/slave/recipes/android_webview_aosp.py:121: pipes=[['cat']],
On 2013/05/09 21:13:38, iannucci wrote:
> Ick. I think I'd (much) rather force ALL stdout for annotator steps to be
> non-interactive.

ah.. exciting news here - this was caused by the run_recipe tool passing
--keep-stdio to annotated_run.py. After removing that option it works fine
without the cat too!

[iannucci, 2013-05-10 20:21:53]

I'll CC you on the new mechanism. I hope to have it up today. I just put up a
pretty ugly CL to add the first layer of tests, in case that gives you an idea
of where we're going: https://chromiumcodereview.appspot.com/14988009/

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
scripts/slave/recipes/android_webview_aosp.py:121: pipes=[['cat']],
On 2013/05/10 15:08:52, mkosiba wrote:
> On 2013/05/09 21:13:38, iannucci wrote:
> > Ick. I think I'd (much) rather force ALL stdout for annotator steps to be
> > non-interactive.
> 
> ah.. exciting news here - this was caused by the run_recipe tool passing
> --keep-stdio to annotated_run.py. After removing that option it works fine
> without the cat too!

Ah, sweet! I should have remembered that.

It would be nice if there was a clever way to disable the tty-ness of the stdio
fd's without closing them completely. In particular, pdb should still be
functional (that's why I added keep-stdio in the first place)... I'm not really
thinking of a way though.

How hard would it be to add a non-interactive arg to repo?

[mkosiba (inactive), 2013-05-14 16:19:59]

On 2013/05/10 20:21:53, iannucci wrote:
> I'll CC you on the new mechanism. I hope to have it up today. I just put up a
> pretty ugly CL to add the first layer of tests, in case that gives you an idea
> of where we're going: https://chromiumcodereview.appspot.com/14988009/

Thanks for the test CL. It sounds like a great idea to have coverage on the
recpies. Have you made any progress on the new mechanism?

> 
>
https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
> File scripts/slave/recipes/android_webview_aosp.py (right):
> 
>
https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
> scripts/slave/recipes/android_webview_aosp.py:121: pipes=[['cat']],
> On 2013/05/10 15:08:52, mkosiba wrote:
> > On 2013/05/09 21:13:38, iannucci wrote:
> > > Ick. I think I'd (much) rather force ALL stdout for annotator steps to be
> > > non-interactive.
> > 
> > ah.. exciting news here - this was caused by the run_recipe tool passing
> > --keep-stdio to annotated_run.py. After removing that option it works fine
> > without the cat too!
> 
> Ah, sweet! I should have remembered that.
> 
> It would be nice if there was a clever way to disable the tty-ness of the
stdio
> fd's without closing them completely. In particular, pdb should still be
> functional (that's why I added keep-stdio in the first place)... I'm not
really
> thinking of a way though.
> 
> How hard would it be to add a non-interactive arg to repo?

Not too hard, however I don't think I want to block this change on the repo
release schedule.

[iannucci, 2013-05-14 18:29:40]

On 2013/05/14 16:19:59, mkosiba wrote:
> On 2013/05/10 20:21:53, iannucci wrote:
> > I'll CC you on the new mechanism. I hope to have it up today. I just put up
a
> > pretty ugly CL to add the first layer of tests, in case that gives you an
idea
> > of where we're going: https://chromiumcodereview.appspot.com/14988009/
> 
> Thanks for the test CL. It sounds like a great idea to have coverage on the
> recpies. Have you made any progress on the new mechanism?

Yeah it's 3/4 baked, but it's dependent on the tests, so I was hoping to land
those first. I'm trying to get this out today.

> 
> > 
> >
>
https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
> > File scripts/slave/recipes/android_webview_aosp.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/14602020/diff/19001/scripts/slave/reci...
> > scripts/slave/recipes/android_webview_aosp.py:121: pipes=[['cat']],
> > On 2013/05/10 15:08:52, mkosiba wrote:
> > > On 2013/05/09 21:13:38, iannucci wrote:
> > > > Ick. I think I'd (much) rather force ALL stdout for annotator steps to
be
> > > > non-interactive.
> > > 
> > > ah.. exciting news here - this was caused by the run_recipe tool passing
> > > --keep-stdio to annotated_run.py. After removing that option it works fine
> > > without the cat too!
> > 
> > Ah, sweet! I should have remembered that.
> > 
> > It would be nice if there was a clever way to disable the tty-ness of the
> stdio
> > fd's without closing them completely. In particular, pdb should still be
> > functional (that's why I added keep-stdio in the first place)... I'm not
> really
> > thinking of a way though.
> > 
> > How hard would it be to add a non-interactive arg to repo?
> 
> Not too hard, however I don't think I want to block this change on the repo
> release schedule.

[mkosiba (inactive), 2013-05-16 15:52:39]

On 2013/05/14 18:29:40, iannucci wrote:
> On 2013/05/14 16:19:59, mkosiba wrote:
> > On 2013/05/10 20:21:53, iannucci wrote:
> > > I'll CC you on the new mechanism. I hope to have it up today. I just put
up
> a
> > > pretty ugly CL to add the first layer of tests, in case that gives you an
> idea
> > > of where we're going: https://chromiumcodereview.appspot.com/14988009/
> > 
> > Thanks for the test CL. It sounds like a great idea to have coverage on the
> > recpies. Have you made any progress on the new mechanism?
> 
> Yeah it's 3/4 baked, but it's dependent on the tests, so I was hoping to land
> those first. I'm trying to get this out today.

Test CL went in. Is there any way I can help you with this? I can change the
recipe to invoke a script instead of delayed-evaluating the DEPS.

We'd really like to get the AOSP bot up and running. Our downstream Android
build just got hit with another build break...

[iannucci, 2013-05-17 09:15:43]

On 2013/05/16 15:52:39, mkosiba wrote:
> On 2013/05/14 18:29:40, iannucci wrote:
> > On 2013/05/14 16:19:59, mkosiba wrote:
> > > On 2013/05/10 20:21:53, iannucci wrote:
> > > > I'll CC you on the new mechanism. I hope to have it up today. I just put
> up
> > a
> > > > pretty ugly CL to add the first layer of tests, in case that gives you
an
> > idea
> > > > of where we're going: https://chromiumcodereview.appspot.com/14988009/
> > > 
> > > Thanks for the test CL. It sounds like a great idea to have coverage on
the
> > > recpies. Have you made any progress on the new mechanism?
> > 
> > Yeah it's 3/4 baked, but it's dependent on the tests, so I was hoping to
land
> > those first. I'm trying to get this out today.
> 
> Test CL went in. Is there any way I can help you with this? I can change the
> recipe to invoke a script instead of delayed-evaluating the DEPS.
> 
> We'd really like to get the AOSP bot up and running. Our downstream Android
> build just got hit with another build break...

Sorry for the wait. The new protocol CL is up:
https://chromiumcodereview.appspot.com/15270004/

This should allow you to re-sync with different deps without the weird __str__
hack. You'll need to add a script to your repo which accepts --json-output
/path/to/json/file, add a step which runs that script in your repo and dumps to
that file, and then another step which consumes that json and does a gclient
config/gclient sync using the new deps. I can help you out with this if needed.

You'll also need to add tests to cover the new recipe :)

Again, sorry for the delay. The recipe stuff is still in flux. Please don't
hesitate to bug me on IM to get this working.

[mkosiba (inactive), 2013-05-20 18:08:43]

On 2013/05/17 09:15:43, iannucci wrote:
> Sorry for the wait. The new protocol CL is up:
> https://chromiumcodereview.appspot.com/15270004/
> 

Thanks for getting it out quickly! I rebased on top of that CL. PTAL

> This should allow you to re-sync with different deps without the weird __str__
> hack. You'll need to add a script to your repo which accepts --json-output
> /path/to/json/file, add a step which runs that script in your repo and dumps
to
> that file, and then another step which consumes that json and does a gclient
> config/gclient sync using the new deps. I can help you out with this if
needed.
> 

done!

> You'll also need to add tests to cover the new recipe :)
> 

done!

[iannucci, 2013-05-20 19:41:38]

lg2m + comments. I'll let agable do the final review.

Thanks for putting up with us while we're getting this recipe stuff sorted :)

Were there any sticky parts about the recipes that you found? Things which
should be doc'd better?

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/common/ann...
File scripts/common/annotator.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/common/ann...
scripts/common/annotator.py:385: pipes=step_dict.get('pipes'))
I think this is no longer necessary?

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/andr...
File scripts/slave/android/generate_whitelist_deps_step.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/andr...
scripts/slave/android/generate_whitelist_deps_step.py:1: #!/usr/bin/env python
WDYT about having this script live in the repo that you're checking out?

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/andr...
scripts/slave/android/generate_whitelist_deps_step.py:18: import simplejson as
json
We don't need this dance any more, since we should always be >= 2.6 (actually,
2.7, but that's only 80% rolled out).

Also, you're using argparse, which requires 2.7 anyway :)

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/andr...
scripts/slave/android/generate_whitelist_deps_step.py:35: default=None)
Technically, None is the default for default :)

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/reci...
File scripts/slave/recipe_util.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/reci...
scripts/slave/recipe_util.py:97: if mock_path_exists:
Let's replace path_exists no matter what. If not mock_path_exists, we can mock
with lambda path: False.

Could we also convert mock_path_exists to a frozenset before closing it in the
lambda? :)

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/unit...
File scripts/slave/unittests/recipes_test.py (right):

https://chromiumcodereview.appspot.com/14602020/diff/33001/scripts/slave/unit...
scripts/slave/unittests/recipes_test.py:127: with
recipe_util.mock_paths(mock_path_exists=me):
Ah, I guess this is implicitly an empty list here

[mkosiba (inactive), 2013-05-21 10:56:07]

On 2013/05/20 19:41:38, iannucci wrote:
> lg2m + comments. I'll let agable do the final review.

Thanks! I've addressed your comments, looking forward to the final review :)

> Were there any sticky parts about the recipes that you found? Things which
> should be doc'd better?

The doc for the step_generator could do with an example or an explanation on why
someone would want to use the step generator - based on the doc alone it's
really hard to figure out how all of that comes together. Other than that it was
a very good experience! Good work guys!

https://codereview.chromium.org/14602020/diff/33001/scripts/common/annotator.py
File scripts/common/annotator.py (right):

https://codereview.chromium.org/14602020/diff/33001/scripts/common/annotator....
scripts/common/annotator.py:385: pipes=step_dict.get('pipes'))
On 2013/05/20 19:41:38, iannucci wrote:
> I think this is no longer necessary?

Done.

https://codereview.chromium.org/14602020/diff/33001/scripts/slave/android/gen...
File scripts/slave/android/generate_whitelist_deps_step.py (right):

https://codereview.chromium.org/14602020/diff/33001/scripts/slave/android/gen...
scripts/slave/android/generate_whitelist_deps_step.py:1: #!/usr/bin/env python
On 2013/05/20 19:41:38, iannucci wrote:
> WDYT about having this script live in the repo that you're checking out?

works for me!

https://codereview.chromium.org/14602020/diff/33001/scripts/slave/android/gen...
scripts/slave/android/generate_whitelist_deps_step.py:18: import simplejson as
json
On 2013/05/20 19:41:38, iannucci wrote:
> We don't need this dance any more, since we should always be >= 2.6 (actually,
> 2.7, but that's only 80% rolled out).
> 
> Also, you're using argparse, which requires 2.7 anyway :)

Done.

https://codereview.chromium.org/14602020/diff/33001/scripts/slave/android/gen...
scripts/slave/android/generate_whitelist_deps_step.py:35: default=None)
On 2013/05/20 19:41:38, iannucci wrote:
> Technically, None is the default for default :)

Done.

https://codereview.chromium.org/14602020/diff/33001/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/33001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:97: if mock_path_exists:
On 2013/05/20 19:41:38, iannucci wrote:
> Let's replace path_exists no matter what. If not mock_path_exists, we can mock
> with lambda path: False.
> 
> Could we also convert mock_path_exists to a frozenset before closing it in the
> lambda? :)

Done.

[mkosiba (inactive), 2013-05-22 17:14:48]

Aaron - PTAL

[agable, 2013-05-22 17:21:23]

Mostly just a bunch of naming nits. All the main logic looks good.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py
File scripts/slave/compile.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:730: clobber()
This is a little weird. We clobber if 'make' failed and we didn't clobber
previously? While that makes some sense, it seems like it should be accompanied
by retry logic. This is basically saying "aw shucks, I failed, and I don't know
why, so I guess I'll try to clean up a little for the next guy just in case."

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:1137: 'source tree')
*containing* the root? This makes it sound like if I have /tmp/mycheckout/src,
I'd want to pass --src-dir=/tmp/mycheckout. The code below makes it clear that
what is really expected is --src-dir=/tmp/mycheckout/src. Please clarify the
help string.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:1140: 'output folder')
Same here.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:70: def mock_paths(mock_path_exists=None):
Thanks for adding this functionality. I find mock_path_exists to be a confusing
name, though. It's a list of paths, so it should be plural, for one thing. Maybe
"paths_to_mock"? Not a big deal but I had to read all the way through the code
to figure out what it was doing because the variable name didn't give me a good
hint.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:387: step_name = lambda n: '[spec: %s] gclient %s'
% (spec_name, n)
I'd rather have the spec_name emitted as a @@@STEP_TEXT@@@ (so it shows up like
the revision annotations on normal gclient update steps) than directly included
in the step name. If that's too hard, though, feel free to leave as-is.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:23: '.repo', 'repo', 'repo')
android-src/.repo/repo/repo? man, our paths are ridiculous sometimes.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:89: steps.step('use repo from
Android source tree', [
This step name doesn't match your others. Maybe 'copy repo from Android'.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:119: repo_sync_step = [
repo re-sync followed immediately by repo sync is a little confusing.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:139: steps.step('remove
potentially stale chromium_org',
This step only exists of chromium_org does, so doesn't need 'potentially stale'
in the title? Sorry for the nits on step names, I'm just too aware of the
limitations of the buildbot waterfall display and these long names will end up
wrapped, ugly, and unfortunate :(

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:144: steps.step('symlink chromium
source into android tree',
Same here.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:161: steps.step('clobber', ['rm',
'-rf', slave_android_out_path]),
What's the difference between this clobber and the clobber contained in
compile.py?

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:164: compile_compiler_option = []
..._options

[agable, 2013-05-22 17:22:08]

Haha good timing with that ping.


On Wed, May 22, 2013 at 10:21 AM, <agable@chromium.org> wrote:

> Mostly just a bunch of naming nits. All the main logic looks good.
>
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/compile.py<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py>
> File scripts/slave/compile.py (right):
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/compile.py#**newcode730<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#newcode730>
> scripts/slave/compile.py:730: clobber()
> This is a little weird. We clobber if 'make' failed and we didn't
> clobber previously? While that makes some sense, it seems like it should
> be accompanied by retry logic. This is basically saying "aw shucks, I
> failed, and I don't know why, so I guess I'll try to clean up a little
> for the next guy just in case."
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/compile.py#**newcode1137<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#newcode1137>
> scripts/slave/compile.py:1137: 'source tree')
> *containing* the root? This makes it sound like if I have
> /tmp/mycheckout/src, I'd want to pass --src-dir=/tmp/mycheckout. The
> code below makes it clear that what is really expected is
> --src-dir=/tmp/mycheckout/src. Please clarify the help string.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/compile.py#**newcode1140<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#newcode1140>
> scripts/slave/compile.py:1140: 'output folder')
> Same here.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipe_util.py<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util.py>
> File scripts/slave/recipe_util.py (right):
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipe_util.py#**newcode70<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util.py#newcode70>
> scripts/slave/recipe_util.py:**70: def mock_paths(mock_path_exists=**
> None):
> Thanks for adding this functionality. I find mock_path_exists to be a
> confusing name, though. It's a list of paths, so it should be plural,
> for one thing. Maybe "paths_to_mock"? Not a big deal but I had to read
> all the way through the code to figure out what it was doing because the
> variable name didn't give me a good hint.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipe_util.py#**newcode387<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util.py#newcode387>
> scripts/slave/recipe_util.py:**387: step_name = lambda n: '[spec: %s]
> gclient %s' % (spec_name, n)
> I'd rather have the spec_name emitted as a @@@STEP_TEXT@@@ (so it shows
> up like the revision annotations on normal gclient update steps) than
> directly included in the step name. If that's too hard, though, feel
> free to leave as-is.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py>
> File scripts/slave/recipes/android_**webview_aosp.py (right):
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode23<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode23>
> scripts/slave/recipes/android_**webview_aosp.py:23: '.repo', 'repo',
> 'repo')
> android-src/.repo/repo/repo? man, our paths are ridiculous sometimes.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode89<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode89>
> scripts/slave/recipes/android_**webview_aosp.py:89: steps.step('use repo
> from Android source tree', [
> This step name doesn't match your others. Maybe 'copy repo from
> Android'.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode119<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode119>
> scripts/slave/recipes/android_**webview_aosp.py:119: repo_sync_step = [
> repo re-sync followed immediately by repo sync is a little confusing.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode139<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode139>
> scripts/slave/recipes/android_**webview_aosp.py:139: steps.step('remove
> potentially stale chromium_org',
> This step only exists of chromium_org does, so doesn't need 'potentially
> stale' in the title? Sorry for the nits on step names, I'm just too
> aware of the limitations of the buildbot waterfall display and these
> long names will end up wrapped, ugly, and unfortunate :(
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode144<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode144>
> scripts/slave/recipes/android_**webview_aosp.py:144: steps.step('symlink
> chromium source into android tree',
> Same here.
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode161<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode161>
> scripts/slave/recipes/android_**webview_aosp.py:161: steps.step('clobber',
> ['rm', '-rf', slave_android_out_path]),
> What's the difference between this clobber and the clobber contained in
> compile.py?
>
> https://codereview.chromium.**org/14602020/diff/49001/**
>
scripts/slave/recipes/android_**webview_aosp.py#newcode164<https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/android_webview_aosp.py#newcode164>
> scripts/slave/recipes/android_**webview_aosp.py:164:
> compile_compiler_option = []
> ..._options
>
>
https://codereview.chromium.**org/14602020/<https://codereview.chromium.org/1...
>

[mkosiba (inactive), 2013-05-23 13:13:38]

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py
File scripts/slave/compile.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:730: clobber()
On 2013/05/22 17:21:23, Aaron Gable wrote:
> This is a little weird. We clobber if 'make' failed and we didn't clobber
> previously? While that makes some sense, it seems like it should be
accompanied
> by retry logic. This is basically saying "aw shucks, I failed, and I don't
know
> why, so I guess I'll try to clean up a little for the next guy just in case."

uh.. yeath.. I just copied this over from main_make but I agree that it's a bit
weird.
For android we'll always be running a clobber build, so it'll probably be
clearer to hardcode it to always happen right before running make.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:1137: 'source tree')
On 2013/05/22 17:21:23, Aaron Gable wrote:
> *containing* the root? This makes it sound like if I have /tmp/mycheckout/src,
> I'd want to pass --src-dir=/tmp/mycheckout. The code below makes it clear that
> what is really expected is --src-dir=/tmp/mycheckout/src. Please clarify the
> help string.

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:1140: 'output folder')
On 2013/05/22 17:21:23, Aaron Gable wrote:
> Same here.

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/compile.py#...
scripts/slave/compile.py:1140: 'output folder')
On 2013/05/22 17:21:23, Aaron Gable wrote:
> Same here.

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:70: def mock_paths(mock_path_exists=None):
On 2013/05/22 17:21:23, Aaron Gable wrote:
> Thanks for adding this functionality. I find mock_path_exists to be a
confusing
> name, though. It's a list of paths, so it should be plural, for one thing.
Maybe
> "paths_to_mock"? Not a big deal but I had to read all the way through the code
> to figure out what it was doing because the variable name didn't give me a
good
> hint.

I struggled quite a bit with a good name for this one. Thanks for the
suggestion!

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:387: step_name = lambda n: '[spec: %s] gclient %s'
% (spec_name, n)
On 2013/05/22 17:21:23, Aaron Gable wrote:
> I'd rather have the spec_name emitted as a @@@STEP_TEXT@@@ (so it shows up
like
> the revision annotations on normal gclient update steps) than directly
included
> in the step name. If that's too hard, though, feel free to leave as-is.

The reason for this is not to differentiate in the output but to work around the
fact that you can't have two steps with the same name in the steps_history list
(which is a dict whose keys are step names).

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
File scripts/slave/recipes/android_webview_aosp.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:23: '.repo', 'repo', 'repo')
On 2013/05/22 17:21:23, Aaron Gable wrote:
> android-src/.repo/repo/repo? man, our paths are ridiculous sometimes.

agreed :)

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:89: steps.step('use repo from
Android source tree', [
On 2013/05/22 17:21:23, Aaron Gable wrote:
> This step name doesn't match your others. Maybe 'copy repo from Android'.

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:119: repo_sync_step = [
On 2013/05/22 17:21:23, Aaron Gable wrote:
> repo re-sync followed immediately by repo sync is a little confusing.

good spot! I actually reshuffled the steps so that I could remove the 'sync -l'
but then forgot to take it out. Duh!

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:139: steps.step('remove
potentially stale chromium_org',
On 2013/05/22 17:21:23, Aaron Gable wrote:
> This step only exists of chromium_org does, so doesn't need 'potentially
stale'
> in the title? Sorry for the nits on step names, I'm just too aware of the
> limitations of the buildbot waterfall display and these long names will end up
> wrapped, ugly, and unfortunate :(

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:144: steps.step('symlink chromium
source into android tree',
On 2013/05/22 17:21:23, Aaron Gable wrote:
> Same here.

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:161: steps.step('clobber', ['rm',
'-rf', slave_android_out_path]),
On 2013/05/22 17:21:23, Aaron Gable wrote:
> What's the difference between this clobber and the clobber contained in
> compile.py?

Done.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipes/and...
scripts/slave/recipes/android_webview_aosp.py:164: compile_compiler_option = []
On 2013/05/22 17:21:23, Aaron Gable wrote:
> ..._options

Done.

[agable, 2013-05-23 17:20:12]

LGTM.

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14602020/diff/49001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:387: step_name = lambda n: '[spec: %s] gclient %s'
% (spec_name, n)
On 2013/05/23 13:13:38, mkosiba wrote:
> On 2013/05/22 17:21:23, Aaron Gable wrote:
> > I'd rather have the spec_name emitted as a @@@STEP_TEXT@@@ (so it shows up
> like
> > the revision annotations on normal gclient update steps) than directly
> included
> > in the step name. If that's too hard, though, feel free to leave as-is.
> 
> The reason for this is not to differentiate in the output but to work around
the
> fact that you can't have two steps with the same name in the steps_history
list
> (which is a dict whose keys are step names).

D'oh. SGTM, then.

[commit-bot: I haz the power, 2013-05-24 12:42:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/14602020/65001

[commit-bot: I haz the power, 2013-05-24 12:43:56]

Change committed as 202070