Robustify and speed up cros_mark_all_as_stable.

Robustify and speed up cros_mark_all_as_stable.

What's new?
 - cros_mark_all_as_stable is now in Python and has unit tests.
 - We now detect and report coding errors that can cause incorrect behavior.
   E.g., if 9999 ebuild or stable ebuild can't be found, we report a hard
   failure so that developers will know about the problem.
 - We now check that git hashes we report actually match up with the
   right repository.
 - Unified diff of changes are now printed to stdout, so that developers
   can see a list of the changes and debug any problems.
 - cros_mark_all_as_stable now takes 2.5 seconds to run.

BUG=chromium-os:7795
TEST=Manually examined diff output from run of cros_mark_all_as_stable 
     on full repository.

Change-Id: I762597c9b94e5f8e8171b83c966ad54e21a65c1b

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

[sosa, 2010-10-16 00:08:29]

Mostly nits to start off with.  Sorry I haven't gotten to it completely yet. 
Was trying to help with redness

http://codereview.chromium.org/3798003/diff/1/2
File cros_mark_all_as_stable (right):

http://codereview.chromium.org/3798003/diff/1/2#newcode1
cros_mark_all_as_stable:1: #!/bin/bash
Why not just remove this altogether and modify cbuildbot?

http://codereview.chromium.org/3798003/diff/1/3
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3798003/diff/1/3#newcode76
cros_mark_as_stable.py:76: """Pick the newest ebuild from a set of ebuilds."""
Returns*

http://codereview.chromium.org/3798003/diff/1/3#newcode80
cros_mark_as_stable.py:80: if vercmp(winner_ver, ebuild.ver) < 0:
Use winner.ver

http://codereview.chromium.org/3798003/diff/1/3#newcode84
cros_mark_as_stable.py:84: 
Two lines

http://codereview.chromium.org/3798003/diff/1/3#newcode114
cros_mark_as_stable.py:114: if len(model_ebuilds) > 1:
model?

http://codereview.chromium.org/3798003/diff/1/3#newcode135
cros_mark_as_stable.py:135: def _BuildEBuildDictionary(overlays, all,
package_list, commit_id_list):
Docstring

http://codereview.chromium.org/3798003/diff/1/3#newcode188
cros_mark_as_stable.py:188: _SimpleRunCommand('git clean -f')
Please don't add this.  PFQ maybe communicate with itself through unversioned
files.

http://codereview.chromium.org/3798003/diff/1/3#newcode304
cros_mark_as_stable.py:304: self.ver = '%s-%s' % (ver_no_rev, rev)
spell out ver

http://codereview.chromium.org/3798003/diff/1/3#newcode320
cros_mark_as_stable.py:320: 
Would much rather have this be part of an Init function rather than check for if
not self.commit_id

http://codereview.chromium.org/3798003/diff/1/3#newcode324
cros_mark_as_stable.py:324: cmd = ('CROS_WORKON_LOCALNAME="%s"
CROS_WORKON_PROJECT="%s" '
Add comment as it took me a while to understand: # Gets CROS_WORKON environment
variables from unstable ebuild.

http://codereview.chromium.org/3798003/diff/1/3#newcode328
cros_mark_as_stable.py:328: % (self.pkgname, self.pkgname, master_ebuild))
percent should line up with '

http://codereview.chromium.org/3798003/diff/1/3#newcode328
cros_mark_as_stable.py:328: % (self.pkgname, self.pkgname, master_ebuild))
master seems too strong ... unstable?

http://codereview.chromium.org/3798003/diff/1/3#newcode343
cros_mark_as_stable.py:343: srcdir = os.path.join(srcroot,
'third_party/kernel/files')
Constant out

http://codereview.chromium.org/3798003/diff/1/3#newcode354
cros_mark_as_stable.py:354: if project != actual_project and project !=
"chromeos-kernel":
use '' not "" also consider using project not in [actual_project,
'chromeos-kernel']

http://codereview.chromium.org/3798003/diff/1/3#newcode359
cros_mark_as_stable.py:359: output = _SimpleRunCommand('cd %s && git rev-parse
HEAD' % srcdir)
Rather than use cd %s consider using git --git-dir=os.path.join(srcdir, '.git')

http://codereview.chromium.org/3798003/diff/1/3#newcode442
cros_mark_as_stable.py:442: old_ebuild_path = self._ebuild.ebuild_path
Add comment to what this is checking for.

http://codereview.chromium.org/3798003/diff/1/3#newcode444
cros_mark_as_stable.py:444: if 0 == os.system(diff_cmd):
Don't use os.system.

[David James, 2010-10-20 23:15:33]

PTAL

http://codereview.chromium.org/3798003/diff/1/2
File cros_mark_all_as_stable (right):

http://codereview.chromium.org/3798003/diff/1/2#newcode1
cros_mark_all_as_stable:1: #!/bin/bash
On 2010/10/16 00:08:29, sosa wrote:
> Why not just remove this altogether and modify cbuildbot?

Done.

http://codereview.chromium.org/3798003/diff/1/3
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3798003/diff/1/3#newcode76
cros_mark_as_stable.py:76: """Pick the newest ebuild from a set of ebuilds."""
On 2010/10/16 00:08:29, sosa wrote:
> Returns*

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode80
cros_mark_as_stable.py:80: if vercmp(winner_ver, ebuild.ver) < 0:
On 2010/10/16 00:08:29, sosa wrote:
> Use winner.ver

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode84
cros_mark_as_stable.py:84: 
On 2010/10/16 00:08:29, sosa wrote:
> Two lines

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode114
cros_mark_as_stable.py:114: if len(model_ebuilds) > 1:
On 2010/10/16 00:08:29, sosa wrote:
> model?

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode135
cros_mark_as_stable.py:135: def _BuildEBuildDictionary(overlays, all,
package_list, commit_id_list):
On 2010/10/16 00:08:29, sosa wrote:
> Docstring

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode188
cros_mark_as_stable.py:188: _SimpleRunCommand('git clean -f')
On 2010/10/16 00:08:29, sosa wrote:
> Please don't add this.  PFQ maybe communicate with itself through unversioned
> files.

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode304
cros_mark_as_stable.py:304: self.ver = '%s-%s' % (ver_no_rev, rev)
On 2010/10/16 00:08:29, sosa wrote:
> spell out ver

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode320
cros_mark_as_stable.py:320: 
On 2010/10/16 00:08:29, sosa wrote:
> Would much rather have this be part of an Init function rather than check for
if
> not self.commit_id
Got rid of the self.commit_id and just made it a function.

http://codereview.chromium.org/3798003/diff/1/3#newcode324
cros_mark_as_stable.py:324: cmd = ('CROS_WORKON_LOCALNAME="%s"
CROS_WORKON_PROJECT="%s" '
On 2010/10/16 00:08:29, sosa wrote:
> Add comment as it took me a while to understand: # Gets CROS_WORKON
environment
> variables from unstable ebuild.

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode328
cros_mark_as_stable.py:328: % (self.pkgname, self.pkgname, master_ebuild))
On 2010/10/16 00:08:29, sosa wrote:
> percent should line up with '

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode328
cros_mark_as_stable.py:328: % (self.pkgname, self.pkgname, master_ebuild))
On 2010/10/16 00:08:29, sosa wrote:
> master seems too strong ... unstable?

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode354
cros_mark_as_stable.py:354: if project != actual_project and project !=
"chromeos-kernel":
On 2010/10/16 00:08:29, sosa wrote:
> use '' not "" also consider using project not in [actual_project,
> 'chromeos-kernel']

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode359
cros_mark_as_stable.py:359: output = _SimpleRunCommand('cd %s && git rev-parse
HEAD' % srcdir)
On 2010/10/16 00:08:29, sosa wrote:
> Rather than use cd %s consider using git --git-dir=os.path.join(srcdir,
'.git')

Doesn't work. srcdir might point at a subdirectory so %s/.git doesn't exist.

http://codereview.chromium.org/3798003/diff/1/3#newcode442
cros_mark_as_stable.py:442: old_ebuild_path = self._ebuild.ebuild_path
On 2010/10/16 00:08:29, sosa wrote:
> Add comment to what this is checking for.

Done.

http://codereview.chromium.org/3798003/diff/1/3#newcode444
cros_mark_as_stable.py:444: if 0 == os.system(diff_cmd):
On 2010/10/16 00:08:29, sosa wrote:
> Don't use os.system.

Done.

[sosa, 2010-10-21 00:06:15]

http://codereview.chromium.org/3798003/diff/11001/12001
File bin/cbuildbot.py (left):

http://codereview.chromium.org/3798003/diff/11001/12001#oldcode199
bin/cbuildbot.py:199: '--commit_ids="%s"' % commit_str,
Why did you take this out?

http://codereview.chromium.org/3798003/diff/11001/12004
File cros_mark_as_stable.py (left):

http://codereview.chromium.org/3798003/diff/11001/12004#oldcode355
cros_mark_as_stable.py:355: if gflags.FLAGS.commit_ids:
I think you may have misterpreted an earlier comment I made ... what I meant to
say was GetCommitID should just return ebuild.commit_id ... this way in testing
... it doesn't recall all the sh code to find the commit id ... ideally
everytime i call GetCommitID it just returns it to me.  I suggested writing an
Init method to put this code in and just set ebuild.commit_id (this is pretty
optional though)

However, I didn't mean to say ... take out old commit id code.  That should
still be there.

http://codereview.chromium.org/3798003/diff/11001/12004
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3798003/diff/11001/12004#newcode17
cros_mark_as_stable.py:17: from portage.versions import catpkgsplit, pkgsplit,
vercmp
you don't use catpkgsplit

http://codereview.chromium.org/3798003/diff/11001/12004#newcode76
cros_mark_as_stable.py:76: """Pick the newest EBuild from a list of EBuild
objects.
Just say Returns the newest EBuild ... and save some ws.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode91
cros_mark_as_stable.py:91: root: Directory containing the ebuilds.
Why do you send these separately?

http://codereview.chromium.org/3798003/diff/11001/12004#newcode94
cros_mark_as_stable.py:94: 
No need for extra line

http://codereview.chromium.org/3798003/diff/11001/12004#newcode140
cros_mark_as_stable.py:140: inside said directories.
Only indent two spaces on continued lines in docstrings

http://codereview.chromium.org/3798003/diff/11001/12004#newcode146
cros_mark_as_stable.py:146: 
Line not needed

http://codereview.chromium.org/3798003/diff/11001/12004#newcode148
cros_mark_as_stable.py:148: for root, dirs, files in os.walk(overlay):
Instead of using root can you have a var that is more descriptive ... I had to
look at os.walk to figure out what you meant by root in _FindStableEBuilds.  I
suggest something like package_dir

http://codereview.chromium.org/3798003/diff/11001/12004#newcode159
cros_mark_as_stable.py:159: overlays[overlay].extend(ebuilds)
I'd rather you return an ebuild rather than an array.  I see you have special
logic for xf86 ... can you just handle that sep and keep track of your current
stable_ebuild for the package?

http://codereview.chromium.org/3798003/diff/11001/12004#newcode438
cros_mark_as_stable.py:438: diff_cmd = ['diff', '-Bu', old_ebuild_path,
new_ebuild_path]
This would indicate an error somewhere else.  I'd much rather you always rev
(consider adding a change to eclass logic where we would want to rebuild/rev but
you wouldn't see anything necessarily in the ebuild).

http://codereview.chromium.org/3798003/diff/11001/12004#newcode510
cros_mark_as_stable.py:510: if worker.RevEBuild(commit_id):
Remove if

[David James, 2010-10-21 07:06:47]

PTAL

http://codereview.chromium.org/3798003/diff/11001/12001
File bin/cbuildbot.py (left):

http://codereview.chromium.org/3798003/diff/11001/12001#oldcode199
bin/cbuildbot.py:199: '--commit_ids="%s"' % commit_str,
On 2010/10/21 00:06:16, sosa wrote:
> Why did you take this out?

See the changes to the other file. I don't think we need the commit_id feature,
because we can just ensure that the buildbot syncs to the right revision in the
first place. Which is a good idea anyway.

http://codereview.chromium.org/3798003/diff/11001/12004
File cros_mark_as_stable.py (left):

http://codereview.chromium.org/3798003/diff/11001/12004#oldcode355
cros_mark_as_stable.py:355: if gflags.FLAGS.commit_ids:
On 2010/10/21 00:06:16, sosa wrote:
> I think you may have misterpreted an earlier comment I made ... what I meant
to
> say was GetCommitID should just return ebuild.commit_id ... this way in
testing
> ... it doesn't recall all the sh code to find the commit id ... ideally
> everytime i call GetCommitID it just returns it to me.  I suggested writing an
> Init method to put this code in and just set ebuild.commit_id (this is pretty
> optional though)
> 
> However, I didn't mean to say ... take out old commit id code.  That should
> still be there.

Now that we're grabbing the commit ids from the checkout, there shouldn't be any
need to check them in. The buildbot should just sync to the revision it wants us
to pin. Don't you think?

http://codereview.chromium.org/3798003/diff/11001/12004
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3798003/diff/11001/12004#newcode17
cros_mark_as_stable.py:17: from portage.versions import catpkgsplit, pkgsplit,
vercmp
On 2010/10/21 00:06:16, sosa wrote:
> you don't use catpkgsplit

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode76
cros_mark_as_stable.py:76: """Pick the newest EBuild from a list of EBuild
objects.
On 2010/10/21 00:06:16, sosa wrote:
> Just say Returns the newest EBuild ... and save some ws.

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode91
cros_mark_as_stable.py:91: root: Directory containing the ebuilds.
On 2010/10/21 00:06:16, sosa wrote:
> Why do you send these separately?

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode94
cros_mark_as_stable.py:94: 
On 2010/10/21 00:06:16, sosa wrote:
> No need for extra line

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode140
cros_mark_as_stable.py:140: inside said directories.
On 2010/10/21 00:06:16, sosa wrote:
> Only indent two spaces on continued lines in docstrings

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode146
cros_mark_as_stable.py:146: 
On 2010/10/21 00:06:16, sosa wrote:
> Line not needed

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode148
cros_mark_as_stable.py:148: for root, dirs, files in os.walk(overlay):
On 2010/10/21 00:06:16, sosa wrote:
> Instead of using root can you have a var that is more descriptive ... I had to
> look at os.walk to figure out what you meant by root in _FindStableEBuilds.  I
> suggest something like package_dir

How about root_dir?

http://codereview.chromium.org/3798003/diff/11001/12004#newcode159
cros_mark_as_stable.py:159: overlays[overlay].extend(ebuilds)
On 2010/10/21 00:06:16, sosa wrote:
> I'd rather you return an ebuild rather than an array.  I see you have special
> logic for xf86 ... can you just handle that sep and keep track of your current
> stable_ebuild for the package?

Done.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode438
cros_mark_as_stable.py:438: diff_cmd = ['diff', '-Bu', old_ebuild_path,
new_ebuild_path]
On 2010/10/21 00:06:16, sosa wrote:
> This would indicate an error somewhere else.  I'd much rather you always rev
> (consider adding a change to eclass logic where we would want to rebuild/rev
but
> you wouldn't see anything necessarily in the ebuild).

We now pass in all ebuilds to this function -- not just ebuilds that are passed
on the command-line. That allows us to detect when the 9999 ebuild has changed
or when the commit hash has changed. This used to happen in a shell script but
is now happening here.

In the old shell script, we did a similar diff, but tried to grep out all the
differences to see if there was any changes. It's easier to just modify the file
and compare the two, I think.

http://codereview.chromium.org/3798003/diff/11001/12004#newcode510
cros_mark_as_stable.py:510: if worker.RevEBuild(commit_id):
On 2010/10/21 00:06:16, sosa wrote:
> Remove if

This is needed. See above.

[sosa, 2010-10-21 17:10:57]

LGTM w/ nits

http://codereview.chromium.org/3798003/diff/11001/12004
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3798003/diff/11001/12004#newcode148
cros_mark_as_stable.py:148: for root, dirs, files in os.walk(overlay):
The issue I have is with root.  When I see root I think root of the src dir or
root of the chroot.  I don't really think of root when I think of dirs that
often don't have subdirs.  I would really prefer not to see root overused here.

On 2010/10/21 07:06:47, David James wrote:
> On 2010/10/21 00:06:16, sosa wrote:
> > Instead of using root can you have a var that is more descriptive ... I had
to
> > look at os.walk to figure out what you meant by root in _FindStableEBuilds. 
I
> > suggest something like package_dir
> 
> How about root_dir?
>

http://codereview.chromium.org/3798003/diff/16001/17004#newcode393
cros_mark_as_stable.py:393: to redirect the new stable ebuild to another file.
Add a Returns here saying when you True / False

http://codereview.chromium.org/3798003/diff/16001/17004#newcode424
cros_mark_as_stable.py:424: # Actually mark this file as stable by removing ~'s.
Can you be more explicit about why this happens in this comment?

[sosa, 2010-10-21 20:14:37]

Hey David, I just realized that --all will fail to push if there are no changes
to push.  Can you add some logic either in Push or Commit that either doesn't
create a stabilizing branch if there are no ebuilds to rev (in the current logic
you could do this by deleting the branch OR change push to check to see if there
are any commits in the branch.