Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4969003:
Update cbuildbot.py to upload prebuilts from preflight buildbot. (Closed)
|
Created:
10 years, 1 month ago by davidjames Modified:
9 years, 4 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
DescriptionUpdate cbuildbot.py to upload prebuilts from preflight buildbot.
This change updates cbuildbot.py to upload prebuilts and reference them via PREFLIGHT_BINHOST.
Also ensure make.conf ends in a newline, as it looks nicer.
BUG=chromium-os:5311
TEST=Test that prebuilt.py updates PREFLIGHT_BINHOST in right make.conf
file. Test that cbuildbot.py runs prebuilt.py with right
arguments. Run unit tests for cbuildbot and prebuilt.py. Test runs of cbuildbot.py with --dryrun. Test upload of Packages file to local web server and check that emerge works with them.
Change-Id: Iad03c6c469e05b9ee1cceff69cbe2bdd51225e25
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c509a90
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review comments. #Patch Set 3 : Add comments. #Patch Set 4 : Dedup packages. #Patch Set 5 : Update cbuildbot tests to pass #Patch Set 6 : Finish testing cbuildbot.py changes #Patch Set 7 : Minor tweaks. #Patch Set 8 : Add lots more unit tests. #
Total comments: 51
Patch Set 9 : Add more cbuildbot unit tests. #
Total comments: 2
Patch Set 10 : Move binhost into chromiumos-overlay to allow for atomic commits! #Patch Set 11 : Remove deduplication to simplify change #Patch Set 12 : Address comments by dianders #
Total comments: 19
Patch Set 13 : Re-upload same change, but with git rebase --mixed cros/master #Patch Set 14 : Address review comments by sosa #
Total comments: 17
Patch Set 15 : Address review feedback #Patch Set 16 : Address review comments. #
Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode490 bin/cbuildbot.py:490: def _UploadPrebuilts(buildroot, board, overlay_config, master): Missing short docstring http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode492 bin/cbuildbot.py:492: '-p', buildroot, '-b', board, '-V', 'preflight', Please use long options http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode498 bin/cbuildbot.py:498: cmd.extend(['-u', 'chromeos-images:/var/www/prebuilt/', any reason we are not taking these as options and hardcoding them here? http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode589 bin/cbuildbot.py:589: buildconfig['master']) Better to do this after the uprev i.e. if we can't uprev ... we shouldn't push the prebuilts.
Uploading of the preflight prebuilts is good for quick turnaround. However to not use up a prebuilt upload for every run (since it is going to be 95%+ same packages) we should only keep the latest prebuilt around. This is different from the full builders uploading. This also means we dont update the make.conf for every prebuilt run but we use a single location to pull in latest preflight prebuilts. And ideally we dont want users to choose/switch between the different binhosts so we should update portage bintree.py/getbinpkg.py to select from a list of bin hosts instead of one (just like the overlays). Thanks Anush On Tue, Nov 16, 2010 at 3:20 PM, <davidjames@chromium.org> wrote: > Reviewers: sosa, scottz, > > Description: > Update cbuildbot.py to upload prebuilts from preflight buildbot. > > Right now we are just uploading the prebuilts and referencing them > via PREFLIGHT_BINHOST. We will start using them later, maybe by > looking at the PREFLIGHT_BINHOST variable. > > Also ensure make.conf ends in a newline, as it looks nicer. > > BUG=chromium-os:5311 > TEST=Test that prebuilt.py updates PREFLIGHT_BINHOST in right make.conf > file. Test that cbuildbot.py runs prebuilt.py with right > arguments. Run unit tests for cbuildbot and prebuilt.py. > > Change-Id: Iad03c6c469e05b9ee1cceff69cbe2bdd51225e25 > > Please review this at http://codereview.chromium.org/4969003/ > > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master > > Affected files: > M bin/cbuildbot.py > M prebuilt.py > > >
PTAL. On 2010/11/17 00:06:04, anush wrote: > Uploading of the preflight prebuilts is good for quick turnaround. > However to not use up a prebuilt upload for every run (since it is > going to be 95%+ same packages) we should only keep the latest > prebuilt around. This is different from the full builders uploading. > This also means we dont update the make.conf for every prebuilt run > but we use a single location to pull in latest preflight prebuilts. If we update prebuilts in-place, then users who are downloading prebuilts may get errors due to inconsistencies in their download. It is better to keep them versioned. We can always delete old directories to save space. > And ideally we dont want users to choose/switch between the different > binhosts so we should update portage bintree.py/getbinpkg.py to select > from a list of bin hosts instead of one (just like the overlays). Yes, this is planned. Once this change is in, I am going to look into whether we can join the two binhosts on the client side. http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode490 bin/cbuildbot.py:490: def _UploadPrebuilts(buildroot, board, overlay_config, master): On 2010/11/16 23:48:06, sosa wrote: > Missing short docstring Done. http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode492 bin/cbuildbot.py:492: '-p', buildroot, '-b', board, '-V', 'preflight', On 2010/11/16 23:48:06, sosa wrote: > Please use long options Done. http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode498 bin/cbuildbot.py:498: cmd.extend(['-u', 'chromeos-images:/var/www/prebuilt/', On 2010/11/16 23:48:06, sosa wrote: > any reason we are not taking these as options and hardcoding them here? Just discussed this over chat. We're trying to keep options away from buildbot config because buildbot config is hard to adjust. http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode589 bin/cbuildbot.py:589: buildconfig['master']) On 2010/11/16 23:48:06, sosa wrote: > Better to do this after the uprev i.e. if we can't uprev ... we shouldn't push > the prebuilts. Done.
On Tue, Nov 16, 2010 at 4:15 PM, <davidjames@chromium.org> wrote: > PTAL. > > On 2010/11/17 00:06:04, anush wrote: >> >> Uploading of the preflight prebuilts is good for quick turnaround. >> However to not use up a prebuilt upload for every run (since it is >> going to be 95%+ same packages) we should only keep the latest >> prebuilt around. This is different from the full builders uploading. >> This also means we dont update the make.conf for every prebuilt run >> but we use a single location to pull in latest preflight prebuilts. > > If we update prebuilts in-place, then users who are downloading prebuilts > may > get errors due to inconsistencies in their download. It is better to keep > them > versioned. We can always delete old directories to save space. I was suggesting update the prebuilt location and switch the gsdview redirect to the current active preflight while we can upload/update the next preflight (i.e we never point to a preflight prebuilt that is being updated). This also prevents cluttering our git log with make.conf updates for every preflight run (which is much more frequent than our full build updates). > >> And ideally we dont want users to choose/switch between the different >> binhosts so we should update portage bintree.py/getbinpkg.py to select >> from a list of bin hosts instead of one (just like the > > overlays). > > Yes, this is planned. Once this change is in, I am going to look into > whether we > can join the two binhosts on the client side. Awesome. Please see if this can be done in core portage itself, so that we dont loose this feature with the next portage update (since we can always upstream portage changes). Thanks Anush > > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py > File bin/cbuildbot.py (right): > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode490 > bin/cbuildbot.py:490: def _UploadPrebuilts(buildroot, board, > overlay_config, master): > On 2010/11/16 23:48:06, sosa wrote: >> >> Missing short docstring > > Done. > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode492 > bin/cbuildbot.py:492: '-p', buildroot, '-b', board, '-V', 'preflight', > On 2010/11/16 23:48:06, sosa wrote: >> >> Please use long options > > Done. > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode498 > bin/cbuildbot.py:498: cmd.extend(['-u', > 'chromeos-images:/var/www/prebuilt/', > On 2010/11/16 23:48:06, sosa wrote: >> >> any reason we are not taking these as options and hardcoding them > > here? > > Just discussed this over chat. We're trying to keep options away from > buildbot config because buildbot config is hard to adjust. > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode589 > bin/cbuildbot.py:589: buildconfig['master']) > On 2010/11/16 23:48:06, sosa wrote: >> >> Better to do this after the uprev i.e. if we can't uprev ... we > > shouldn't push >> >> the prebuilts. > > Done. > > http://codereview.chromium.org/4969003/ >
On 2010/11/17 03:54:30, anush wrote: > On Tue, Nov 16, 2010 at 4:15 PM, <mailto:davidjames@chromium.org> wrote: > > PTAL. > > > > On 2010/11/17 00:06:04, anush wrote: > >> > >> Uploading of the preflight prebuilts is good for quick turnaround. > >> However to not use up a prebuilt upload for every run (since it is > >> going to be 95%+ same packages) we should only keep the latest > >> prebuilt around. This is different from the full builders uploading. > >> This also means we dont update the make.conf for every prebuilt run > >> but we use a single location to pull in latest preflight prebuilts. > > > > If we update prebuilts in-place, then users who are downloading prebuilts > > may > > get errors due to inconsistencies in their download. It is better to keep > > them > > versioned. We can always delete old directories to save space. > > I was suggesting update the prebuilt location and switch the gsdview > redirect to the current active preflight while we can upload/update > the next preflight (i.e we never point to a preflight prebuilt that is > being updated). This also prevents cluttering our git log with > make.conf updates for every preflight run (which is much more frequent > than our full build updates). Without changes to Portage, this isn't possible, because the downloads of prebuilts are not atomic. After a client downloads the Packages file, they expect that (1) all the packages mentioned in the packages file will be available; and (2) the packages mentioned will match the SHA1 in the packages file. So if packages are changed underneath of Portage, or are deleted, Portage will not be happy. See also http://code.google.com/p/chromium-os/issues/detail?id=3225 > >> And ideally we dont want users to choose/switch between the different > >> binhosts so we should update portage bintree.py/getbinpkg.py to select > >> from a list of bin hosts instead of one (just like the > > > > overlays). > > > > Yes, this is planned. Once this change is in, I am going to look into > > whether we > > can join the two binhosts on the client side. > > Awesome. Please see if this can be done in core portage itself, so > that we dont loose this feature with the next portage update (since we > can always upstream portage changes). > > Thanks > Anush > > > > > > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py > > File bin/cbuildbot.py (right): > > > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode490 > > bin/cbuildbot.py:490: def _UploadPrebuilts(buildroot, board, > > overlay_config, master): > > On 2010/11/16 23:48:06, sosa wrote: > >> > >> Missing short docstring > > > > Done. > > > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode492 > > bin/cbuildbot.py:492: '-p', buildroot, '-b', board, '-V', 'preflight', > > On 2010/11/16 23:48:06, sosa wrote: > >> > >> Please use long options > > > > Done. > > > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode498 > > bin/cbuildbot.py:498: cmd.extend(['-u', > > 'chromeos-images:/var/www/prebuilt/', > > On 2010/11/16 23:48:06, sosa wrote: > >> > >> any reason we are not taking these as options and hardcoding them > > > > here? > > > > Just discussed this over chat. We're trying to keep options away from > > buildbot config because buildbot config is hard to adjust. > > > > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode589 > > bin/cbuildbot.py:589: buildconfig['master']) > > On 2010/11/16 23:48:06, sosa wrote: > >> > >> Better to do this after the uprev i.e. if we can't uprev ... we > > > > shouldn't push > >> > >> the prebuilts. > > > > Done. > > > > http://codereview.chromium.org/4969003/ > > I'll look into this separately after this change is in. Cheers, David
On Wed, Nov 17, 2010 at 2:21 PM, <davidjames@chromium.org> wrote: > On 2010/11/17 03:54:30, anush wrote: >> >> On Tue, Nov 16, 2010 at 4:15 PM, <mailto:davidjames@chromium.org> wrote: >> > PTAL. >> > >> > On 2010/11/17 00:06:04, anush wrote: >> >> >> >> Uploading of the preflight prebuilts is good for quick turnaround. >> >> However to not use up a prebuilt upload for every run (since it is >> >> going to be 95%+ same packages) we should only keep the latest >> >> prebuilt around. This is different from the full builders uploading. >> >> This also means we dont update the make.conf for every prebuilt run >> >> but we use a single location to pull in latest preflight prebuilts. >> > >> > If we update prebuilts in-place, then users who are downloading >> > prebuilts >> > may >> > get errors due to inconsistencies in their download. It is better to >> > keep >> > them >> > versioned. We can always delete old directories to save space. > >> I was suggesting update the prebuilt location and switch the gsdview >> redirect to the current active preflight while we can upload/update >> the next preflight (i.e we never point to a preflight prebuilt that is >> being updated). This also prevents cluttering our git log with >> make.conf updates for every preflight run (which is much more frequent >> than our full build updates). > > Without changes to Portage, this isn't possible, because the downloads of > prebuilts are not atomic. After a client downloads the Packages file, they > expect that (1) all the packages mentioned in the packages file will be > available; and (2) the packages mentioned will match the SHA1 in the > packages > file. So if packages are changed underneath of Portage, or are deleted, > Portage > will not be happy. I was suggesting the change on the server side not on portage. What if we upload to prebuilt_<buildno>/ prebuilt_<buildno+1>/ Just like what you already have but instead of updating the make.conf we update the server side's "latest" symlink to point to one of those. It is an atomic operation that gets updated to one version or the other, so we can avoid the Issue 3225. > > See also http://code.google.com/p/chromium-os/issues/detail?id=3225 > >> >> And ideally we dont want users to choose/switch between the different >> >> binhosts so we should update portage bintree.py/getbinpkg.py to select >> >> from a list of bin hosts instead of one (just like the >> > >> > overlays). >> > >> > Yes, this is planned. Once this change is in, I am going to look into >> > whether we >> > can join the two binhosts on the client side. > >> Awesome. Please see if this can be done in core portage itself, so >> that we dont loose this feature with the next portage update (since we >> can always upstream portage changes). > > >> Thanks >> Anush > >> > >> > >> > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py >> > File bin/cbuildbot.py (right): >> > >> > >> > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode490 >> > bin/cbuildbot.py:490: def _UploadPrebuilts(buildroot, board, >> > overlay_config, master): >> > On 2010/11/16 23:48:06, sosa wrote: >> >> >> >> Missing short docstring >> > >> > Done. >> > >> > >> > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode492 >> > bin/cbuildbot.py:492: '-p', buildroot, '-b', board, '-V', 'preflight', >> > On 2010/11/16 23:48:06, sosa wrote: >> >> >> >> Please use long options >> > >> > Done. >> > >> > >> > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode498 >> > bin/cbuildbot.py:498: cmd.extend(['-u', >> > 'chromeos-images:/var/www/prebuilt/', >> > On 2010/11/16 23:48:06, sosa wrote: >> >> >> >> any reason we are not taking these as options and hardcoding them >> > >> > here? >> > >> > Just discussed this over chat. We're trying to keep options away from >> > buildbot config because buildbot config is hard to adjust. >> > >> > >> > http://codereview.chromium.org/4969003/diff/1/bin/cbuildbot.py#newcode589 >> > bin/cbuildbot.py:589: buildconfig['master']) >> > On 2010/11/16 23:48:06, sosa wrote: >> >> >> >> Better to do this after the uprev i.e. if we can't uprev ... we >> > >> > shouldn't push >> >> >> >> the prebuilts. >> > >> > Done. >> > >> > http://codereview.chromium.org/4969003/ >> > > > I'll look into this separately after this change is in. > > Cheers, > > David > > http://codereview.chromium.org/4969003/ >
I should have mentioned I'm ok with as is but I'm letting you and anush duke his comments out.
Here is a potential approach to deduping the packages. Please take a look.
David, Word on the street is that you're going to make a bunch more changes. Publishing what I've got, which is sorta middle of things. I'll look at this again when I see your next upload. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode25 bin/cbuildbot.py:25: _PACKAGE_FILE = '%(buildroot)s/src/scripts/cbuildbot_package.list' Please add comments about what these are: the names of portage environment variables. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode305 bin/cbuildbot.py:305: Describe Args and Returns according to standard. Args: buildroot: The current sysroot, like "/home/xyz/trunk" board: The name of the board, like "x86-generic". envvar: The environment variable to get, like "PORTAGE_BINHOST". Returns: The value of the environment variable, as a string. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode523 bin/cbuildbot.py:523: present will not be uploaded twice. Add comment about binhosts: Note: it's expected that some of the strings in this list will simply be the empty string. We'll act as if those empty strings weren't in the list. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode528 bin/cbuildbot.py:528: '--prepend-version', 'preflight', '--key', _PREFLIGHT_BINHOST] I'm curious why you don't just import a function from prebuilt.py and call it. Going through the command line seems awkward... http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py File chromite/lib/binpkg.py (right): http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:14: class PackageIndex(object): Docstring? ...would be nice to include a spec for what a package file should look like. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:17: """Constructor.""" All of these members are public? Can you document what they are supposed to contain? http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:23: """Read entry from packages file. Read entries (plural). http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:24: Docstring: This will read entries that look like: key: value ...until a blank line is encountered. Any lines without a colon in them are ignored. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:26: pkgfile: A python file object. Returns: ? http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:41: This will terminate the list of items with a blank line. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:44: items: A list of tuples containing the entries to write. ...containing (key, value) pairs to write. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:46: for k, v in items: assert ':' not in k http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:52: This is a shortcut for calling ReadHeader() and ReadBody(). ...should be noted, since ReadHeader() and ReadBody() are public. ...or should ReadHeader() and ReadBody() not be public? http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:71: pkgfile: A python file object. The header and the blank line following the header should have already been read from the file (using ReadHeader()). http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:72: """ Comment: Read all the sections in the body by looping until we get to the end of the file, or get two blank lines in a row. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:76: break In order to be a valid body section, there must be a CPV key in the section. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:77: if d.get('CPV'): if 'CPV' in d: http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:82: Document: This has a side effect of updating the TIMESTAMP and PACKAGES header entries if the modified bit is set. Why use PACKAGES and not NUM_PACKAGES? Document: does not write out header entries with no value. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:88: self.header['PACKAGES'] = str(len(self.packages)) Should we be clearing .modified? http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:92: for k in keys if self.header[k]]) AKA: self._WritePkgIndex(pkgfile, sorted((k, v) for (k, v) in self.header.iteritems() if v)) http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:93: for metadata in sorted(self.packages, key=operator.itemgetter('CPV')): Comment: Will write each item in self.packages (is this one package?) to the file. Order by the CPV (what is the CPV?). http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:101: def GrabRemotePackageIndex(binhost_url): Technically, this could be a class method on PackageIndex (works sorta like an alternate constructor). That might be overkill, though, so feel free to ignore. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:108: A PackageIndex object COmment: Might return None in certain error conditions. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:111: def _RetryUrlOpen(url, tries=3): Why does this need to be an encapsulated function? Move to top level. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:115: for HTTP errors with a non-5xx code. Does this timeout, or can it ever get stuck forever in bad network conditions? http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:115: for HTTP errors with a non-5xx code. Comment: We will sleep for 10 seconds before retrying. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:122: The result of urllib2.urlopen(url). This is a file-like object. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:140: url = os.path.join(binhost_url, 'Packages') No, not os.path.join. This will fail on windows or anything with a path separator other than '/'. Try urlparse.urljoin(). http://codereview.chromium.org/4969003/diff/31001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode157 prebuilt.py:157: (Default: PORTAGE_BINHOST) Document skip_push too http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode235 prebuilt.py:235: def FilterPackagesIndex(pkgindex): Would have expected this to be a method as part of the PackageIndex object taking a predicate function (ShouldFilterPackage). http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode248 prebuilt.py:248: def ReadLocalPackageIndex(package_path): I would have expected this to be in binpkg.py to be symmetric to the GrabRemotePackageIndex() function. http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode252 prebuilt.py:252: package_path: Directory containing Packages file. Returns: ? http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode261 prebuilt.py:261: def RebasePackageIndex(pkgindex, binhost_base_url, path_prefix): Again, would have expected to be a method in PackageIndex class. Should mention that this will make the PATH for the packages all look like: path_prefix/packageCPV.tbz2 http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode275 prebuilt.py:275: """Write pkgindex to a temporary file. Again, would have expected to be a method in PackageIndex class. http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode281 prebuilt.py:281: A temporary file containing the packages from pkgindex. This will be a tempfile.NamedTemporaryFile(), which will be left seeked back at 0. http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode305 prebuilt.py:305: db[sha1] = os.path.join(base_uri, path) Don't use os.path.join(). This isn't an os path. Try urlparse.urljoin().
I've taken out the deduping per scottz's comments so as to simplify the CL and allow for easier review. I'll move the deduping to a separate CL. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode25 bin/cbuildbot.py:25: _PACKAGE_FILE = '%(buildroot)s/src/scripts/cbuildbot_package.list' On 2010/11/23 21:47:39, dianders wrote: > Please add comments about what these are: the names of portage environment > variables. Done. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode305 bin/cbuildbot.py:305: On 2010/11/23 21:47:39, dianders wrote: > Describe Args and Returns according to standard. > > Args: > buildroot: The current sysroot, like "/home/xyz/trunk" > board: The name of the board, like "x86-generic". > envvar: The environment variable to get, like "PORTAGE_BINHOST". > > Returns: > The value of the environment variable, as a string. Done. http://codereview.chromium.org/4969003/diff/31001/bin/cbuildbot.py#newcode528 bin/cbuildbot.py:528: '--prepend-version', 'preflight', '--key', _PREFLIGHT_BINHOST] On 2010/11/23 21:47:39, dianders wrote: > I'm curious why you don't just import a function from prebuilt.py and call it. > Going through the command line seems awkward... That's a good point. I like having a little separation between programs so that there's no risk the two can affect each other. Integrating using the command-line ensures that there's no tight coupling between the two programs. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py File chromite/lib/binpkg.py (right): http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:14: class PackageIndex(object): On 2010/11/23 21:47:39, dianders wrote: > Docstring? > > ...would be nice to include a spec for what a package file should look like. Thanks for all the feedback! Will address your feedback when I resubmit this part of the CL. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:82: On 2010/11/23 21:47:39, dianders wrote: > Document: This has a side effect of updating the TIMESTAMP and PACKAGES header > entries if the modified bit is set. > > Why use PACKAGES and not NUM_PACKAGES? That'd be a better name, but Portage calls the variable "PACKAGES" so we need to use the same name for compatibility (we create the file -- Portage reads it and expects it to be called "PACKAGES") > > Document: does not write out header entries with no value. http://codereview.chromium.org/4969003/diff/31001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/31001/prebuilt.py#newcode157 prebuilt.py:157: (Default: PORTAGE_BINHOST) On 2010/11/23 21:47:39, dianders wrote: > Document skip_push too Will do when I resubmit this part of the patch. I've deleted this part for now per scottz's advice to add deduping as a followup commit. Same for comments below.
http://codereview.chromium.org/4969003/diff/34001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/34001/bin/cbuildbot.py#newcode637 bin/cbuildbot.py:637: _UprevPush(buildroot, tracking_branch, buildconfig['board'], I thought you said you wanted this in the same commit? This is two sep commits http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode537 bin/cbuildbot.py:537: '--binhost-base-url', 'http://chromeos-prebuilt']) Can you put each of this long options on their own line? It's much easier to read option=arg and also modify. Otherwise for example someone may come back later and accidentally add a new option between --key and _PREFLIGHT_BINHOST http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode547 bin/cbuildbot.py:547: help='root directory where build occurs', default=".") Nit: Most other code sets cwd, is it possible to use cwd like other funcs? http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode600 bin/cbuildbot.py:600: if not os.path.isdir(chroot_path): Is sudo necessary? Is shutil.rmtree not sufficient? http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode650 bin/cbuildbot.py:650: if not buildconfig['master'] and buildconfig['important']: Scott has a if/else convention of a newline after any control block. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py File bin/cbuildbot_unittest.py (right): http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:174: drop_file = '%s/src/scripts/cbuildbot_package.list' % self._buildroot Can you reference the var in cbuildbot instead? http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:200: drop_file = '%s/src/scripts/cbuildbot_package.list' % self._buildroot Same here http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:216: def testGetPortageEnvVar(self): Doc string line...useful when test fails http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:228: def testUploadPublicPrebuilts(self): Doc string line...useful when test fails http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:235: def testUploadPrivatePrebuilts(self): Doc string line...useful when test fails http://codereview.chromium.org/4969003/diff/54001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/54001/prebuilt.py#newcode156 prebuilt.py:156: (Default: PORTAGE_BINHOST) Only indent by 2 here (see above) http://codereview.chromium.org/4969003/diff/54001/prebuilt.py#newcode426 prebuilt.py:426: key: The variable key to update in the git file. (Default: PORTAGE_BINHOST) sync_binhost_conf?
http://codereview.chromium.org/4969003/diff/34001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/34001/bin/cbuildbot.py#newcode637 bin/cbuildbot.py:637: _UprevPush(buildroot, tracking_branch, buildconfig['board'], On 2010/11/23 23:56:21, sosa wrote: > I thought you said you wanted this in the same commit? This is two sep commits Yeah, this was a bad merge. Fixed this. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode537 bin/cbuildbot.py:537: '--binhost-base-url', 'http://chromeos-prebuilt']) On 2010/11/23 23:56:21, sosa wrote: > Can you put each of this long options on their own line? It's much easier to > read option=arg and also modify. Otherwise for example someone may come back > later and accidentally add a new option between --key and _PREFLIGHT_BINHOST Done. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode547 bin/cbuildbot.py:547: help='root directory where build occurs', default=".") On 2010/11/23 23:56:21, sosa wrote: > Nit: Most other code sets cwd, is it possible to use cwd like other funcs? Done. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode600 bin/cbuildbot.py:600: if not os.path.isdir(chroot_path): On 2010/11/23 23:56:21, sosa wrote: > Is sudo necessary? Is shutil.rmtree not sufficient? Yeah, it's necessary unfortunately. There are some files in the board that are owned by root. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot.py#newcode650 bin/cbuildbot.py:650: if not buildconfig['master'] and buildconfig['important']: On 2010/11/23 23:56:21, sosa wrote: > Scott has a if/else convention of a newline after any control block. OK, added the newline back. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py File bin/cbuildbot_unittest.py (right): http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:216: def testGetPortageEnvVar(self): On 2010/11/23 23:56:21, sosa wrote: > Doc string line...useful when test fails Done. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:228: def testUploadPublicPrebuilts(self): On 2010/11/23 23:56:21, sosa wrote: > Doc string line...useful when test fails Done. http://codereview.chromium.org/4969003/diff/54001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:235: def testUploadPrivatePrebuilts(self): On 2010/11/23 23:56:21, sosa wrote: > Doc string line...useful when test fails Done. http://codereview.chromium.org/4969003/diff/54001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/54001/prebuilt.py#newcode156 prebuilt.py:156: (Default: PORTAGE_BINHOST) On 2010/11/23 23:56:21, sosa wrote: > Only indent by 2 here (see above) Done.
LGTM http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot_unittest.py File bin/cbuildbot_unittest.py (right): http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot_unittest.py#n... bin/cbuildbot_unittest.py:200: drop_file = cbuildbot._PACKAGE_FILE % {'buildroot': self._buildroot} Thanks for fixing this unittest :) http://codereview.chromium.org/4969003/diff/60001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode62 prebuilt.py:62: _BINHOST_CONF_DIR = 'src/third_party/chromiumos-overlay/chromeos/binhost' As an FYI this list is sorta getting out of hand. We should probably consider storing it somewhere else besides a list of constants.
I'm not sure that I'm able to see much here without understanding much more about binhost, our prebuilds, and mox. That's beyond what I have time to learn right now. :( ...but nothing obvious stands out, so LGTM w/ nits. http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py#newcode542 bin/cbuildbot.py:542: '--key', _PREFLIGHT_BINHOST] Still a little curious why this is done through RunCommand rather than simply importing a function from prebuilt.py. Also a NIT that the two paths in this function should be using os.path.join(), though not a big deal. http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py#newcode590 bin/cbuildbot.py:590: board = buildconfig['board'] Curious about whether these two lines should be in the try block. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode439 prebuilt.py:439: '%s/host/%s.conf' % (_BINHOST_CONF_DIR, _HOST_TARGET)) Just pass 4 params to os.path.join: os.path.join(build_path, _BINHOST_CONF_DIR, 'host', HOST_TARGET) http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode447 prebuilt.py:447: '%s/target/%s.conf' % (_BINHOST_CONF_DIR, board)) Same: 4 params to os.path.join.
http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py#newcode542 bin/cbuildbot.py:542: '--key', _PREFLIGHT_BINHOST] Agreed on both, if we want to convert to importing later that is fine but os.path.join should be used for the path. On 2010/11/24 01:30:31, diandersAtChromium wrote: > Still a little curious why this is done through RunCommand rather than simply > importing a function from prebuilt.py. > > Also a NIT that the two paths in this function should be using os.path.join(), > though not a big deal. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode62 prebuilt.py:62: _BINHOST_CONF_DIR = 'src/third_party/chromiumos-overlay/chromeos/binhost' +1 On 2010/11/24 00:40:02, sosa wrote: > As an FYI this list is sorta getting out of hand. We should probably consider > storing it somewhere else besides a list of constants. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode439 prebuilt.py:439: '%s/host/%s.conf' % (_BINHOST_CONF_DIR, _HOST_TARGET)) Agreed but the last one will still need the .conf added. On 2010/11/24 01:30:31, diandersAtChromium wrote: > Just pass 4 params to os.path.join: > > os.path.join(build_path, _BINHOST_CONF_DIR, 'host', HOST_TARGET) http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode471 prebuilt.py:471: if sync_binhost_conf: Move this into a function. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode477 prebuilt.py:477: f = file(binhost_conf, 'w') Nit, use something better than f please. I know you use it for three lines but I still prefer to keep our variable names coherent throughout.
http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py File chromite/lib/binpkg.py (right): http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:14: class PackageIndex(object): On 2010/11/23 21:47:39, dianders wrote: > Docstring? > > ...would be nice to include a spec for what a package file should look like. Done. (See http://codereview.chromium.org/5344002/ ) http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:17: """Constructor.""" On 2010/11/23 21:47:39, dianders wrote: > All of these members are public? Can you document what they are supposed to > contain? Done. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:23: """Read entry from packages file. On 2010/11/23 21:47:39, dianders wrote: > Read entries (plural). Done. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:24: On 2010/11/23 21:47:39, dianders wrote: > Docstring: > > This will read entries that look like: > > key: value > > ...until a blank line is encountered. > > Any lines without a colon in them are ignored. Done. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:26: pkgfile: A python file object. On 2010/11/23 21:47:39, dianders wrote: > Returns: ? Done. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:41: On 2010/11/23 21:47:39, dianders wrote: > This will terminate the list of items with a blank line. Done. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:44: items: A list of tuples containing the entries to write. On 2010/11/23 21:47:39, dianders wrote: > ...containing (key, value) pairs to write. Done. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:46: for k, v in items: On 2010/11/23 21:47:39, dianders wrote: > assert ':' not in k Skipped this one. http://codereview.chromium.org/4969003/diff/31001/chromite/lib/binpkg.py#newc... chromite/lib/binpkg.py:52: On 2010/11/23 21:47:39, dianders wrote: > This is a shortcut for calling ReadHeader() and ReadBody(). > > ...should be noted, since ReadHeader() and ReadBody() are public. ...or should > ReadHeader() and ReadBody() not be public? Made those two functions private. http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py File bin/cbuildbot.py (right): http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py#newcode542 bin/cbuildbot.py:542: '--key', _PREFLIGHT_BINHOST] On 2010/11/29 17:56:39, scottz wrote: > Agreed on both, if we want to convert to importing later that is fine but > os.path.join should be used for the path. > > On 2010/11/24 01:30:31, diandersAtChromium wrote: > > Still a little curious why this is done through RunCommand rather than simply > > importing a function from prebuilt.py. > > > > Also a NIT that the two paths in this function should be using os.path.join(), > > though not a big deal. > Which of the following do you mean? 1. os.path.join(buildroot, 'src/scripts/prebuilt.py') 2. os.path.join(buildroot, 'src', 'scripts', 'prebuilt.py') If it's the former, I agree, because it's helpful to catch the case where buildroot maybe ends in a slash and ensures we don't have two slashes in the path. If it's the latter, should we convert all similar examples to use os.path.join? E.g. the _SetupBoard function uses './setup_board'. Should that be os.path.join('.', 'setup_board') ? That seems a lot more verbose. importing is a bit of a pain because the two scripts use different versions of cros_build_lib (one uses the chromite version, the other does not.) If you import one into the other you might confuse Python because it changes the import path. That should be cleaned up but we haven't merged in chromite yet. One advantage of using command-line interaction between the two scripts is it ensures we can't have any complex interdependencies between the two scripts such as the above. But there are also advantages to importing, so once we fix the above problem I'd be happy with either approach. http://codereview.chromium.org/4969003/diff/60001/bin/cbuildbot.py#newcode590 bin/cbuildbot.py:590: board = buildconfig['board'] On 2010/11/24 01:30:31, diandersAtChromium wrote: > Curious about whether these two lines should be in the try block. Done. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py File prebuilt.py (right): http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode439 prebuilt.py:439: '%s/host/%s.conf' % (_BINHOST_CONF_DIR, _HOST_TARGET)) On 2010/11/29 17:56:39, scottz wrote: > Agreed but the last one will still need the .conf added. > > > On 2010/11/24 01:30:31, diandersAtChromium wrote: > > Just pass 4 params to os.path.join: > > > > os.path.join(build_path, _BINHOST_CONF_DIR, 'host', HOST_TARGET) > Done. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode447 prebuilt.py:447: '%s/target/%s.conf' % (_BINHOST_CONF_DIR, board)) On 2010/11/24 01:30:31, diandersAtChromium wrote: > Same: 4 params to os.path.join. Done. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode471 prebuilt.py:471: if sync_binhost_conf: On 2010/11/29 17:56:39, scottz wrote: > Move this into a function. Done. http://codereview.chromium.org/4969003/diff/60001/prebuilt.py#newcode477 prebuilt.py:477: f = file(binhost_conf, 'w') On 2010/11/29 17:56:39, scottz wrote: > Nit, use something better than f please. I know you use it for three lines but I > still prefer to keep our variable names coherent throughout. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
