Update cbuildbot.py and prebuilt.py to deduplicate preflight prebuilts.

Update cbuildbot.py and prebuilt.py to deduplicate preflight prebuilts.

Preflight prebuilts reference the last full build, so they should always be complete as long as the last full build is still there.

Also add host prebuilts to the preflight prebuilts.

BUG=chromium-os:5311
TEST=Run unit tests for cbuildbot and prebuilt.py. Test runs of cbuildbot.py with --dryrun.

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

[davidjames, 2010-11-24 20:47:58]

PTAL. This addresses dianders' and scottz's comments

[diandersAtChromium, 2010-11-25 00:45:37]

Didn't get a chance to look at unit tests (still haven't learned mox).  See
other comments.

NOTE: Still shouldn't submit on my review alone.  Learning this code better, but
still not an expert.

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py
File chromite/lib/binpkg.py (right):

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:28: a unique identifier for the package.
Thank you for desc!  Very helpful!  :)

BTW: having this class to manage the packages index file is much more elegant
than the old way things were done in prebuilt.py.  :)

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:89: assert d, 'Packages entry must contain at least one
key/value pair'
Could happen if file ever has two blank lines (even at the end of the file).  I
guess that is an illegal situation?

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:105: entry: A dictionary of the key/value pairs to write.
Any dictionary items with a blank value will not be written out.  I hope (?)
that is on purpose.  Does it happen in practice?

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:115: """
Why does this update the existing header dictionary object.  Are you expecting
it to contain something already and you want to merge the two?  Are you
expecting that someone kept a reference to it and you want that reference to be
up to date (if so, you should also add a "clear")?

If neither of two, you should just assign with self.header =

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:126: """
assert not self.packages, "Should only call _ReadBody once"

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:167: pkgindexes: A list of pkgindex files that have
already been uploaded.
Not a list of files.  A list of other PackageIndex objects containing info about
packages that have already been uploaded.

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:203: 
NOTE: Has a side effect of updating TIMESTAMP and PACKAGES member's of the
header.

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:268: A PackageIndex object.
...or None if the database is missing.

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py
File prebuilt.py (right):

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode309
prebuilt.py:309: remote_path: The base path to the remote paths.
remote_path ==> base_remote_path

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode310
prebuilt.py:310: pkgs: The packages to upload.
(a list of dictionaries, one per package).

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode358
prebuilt.py:358: sync_binhost_conf=False):
I worry a little bit about adding a parameter anywhere other than the end of the
param list to a function that appears to be public.

...but a quick grep -r in the src/scripts doesn't seem to show this function as
being called outside of this file, so I guess it's OK.  Technically, this
function should probably be made private.

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode359
prebuilt.py:359: """Upload Host prebuilt files to Google Storage space.
Not only to Google Storage space.  Also supports ssh.

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode363
prebuilt.py:363: upload_location: The upload location.
Missing desc for version and binhost_base_url.

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode363
prebuilt.py:363: upload_location: The upload location.
upload_location: Can be gs:// style URL, or just host:path for SSH.

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode373
prebuilt.py:373: uploading duplicate files, we just link to the old files.
Comment: Each of these should be a PackageIndex object.

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode400
prebuilt.py:400: pkgindex.RemoveFilteredPackages(lambda pkg:
ShouldFilterPackage(pkg))
Any reason not to just pass in ShouldFilterPackage directly?

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode420
prebuilt.py:420: d = { 'pkgindex': tmp_packages_file.name,
Technically, maybe should have os.path.abspath() on the temp file name, since
you are changing directories now.  ...although seems unlikely that we'll ever
run someplace where temp files are relative...

http://codereview.chromium.org/5344002/diff/29001/prebuilt.py#newcode469
prebuilt.py:469: help='Previous binhost URL')
Indicate in help that this option can be specified more than once?

[sosa, 2010-11-25 00:59:44]

I'm confused.  I thought I already LGTM'd half of this.  Did the two CL's get
merged back into one?

[davidjames, 2010-11-25 01:55:32]

On 2010/11/25 00:59:44, sosa wrote:
> I'm confused.  I thought I already LGTM'd half of this.  Did the two CL's get
> merged back into one?

Yes. git cl upload got confused. Try out patch set #4 (which is the same as #3,
except that I ran git reset --mixed to fix up the diff so that it's against the
branch again)

[davidjames, 2010-11-29 21:54:17]

PTAL

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py
File chromite/lib/binpkg.py (right):

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:89: assert d, 'Packages entry must contain at least one
key/value pair'
On 2010/11/25 00:45:37, diandersAtChromium wrote:
> Could happen if file ever has two blank lines (even at the end of the file). 
I
> guess that is an illegal situation?

The parser that comes with Portage does not output multiple blank lines at the
end of the file, but when it reads blank lines, it quits and assumes the file is
finished.

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:105: entry: A dictionary of the key/value pairs to write.
On 2010/11/25 00:45:37, diandersAtChromium wrote:
> Any dictionary items with a blank value will not be written out.  I hope (?)
> that is on purpose.  Does it happen in practice?

This behavior is copied from Portage. Not sure if this happens in practice but
they do have code for handling the situation.

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:115: """
On 2010/11/25 00:45:37, diandersAtChromium wrote:
> Why does this update the existing header dictionary object.  Are you expecting
> it to contain something already and you want to merge the two?  Are you
> expecting that someone kept a reference to it and you want that reference to
be
> up to date (if so, you should also add a "clear")?
> 
> If neither of two, you should just assign with self.header = 

Done.

http://codereview.chromium.org/5344002/diff/29001/chromite/lib/binpkg.py#newc...
chromite/lib/binpkg.py:268: A PackageIndex object.
On 2010/11/25 00:45:37, diandersAtChromium wrote:
> ...or None if the database is missing.

Done.

[sosa, 2010-11-30 00:54:15]

Quick questions

http://codereview.chromium.org/5344002/diff/45001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/5344002/diff/45001/bin/cbuildbot.py#newcode655
bin/cbuildbot.py:655: if not new_binhost:
Why not just set it before you use it?

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py
File prebuilt.py (left):

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py#oldcode231
prebuilt.py:231: def _ShouldFilterPackageFileSection(section):
Why is this being removed?  Part of other CL?

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py
File prebuilt.py (right):

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py#newcode395
prebuilt.py:395: pkgindexes: Old uploaded prebuilts to compare against. Instead
of
Why no _?

[davidjames, 2010-11-30 02:28:52]

http://codereview.chromium.org/5344002/diff/45001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/5344002/diff/45001/bin/cbuildbot.py#newcode655
bin/cbuildbot.py:655: if not new_binhost:
On 2010/11/30 00:54:15, sosa wrote:
> Why not just set it before you use it?

Done.

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py
File prebuilt.py (left):

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py#oldcode231
prebuilt.py:231: def _ShouldFilterPackageFileSection(section):
On 2010/11/30 00:54:15, sosa wrote:
> Why is this being removed?  Part of other CL?

The package file filtering now happens in UploadPrebuilt() instead of in
_GsUpload(). This allows us to modify the packages file unconditionally, even if
it's not being uploaded to Google Storage (e.g. it's being uploaded via rsync).

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py
File prebuilt.py (right):

http://codereview.chromium.org/5344002/diff/45001/prebuilt.py#newcode395
prebuilt.py:395: pkgindexes: Old uploaded prebuilts to compare against. Instead
of
On 2010/11/30 00:54:15, sosa wrote:
> Why no _?

Done.

[sosa, 2010-11-30 19:30:28]

LGTM