Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3452032:
First submission of prebuilt upload script. (Closed)
|
Created:
10 years, 2 months ago by scottz Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Base URL:
ssh://git@gitrw.chromium.org:9222/crosutils Visibility:
Public. |
DescriptionFirst submission of prebuilt upload script.
This introduces a way to upload prebuilts from the command line.
The following features are available
* Multiprocess upload (cuts time down by a 1/5th for uploading)
* Versioned uploading with git file commital
* Host prebuilt uploading
* Board prebuilt uploading
BUG=chromium-os:4843, chromium-os:5855
TEST=./prebuilt_unittest.py
{'/b/cbuild/build/chroot/build/x86-dogfood/packages/x11-misc/util-macros-1.5.0.tbz2':
'gs://chromeos-prebuilt/host/version/packages/x11-misc/util-macros-1.5.0.tbz2',
'/b/cbuild/build/chroot/build/x86-dogf}
.FILTERING /usr/local/package/oob
FILTERING /var/tmp/bibby.file
.....Updating stage 20100309/stage3-amd64-20100309.tar.bz2 to stage
test_update
.
----------------------------------------------------------------------
Ran 7 tests in 0.002s
OK
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=9d892dd
Patch Set 1 #Patch Set 2 : Add in __init__.py files #
Total comments: 65
Patch Set 3 : Update prebuilt and prebuilt_unittest to address comments #
Total comments: 27
Patch Set 4 : Addressing more comments #
Total comments: 3
Patch Set 5 : Last round of updates change a few variables and update where I am printing to #
Messages
Total messages: 13 (0 generated)
One thing you will notice is that I am using RunCommand with shell=True. I started to work on updating RunCommand to work the way it was originally intended and it ended up being a lot more work than I wanted to bite off, and I want to get this CL out ASAP as it is crucial. I will file a bug about that and someone else can pick it up or I will get around to it.
Mostly nits http://codereview.chromium.org/3452032/diff/2001/3003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/2001/3003#newcode34 prebuilt.py:34: _RSYNC_CMD = 'sudo /usr/bin/rsync -a ' Maybe keep it as a list here? i.e. ['sudo', '/usr/bin/rsync', '-a' ]. Also, since this is coded, can you use the full flag name rather than the abbreviated one? http://codereview.chromium.org/3452032/diff/2001/3003#newcode36 prebuilt.py:36: _BOARD_PATH = 'chroot/build/%s' Since you're defining a format script way before you use it you should use the format: 'chroot/build/%(board)s so it's clear what the var is later. http://codereview.chromium.org/3452032/diff/2001/3003#newcode40 prebuilt.py:40: _GS_BOARD_PATH = 'board/%s/%s/' same here ... board/%(var1)s/%(var2)s http://codereview.chromium.org/3452032/diff/2001/3003#newcode71 prebuilt.py:71: file_fh = open(filename, 'w') Use a diff var name for the second file handle? new_file_fh? http://codereview.chromium.org/3452032/diff/2001/3003#newcode92 prebuilt.py:92: git_ssh_config_cmd = ('git config ' alignment issues http://codereview.chromium.org/3452032/diff/2001/3003#newcode107 prebuilt.py:107: return datetime.datetime.now().strftime('%d.%m.%y.%H%M%S') Don't you want this to be unique per invocation? i.e. set global version the first time and if that is not none return that? http://codereview.chromium.org/3452032/diff/2001/3003#newcode111 prebuilt.py:111: """Load a file with keywords on a perline basis. per line* http://codereview.chromium.org/3452032/diff/2001/3003#newcode117 prebuilt.py:117: global FILTER_PACKAGES Why define FILTER_PACKAGES at the top and here as a static var? Can you remove one or the other? http://codereview.chromium.org/3452032/diff/2001/3003#newcode122 prebuilt.py:122: def FilterPackage(file_path): This returns True or False and doesn't really take an action so the name is kind of misleading? Rename to ShouldFilterPackage would make it more clear http://codereview.chromium.org/3452032/diff/2001/3003#newcode135 prebuilt.py:135: if name in file_path: can you be a little more exact instead of using in? some regexp? http://codereview.chromium.org/3452032/diff/2001/3003#newcode142 prebuilt.py:142: def _GsUpload(args): How did you decide to make this private and not some of the others? http://codereview.chromium.org/3452032/diff/2001/3003#newcode146 prebuilt.py:146: args: a set of arguments that contains local_file and remote_file. Specifically a "2-tuple" of strings o/w first line will fail. http://codereview.chromium.org/3452032/diff/2001/3003#newcode158 prebuilt.py:158: print 'Failed to sync %s -> %s, retryings' % (local_file, remote_file) retryings? http://codereview.chromium.org/3452032/diff/2001/3003#newcode160 prebuilt.py:160: print 'Retry failed we should probably return the file that faild?' Nicer print statement? http://codereview.chromium.org/3452032/diff/2001/3003#newcode170 prebuilt.py:170: pool: integer of maximum proesses to have at the same time period at end of sentence http://codereview.chromium.org/3452032/diff/2001/3003#newcode195 prebuilt.py:195: filename = file_path.replace(strip_str, '').lstrip('/') This is sort of fragile depending on the strip string. You may want to consider something else. http://codereview.chromium.org/3452032/diff/2001/3003#newcode224 prebuilt.py:224: def BoardPackages(board, build_path, bucket, git_file=None): Add prefix to both these methods with a verb. Consider Upload http://codereview.chromium.org/3452032/diff/2001/3003#newcode225 prebuilt.py:225: """Upload board packages to Google Storage. Can you take out the commanalities of this function and above and make a common function to do most of the work? http://codereview.chromium.org/3452032/diff/2001/3004 File prebuilt_unittest.py (right): http://codereview.chromium.org/3452032/diff/2001/3004#newcode1 prebuilt_unittest.py:1: #!/usr/bin/python missing copyright and module docstring. http://codereview.chromium.org/3452032/diff/2001/3004#newcode5 prebuilt_unittest.py:5: import prebuilt alpha. http://codereview.chromium.org/3452032/diff/2001/3004#newcode12 prebuilt_unittest.py:12: Kill one of these extra lines. http://codereview.chromium.org/3452032/diff/2001/3004#newcode25 prebuilt_unittest.py:25: Read the contents of self.version_file and return as a list This can be one line. Period at end http://codereview.chromium.org/3452032/diff/2001/3004#newcode152 prebuilt_unittest.py:152: So many extra lines!
Nits http://codereview.chromium.org/3452032/diff/2001/3003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/2001/3003#newcode38 prebuilt.py:38: os.environ['BOTO_CONFIG'] = _BOTO_CONFIG This belongs in main(). If other scripts are including this script as a module they might want to use a different boto config. http://codereview.chromium.org/3452032/diff/2001/3003#newcode57 prebuilt.py:57: for line in file_fh.readlines(): No need for the ".readlines()" here. http://codereview.chromium.org/3452032/diff/2001/3003#newcode58 prebuilt.py:58: file_var, file_val = line.split() should this be line[:-1].split() ? http://codereview.chromium.org/3452032/diff/2001/3003#newcode117 prebuilt.py:117: global FILTER_PACKAGES Please don't redefine globals in functions :) http://codereview.chromium.org/3452032/diff/2001/3003#newcode118 prebuilt.py:118: FILTER_PACKAGES = [filter.strip() for filter in filter_fh.readlines()] No need for .readlines() http://codereview.chromium.org/3452032/diff/2001/3003#newcode132 prebuilt.py:132: False otherwise Trailing period. http://codereview.chromium.org/3452032/diff/2001/3003#newcode155 prebuilt.py:155: output = cros_build_lib.RunCommand(cmd, print_cmd=False, shell=True) The _Run function in cros_build_packages which adds retries to RunCommand sure would be useful here. http://codereview.chromium.org/3452032/diff/2001/3003#newcode177 prebuilt.py:177: pool.map(_GsUpload, workers) pool.map is buggy --- see http://bugs.python.org/issue9205 . recommend map_async instead. See the _RunManyParallel function in cros_build_packages. http://codereview.chromium.org/3452032/diff/2001/3003#newcode221 prebuilt.py:221: RevGitFile(git_file, 'amd64', version) Can you make the host architecture a constant that is defined at the top?
PTAL Awesome comments guys :) -Scott On 2010/09/28 20:42:41, davidjames wrote: > Nits > > http://codereview.chromium.org/3452032/diff/2001/3003 > File prebuilt.py (right): > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode38 > prebuilt.py:38: os.environ['BOTO_CONFIG'] = _BOTO_CONFIG > This belongs in main(). If other scripts are including this script as a module > they might want to use a different boto config. > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode57 > prebuilt.py:57: for line in file_fh.readlines(): > No need for the ".readlines()" here. > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode58 > prebuilt.py:58: file_var, file_val = line.split() > should this be line[:-1].split() ? > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode117 > prebuilt.py:117: global FILTER_PACKAGES > Please don't redefine globals in functions :) > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode118 > prebuilt.py:118: FILTER_PACKAGES = [filter.strip() for filter in > filter_fh.readlines()] > No need for .readlines() > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode132 > prebuilt.py:132: False otherwise > Trailing period. > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode155 > prebuilt.py:155: output = cros_build_lib.RunCommand(cmd, print_cmd=False, > shell=True) > The _Run function in cros_build_packages which adds retries to RunCommand sure > would be useful here. > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode177 > prebuilt.py:177: pool.map(_GsUpload, workers) > pool.map is buggy --- see http://bugs.python.org/issue9205 . recommend map_async > instead. See the _RunManyParallel function in cros_build_packages. > > http://codereview.chromium.org/3452032/diff/2001/3003#newcode221 > prebuilt.py:221: RevGitFile(git_file, 'amd64', version) > Can you make the host architecture a constant that is defined at the top?
http://codereview.chromium.org/3452032/diff/2001/3003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/2001/3003#newcode34 prebuilt.py:34: _RSYNC_CMD = 'sudo /usr/bin/rsync -a ' Heh actually that is old cruft that can just be removed! On 2010/09/28 19:34:45, sosa wrote: > Maybe keep it as a list here? i.e. ['sudo', '/usr/bin/rsync', '-a' ]. Also, > since this is coded, can you use the full flag name rather than the abbreviated > one? http://codereview.chromium.org/3452032/diff/2001/3003#newcode36 prebuilt.py:36: _BOARD_PATH = 'chroot/build/%s' On 2010/09/28 19:34:45, sosa wrote: > Since you're defining a format script way before you use it you should use the > format: > > 'chroot/build/%(board)s > > so it's clear what the var is later. Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode38 prebuilt.py:38: os.environ['BOTO_CONFIG'] = _BOTO_CONFIG On 2010/09/28 20:42:41, davidjames wrote: > This belongs in main(). If other scripts are including this script as a module > they might want to use a different boto config. Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode40 prebuilt.py:40: _GS_BOARD_PATH = 'board/%s/%s/' On 2010/09/28 19:34:45, sosa wrote: > same here ... board/%(var1)s/%(var2)s Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode57 prebuilt.py:57: for line in file_fh.readlines(): On 2010/09/28 20:42:41, davidjames wrote: > No need for the ".readlines()" here. Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode58 prebuilt.py:58: file_var, file_val = line.split() No, unless I am missing something? Each line is "Name Value" On 2010/09/28 20:42:41, davidjames wrote: > should this be line[:-1].split() ? http://codereview.chromium.org/3452032/diff/2001/3003#newcode71 prebuilt.py:71: file_fh = open(filename, 'w') On 2010/09/28 19:34:45, sosa wrote: > Use a diff var name for the second file handle? new_file_fh? Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode92 prebuilt.py:92: git_ssh_config_cmd = ('git config ' On 2010/09/28 19:34:45, sosa wrote: > alignment issues Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode107 prebuilt.py:107: return datetime.datetime.now().strftime('%d.%m.%y.%H%M%S') It doesn't need to be more unique than this. It should not be called for the same board multiple times as the source of the prebuilts should only be one host. On 2010/09/28 19:34:45, sosa wrote: > Don't you want this to be unique per invocation? i.e. set global version the > first time and if that is not none return that? http://codereview.chromium.org/3452032/diff/2001/3003#newcode111 prebuilt.py:111: """Load a file with keywords on a perline basis. On 2010/09/28 19:34:45, sosa wrote: > per line* Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode117 prebuilt.py:117: global FILTER_PACKAGES I actually meant to make it more of a way to augment it so that a list could be used at the top as well as a file for private packages. If you guys really hate this I can plumb it through take another look post append change. On 2010/09/28 20:42:41, davidjames wrote: > Please don't redefine globals in functions :) http://codereview.chromium.org/3452032/diff/2001/3003#newcode117 prebuilt.py:117: global FILTER_PACKAGES On 2010/09/28 19:34:45, sosa wrote: > Why define FILTER_PACKAGES at the top and here as a static var? Can you remove > one or the other? I actually wanted both for an option to store filtered keywords at the top if they were not sensitive and we always wanted to filter them. See my response to Davidjames then comment again please. http://codereview.chromium.org/3452032/diff/2001/3003#newcode118 prebuilt.py:118: FILTER_PACKAGES = [filter.strip() for filter in filter_fh.readlines()] On 2010/09/28 20:42:41, davidjames wrote: > No need for .readlines() Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode122 prebuilt.py:122: def FilterPackage(file_path): On 2010/09/28 19:34:45, sosa wrote: > This returns True or False and doesn't really take an action so the name is kind > of misleading? Rename to ShouldFilterPackage would make it more clear Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode135 prebuilt.py:135: if name in file_path: On 2010/09/28 19:34:45, sosa wrote: > can you be a little more exact instead of using in? some regexp? If I were to use regex I would use .*?KEYWORD.*? which is essentially in. I want filters to be broad. I'd rather filter out more than necessary than less. It will be fine tuned as issues come up. http://codereview.chromium.org/3452032/diff/2001/3003#newcode142 prebuilt.py:142: def _GsUpload(args): On 2010/09/28 19:34:45, sosa wrote: > How did you decide to make this private and not some of the others? I made this private because the way it is called is very specific to being wrapped with a multiprocess call. http://codereview.chromium.org/3452032/diff/2001/3003#newcode146 prebuilt.py:146: args: a set of arguments that contains local_file and remote_file. On 2010/09/28 19:34:45, sosa wrote: > Specifically a "2-tuple" of strings o/w first line will fail. Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode155 prebuilt.py:155: output = cros_build_lib.RunCommand(cmd, print_cmd=False, shell=True) So would the async call you have that hasn't made it into the cros_build_lib library :). I don't want to wait but I will commit to porting to it when either I or someone else bubbles it into the library. On 2010/09/28 20:42:41, davidjames wrote: > The _Run function in cros_build_packages which adds retries to RunCommand sure > would be useful here. http://codereview.chromium.org/3452032/diff/2001/3003#newcode158 prebuilt.py:158: print 'Failed to sync %s -> %s, retryings' % (local_file, remote_file) It is a new word :) On 2010/09/28 19:34:45, sosa wrote: > retryings? http://codereview.chromium.org/3452032/diff/2001/3003#newcode160 prebuilt.py:160: print 'Retry failed we should probably return the file that faild?' On 2010/09/28 19:34:45, sosa wrote: > Nicer print statement? Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode170 prebuilt.py:170: pool: integer of maximum proesses to have at the same time On 2010/09/28 19:34:45, sosa wrote: > period at end of sentence Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode177 prebuilt.py:177: pool.map(_GsUpload, workers) Changed to be similar to that function and added a TODO. I say bump on the priority of moving these functions into the lib. On 2010/09/28 20:42:41, davidjames wrote: > pool.map is buggy --- see http://bugs.python.org/issue9205 . recommend map_async > instead. See the _RunManyParallel function in cros_build_packages. http://codereview.chromium.org/3452032/diff/2001/3003#newcode195 prebuilt.py:195: filename = file_path.replace(strip_str, '').lstrip('/') I think that it is a reasonable approach considering the type of strings I am dealing with will always be paths An example is. /b/cbuild/chroot/build/board-x86/packages/... and the strip would be /b/cbuild/chroot/build/board-x86 The other approach would be doing regex to do the same thing which is just as functional as replace for something this direct. On 2010/09/28 19:34:45, sosa wrote: > This is sort of fragile depending on the strip string. You may want to consider > something else. http://codereview.chromium.org/3452032/diff/2001/3003#newcode221 prebuilt.py:221: RevGitFile(git_file, 'amd64', version) On 2010/09/28 20:42:41, davidjames wrote: > Can you make the host architecture a constant that is defined at the top? Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode224 prebuilt.py:224: def BoardPackages(board, build_path, bucket, git_file=None): On 2010/09/28 19:34:45, sosa wrote: > Add prefix to both these methods with a verb. Consider Upload Done. http://codereview.chromium.org/3452032/diff/2001/3003#newcode225 prebuilt.py:225: """Upload board packages to Google Storage. I merged them. You would have cried initially I had what is GenerateUploadDict in both :P On 2010/09/28 19:34:45, sosa wrote: > Can you take out the commanalities of this function and above and make a common > function to do most of the work? http://codereview.chromium.org/3452032/diff/2001/3004 File prebuilt_unittest.py (right): http://codereview.chromium.org/3452032/diff/2001/3004#newcode1 prebuilt_unittest.py:1: #!/usr/bin/python On 2010/09/28 19:34:45, sosa wrote: > missing copyright and module docstring. Done. http://codereview.chromium.org/3452032/diff/2001/3004#newcode5 prebuilt_unittest.py:5: import prebuilt On 2010/09/28 19:34:45, sosa wrote: > alpha. Done. http://codereview.chromium.org/3452032/diff/2001/3004#newcode12 prebuilt_unittest.py:12: On 2010/09/28 19:34:45, sosa wrote: > Kill one of these extra lines. Done. http://codereview.chromium.org/3452032/diff/2001/3004#newcode25 prebuilt_unittest.py:25: Read the contents of self.version_file and return as a list On 2010/09/28 19:34:45, sosa wrote: > This can be one line. Period at end Done. http://codereview.chromium.org/3452032/diff/2001/3004#newcode152 prebuilt_unittest.py:152: KILL THEM ALL RARRRRRRRRRRRR On 2010/09/28 19:34:45, sosa wrote: > So many extra lines!
More nits http://codereview.chromium.org/3452032/diff/2001/3003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/2001/3003#newcode195 prebuilt.py:195: filename = file_path.replace(strip_str, '').lstrip('/') ok. On 2010/09/28 21:57:39, scottz wrote: > I think that it is a reasonable approach considering the type of strings I am > dealing with will always be paths > > An example is. > > /b/cbuild/chroot/build/board-x86/packages/... > > and the strip would be > > /b/cbuild/chroot/build/board-x86 > > The other approach would be doing regex to do the same thing which is just as > functional as replace for something this direct. > > On 2010/09/28 19:34:45, sosa wrote: > > This is sort of fragile depending on the strip string. You may want to > consider > > something else. > > http://codereview.chromium.org/3452032/diff/10001/11003#newcode121 prebuilt.py:121: set here is redundant since FILTER_PACKAGES is already a set. http://codereview.chromium.org/3452032/diff/10001/11003#newcode166 prebuilt.py:166: Create a pool of process and call _GsUpload with the proper arguments. For your todo's use TODO(<name>): so if someone else sees it later they know who to talk to quickly. http://codereview.chromium.org/3452032/diff/10001/11003#newcode196 prebuilt.py:196: gs_file_path = os.path.join(gs_path, filename) so many extra lines!! http://codereview.chromium.org/3452032/diff/10001/11003#newcode227 prebuilt.py:227: Args: board? http://codereview.chromium.org/3452032/diff/10001/11003#newcode232 prebuilt.py:232: push it. if not board. Add comment about the case. http://codereview.chromium.org/3452032/diff/10001/11004 File prebuilt_unittest.py (right): http://codereview.chromium.org/3452032/diff/10001/11004#newcode43 prebuilt_unittest.py:43: def testAddVariableThatDoesnotExist(self): DoesNot* http://codereview.chromium.org/3452032/diff/10001/11004#newcode96 prebuilt_unittest.py:96: files_to_sync = ['/b/cbuild/build/chroot/build/x86-dogfood/packages/x11-misc/shared-mime-info-0.70.tbz2', var out /b/cbuild/build/chroot/build/x86-dogfood/packages/x11-misc or define strip_path before and use it to get these to be less than 80 chars? http://codereview.chromium.org/3452032/diff/10001/11004#newcode103 prebuilt_unittest.py:103: only one line here http://codereview.chromium.org/3452032/diff/10001/11004#newcode112 prebuilt_unittest.py:112: ditto http://codereview.chromium.org/3452032/diff/10001/11004#newcode119 prebuilt_unittest.py:119: results[entry] = os.path.join(gs_bucket_path, alignment http://codereview.chromium.org/3452032/diff/10001/11004#newcode130 prebuilt_unittest.py:130: print result debug?
LGTM w/nit. Check with sosa also in case he has more review comments. http://codereview.chromium.org/3452032/diff/2001/3003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/2001/3003#newcode58 prebuilt.py:58: file_var, file_val = line.split() On 2010/09/28 21:57:39, scottz wrote: > No, unless I am missing something? > > Each line is > > "Name Value" Wouldn't it be "Name Value\n"? >>> "foo var\n".split(" ") ['foo', 'var\n'] >>> "foo var\n".split() ['foo', 'var'] Looks like split() with no arguments strips the newline anyway so you're OK without the trimming here. http://codereview.chromium.org/3452032/diff/10001/11003#newcode119 prebuilt.py:119: return FILTER_PACKAGES You don't need this anymore because you're not reassigning FILTER_PACKAGES. http://codereview.chromium.org/3452032/diff/10001/11003#newcode121 prebuilt.py:121: No need for the enclosing set() here. A simple list comprehension will suffice.
http://codereview.chromium.org/3452032/diff/10001/11003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/10001/11003#newcode119 prebuilt.py:119: global FILTER_PACKAGES On 2010/09/28 22:30:30, davidjames wrote: > You don't need this anymore because you're not reassigning FILTER_PACKAGES. Looks like rietveld messed up the context on my comment, so I'll be more explicit. You don't need "global FILTER_PACKAGES" anymore because you're not reassigning FILTER_PACKAGES anymore.
Alright take another look please :) http://codereview.chromium.org/3452032/diff/10001/11003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/10001/11003#newcode119 prebuilt.py:119: global FILTER_PACKAGES On 2010/09/28 22:30:30, davidjames wrote: > You don't need this anymore because you're not reassigning FILTER_PACKAGES. Done. http://codereview.chromium.org/3452032/diff/10001/11003#newcode119 prebuilt.py:119: global FILTER_PACKAGES On 2010/09/28 22:33:03, davidjames wrote: > On 2010/09/28 22:30:30, davidjames wrote: > > You don't need this anymore because you're not reassigning FILTER_PACKAGES. > > Looks like rietveld messed up the context on my comment, so I'll be more > explicit. > > You don't need "global FILTER_PACKAGES" anymore because you're not reassigning > FILTER_PACKAGES anymore. Done. http://codereview.chromium.org/3452032/diff/10001/11003#newcode121 prebuilt.py:121: FILTER_PACKAGES.update(set([filter.strip() for filter in filter_fh])) On 2010/09/28 22:26:42, sosa wrote: > set here is redundant since FILTER_PACKAGES is already a set. Done. http://codereview.chromium.org/3452032/diff/10001/11003#newcode121 prebuilt.py:121: FILTER_PACKAGES.update(set([filter.strip() for filter in filter_fh])) On 2010/09/28 22:30:30, davidjames wrote: > No need for the enclosing set() here. A simple list comprehension will suffice. Done. http://codereview.chromium.org/3452032/diff/10001/11003#newcode166 prebuilt.py:166: # TODO: potentially return what failed so we can do something with it but On 2010/09/28 22:26:42, sosa wrote: > For your todo's use TODO(<name>): so if someone else sees it later they know who > to talk to quickly. Done. http://codereview.chromium.org/3452032/diff/10001/11003#newcode196 prebuilt.py:196: I swear I don't put those there! On 2010/09/28 22:26:42, sosa wrote: > so many extra lines!! http://codereview.chromium.org/3452032/diff/10001/11003#newcode227 prebuilt.py:227: git_file: If set, update this file with a host/version combo, commit and On 2010/09/28 22:26:42, sosa wrote: > board? Done. http://codereview.chromium.org/3452032/diff/10001/11003#newcode232 prebuilt.py:232: if board is None: On 2010/09/28 22:26:42, sosa wrote: > if not board. Add comment about the case. Done. http://codereview.chromium.org/3452032/diff/10001/11004 File prebuilt_unittest.py (right): http://codereview.chromium.org/3452032/diff/10001/11004#newcode43 prebuilt_unittest.py:43: def testAddVariableThatDoesnotExist(self): On 2010/09/28 22:26:42, sosa wrote: > DoesNot* Done. http://codereview.chromium.org/3452032/diff/10001/11004#newcode103 prebuilt_unittest.py:103: On 2010/09/28 22:26:42, sosa wrote: > only one line here Done. http://codereview.chromium.org/3452032/diff/10001/11004#newcode112 prebuilt_unittest.py:112: On 2010/09/28 22:26:42, sosa wrote: > ditto Done. http://codereview.chromium.org/3452032/diff/10001/11004#newcode119 prebuilt_unittest.py:119: results[entry] = os.path.join(gs_bucket_path, On 2010/09/28 22:26:42, sosa wrote: > alignment Done. http://codereview.chromium.org/3452032/diff/10001/11004#newcode130 prebuilt_unittest.py:130: print result On 2010/09/28 22:26:42, sosa wrote: > debug? Done.
LGTM with a few nits. Thanks for writing unit tests! http://codereview.chromium.org/3452032/diff/18001/19003 File prebuilt.py (right): http://codereview.chromium.org/3452032/diff/18001/19003#newcode35 prebuilt.py:35: FILTER_PACKAGES = set() why is this public and the rest non-public? http://codereview.chromium.org/3452032/diff/18001/19003#newcode235 prebuilt.py:235: # TODO: eventually add support for different host_targets TODO(who) http://codereview.chromium.org/3452032/diff/18001/19003#newcode259 prebuilt.py:259: print msg print >> sys.stderr, msg
Alright updated last round PTAL
LGTM
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
