Update cros_mark_as_stable.py to also update the private overlay

Update cros_mark_as_stable.py to also update the private overlay

BUG=chromium-os:7218
TEST=./cros_mark_all_as_stable --tracking_branch=cros/master

Change-Id: I0f7c4e2d2aa4fc6325721901a710bfc33fa39a91

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

[sosa, 2010-10-08 20:12:27]

Hey David, looking great over all.  The main thing I have to say is that the
main() function is getting too congested so I would really like to see you break
the overlays logic into its own functions if possible.

http://codereview.chromium.org/3516025/diff/2001/3001
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3516025/diff/2001/3001#newcode51
cros_mark_as_stable.py:51: _COMMAND_DICTIONARY = {
Make a reference here for ebuilds

http://codereview.chromium.org/3516025/diff/2001/3001#newcode220
cros_mark_as_stable.py:220: return path.rstrip()
Why not just strip()?

http://codereview.chromium.org/3516025/diff/2001/3001#newcode284
cros_mark_as_stable.py:284: shutil.copyfile(self._ebuild.ebuild_path,
new_ebuild_path)
What's this?  Are you revving non-workon packages here?

http://codereview.chromium.org/3516025/diff/2001/3001#newcode343
cros_mark_as_stable.py:343: overlays = {
Can you add a function that just added the overlays when they exist in your path
(i.e. replace this with

AddOverlayIfExists('%s/overlays' % gflags.FLAGS.srcroot), etc

This will avoid os.path.exists checks later and avoid unnecessary checks against
the overlays in your logic.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode348
cros_mark_as_stable.py:348: for index in range(len(package_list)):
Can you wrap this whole thing into a function (i.e. BuildEBuildDictionary) and
add a unit test.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode354
cros_mark_as_stable.py:354: overlay = os.path.abspath(ebuild.ebuild_path)
Based on the documentation of _FindEbuildPath ... abspath is unnecessary.  If
you still need it for some reason you should add it in that function not here.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode355
cros_mark_as_stable.py:355: while overlay:
Just check if the ebuild path is a substring of something in overlays rather
than going up with dirname's i.e. for overlay in overlays: if
ebuild.startswith(overlay), doblah

http://codereview.chromium.org/3516025/diff/2001/3001#newcode361
cros_mark_as_stable.py:361: Warning('No overlay found for %s' %
ebuild.ebuild_path)
Probably should be a die.  If we can't find that path, something is wrong
otherwise we might mask errors

http://codereview.chromium.org/3516025/diff/2001/3001#newcode390
cros_mark_as_stable.py:390: (package_list[:index], overlay))
This doesn't print out the right package ... use ebuild.package

[David James, 2010-10-11 17:53:44]

http://codereview.chromium.org/3516025/diff/2001/3001
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3516025/diff/2001/3001#newcode51
cros_mark_as_stable.py:51: _COMMAND_DICTIONARY = {
On 2010/10/08 20:12:28, sosa wrote:
> Make a reference here for ebuilds

I don't understand. What would you like me to change here?

http://codereview.chromium.org/3516025/diff/2001/3001#newcode220
cros_mark_as_stable.py:220: return path.rstrip()
On 2010/10/08 20:12:28, sosa wrote:
> Why not just strip()?

We're just stripping off the trailing newline here (on the right side). There
shouldn't be any other whitespace on the line. Since that's all we need I think
rstrip is more suited.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode284
cros_mark_as_stable.py:284: shutil.copyfile(self._ebuild.ebuild_path,
new_ebuild_path)
On 2010/10/08 20:12:28, sosa wrote:
> What's this?  Are you revving non-workon packages here? 

No. This is just error handling for when a user forgot to make a 9999 ebuild. We
actually have a package right now (chromeos-firmware) which has this problem.

I suppose we could Die() with an error here, but if we're going to do that then
we should fix the existing ebuilds first.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode343
cros_mark_as_stable.py:343: overlays = {
On 2010/10/08 20:12:28, sosa wrote:
> Can you add a function that just added the overlays when they exist in your
path
> (i.e. replace this with
> 
> AddOverlayIfExists('%s/overlays' % gflags.FLAGS.srcroot), etc
> 
> This will avoid os.path.exists checks later and avoid unnecessary checks
against
> the overlays in your logic.

I looked into this. I think what we really want to do is call portageq-%s envvar
PORTDIR_OVERLAY and grab the list of overlays from portage. 

The flip side of doing this though is that it means we need to make sure the
board we got is correct. Looking at cbuildbot.py, it looks like the board is not
passed in to cros_mark_as_stable.py when it calls clean, so messing around with
board overlays would be a bad idea until we get that working properly. I think
we should address this in a separate change as this change is already big
enough.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode348
cros_mark_as_stable.py:348: for index in range(len(package_list)):
On 2010/10/08 20:12:28, sosa wrote:
> Can you wrap this whole thing into a function (i.e. BuildEBuildDictionary) and
> add a unit test.

Done.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode354
cros_mark_as_stable.py:354: overlay = os.path.abspath(ebuild.ebuild_path)
On 2010/10/08 20:12:28, sosa wrote:
> Based on the documentation of _FindEbuildPath ... abspath is unnecessary.  If
> you still need it for some reason you should add it in that function not here.

Done.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode355
cros_mark_as_stable.py:355: while overlay:
On 2010/10/08 20:12:28, sosa wrote:
> Just check if the ebuild path is a substring of something in overlays rather
> than going up with dirname's i.e. for overlay in overlays: if
> ebuild.startswith(overlay), doblah  

Done.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode361
cros_mark_as_stable.py:361: Warning('No overlay found for %s' %
ebuild.ebuild_path)
On 2010/10/08 20:12:28, sosa wrote:
> Probably should be a die.  If we can't find that path, something is wrong
> otherwise we might mask errors

Done.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode390
cros_mark_as_stable.py:390: (package_list[:index], overlay))
On 2010/10/08 20:12:28, sosa wrote:
> This doesn't print out the right package ... use ebuild.package

Done.

[sosa, 2010-10-11 18:14:03]

Mostly nits left.

http://codereview.chromium.org/3516025/diff/2001/3001
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3516025/diff/2001/3001#newcode51
cros_mark_as_stable.py:51: _COMMAND_DICTIONARY = {
My bad.  I thought you were adding a command originally.  Ignore comment.

On 2010/10/11 17:53:44, David James wrote:
> On 2010/10/08 20:12:28, sosa wrote:
> > Make a reference here for ebuilds
> 
> I don't understand. What would you like me to change here?
>

http://codereview.chromium.org/3516025/diff/2001/3001#newcode220
cros_mark_as_stable.py:220: return path.rstrip()
Fair enough

On 2010/10/11 17:53:44, David James wrote:
> On 2010/10/08 20:12:28, sosa wrote:
> > Why not just strip()?
> 
> We're just stripping off the trailing newline here (on the right side). There
> shouldn't be any other whitespace on the line. Since that's all we need I
think
> rstrip is more suited.
>

http://codereview.chromium.org/3516025/diff/2001/3001#newcode284
cros_mark_as_stable.py:284: shutil.copyfile(self._ebuild.ebuild_path,
new_ebuild_path)
Let's die and fix the ebuild then this will prevent us from making the error
again.

On 2010/10/11 17:53:44, David James wrote:
> On 2010/10/08 20:12:28, sosa wrote:
> > What's this?  Are you revving non-workon packages here? 
> 
> No. This is just error handling for when a user forgot to make a 9999 ebuild.
We
> actually have a package right now (chromeos-firmware) which has this problem.
> 
> I suppose we could Die() with an error here, but if we're going to do that
then
> we should fix the existing ebuilds first.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode343
cros_mark_as_stable.py:343: overlays = {
OK

On 2010/10/11 17:53:44, David James wrote:
> On 2010/10/08 20:12:28, sosa wrote:
> > Can you add a function that just added the overlays when they exist in your
> path
> > (i.e. replace this with
> > 
> > AddOverlayIfExists('%s/overlays' % gflags.FLAGS.srcroot), etc
> > 
> > This will avoid os.path.exists checks later and avoid unnecessary checks
> against
> > the overlays in your logic.
> 
> I looked into this. I think what we really want to do is call portageq-%s
envvar
> PORTDIR_OVERLAY and grab the list of overlays from portage. 
> 
> The flip side of doing this though is that it means we need to make sure the
> board we got is correct. Looking at cbuildbot.py, it looks like the board is
not
> passed in to cros_mark_as_stable.py when it calls clean, so messing around
with
> board overlays would be a bad idea until we get that working properly. I think
> we should address this in a separate change as this change is already big
> enough.

http://codereview.chromium.org/3516025/diff/2001/3001#newcode348
cros_mark_as_stable.py:348: for index in range(len(package_list)):
Thanks!

On 2010/10/11 17:53:44, David James wrote:
> On 2010/10/08 20:12:28, sosa wrote:
> > Can you wrap this whole thing into a function (i.e. BuildEBuildDictionary)
and
> > add a unit test.
> 
> Done.

http://codereview.chromium.org/3516025/diff/11001/12001#newcode72
cros_mark_as_stable.py:72: """Returns true if the git branch is on the
stabilizing branch."""
Either use EBuild or switch others to follow Ebuild spelling.  I don't honestly
care which way but they should be consistent.

http://codereview.chromium.org/3516025/diff/11001/12001#newcode75
cros_mark_as_stable.py:75: 
Use '' for consistency

http://codereview.chromium.org/3516025/diff/11001/12002
File cros_mark_as_stable_unittest.py (right):

http://codereview.chromium.org/3516025/diff/11001/12002#newcode247
cros_mark_as_stable_unittest.py:247: cros_mark_as_stable._SimpleRunCommand(cmd %
self.package).AndReturn(self.ebuild_path)
Watch the 80 chars here

http://codereview.chromium.org/3516025/diff/11001/12002#newcode250
cros_mark_as_stable_unittest.py:250: self.assertTrue(overlays["/path"])
assertEquals to self.package?

http://codereview.chromium.org/3516025/diff/11001/12002#newcode261
cros_mark_as_stable_unittest.py:261:
cros_mark_as_stable._BuildEbuildDictionary(overlays, [self.package], [])
assertNotIn?

[David James, 2010-10-12 14:30:03]

http://codereview.chromium.org/3516025/diff/11001/12001
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/3516025/diff/11001/12001#newcode72
cros_mark_as_stable.py:72: def _BuildEbuildDictionary(overlays, package_list,
commit_id_list):
On 2010/10/11 18:14:03, sosa wrote:
> Either use EBuild or switch others to follow Ebuild spelling.  I don't
honestly
> care which way but they should be consistent.

Done.

http://codereview.chromium.org/3516025/diff/11001/12001#newcode75
cros_mark_as_stable.py:75: commit_id = ""
On 2010/10/11 18:14:03, sosa wrote:
> Use '' for consistency

Done.

http://codereview.chromium.org/3516025/diff/11001/12002
File cros_mark_as_stable_unittest.py (right):

http://codereview.chromium.org/3516025/diff/11001/12002#newcode247
cros_mark_as_stable_unittest.py:247: cros_mark_as_stable._SimpleRunCommand(cmd %
self.package).AndReturn(self.ebuild_path)
On 2010/10/11 18:14:03, sosa wrote:
> Watch the 80 chars here

Done.

http://codereview.chromium.org/3516025/diff/11001/12002#newcode250
cros_mark_as_stable_unittest.py:250: self.assertTrue(overlays["/path"])
On 2010/10/11 18:14:03, sosa wrote:
> assertEquals to self.package?

Done.

http://codereview.chromium.org/3516025/diff/11001/12002#newcode261
cros_mark_as_stable_unittest.py:261:
cros_mark_as_stable._BuildEbuildDictionary(overlays, [self.package], [])
On 2010/10/11 18:14:03, sosa wrote:
> assertNotIn?

Added asserts.

[sosa, 2010-10-14 20:42:02]

LGTM

[anush, 2010-10-14 21:03:46]

LGTM

On Thu, Oct 14, 2010 at 1:42 PM, <sosa@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/3516025/show
>