Be more selective about what packages we remove so that we can detect conflicts.

Be more selective about what packages we remove so that we can detect conflicts.

Right now, the preflight queue unmerges all changed packages prior to
upgrading them. This is done to ensure that any lingering state from
failed package uprevs is cleaned up.

This change updates cros_mark_as_stable to be more selective. This both improves
speed and allows for more conflicts to be detected.

Change-Id: I3134918eb16b41e8882c8af1f1bdf9052bafd9ad

BUG=chromium-os:10127
TEST=Test uprev. Run unit tests.

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

[sosa, 2010-12-09 19:09:04]

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py
File cros_mark_as_stable.py (left):

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#oldcode327
cros_mark_as_stable.py:327: self.ebuild_path_no_revision = '%s-%s' %
(self.ebuild_path_no_version,
No.  This is used by cros_mark_chrome_as_stable.

This change also isn't mentioned in your issue description

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

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#newcode457
cros_mark_as_stable.py:457: stable_version_no_rev = '0.0.1'
extra line

[diandersAtChromium, 2010-12-10 23:58:54]

Aside from "=" sign being created in the drop_file (see comments), seems OK to
me.  ...but, of course, I'm no expert.

So the way that this is working is that it is only attempting to unmerge failed
"uprevs" by being explicit about what version to unmerge?  It specifically tries
to unmerge an uprev of the current package in cases where it used to unmerge the
package in general (and it doesn't care that the uprev might not exist, since a
no-op unmerge doesn't seem to be a problem).

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

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#newcode476
cros_mark_as_stable.py:476: os.unlink(new_stable_ebuild_path)
Leave in explicit return of None?

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#newcode573
cros_mark_as_stable.py:573: fh.write(' '.join(revved_packages))
Won't this file now contain "=" before each package?  Is that a problem?  ...it
sure seems like it.  It looks like cbuildbot uses this, which then gets used in
cros_run_unit_tests.

[davidjames, 2010-12-21 19:48:29]

PTAL.

> So the way that this is working is that it is only attempting to unmerge
failed
> "uprevs" by being explicit about what version to unmerge?  It specifically
tries
> to unmerge an uprev of the current package in cases where it used to unmerge
the
> package in general (and it doesn't care that the uprev might not exist, since
a
> no-op unmerge doesn't seem to be a problem).

Yup -- that's right.

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

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#newcode457
cros_mark_as_stable.py:457: stable_version_no_rev = '0.0.1'
On 2010/12/09 19:09:04, sosa wrote:
> extra line

Looks like an artifact of rietveld -- there's no extra line here.

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#newcode476
cros_mark_as_stable.py:476: os.unlink(new_stable_ebuild_path)
On 2010/12/10 23:58:54, diandersAtChromium wrote:
> Leave in explicit return of None?

Done.

http://codereview.chromium.org/5513012/diff/1/cros_mark_as_stable.py#newcode573
cros_mark_as_stable.py:573: fh.write(' '.join(revved_packages))
On 2010/12/10 23:58:54, diandersAtChromium wrote:
> Won't this file now contain "=" before each package?  Is that a problem? 
...it
> sure seems like it.  It looks like cbuildbot uses this, which then gets used
in
> cros_run_unit_tests.

Great catch. Fixed -- I changed my patch so that revved_packages was left alone.

[sosa, 2010-12-21 20:37:29]

LGTM w/ some nits

http://codereview.chromium.org/5513012/diff/14001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/5513012/diff/14001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:464: stable_version_no_rev = '0.0.1'
I'd prefer to see an extra line after the else clause but it's up to you

http://codereview.chromium.org/5513012/diff/14001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:560: revved_package_atoms = []
Optional nit: Maybe rename to new_atoms or new_revved_atoms since otherwise it's
unclear from the variable name whether this points to the old or new atoms.

[davidjames, 2010-12-21 20:46:32]

Pushing

http://codereview.chromium.org/5513012/diff/14001/cros_mark_as_stable.py
File cros_mark_as_stable.py (right):

http://codereview.chromium.org/5513012/diff/14001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:464: stable_version_no_rev = '0.0.1'
On 2010/12/21 20:37:29, sosa wrote:
> I'd prefer to see an extra line after the else clause but it's up to you

I get what you mean now. Done.

http://codereview.chromium.org/5513012/diff/14001/cros_mark_as_stable.py#newc...
cros_mark_as_stable.py:560: revved_package_atoms = []
On 2010/12/21 20:37:29, sosa wrote:
> Optional nit: Maybe rename to new_atoms or new_revved_atoms since otherwise
it's
> unclear from the variable name whether this points to the old or new atoms.

Done.