Merge MVP script into cbuildbot.

Merge MVP script into cbuildbot.

This is done by adding two new configuration options:

git_url -- git repository URL for our manifests.
manifest_version -- Per-build manifest to be stored in GIT 

The first defaults to the external git url, and is used for fetching code most of the time. The second, if not None is the git URL for fetching/storing detailed manifest files and the associated build status.

The new file manifest_version.py is a copy (plus edits) of mvp.py from crostools. It's mostly unchanged, except for some minimalization of argument lists and the addition of a 'debug' concept, which means to not push changes to git repositories. This debug concept is controlled by the cbuildbot --debug command line option.

A few new unit tests were added, but not enough over all.

BUG=chromium-os:12467
TEST=

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=f3eac24

[dgarrett, 2011-04-04 23:17:12]

This is NOT a real request for review, but just me showing off how I'm trying to
get MVP working inside of cbuildbot, based on djmm's initial changes.

manifest_version.py in the initial upload is an unmodified copy of mvp.py, and
the second upload shows my intial changes to that file.

This code isn't working yet, but people were interested in seeing it, so sending
out now.

[dgarrett, 2011-04-08 22:27:39]

Okay, really ready to start reviewing.

It was uploaded in stages, the original manifest_versions.py is identical to the
original mvp.py.

[dgarrett, 2011-04-08 23:21:41]

I'm... I missed something. Updating another fix as soon as the test run finishes
(and passes).

[dgarrett, 2011-04-08 23:41:33]

PTAL, I fixed the --dry_run usage that was hard coded, and instead tied it to
cbuildbot --debut.

[sosa, 2011-04-11 19:03:54]

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py
File buildbot/cbuildbot.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:94: stages.ManifestVersionedSyncCompletionStage(bot_id,
indentation is wrong here

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:100: # Send failure to master.
Maybe the two of these should be combined into a ReportFailureStage?

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:190: help='Run the buildbot on internal manifest')
Change help to indicate an override of config.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:121: def ManifestCheckout(buildroot,
tracking_branch, nextversion,
next_version*

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.py
File buildbot/cbuildbot_config.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.p...
buildbot/cbuildbot_config.py:50: External: http://git.chromium.org/git/manifest
indentation?

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.p...
buildbot/cbuildbot_config.py:53: manifest_version -- Per-build manifest to be
stored in GIT repo at specified
Maybe re-word as:  URL to GIT repo to store per-build manifest.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.p...
buildbot/cbuildbot_config.py:54: URL. Usually None or
indentation?

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:14: import chromite.buildbot.manifest_version as
manifest_version
alphabetical order is incorrect here.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages_u...
File buildbot/cbuildbot_stages_unittest.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages_u...
buildbot/cbuildbot_stages_unittest.py:22: import
chromite.buildbot.manifest_version as manifest_version
alpha

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages_u...
buildbot/cbuildbot_stages_unittest.py:251: """Tests basic
ManifestVersionedSyncStage"""
The following three docstrings are copies.  Can you change to reflect each test
:)

http://codereview.chromium.org/6691047/diff/11001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6691047/diff/11001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:305: """
One line short docstring on first line i.e. Returns value from a file/key pair

http://codereview.chromium.org/6691047/diff/11001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:310: if os.path.isfile(filename):
filename can be an obj or file?  I'd think it be cleaner if it was one or the
other

http://codereview.chromium.org/6691047/diff/11001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:318: break
extra line after

http://codereview.chromium.org/6691047/diff/11001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:319: return pat.strip()
File remains opened :(

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version.py
File buildbot/manifest_version.py (right):

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version.p...
buildbot/manifest_version.py:7: A script to generate and store the manifests for
the canary builders to use.
Change docstring to reflect change to library

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version.p...
buildbot/manifest_version.py:451: 
two lines

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version.p...
buildbot/manifest_version.py:671: 
two lines

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version.p...
buildbot/manifest_version.py:753: 
two lines

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version_u...
File buildbot/manifest_version_unittest.py (right):

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version_u...
buildbot/manifest_version_unittest.py:37: #$TOOLS/mvp.py --build-name=$BOARD
--status=pass \
Remove comments lines here.

http://codereview.chromium.org/6691047/diff/14001/buildbot/manifest_version_u...
buildbot/manifest_version_unittest.py:71: #$TOOLS/mvp.py --build-name=$BOARD
--status=pass \
Ditto

[dgarrett, 2011-04-11 20:53:13]

Thanks for the review!

Here are the fixes.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py
File buildbot/cbuildbot.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:94: stages.ManifestVersionedSyncCompletionStage(bot_id,
On 2011/04/11 19:03:54, sosa wrote:
> indentation is wrong here

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:100: # Send failure to master.
On 2011/04/11 19:03:54, sosa wrote:
> Maybe the two of these should be combined into a ReportFailureStage?

If that could be done for both success and failure, I'd agree. But since the
successes depend on different conditions, I disagree.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:190: help='Run the buildbot on internal manifest')
On 2011/04/11 19:03:54, sosa wrote:
> Change help to indicate an override of config.

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:121: def ManifestCheckout(buildroot,
tracking_branch, nextversion,
On 2011/04/11 19:03:54, sosa wrote:
> next_version*

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.py
File buildbot/cbuildbot_config.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.p...
buildbot/cbuildbot_config.py:50: External: http://git.chromium.org/git/manifest
On 2011/04/11 19:03:54, sosa wrote:
> indentation?

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.p...
buildbot/cbuildbot_config.py:53: manifest_version -- Per-build manifest to be
stored in GIT repo at specified
On 2011/04/11 19:03:54, sosa wrote:
> Maybe re-word as:  URL to GIT repo to store per-build manifest.

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_config.p...
buildbot/cbuildbot_config.py:54: URL. Usually None or
On 2011/04/11 19:03:54, sosa wrote:
> indentation?

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:14: import chromite.buildbot.manifest_version as
manifest_version
On 2011/04/11 19:03:54, sosa wrote:
> alphabetical order is incorrect here.

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages_u...
File buildbot/cbuildbot_stages_unittest.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages_u...
buildbot/cbuildbot_stages_unittest.py:22: import
chromite.buildbot.manifest_version as manifest_version
On 2011/04/11 19:03:54, sosa wrote:
> alpha

Done.

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot_stages_u...
buildbot/cbuildbot_stages_unittest.py:251: """Tests basic
ManifestVersionedSyncStage"""
On 2011/04/11 19:03:54, sosa wrote:
> The following three docstrings are copies.  Can you change to reflect each
test
> :)

Done.

http://codereview.chromium.org/6691047/diff/14001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6691047/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:304: def GetValueFromFile(filename, varname):
This method was left over from DJMM's changes, but it no longer used, so I just
pulled it.

[sosa, 2011-04-11 23:34:01]

Raja can you comment on the patches to mvp?

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py
File buildbot/cbuildbot.py (right):

http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
buildbot/cbuildbot.py:100: # Send failure to master.
I think maybe you should just make it into one stage by copying this code as is
and putting it in there ... yes conceptually not very different than what you
have, but what will get printed by the stage summary may be cleaner i.e.
ReportFailureStage is clear what the stage is and that if it has a bug is
clearly a problem with the buildbot rather than a work-on check-in

On 2011/04/11 20:53:13, dgarrett wrote:
> On 2011/04/11 19:03:54, sosa wrote:
> > Maybe the two of these should be combined into a ReportFailureStage?
> 
> If that could be done for both success and failure, I'd agree. But since the
> successes depend on different conditions, I disagree.

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:142: #print 'MANIFEST:' + manifest;
commented out?

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:299:
version_file='src/third_party/chromiumos-overlay/chromeos/config'
os.path.join

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:302: incr_type= increment,
no space

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:306: print "AUTOREV: Nothing to build!"
'' not ""

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:332: if not
ManifestVersionedSyncStage.build_version:
When would we get here?  SHouldn't this be an asert check?

[Raja Aluri, 2011-04-11 23:36:29]

Looking...
Raja

On Mon, Apr 11, 2011 at 4:34 PM, <sosa@chromium.org> wrote:

> Raja can you comment on the patches to mvp?
>
>
>
> http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py
> File buildbot/cbuildbot.py (right):
>
>
>
http://codereview.chromium.org/6691047/diff/11001/buildbot/cbuildbot.py#newco...
> buildbot/cbuildbot.py:100: # Send failure to master.
> I think maybe you should just make it into one stage by copying this
> code as is and putting it in there ... yes conceptually not very
> different than what you have, but what will get printed by the stage
> summary may be cleaner i.e. ReportFailureStage is clear what the stage
> is and that if it has a bug is clearly a problem with the buildbot
> rather than a work-on check-in
>
>
> On 2011/04/11 20:53:13, dgarrett wrote:
>
>> On 2011/04/11 19:03:54, sosa wrote:
>> > Maybe the two of these should be combined into a ReportFailureStage?
>>
>
>  If that could be done for both success and failure, I'd agree. But
>>
> since the
>
>> successes depend on different conditions, I disagree.
>>
>
>
>
http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_commands.py
>
> File buildbot/cbuildbot_commands.py (right):
>
>
>
http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_commands...
> buildbot/cbuildbot_commands.py:142: #print 'MANIFEST:' + manifest;
> commented out?
>
>
> http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.py
>
> File buildbot/cbuildbot_stages.py (right):
>
>
>
http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
> buildbot/cbuildbot_stages.py:299:
> version_file='src/third_party/chromiumos-overlay/chromeos/config'
> os.path.join
>
>
>
http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
> buildbot/cbuildbot_stages.py:302: incr_type= increment,
> no space
>
>
>
http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
> buildbot/cbuildbot_stages.py:306: print "AUTOREV: Nothing to build!"
> '' not ""
>
>
>
http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
> buildbot/cbuildbot_stages.py:332: if not
> ManifestVersionedSyncStage.build_version:
> When would we get here?  SHouldn't this be an asert check?
>
>
> http://codereview.chromium.org/6691047/
>

[Raja Aluri, 2011-04-12 23:16:43]

1) If we could some how set a class level variable for dry_run, that would be
nice, instead of passing the args along all over the place. But I will leave
this to you
2) Can you please somehow incorporate the timer into the cros_lib.runCommand, if
possible.
3) board name is not same as the build_name. I think it becomes an issue for us,
when we need to do multiple variants of release builds for the same boards.
(like factory, release, debug etc.,)

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.py
File buildbot/manifest_version.py (right):

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:13: import optparse
unused import

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:17: import sys
unused import

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:61: git_server: gitserver URI information
docstrings don't match the argument changes anymore

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:411: git_server: git server URL
docstrings out of date

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:422: message: Commit message
docstrings out of date

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:484: cros_source: GitMirror Object
docstrings out of date

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:575: 
cros_source, repo_url, source_dir, branch,

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:594: manifest2: Second manifest file to compare
docstrings out of date

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:674: board, incr_type, latest=False,
board and build_name are not necessarily equivalent.
We can have two different builds for the same board. Eg., release debug etc.,
It was a thought out choice to use the build_name

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:679: tmp_dir: Temporary working directory, best
shared with GenerateWorkload
best shared with GenerateWorkload
doesn't convey much to me. Do you want to elaborate or drop that part of the
sentence

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:781: CloneGitRepo(ver_manifests, git_server,
working_dir)
Function mismatch

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:789: board,
argument formatting is not compatible with the rest of the file

[dgarrett, 2011-04-12 23:21:56]

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:142: #print 'MANIFEST:' + manifest;
On 2011/04/11 23:34:01, sosa wrote:
> commented out?

Removed.

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:299:
version_file='src/third_party/chromiumos-overlay/chromeos/config'
On 2011/04/11 23:34:01, sosa wrote:
> os.path.join

Done.

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:302: incr_type= increment,
On 2011/04/11 23:34:01, sosa wrote:
> no space

Done.

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:306: print "AUTOREV: Nothing to build!"
On 2011/04/11 23:34:01, sosa wrote:
> '' not ""

Done.

http://codereview.chromium.org/6691047/diff/15005/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:332: if not
ManifestVersionedSyncStage.build_version:
On 2011/04/11 23:34:01, sosa wrote:
> When would we get here?  SHouldn't this be an asert check?

This stage will be reached, no matter where we errored out earlier. If
ManifestVersionedSyncStage hit and error (that would have been reported
normally), or even if something broken before then, this tage will still run.

I didn't want a bogus error here masking the real error that happened earlier.

I enhanced the comment.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.py
File buildbot/manifest_version.py (right):

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:572:
PushChanges(os.path.dirname(version_info.version_file), message,
This line originally passed use_repo=True to PushChanges. Is this correct? How
would it ever be right?

[Raja Aluri, 2011-04-12 23:26:04]

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.py
File buildbot/manifest_version.py (right):

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:572:
PushChanges(os.path.dirname(version_info.version_file), message,
On 2011/04/12 23:21:56, dgarrett wrote:
> This line originally passed use_repo=True to PushChanges. Is this correct? How
> would it ever be right?
That's correct though.IT should use repo. This file is in the chromeos source
tree. not in manfest-versions.git

[dgarrett, 2011-04-12 23:39:27]

Raja, I'm finding a call path that seems incorrect to me, but which I don't
think I modified.

GenerateWorkload calls:
  build_specs.ExistsBuildSpec(current_build)

  If that returns true, then GenerateNewBuildSpec will call PushChanges with
use_repo=True.

This is happening for me during local test runs, which causes my repo errors.

[dgarrett, 2011-04-13 01:11:19]

Found it. I was passing dry_run along as a positional argument and it was ending
up in use_repo.

[dgarrett, 2011-04-13 17:55:18]

Okay, I think it's ready to go in now, if everyone agrees.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.py
File buildbot/manifest_version.py (right):

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:13: import optparse
On 2011/04/12 23:16:43, Raja Aluri wrote:
> unused import

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:17: import sys
On 2011/04/12 23:16:43, Raja Aluri wrote:
> unused import

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:61: git_server: gitserver URI information
On 2011/04/12 23:16:43, Raja Aluri wrote:
> docstrings don't match the argument changes anymore

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:411: git_server: git server URL
On 2011/04/12 23:16:43, Raja Aluri wrote:
> docstrings out of date

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:422: message: Commit message
On 2011/04/12 23:16:43, Raja Aluri wrote:
> docstrings out of date

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:484: cros_source: GitMirror Object
On 2011/04/12 23:16:43, Raja Aluri wrote:
> docstrings out of date

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:575: 
On 2011/04/12 23:16:43, Raja Aluri wrote:
> cros_source, repo_url, source_dir, branch,

You were right, but the correct answer is that cros_source has the url already
attached, so I removed the argument repo_url.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:594: manifest2: Second manifest file to compare
On 2011/04/12 23:16:43, Raja Aluri wrote:
> docstrings out of date

This one's still current.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:679: tmp_dir: Temporary working directory, best
shared with GenerateWorkload
On 2011/04/12 23:16:43, Raja Aluri wrote:
> best shared with GenerateWorkload
> doesn't convey much to me. Do you want to elaborate or drop that part of the
> sentence

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:679: tmp_dir: Temporary working directory, best
shared with GenerateWorkload
On 2011/04/12 23:16:43, Raja Aluri wrote:
> best shared with GenerateWorkload
> doesn't convey much to me. Do you want to elaborate or drop that part of the
> sentence

Done.

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:781: CloneGitRepo(ver_manifests, git_server,
working_dir)
On 2011/04/12 23:16:43, Raja Aluri wrote:
> Function mismatch

Good catch!

http://codereview.chromium.org/6691047/diff/13004/buildbot/manifest_version.p...
buildbot/manifest_version.py:789: board,
On 2011/04/12 23:16:43, Raja Aluri wrote:
> argument formatting is not compatible with the rest of the file

Done.

[sosa, 2011-04-13 18:35:10]

LGTM with a cpl nits

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:290: if not branch[1]:
if this is an array of length 1 won't this throw an exception?  Maybe if
len[branch[1]] == 1

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:292: increment = 'branch' # TODO Is this right?
Remove TODO

[Raja Aluri, 2011-04-13 18:51:33]

LGTM

[dgarrett, 2011-04-13 20:57:35]

Okay, this change addresses Chris's concerns about branch name handling. I want
to be clear that I'm not sure what a tracking branch is, or how they are named,
so I'm just hoping this is right.

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:290: if not branch[1]:
On 2011/04/13 18:35:10, sosa wrote:
> if this is an array of length 1 won't this throw an exception?  Maybe if
> len[branch[1]] == 1

For that matter, what is a tracking branch, and how are they formatted? I have
no idea, but I'll follow your suggestion.

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:292: increment = 'branch' # TODO Is this right?
On 2011/04/13 18:35:10, sosa wrote:
> Remove TODO 

Done.

[djmm, 2011-04-14 00:37:49]

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.py
File buildbot/cbuildbot_stages.py (right):

http://codereview.chromium.org/6691047/diff/21002/buildbot/cbuildbot_stages.p...
buildbot/cbuildbot_stages.py:290: if not branch[1]:
On 2011/04/13 20:57:35, dgarrett wrote:
> On 2011/04/13 18:35:10, sosa wrote:
> > if this is an array of length 1 won't this throw an exception?  Maybe if
> > len[branch[1]] == 1

Yes that's probably better.

> 
> For that matter, what is a tracking branch, and how are they formatted? I have
> no idea, but I'll follow your suggestion.

--tracking-branch comes into cbuildbot as something like "cros/0.11.227.B" and
we need to pass just 0.11.227.B to mvp.

[dgarrett, 2011-04-14 00:44:03]

Excellent, thanks!