Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4425004:
Adding a script to generate au-geneate.zip file
|
Created:
10 years, 1 month ago by Raja Aluri Modified:
10 years, 1 month ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
DescriptionAdding a script to generate au-geneate.zip file
Change-Id: Ifc47ed28dc3efc0e7ebd018f6703b36913ffd39c
BUG=8716
TEST=Ran the script inside the chroot to makesure it is generating the package.
Patch Set 1 #
Total comments: 76
Patch Set 2 : "Adding a script to generate au-geneate.zip file" #
Total comments: 44
Patch Set 3 : Fixing code review comments #
Total comments: 43
Patch Set 4 : Fixing code review comments #Patch Set 5 : "Fixing code review comments" #Patch Set 6 : "Fixing code review comments" #
Total comments: 2
Messages
Total messages: 14 (0 generated)
LGTM
initial pass. http://codereview.chromium.org/4425004/diff/1/3 File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/1/3#newcode28 generate_au_zip.py:28: args: None If there are no arguments there is no need for this. http://codereview.chromium.org/4425004/diff/1/3#newcode32 generate_au_zip.py:32: parser = optparse.OptionParser() Move these into main and drop this whole function http://codereview.chromium.org/4425004/diff/1/3#newcode46 generate_au_zip.py:46: def ValidateOptions(options, args): This should be in main as well. http://codereview.chromium.org/4425004/diff/1/3#newcode59 generate_au_zip.py:59: if options.debug: Validating options should not be modifying things. http://codereview.chromium.org/4425004/diff/1/3#newcode63 generate_au_zip.py:63: options.output_dir='/tmp/au-generator' This should be the default set in the parser setup. http://codereview.chromium.org/4425004/diff/1/3#newcode79 generate_au_zip.py:79: logging.error("Invalid option passed for dest_files_root") shouldn't we be dying here? http://codereview.chromium.org/4425004/diff/1/3#newcode81 generate_au_zip.py:81: ldd_files = ['/usr/bin/delta_generator', ldd files? Is there a better name for this? they look like random binaries to me. http://codereview.chromium.org/4425004/diff/1/3#newcode86 generate_au_zip.py:86: static_files = ['~/trunk/src/scripts/common.sh', same deal. Static files in what regard? They never change? http://codereview.chromium.org/4425004/diff/1/3#newcode104 generate_au_zip.py:104: all_files =ldd_files + static_files allfiles = ldd_files... http://codereview.chromium.org/4425004/diff/1/3#newcode105 generate_au_zip.py:105: all_files = map(os.path.expanduser,all_files) os.path.expanduser, all_files http://codereview.chromium.org/4425004/diff/1/3#newcode106 generate_au_zip.py:106: logging.debug("all files that need to be copied = %s",all_files) comma, SPACE please apply this to all areas. http://codereview.chromium.org/4425004/diff/1/3#newcode113 generate_au_zip.py:113: for file_name in ldd_files: this whole for loop should be broken out into a function that returns the list of files you want. http://codereview.chromium.org/4425004/diff/1/3#newcode116 generate_au_zip.py:116: stdout_data ='' the format is "variable = value" Note the spaces. You do that in most places please do it everywhere. http://codereview.chromium.org/4425004/diff/1/3#newcode121 generate_au_zip.py:121: stderr=subprocess.PIPE ) align cmd and stderr= no space after PIPE) http://codereview.chromium.org/4425004/diff/1/3#newcode129 generate_au_zip.py:129: library_list =[] library_list = [] http://codereview.chromium.org/4425004/diff/1/3#newcode131 generate_au_zip.py:131: if stdout_data: should you not always have stdout_data from ldd if the command completes without error? http://codereview.chromium.org/4425004/diff/1/3#newcode136 generate_au_zip.py:136: if library_list: as above shouldn't this always be set? If not aren't we in trouble already? http://codereview.chromium.org/4425004/diff/1/3#newcode147 generate_au_zip.py:147: for k,v in recurse_dirs.iteritems(): please use variables that are descriptive. Nice usage of iteritems :) http://codereview.chromium.org/4425004/diff/1/3#newcode160 generate_au_zip.py:160: docstring? http://codereview.chromium.org/4425004/diff/1/3#newcode162 generate_au_zip.py:162: if not temp_dir: Instead of setting a default just make it a required argument or is there a reason to call CleanUp with None? http://codereview.chromium.org/4425004/diff/1/3#newcode165 generate_au_zip.py:165: logging.info("Removed tempdir = %s",temp_dir) empty lines between functions http://codereview.chromium.org/4425004/diff/1/3#newcode166 generate_au_zip.py:166: def GenerateZipFile(base_name=None,format='zip',root_dir=None): docstring http://codereview.chromium.org/4425004/diff/1/3#newcode168 generate_au_zip.py:168: return False same as above, if a function should never get a None value for an option then don't set a default in the function definition. http://codereview.chromium.org/4425004/diff/1/3#newcode179 generate_au_zip.py:179: print >>sys.stderr, "Execution failed:", e Why are we not using logging anymore? http://codereview.chromium.org/4425004/diff/1/3#newcode180 generate_au_zip.py:180: os.chdir(current_dir) This should be wrapped in a try: finally: so that it is always executed. http://codereview.chromium.org/4425004/diff/1/3#newcode181 generate_au_zip.py:181: return False Always return False? http://codereview.chromium.org/4425004/diff/1/3#newcode184 generate_au_zip.py:184: def ExcludeBlacklist(library_list=None,black_list=None): docstrings http://codereview.chromium.org/4425004/diff/1/3#newcode185 generate_au_zip.py:185: if not library_list: same as above with default arguments. Do you know what I mean when I say this? I'd be happy to discuss this with you http://codereview.chromium.org/4425004/diff/1/3#newcode193 generate_au_zip.py:193: if re.search(pattern, library): no need for regex or generating a regex pattern. Use the list. I imagine the values you are using are very exact so a if library in black_list: logging..... continue # to get out of hte for loop http://codereview.chromium.org/4425004/diff/1/3#newcode196 generate_au_zip.py:196: return_list.append(library) You should continue in the if statement and the else is not needed. http://codereview.chromium.org/4425004/diff/1/3#newcode200 generate_au_zip.py:200: def SplitAndStrip(data=None): default values, docstring.. The docstring should ahve information of "Data example of this...." so when I read through what you are doing I know why you are actually parsing. http://codereview.chromium.org/4425004/diff/1/3#newcode224 generate_au_zip.py:224: if not output_dir: defaults. Looking thrugh this it doesn't look like you should need any of these checks. http://codereview.chromium.org/4425004/diff/1/3#newcode242 generate_au_zip.py:242: def main(): spaces http://codereview.chromium.org/4425004/diff/1/3#newcode247 generate_au_zip.py:247: #SetConsoleLogger() Remove these http://codereview.chromium.org/4425004/diff/1/3#newcode256 generate_au_zip.py:256: dest_files_root = os.path.join(temp_dir,'au-generator') Since you only use CreateTempDir for this purpose you might as well make it a CreateTempAuGenDir and have it return the au-generator path you want
PTAL http://codereview.chromium.org/4425004/diff/1/3 File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/1/3#newcode28 generate_au_zip.py:28: args: None On 2010/11/05 01:22:46, scottz-goog wrote: > If there are no arguments there is no need for this. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode46 generate_au_zip.py:46: def ValidateOptions(options, args): On 2010/11/05 01:22:46, scottz-goog wrote: > This should be in main as well. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode59 generate_au_zip.py:59: if options.debug: On 2010/11/05 01:22:46, scottz-goog wrote: > Validating options should not be modifying things. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode63 generate_au_zip.py:63: options.output_dir='/tmp/au-generator' On 2010/11/05 01:22:46, scottz-goog wrote: > This should be the default set in the parser setup. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode79 generate_au_zip.py:79: logging.error("Invalid option passed for dest_files_root") On 2010/11/05 01:22:46, scottz-goog wrote: > shouldn't we be dying here? Done. http://codereview.chromium.org/4425004/diff/1/3#newcode81 generate_au_zip.py:81: ldd_files = ['/usr/bin/delta_generator', On 2010/11/05 01:22:46, scottz-goog wrote: > ldd files? Is there a better name for this? they look like random binaries to > me. Added the explanation in the comments http://codereview.chromium.org/4425004/diff/1/3#newcode86 generate_au_zip.py:86: static_files = ['~/trunk/src/scripts/common.sh', On 2010/11/05 01:22:46, scottz-goog wrote: > same deal. Static files in what regard? They never change? Added the explanation in the comments http://codereview.chromium.org/4425004/diff/1/3#newcode104 generate_au_zip.py:104: all_files =ldd_files + static_files On 2010/11/05 01:22:46, scottz-goog wrote: > allfiles = ldd_files... Done. http://codereview.chromium.org/4425004/diff/1/3#newcode105 generate_au_zip.py:105: all_files = map(os.path.expanduser,all_files) On 2010/11/05 01:22:46, scottz-goog wrote: > os.path.expanduser, all_files > Done. http://codereview.chromium.org/4425004/diff/1/3#newcode113 generate_au_zip.py:113: for file_name in ldd_files: On 2010/11/05 01:22:46, scottz-goog wrote: > this whole for loop should be broken out into a function that returns the list > of files you want. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode116 generate_au_zip.py:116: stdout_data ='' On 2010/11/05 01:22:46, scottz-goog wrote: > the format is > "variable = value" > > Note the spaces. You do that in most places please do it everywhere. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode121 generate_au_zip.py:121: stderr=subprocess.PIPE ) On 2010/11/05 01:22:46, scottz-goog wrote: > align cmd and stderr= no space after PIPE) Done. http://codereview.chromium.org/4425004/diff/1/3#newcode129 generate_au_zip.py:129: library_list =[] On 2010/11/05 01:22:46, scottz-goog wrote: > library_list = [] Done. http://codereview.chromium.org/4425004/diff/1/3#newcode131 generate_au_zip.py:131: if stdout_data: On 2010/11/05 01:22:46, scottz-goog wrote: > should you not always have stdout_data from ldd if the command completes without > error? Just be extra cautious,because of ldd doesn't give structure output http://codereview.chromium.org/4425004/diff/1/3#newcode136 generate_au_zip.py:136: if library_list: On 2010/11/05 01:22:46, scottz-goog wrote: > as above shouldn't this always be set? If not aren't we in trouble already? It can happen that all the output is stripped. Like 'not a dyanamic executable' is a valid output and that gets stripped completely. http://codereview.chromium.org/4425004/diff/1/3#newcode147 generate_au_zip.py:147: for k,v in recurse_dirs.iteritems(): On 2010/11/05 01:22:46, scottz-goog wrote: > please use variables that are descriptive. Nice usage of iteritems :) Done. http://codereview.chromium.org/4425004/diff/1/3#newcode160 generate_au_zip.py:160: On 2010/11/05 01:22:46, scottz-goog wrote: > docstring? Done. http://codereview.chromium.org/4425004/diff/1/3#newcode162 generate_au_zip.py:162: if not temp_dir: On 2010/11/05 01:22:46, scottz-goog wrote: > Instead of setting a default just make it a required argument or is there a > reason to call CleanUp with None? It's a harmless non-fatal operation. Don't want the script to fail for this. if it's unmounting a mount or something, I would make not non-benign. http://codereview.chromium.org/4425004/diff/1/3#newcode165 generate_au_zip.py:165: logging.info("Removed tempdir = %s",temp_dir) On 2010/11/05 01:22:46, scottz-goog wrote: > empty lines between functions Done. http://codereview.chromium.org/4425004/diff/1/3#newcode168 generate_au_zip.py:168: return False On 2010/11/05 01:22:46, scottz-goog wrote: > same as above, if a function should never get a None value for an option then > don't set a default in the function definition. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode179 generate_au_zip.py:179: print >>sys.stderr, "Execution failed:", e On 2010/11/05 01:22:46, scottz-goog wrote: > Why are we not using logging anymore? Done. http://codereview.chromium.org/4425004/diff/1/3#newcode180 generate_au_zip.py:180: os.chdir(current_dir) On 2010/11/05 01:22:46, scottz-goog wrote: > This should be wrapped in a try: finally: so that it is always executed. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode181 generate_au_zip.py:181: return False On 2010/11/05 01:22:46, scottz-goog wrote: > Always return False? Mistake. Fixed http://codereview.chromium.org/4425004/diff/1/3#newcode184 generate_au_zip.py:184: def ExcludeBlacklist(library_list=None,black_list=None): On 2010/11/05 01:22:46, scottz-goog wrote: > docstrings Done. http://codereview.chromium.org/4425004/diff/1/3#newcode193 generate_au_zip.py:193: if re.search(pattern, library): On 2010/11/05 01:22:46, scottz-goog wrote: > no need for regex or generating a regex pattern. Use the list. I imagine the > values you are using are very exact so a > > if library in black_list: > logging..... > continue # to get out of hte for loop The values are not exact. libc.so.1 -> we only look for lib.c.so* and etc http://codereview.chromium.org/4425004/diff/1/3#newcode200 generate_au_zip.py:200: def SplitAndStrip(data=None): On 2010/11/05 01:22:46, scottz-goog wrote: > default values, docstring.. > > The docstring should ahve information of "Data example of this...." so when I > read through what you are doing I know why you are actually parsing. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode224 generate_au_zip.py:224: if not output_dir: On 2010/11/05 01:22:46, scottz-goog wrote: > defaults. > > Looking thrugh this it doesn't look like you should need any of these checks. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode242 generate_au_zip.py:242: def main(): On 2010/11/05 01:22:46, scottz-goog wrote: > spaces Done. http://codereview.chromium.org/4425004/diff/1/3#newcode247 generate_au_zip.py:247: #SetConsoleLogger() On 2010/11/05 01:22:46, scottz-goog wrote: > Remove these Done. http://codereview.chromium.org/4425004/diff/1/3#newcode256 generate_au_zip.py:256: dest_files_root = os.path.join(temp_dir,'au-generator') On 2010/11/05 01:22:46, scottz-goog wrote: > Since you only use CreateTempDir for this purpose you might as well make it a > CreateTempAuGenDir and have it return the au-generator path you want I also need the parent directory for storing the final .zip file. I don't want to generate the .zip file in the files directory.
Not to gate this, but could you either add unit tests or file a high-priority issue on yourself to add unit tests later to this script?
Mostly nits. However, one big issue you might wanna address is that you check your args way too much when I think a lot of the rest of your code assumes that you don't. i.e. you check if lists are empty or None and return (not always a False). I think with that you're gonna run into some issues down the line where you might have your code run fine but not actually do anything (returns 0) because you messed an arg somewhere where you send the wrong item. Because of that it'll be harder to debug and you won't have stack traces. So you might want to do away with all the checks. http://codereview.chromium.org/4425004/diff/6001/7002 File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/6001/7002#newcode2 generate_au_zip.py:2: # -*- coding: utf-8 -*- Don't need this coding. http://codereview.chromium.org/4425004/diff/6001/7002#newcode17 generate_au_zip.py:17: import string Don't import string and just use the string instances to call the functions http://codereview.chromium.org/4425004/diff/6001/7002#newcode20 generate_au_zip.py:20: import time You don't use time http://codereview.chromium.org/4425004/diff/6001/7002#newcode21 generate_au_zip.py:21: import tempfile tempfile is not in alphabetical order http://codereview.chromium.org/4425004/diff/6001/7002#newcode32 generate_au_zip.py:32: Generates a tempdir Generally for docstrings you just say ... Returns a tempdir i.e. return and what it does in one line http://codereview.chromium.org/4425004/diff/6001/7002#newcode37 generate_au_zip.py:37: logging.info('Using tempdir = %s', temp_dir) Never used logging before but is this the equivalent of 'Using tempdir = %s' % temp_dir? http://codereview.chromium.org/4425004/diff/6001/7002#newcode46 generate_au_zip.py:46: Returns: If no returns, don't mention. http://codereview.chromium.org/4425004/diff/6001/7002#newcode52 generate_au_zip.py:52: sys.exit(1) May want to consider throwing exceptions rather than calling sys.exit -- python prefers exceptions over fails. http://codereview.chromium.org/4425004/diff/6001/7002#newcode53 generate_au_zip.py:53: #Files that need to go through ldd Space between # and first word of comments here and all around. http://codereview.chromium.org/4425004/diff/6001/7002#newcode94 generate_au_zip.py:94: logging.error("Directory given for %s exapnded to %s doens't exist.", typo with expanded http://codereview.chromium.org/4425004/diff/6001/7002#newcode100 generate_au_zip.py:100: Two lines between top-level defs http://codereview.chromium.org/4425004/diff/6001/7002#newcode103 generate_au_zip.py:103: Genearates a list of deps for given dynamic executbles. Returns rather than generates http://codereview.chromium.org/4425004/diff/6001/7002#newcode143 generate_au_zip.py:143: def CleanUP(temp_dir): Up* http://codereview.chromium.org/4425004/diff/6001/7002#newcode148 generate_au_zip.py:148: Returns: None Unnecessary returns clause http://codereview.chromium.org/4425004/diff/6001/7002#newcode151 generate_au_zip.py:151: if not temp_dir: generally prefer if {blah} rather than an early if with a returns but it's up to you (especially since it's only two lines). Also having gone through all the code ... isn't it an error for that not to be set? http://codereview.chromium.org/4425004/diff/6001/7002#newcode159 generate_au_zip.py:159: Generates the zip file Returns true if able to generate zip file http://codereview.chromium.org/4425004/diff/6001/7002#newcode175 generate_au_zip.py:175: logging.error('Execution failed:%s', e) want to print output? http://codereview.chromium.org/4425004/diff/6001/7002#newcode196 generate_au_zip.py:196: return library_list May want to just default black_list to [] and remove both these if's to have less code. http://codereview.chromium.org/4425004/diff/6001/7002#newcode201 generate_au_zip.py:201: logging.debug('PATTERN: %s=', pattern) Might be better to compile (compiled_re = re.compile(pattern) .. compiled_re.search(library) http://codereview.chromium.org/4425004/diff/6001/7002#newcode222 generate_au_zip.py:222: Extra line http://codereview.chromium.org/4425004/diff/6001/7002#newcode253 generate_au_zip.py:253: boolean Be more explicit .. i.e. returns True when http://codereview.chromium.org/4425004/diff/6001/7002#newcode275 generate_au_zip.py:275: default=False, help='Verbose Default: "False"',) Why False in quotes? http://codereview.chromium.org/4425004/diff/6001/7002#newcode282 generate_au_zip.py:282: action='store_true', help='Keep the temp files...',) four spaces indent here http://codereview.chromium.org/4425004/diff/6001/7002#newcode304 generate_au_zip.py:304: except Exception: This try/except clause is redundant
PTAL. http://codereview.chromium.org/4425004/diff/1/3 File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/1/3#newcode185 generate_au_zip.py:185: if not library_list: On 2010/11/05 01:22:46, scottz-goog wrote: > same as above with default arguments. Do you know what I mean when I say this? > I'd be happy to discuss this with you Done. http://codereview.chromium.org/4425004/diff/1/3#newcode193 generate_au_zip.py:193: if re.search(pattern, library): On 2010/11/05 01:22:46, scottz-goog wrote: > no need for regex or generating a regex pattern. Use the list. I imagine the > values you are using are very exact so a > > if library in black_list: > logging..... > continue # to get out of hte for loop This is not an exact match to the last dot. regex is needed http://codereview.chromium.org/4425004/diff/1/3#newcode196 generate_au_zip.py:196: return_list.append(library) On 2010/11/05 01:22:46, scottz-goog wrote: > You should continue in the if statement and the else is not needed. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode200 generate_au_zip.py:200: def SplitAndStrip(data=None): On 2010/11/05 01:22:46, scottz-goog wrote: > default values, docstring.. > > The docstring should ahve information of "Data example of this...." so when I > read through what you are doing I know why you are actually parsing. Done. http://codereview.chromium.org/4425004/diff/1/3#newcode247 generate_au_zip.py:247: #SetConsoleLogger() On 2010/11/05 01:22:46, scottz-goog wrote: > Remove these Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode17 generate_au_zip.py:17: # GLOBALS On 2010/11/05 21:40:38, sosa wrote: > Don't import string and just use the string instances to call the functions Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode21 generate_au_zip.py:21: logging.basicConfig(level=logging.INFO, format=logging_format, On 2010/11/05 21:40:38, sosa wrote: > tempfile is not in alphabetical order Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode32 generate_au_zip.py:32: parser = optparse.OptionParser() On 2010/11/05 21:40:38, sosa wrote: > Generally for docstrings you just say ... Returns a tempdir i.e. return and what > it does in one line Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode37 generate_au_zip.py:37: help='Specify the output location for copying the zipfile') On 2010/11/05 21:40:38, sosa wrote: > Never used logging before but is this the equivalent of 'Using tempdir = %s' % > temp_dir? That's correct http://codereview.chromium.org/4425004/diff/6001/7002#newcode46 generate_au_zip.py:46: def ValidateOptions(options, args): On 2010/11/05 21:40:38, sosa wrote: > If no returns, don't mention. Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode53 generate_au_zip.py:53: args: args from parser objects On 2010/11/05 21:40:38, sosa wrote: > Space between # and first word of comments here and all around. Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode94 generate_au_zip.py:94: 'librt.so', On 2010/11/05 21:40:38, sosa wrote: > typo with expanded Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode100 generate_au_zip.py:100: 'libdl.so', On 2010/11/05 21:40:38, sosa wrote: > Two lines between top-level defs Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode103 generate_au_zip.py:103: On 2010/11/05 21:40:38, sosa wrote: > Returns rather than generates Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode143 generate_au_zip.py:143: for file_name in all_files: On 2010/11/05 21:40:38, sosa wrote: > Up* Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode148 generate_au_zip.py:148: logging.info("Processing directory %s",k) On 2010/11/05 21:40:38, sosa wrote: > Unnecessary returns clause Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode151 generate_au_zip.py:151: logging.error("Directory given for %s exapnded to %s doens't exist.", On 2010/11/05 21:40:38, sosa wrote: > generally prefer if {blah} rather than an early if with a returns but it's up to > you (especially since it's only two lines). > > Also having gone through all the code ... isn't it an error for that not to be > set? In this code it's an error. I am trying to make these functions generic enough, so that in the future some of these can be reused. That's why I started making those checks, since I don't know all the callers to this function. http://codereview.chromium.org/4425004/diff/6001/7002#newcode159 generate_au_zip.py:159: """ On 2010/11/05 21:40:38, sosa wrote: > Returns true if able to generate zip file Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode175 generate_au_zip.py:175: try: On 2010/11/05 21:40:38, sosa wrote: > want to print output? Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode196 generate_au_zip.py:196: return_list.append(library) On 2010/11/05 21:40:38, sosa wrote: > May want to just default black_list to [] and remove both these if's to have > less code. Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode201 generate_au_zip.py:201: if not data: On 2010/11/05 21:40:38, sosa wrote: > Might be better to compile (compiled_re = re.compile(pattern) .. > compiled_re.search(library) Done http://codereview.chromium.org/4425004/diff/6001/7002#newcode222 generate_au_zip.py:222: boolean On 2010/11/05 21:40:38, sosa wrote: > Extra line Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode253 generate_au_zip.py:253: parser.print_help() On 2010/11/05 21:40:38, sosa wrote: > Be more explicit .. i.e. returns True when Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode275 generate_au_zip.py:275: On 2010/11/05 21:40:38, sosa wrote: > Why False in quotes? Mistake Done. http://codereview.chromium.org/4425004/diff/6001/7002#newcode282 generate_au_zip.py:282: On 2010/11/05 21:40:38, sosa wrote: > four spaces indent here Done.
Few more comments http://codereview.chromium.org/4425004/diff/12001/13002 File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/12001/13002#newcode2 generate_au_zip.py:2: # -*- coding: utf-8 -*- Still don't need this http://codereview.chromium.org/4425004/diff/12001/13002#newcode30 generate_au_zip.py:30: """ Preference is to save whitespace ... so if """docstring.""" fits it one line than you should have it that way. http://codereview.chromium.org/4425004/diff/12001/13002#newcode153 generate_au_zip.py:153: True True if? http://codereview.chromium.org/4425004/diff/12001/13002#newcode163 generate_au_zip.py:163: logging.error('Execution failed:%s', e.strerror) If you don't use the output variable (output = above) then you shouldn't set it. http://codereview.chromium.org/4425004/diff/12001/13002#newcode171 generate_au_zip.py:171: def _ExcludeBlacklist(library_list, black_list): blacklist=[] as default up here?
My only nits: - unittest! - since this is a new file, let's try to follow the style/naming suggestions on our style guide: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... On Mon, Nov 8, 2010 at 3:57 PM, <sosa@chromium.org> wrote: > Few more comments > > > http://codereview.chromium.org/4425004/diff/12001/13002 > > File generate_au_zip.py (right): > > http://codereview.chromium.org/4425004/diff/12001/13002#newcode2 > > generate_au_zip.py:2: # -*- coding: utf-8 -*- > Still don't need this > > http://codereview.chromium.org/4425004/diff/12001/13002#newcode30 > generate_au_zip.py:30: """ > Preference is to save whitespace ... so if """docstring.""" fits it one > line than you should have it that way. > > http://codereview.chromium.org/4425004/diff/12001/13002#newcode153 > generate_au_zip.py:153: True > True if? > > http://codereview.chromium.org/4425004/diff/12001/13002#newcode163 > generate_au_zip.py:163: logging.error('Execution failed:%s', e.strerror) > If you don't use the output variable (output = above) then you shouldn't > set it. > > http://codereview.chromium.org/4425004/diff/12001/13002#newcode171 > generate_au_zip.py:171: def _ExcludeBlacklist(library_list, black_list): > blacklist=[] as default up here? > > > http://codereview.chromium.org/4425004/show >
Adlr: watch out for python. Chromium follows (from http://dev.chromium.org/chromium-os): Python code follows PEP-8, except: 2-space indent instead of 4-space MixedCase for method and function names (NOT lower_case_with_underscores) On Mon, Nov 8, 2010 at 4:04 PM, Andrew de los Reyes <adlr@chromium.org> wrote: > My only nits: > - unittest! > - since this is a new file, let's try to follow the style/naming suggestions > on our style guide: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... > > On Mon, Nov 8, 2010 at 3:57 PM, <sosa@chromium.org> wrote: >> >> Few more comments >> >> >> http://codereview.chromium.org/4425004/diff/12001/13002 >> File generate_au_zip.py (right): >> >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode2 >> generate_au_zip.py:2: # -*- coding: utf-8 -*- >> Still don't need this >> >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode30 >> generate_au_zip.py:30: """ >> Preference is to save whitespace ... so if """docstring.""" fits it one >> line than you should have it that way. >> >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode153 >> generate_au_zip.py:153: True >> True if? >> >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode163 >> generate_au_zip.py:163: logging.error('Execution failed:%s', e.strerror) >> If you don't use the output variable (output = above) then you shouldn't >> set it. >> >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode171 >> generate_au_zip.py:171: def _ExcludeBlacklist(library_list, black_list): >> blacklist=[] as default up here? >> >> http://codereview.chromium.org/4425004/show > >
Ah, good to know. sorry. -andrew On Mon, Nov 8, 2010 at 4:30 PM, Chris Sosa <sosa@chromium.org> wrote: > Adlr: watch out for python. Chromium follows (from > http://dev.chromium.org/chromium-os): > > Python code follows PEP-8, except: > 2-space indent instead of 4-space > MixedCase for method and function names (NOT lower_case_with_underscores) > > > > On Mon, Nov 8, 2010 at 4:04 PM, Andrew de los Reyes <adlr@chromium.org> > wrote: > > My only nits: > > - unittest! > > - since this is a new file, let's try to follow the style/naming > suggestions > > on our style guide: > > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... > > > > On Mon, Nov 8, 2010 at 3:57 PM, <sosa@chromium.org> wrote: > >> > >> Few more comments > >> > >> > >> http://codereview.chromium.org/4425004/diff/12001/13002 > >> File generate_au_zip.py (right): > >> > >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode2 > >> generate_au_zip.py:2: # -*- coding: utf-8 -*- > >> Still don't need this > >> > >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode30 > >> generate_au_zip.py:30: """ > >> Preference is to save whitespace ... so if """docstring.""" fits it one > >> line than you should have it that way. > >> > >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode153 > >> generate_au_zip.py:153: True > >> True if? > >> > >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode163 > >> generate_au_zip.py:163: logging.error('Execution failed:%s', e.strerror) > >> If you don't use the output variable (output = above) then you shouldn't > >> set it. > >> > >> http://codereview.chromium.org/4425004/diff/12001/13002#newcode171 > >> generate_au_zip.py:171: def _ExcludeBlacklist(library_list, black_list): > >> blacklist=[] as default up here? > >> > >> http://codereview.chromium.org/4425004/show > > > > >
Almost there :) http://codereview.chromium.org/4425004/diff/1/3 File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/1/3#newcode131 generate_au_zip.py:131: if stdout_data: On 2010/11/05 18:58:01, Raja Aluri wrote: > On 2010/11/05 01:22:46, scottz-goog wrote: > > should you not always have stdout_data from ldd if the command completes > without > > error? > Just be extra cautious,because of ldd doesn't give structure output > Then shouldn't you die if you don't have output? It isn't really being cautious but adding ambiguity to your flow. http://codereview.chromium.org/4425004/diff/1/3#newcode136 generate_au_zip.py:136: if library_list: On 2010/11/05 18:58:01, Raja Aluri wrote: > On 2010/11/05 01:22:46, scottz-goog wrote: > > as above shouldn't this always be set? If not aren't we in trouble already? > It can happen that all the output is stripped. > Like 'not a dyanamic executable' is a valid output and that gets stripped > completely. > If that is the case you should add an if statement higher that checks for that and acts accordingly. i.e if not stdout_data: return... http://codereview.chromium.org/4425004/diff/1/3#newcode162 generate_au_zip.py:162: if not temp_dir: On 2010/11/05 18:58:01, Raja Aluri wrote: > On 2010/11/05 01:22:46, scottz-goog wrote: > > Instead of setting a default just make it a required argument or is there a > > reason to call CleanUp with None? > It's a harmless non-fatal operation. Don't want the script to fail for this. if > it's unmounting a mount or something, I would make not non-benign. > If it is harmless then you should be running it all the time, it is definitely an error if you are not sending a variable required for the function to work. http://codereview.chromium.org/4425004/diff/12001/13002#newcode9 generate_au_zip.py:9: import os No spaces needed here and might as well end with a period. http://codereview.chromium.org/4425004/diff/12001/13002#newcode48 generate_au_zip.py:48: Validates the set of given options on the command line before executing the Is there any other general name we can give these? support_programs it just seems like a misnomer to call the list static_files. I know this is nit picking so i'll leave it up to you but two of the 3 files are scripts not statically linked files. http://codereview.chromium.org/4425004/diff/12001/13002#newcode53 generate_au_zip.py:53: args: args from parser objects Is this ever going to be more than this one path? Right now we do a for loop over it and it has one item. http://codereview.chromium.org/4425004/diff/12001/13002#newcode76 generate_au_zip.py:76: ' '.join(all_files) ? This avoids putting a list output in the log message. http://codereview.chromium.org/4425004/diff/12001/13002#newcode82 generate_au_zip.py:82: '/usr/bin/bsdiff', no parans necessary here others can disagree but in my experience it is generally written as for source_dir, target_dir in .... http://codereview.chromium.org/4425004/diff/12001/13002#newcode91 generate_au_zip.py:91: shutil.copytree(src, dst, symlinks=False, ignore=None) I assume you want symlinks=False, it looks like we are using the defaults lets just drop the added options. http://codereview.chromium.org/4425004/diff/12001/13002#newcode94 generate_au_zip.py:94: 'librt.so', Move this above any function that calls it please. It is a general convention that keeps the flow of the file consistent. http://codereview.chromium.org/4425004/diff/12001/13002#newcode94 generate_au_zip.py:94: 'librt.so', (ldd_files, black_list) http://codereview.chromium.org/4425004/diff/12001/13002#newcode121 generate_au_zip.py:121: stderr=subprocess.PIPE ) if not stdout_data: Error/Do whatever is proper. Then remove all the other if checks as they should pass assuming we have a stdout_data. http://codereview.chromium.org/4425004/diff/12001/13002#newcode181 generate_au_zip.py:181: return False r'|'.join(black_list)? http://codereview.chromium.org/4425004/diff/12001/13002#newcode196 generate_au_zip.py:196: return_list.append(library) Should be above any functions that call it. http://codereview.chromium.org/4425004/diff/12001/13002#newcode210 generate_au_zip.py:210: continue Why should this ever be called with a None value? http://codereview.chromium.org/4425004/diff/12001/13002#newcode233 generate_au_zip.py:233: logging.error("Zip file %s doesn't exist. Returning False",zip_file_name) False if it fails... http://codereview.chromium.org/4425004/diff/12001/13002#newcode248 generate_au_zip.py:248: #logging.info('Logging to Console...') Docstring all on one line. http://codereview.chromium.org/4425004/diff/12001/13002#newcode254 generate_au_zip.py:254: sys.exit(1) ('-d' Also properly align default with the rest of the arguments. http://codereview.chromium.org/4425004/diff/12001/13002#newcode265 generate_au_zip.py:265: Generally for clarity it is good to separate if statements with spaces after them. http://codereview.chromium.org/4425004/diff/12001/13002#newcode271 generate_au_zip.py:271: this is no longer a keyword option as defined above. http://codereview.chromium.org/4425004/diff/12001/13002#newcode273 generate_au_zip.py:273: also no longer keyword options. http://codereview.chromium.org/4425004/diff/12001/13002#newcode275 generate_au_zip.py:275: same.. http://codereview.chromium.org/4425004/diff/12001/13002#newcode277 generate_au_zip.py:277: same.. http://codereview.chromium.org/4425004/diff/12001/13002#newcode282 generate_au_zip.py:282: sys.exit(0) is not necessary the exit code will be zero if you get here.
PTAL http://codereview.chromium.org/4425004/diff/1/generate_au_zip.py File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/1/generate_au_zip.py#newcode136 generate_au_zip.py:136: if library_list: On 2010/11/09 03:52:50, scottz-goog wrote: > On 2010/11/05 18:58:01, Raja Aluri wrote: > > On 2010/11/05 01:22:46, scottz-goog wrote: > > > as above shouldn't this always be set? If not aren't we in trouble already? > > It can happen that all the output is stripped. > > Like 'not a dyanamic executable' is a valid output and that gets stripped > > completely. > > > > If that is the case you should add an if statement higher that checks for that > and acts accordingly. i.e > if not stdout_data: > return... It can become empty after the _SplitAndStrip and still not an error. http://codereview.chromium.org/4425004/diff/1/generate_au_zip.py#newcode136 generate_au_zip.py:136: if library_list: On 2010/11/09 03:52:50, scottz-goog wrote: > On 2010/11/05 18:58:01, Raja Aluri wrote: > > On 2010/11/05 01:22:46, scottz-goog wrote: > > > as above shouldn't this always be set? If not aren't we in trouble already? > > It can happen that all the output is stripped. > > Like 'not a dyanamic executable' is a valid output and that gets stripped > > completely. > > > > If that is the case you should add an if statement higher that checks for that > and acts accordingly. i.e > if not stdout_data: > return... Done. http://codereview.chromium.org/4425004/diff/1/generate_au_zip.py#newcode162 generate_au_zip.py:162: if not temp_dir: On 2010/11/09 03:52:50, scottz-goog wrote: > On 2010/11/05 18:58:01, Raja Aluri wrote: > > On 2010/11/05 01:22:46, scottz-goog wrote: > > > Instead of setting a default just make it a required argument or is there > a > > > reason to call CleanUp with None? > > It's a harmless non-fatal operation. Don't want the script to fail for this. > if > > it's unmounting a mount or something, I would make not non-benign. > > > > If it is harmless then you should be running it all the time, it is definitely > an error if you are not sending a variable required for the function to work. > Changed to if os.path.exists(temp_dir): I should have done this check regardless. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode48 generate_au_zip.py:48: Validates the set of given options on the command line before executing the On 2010/11/09 03:52:50, scottz-goog wrote: > Is there any other general name we can give these? support_programs it just > seems like a misnomer to call the list static_files. I know this is nit picking > so i'll leave it up to you but two of the 3 files are scripts not statically > linked files. Lack of a better name, I can call it non_ldd_files. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode53 generate_au_zip.py:53: args: args from parser objects On 2010/11/09 03:52:50, scottz-goog wrote: > Is this ever going to be more than this one path? Right now we do a for loop > over it and it has one item. For now it's only one directory, but in the future if you want to add one more, you don't need to change the code for copying http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode82 generate_au_zip.py:82: '/usr/bin/bsdiff', On 2010/11/09 03:52:50, scottz-goog wrote: > no parans necessary here others can disagree but in my experience it is > generally written as for source_dir, target_dir in .... > Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode91 generate_au_zip.py:91: On 2010/11/09 03:52:50, scottz-goog wrote: > shutil.copytree(src, dst, symlinks=False, ignore=None) > > I assume you want symlinks=False, it looks like we are using the defaults lets > just drop the added options. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode94 generate_au_zip.py:94: 'librt.so', On 2010/11/09 03:52:50, scottz-goog wrote: > (ldd_files, black_list) Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode94 generate_au_zip.py:94: 'librt.so', On 2010/11/09 03:52:50, scottz-goog wrote: > Move this above any function that calls it please. It is a general convention > that keeps the flow of the file consistent. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode181 generate_au_zip.py:181: return False On 2010/11/09 03:52:50, scottz-goog wrote: > r'|'.join(black_list)? Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode196 generate_au_zip.py:196: return_list.append(library) On 2010/11/09 03:52:50, scottz-goog wrote: > Should be above any functions that call it. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode210 generate_au_zip.py:210: continue On 2010/11/09 03:52:50, scottz-goog wrote: > Why should this ever be called with a None value? Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode233 generate_au_zip.py:233: logging.error("Zip file %s doesn't exist. Returning False",zip_file_name) On 2010/11/09 03:52:50, scottz-goog wrote: > False if it fails... Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode248 generate_au_zip.py:248: #logging.info('Logging to Console...') On 2010/11/09 03:52:50, scottz-goog wrote: > Docstring all on one line. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode254 generate_au_zip.py:254: sys.exit(1) On 2010/11/09 03:52:50, scottz-goog wrote: > ('-d' > Also properly align default with the rest of the arguments. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode265 generate_au_zip.py:265: On 2010/11/09 03:52:50, scottz-goog wrote: > Generally for clarity it is good to separate if statements with spaces after > them. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode271 generate_au_zip.py:271: On 2010/11/09 03:52:50, scottz-goog wrote: > this is no longer a keyword option as defined above. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode273 generate_au_zip.py:273: On 2010/11/09 03:52:50, scottz-goog wrote: > also no longer keyword options. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode277 generate_au_zip.py:277: On 2010/11/09 03:52:50, scottz-goog wrote: > same.. Done. http://codereview.chromium.org/4425004/diff/12001/generate_au_zip.py#newcode282 generate_au_zip.py:282: On 2010/11/09 03:52:50, scottz-goog wrote: > sys.exit(0) is not necessary the exit code will be zero if you get here. Done.
One question though is that you realize if I provide no arguments this script will run right? Is that the intended operation? After you fix the below ones go ahead and submit the script. LGTM http://codereview.chromium.org/4425004/diff/26001/generate_au_zip.py File generate_au_zip.py (right): http://codereview.chromium.org/4425004/diff/26001/generate_au_zip.py#newcode137 generate_au_zip.py:137: logging.debug('Given files that need to be copied = %s' .join(all_files)) % ' '.join(all_files) http://codereview.chromium.org/4425004/diff/26001/generate_au_zip.py#newcode177 generate_au_zip.py:177: stdout=subprocess.PIPE).communicate()[0] align stdout with ['z.. |
