Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827037:
Add --fast to build_image (Closed)
|
Created:
10 years, 5 months ago by Nick Sanders Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Base URL:
ssh://gitrw.chromium.org/crosutils.git Visibility:
Public. |
DescriptionAdd --fast to build_image
Include checked in parallel emerge,
with an optional (default false) argument
in build_image to turn it on.
BUG=None
TEST="Run build_image --fast"
Patch Set 1 #Patch Set 2 : 80 char #
Total comments: 72
Patch Set 3 : lint a lot #Patch Set 4 : untypo #Patch Set 5 : todo #Patch Set 6 : .. #
Total comments: 7
Messages
Total messages: 9 (0 generated)
Please run pylint on parallel_emerge http://codereview.chromium.org/2827037/diff/2001/3002 File parallel_emerge (right): http://codereview.chromium.org/2827037/diff/2001/3002#newcode39 parallel_emerge:39: import sys, os, subprocess, shlex, time, re, commands, tempfile alphabetize and put on separate lines http://codereview.chromium.org/2827037/diff/2001/3002#newcode41 parallel_emerge:41: def usage(): These function names do not follow python style. Please rename to starting with capital letters. http://codereview.chromium.org/2827037/diff/2001/3002#newcode52 parallel_emerge:52: # Globals: package we are building, board we are targetting, misspelling of targeting http://codereview.chromium.org/2827037/diff/2001/3002#newcode55 parallel_emerge:55: emerge_args = '' Use consistent "" or '' not an intermix http://codereview.chromium.org/2827037/diff/2001/3002#newcode62 parallel_emerge:62: two lines before top-level function declarations http://codereview.chromium.org/2827037/diff/2001/3002#newcode64 parallel_emerge:64: # Is there an open source python flags lib? You can use gflags after adding it to the chroot. I'm planning on adding it myself for another script in another CL http://codereview.chromium.org/2827037/diff/2001/3002#newcode78 parallel_emerge:78: board_arg = m.group('board') Use if / else rather than continue http://codereview.chromium.org/2827037/diff/2001/3002#newcode93 parallel_emerge:93: two lines before top-level function declarations http://codereview.chromium.org/2827037/diff/2001/3002#newcode98 parallel_emerge:98: if board != None: if board http://codereview.chromium.org/2827037/diff/2001/3002#newcode117 parallel_emerge:117: while (depsproc.poll() == None): while not descproc.poll() http://codereview.chromium.org/2827037/diff/2001/3002#newcode118 parallel_emerge:118: seconds = seconds + 1 += 1 http://codereview.chromium.org/2827037/diff/2001/3002#newcode144 parallel_emerge:144: two lines before top-level function declarations http://codereview.chromium.org/2827037/diff/2001/3002#newcode145 parallel_emerge:145: # Regex the emerge --tree output to generate a nested dict of dependencies. Note that this code and cros_extract_deps -> ParseDepLines seem very similiar. Reuse would be great. http://codereview.chromium.org/2827037/diff/2001/3002#newcode147 parallel_emerge:147: # lines: text dump from 'emerge -p --tree package' Two line indentations http://codereview.chromium.org/2827037/diff/2001/3002#newcode253 parallel_emerge:253: two lines before top-level function declarations http://codereview.chromium.org/2827037/diff/2001/3002#newcode261 parallel_emerge:261: two lines before top-level function declarations http://codereview.chromium.org/2827037/diff/2001/3002#newcode272 parallel_emerge:272: thispkg = deps_map.setdefault(pkg, {'needs':{}, 'provides':set(), 'action': 'nomerge'}) 80 lines http://codereview.chromium.org/2827037/diff/2001/3002#newcode272 parallel_emerge:272: thispkg = deps_map.setdefault(pkg, {'needs':{}, 'provides':set(), 'action': 'nomerge'}) this_pkg http://codereview.chromium.org/2827037/diff/2001/3002#newcode277 parallel_emerge:277: deppkg = deps_map[dep] _'s in variable http://codereview.chromium.org/2827037/diff/2001/3002#newcode348 parallel_emerge:348: self._emerge_queue = [x for x in deps_map if len(deps_map[x]['needs']) == 0 ] 80 chars http://codereview.chromium.org/2827037/diff/2001/3002#newcode411 parallel_emerge:411: while len(self._deps_map) > 0: see other comments about arrays http://codereview.chromium.org/2827037/diff/2001/3002#newcode413 parallel_emerge:413: if len(self._emerge_queue) > 0: if self._emerge_queue http://codereview.chromium.org/2827037/diff/2001/3002#newcode435 parallel_emerge:435: if (len(self._emerge_queue) == 0 and I believe instead of checking against len == 0 you can just check self._emerge_queue etc i.e. if self._emerge_equeue and ... http://codereview.chromium.org/2827037/diff/2001/3002#newcode445 parallel_emerge:445: if job.poll() != None: if job.poll() http://codereview.chromium.org/2827037/diff/2001/3002#newcode466 parallel_emerge:466: Only two lines between end of function
http://codereview.chromium.org/2827037/diff/2001/3002 File parallel_emerge (right): http://codereview.chromium.org/2827037/diff/2001/3002#newcode49 parallel_emerge:49: secret_deps = {'dev-java/bcel': 'dev-java/java-config', Is this one still necessary, now that we get a valid dependency tree from emerge? I think we can remove this now because dev-java/bcel explicitly depends on dev-java/java-config. We might want to deprecate secret deps in favor of just adding dependencies to the package that needs the dependencies. I do admit the secret deps are useful for debugging, though (I've used it). http://codereview.chromium.org/2827037/diff/2001/3002#newcode64 parallel_emerge:64: # Is there an open source python flags lib? optparse is standard and is included with python itself. Don't know if it meets your needs here. http://codereview.chromium.org/2827037/diff/2001/3002#newcode74 parallel_emerge:74: if arg[0] == '-' or arg == 'y': Nice hack to make --with-bdeps y parse correctly. But this breaks on --with-bdeps n and probably other things too. Worth a TODO. http://codereview.chromium.org/2827037/diff/2001/3002#newcode256 parallel_emerge:256: def print_tree(deps, depth = ""): s/ = /=/ (default arguments have no spaces around the equal sign) http://codereview.chromium.org/2827037/diff/2001/3002#newcode281 parallel_emerge:281: thispkg['provides'].add(dep) I think we want to just skip over runtime_post dependencies altogether, since ordering of runtime_post dependencies doesn't matter (they can merge before or after -- it doesn't matter when they are merged). See http://help.lockergnome.com/linux/gentoo-dev-PDEPEND-behaviour--ftopict483842... http://codereview.chromium.org/2827037/diff/2001/3002#newcode304 parallel_emerge:304: sanitize_dep(dep, dep, [dep], 8) As we discussed, we should plan on replacing this circular dep fixer with something that looks at the order that portage merges the packages and eliminates any dependencies that contravene that ordering. Worth a TODO. When we implement the better circular dep fixer, it'll also print us a list of bogus dependencies, so we can see what dependencies are treated as optional by portage. http://codereview.chromium.org/2827037/diff/2001/3002#newcode363 parallel_emerge:363: if target.find("original-") != -1: target.startswith("original-") ? http://codereview.chromium.org/2827037/diff/2001/3002#newcode364 parallel_emerge:364: # "orignial-" signifies one of the packages we originally requiested. s/orignial/original/ s/requiested/requested/ http://codereview.chromium.org/2827037/diff/2001/3002#newcode368 parallel_emerge:368: # an actual install. --select doesn't do what we want here. It still installs the package, so we should add a TODO here. http://codereview.chromium.org/2827037/diff/2001/3002#newcode383 parallel_emerge:383: portage_env=os.environ.copy() s/=/ = /
http://codereview.chromium.org/2827037/diff/2001/3002 File parallel_emerge (right): http://codereview.chromium.org/2827037/diff/2001/3002#newcode368 parallel_emerge:368: # an actual install. On 2010/06/30 06:48:51, davidjames wrote: > --select doesn't do what we want here. It still installs the package, so we > should add a TODO here. I think --noreplace is what you want here. With --noreplace it doesn't install the package if it's already installed, but it does update world. --select is the default, but it doesn't hurt. --nodeps might be handy to use here too if it improves the speed (and avoids confusion where emerge tries to maybe install dependencies that you've already taken care of).
http://codereview.chromium.org/2827037/diff/2001/3001 File build_image (right): http://codereview.chromium.org/2827037/diff/2001/3001#newcode81 build_image:81: EMERGE_BOARD_CMD="$(dirname $0)/parallel_emerge --board=${FLAGS_board}" Convention is: "${SCRIPTS_DIR}"/parallel_emerge
Also added delayed retry from David's patch. http://codereview.chromium.org/2827037/diff/2001/3001 File build_image (right): http://codereview.chromium.org/2827037/diff/2001/3001#newcode81 build_image:81: EMERGE_BOARD_CMD="$(dirname $0)/parallel_emerge --board=${FLAGS_board}" On 2010/06/30 16:28:20, Mandeep Singh Baines wrote: > Convention is: > > "${SCRIPTS_DIR}"/parallel_emerge Done. http://codereview.chromium.org/2827037/diff/2001/3002 File parallel_emerge (right): http://codereview.chromium.org/2827037/diff/2001/3002#newcode39 parallel_emerge:39: import sys, os, subprocess, shlex, time, re, commands, tempfile On 2010/06/30 06:22:13, sosa wrote: > alphabetize and put on separate lines Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode41 parallel_emerge:41: def usage(): On 2010/06/30 06:22:13, sosa wrote: > These function names do not follow python style. Please rename to starting with > capital letters. Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode49 parallel_emerge:49: secret_deps = {'dev-java/bcel': 'dev-java/java-config', On 2010/06/30 06:48:51, davidjames wrote: > Is this one still necessary, now that we get a valid dependency tree from > emerge? > > I think we can remove this now because dev-java/bcel explicitly depends on > dev-java/java-config. > > We might want to deprecate secret deps in favor of just adding dependencies to > the package that needs the dependencies. I do admit the secret deps are useful > for debugging, though (I've used it). Adding deps to packages is not usually easy to try, since many are pulled from external sources. Might be good to keep it for debugging. http://codereview.chromium.org/2827037/diff/2001/3002#newcode52 parallel_emerge:52: # Globals: package we are building, board we are targetting, On 2010/06/30 06:22:13, sosa wrote: > misspelling of targeting Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode55 parallel_emerge:55: emerge_args = '' On 2010/06/30 06:22:13, sosa wrote: > Use consistent "" or '' not an intermix Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode62 parallel_emerge:62: On 2010/06/30 06:22:13, sosa wrote: > two lines before top-level function declarations Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode64 parallel_emerge:64: # Is there an open source python flags lib? On 2010/06/30 06:48:51, davidjames wrote: > optparse is standard and is included with python itself. Don't know if it meets > your needs here. currently parallel_emerge has to conform to emerge's somewhat irregular argument format. http://codereview.chromium.org/2827037/diff/2001/3002#newcode74 parallel_emerge:74: if arg[0] == '-' or arg == 'y': On 2010/06/30 06:48:51, davidjames wrote: > Nice hack to make --with-bdeps y parse correctly. But this breaks on > --with-bdeps n and probably other things too. Worth a TODO. According to the docs, --with-bdeps is the only non dashed arg, so adding a "n" should fix this. http://codereview.chromium.org/2827037/diff/2001/3002#newcode78 parallel_emerge:78: board_arg = m.group('board') On 2010/06/30 06:22:13, sosa wrote: > Use if / else rather than continue Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode93 parallel_emerge:93: On 2010/06/30 06:22:13, sosa wrote: > two lines before top-level function declarations Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode98 parallel_emerge:98: if board != None: On 2010/06/30 06:22:13, sosa wrote: > if board Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode117 parallel_emerge:117: while (depsproc.poll() == None): On 2010/06/30 06:22:13, sosa wrote: > while not descproc.poll() Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode118 parallel_emerge:118: seconds = seconds + 1 On 2010/06/30 06:22:13, sosa wrote: > += 1 Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode144 parallel_emerge:144: On 2010/06/30 06:22:13, sosa wrote: > two lines before top-level function declarations Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode145 parallel_emerge:145: # Regex the emerge --tree output to generate a nested dict of dependencies. On 2010/06/30 06:22:13, sosa wrote: > Note that this code and cros_extract_deps -> ParseDepLines seem very similiar. > Reuse would be great. Hmm, I hadn't realized my code got reused before I checked it in.. It looks like they are not quite close enough to directly put the code in a library and call it, but I'll make a depsfetching library in another CL, and direct both programs to import it. http://codereview.chromium.org/2827037/diff/2001/3002#newcode147 parallel_emerge:147: # lines: text dump from 'emerge -p --tree package' On 2010/06/30 06:22:13, sosa wrote: > Two line indentations Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode253 parallel_emerge:253: On 2010/06/30 06:22:13, sosa wrote: > two lines before top-level function declarations Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode256 parallel_emerge:256: def print_tree(deps, depth = ""): On 2010/06/30 06:48:51, davidjames wrote: > s/ = /=/ (default arguments have no spaces around the equal sign) Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode261 parallel_emerge:261: On 2010/06/30 06:22:13, sosa wrote: > two lines before top-level function declarations Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode272 parallel_emerge:272: thispkg = deps_map.setdefault(pkg, {'needs':{}, 'provides':set(), 'action': 'nomerge'}) On 2010/06/30 06:22:13, sosa wrote: > 80 lines Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode272 parallel_emerge:272: thispkg = deps_map.setdefault(pkg, {'needs':{}, 'provides':set(), 'action': 'nomerge'}) On 2010/06/30 06:22:13, sosa wrote: > this_pkg Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode277 parallel_emerge:277: deppkg = deps_map[dep] On 2010/06/30 06:22:13, sosa wrote: > _'s in variable Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode281 parallel_emerge:281: thispkg['provides'].add(dep) On 2010/06/30 06:48:51, davidjames wrote: > I think we want to just skip over runtime_post dependencies altogether, since > ordering of runtime_post dependencies doesn't matter (they can merge before or > after -- it doesn't matter when they are merged). See > http://help.lockergnome.com/linux/gentoo-dev-PDEPEND-behaviour--ftopict483842... Still sounds like a lively debate on the thread, and it's not an issue for build_image.. Maybe we can do best effort PDEPEND, and just break cycles on PDEPEND? http://codereview.chromium.org/2827037/diff/2001/3002#newcode304 parallel_emerge:304: sanitize_dep(dep, dep, [dep], 8) On 2010/06/30 06:48:51, davidjames wrote: > As we discussed, we should plan on replacing this circular dep fixer with > something that looks at the order that portage merges the packages and > eliminates any dependencies that contravene that ordering. Worth a TODO. > > When we implement the better circular dep fixer, it'll also print us a list of > bogus dependencies, so we can see what dependencies are treated as optional by > portage. TODO added. http://codereview.chromium.org/2827037/diff/2001/3002#newcode348 parallel_emerge:348: self._emerge_queue = [x for x in deps_map if len(deps_map[x]['needs']) == 0 ] On 2010/06/30 06:22:13, sosa wrote: > 80 chars Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode363 parallel_emerge:363: if target.find("original-") != -1: On 2010/06/30 06:48:51, davidjames wrote: > target.startswith("original-") ? Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode364 parallel_emerge:364: # "orignial-" signifies one of the packages we originally requiested. On 2010/06/30 06:48:51, davidjames wrote: > s/orignial/original/ > s/requiested/requested/ Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode368 parallel_emerge:368: # an actual install. On 2010/06/30 07:00:02, davidjames wrote: > On 2010/06/30 06:48:51, davidjames wrote: > > --select doesn't do what we want here. It still installs the package, so we > > should add a TODO here. > I think --noreplace is what you want here. With --noreplace it doesn't install > the package if it's already installed, but it does update world. > > --select is the default, but it doesn't hurt. --nodeps might be handy to use > here too if it improves the speed (and avoids confusion where emerge tries to > maybe install dependencies that you've already taken care of). added --noreplace --nodeps. Thanks for catching nodeps, it was supposed to be there! http://codereview.chromium.org/2827037/diff/2001/3002#newcode383 parallel_emerge:383: portage_env=os.environ.copy() On 2010/06/30 06:48:51, davidjames wrote: > s/=/ = / Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode411 parallel_emerge:411: while len(self._deps_map) > 0: On 2010/06/30 06:22:13, sosa wrote: > see other comments about arrays Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode413 parallel_emerge:413: if len(self._emerge_queue) > 0: On 2010/06/30 06:22:13, sosa wrote: > if self._emerge_queue Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode435 parallel_emerge:435: if (len(self._emerge_queue) == 0 and On 2010/06/30 06:22:13, sosa wrote: > I believe instead of checking against len == 0 you can just check > self._emerge_queue etc i.e. if self._emerge_equeue and ... Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode445 parallel_emerge:445: if job.poll() != None: On 2010/06/30 06:22:13, sosa wrote: > if job.poll() Done. http://codereview.chromium.org/2827037/diff/2001/3002#newcode466 parallel_emerge:466: On 2010/06/30 06:22:13, sosa wrote: > Only two lines between end of function Done.
LGTM! Verified that build_packages --nousepkg --fast succeeds with this change.
FYI: When you have multiple reviewers that have made comments, please do not commit until you get LGTM's from them all or a long time has passed. On Wed, Jun 30, 2010 at 11:20 PM, <davidjames@chromium.org> wrote: > LGTM! > > Verified that build_packages --nousepkg --fast succeeds with this change. > > http://codereview.chromium.org/2827037/show >
Should be addressed in a sep CL http://codereview.chromium.org/2827037/diff/22001/23002 File parallel_emerge (right): http://codereview.chromium.org/2827037/diff/22001/23002#newcode1 parallel_emerge:1: #!/usr/bin/python2.6 Please refactor this to cros_parallel_emerge and modify build_image accordingly. http://codereview.chromium.org/2827037/diff/22001/23002#newcode44 parallel_emerge:44: import os These are not in alphabetical order. http://codereview.chromium.org/2827037/diff/22001/23002#newcode70 parallel_emerge:70: # environment veriables. variables* http://codereview.chromium.org/2827037/diff/22001/23002#newcode480 parallel_emerge:480: """Mark a target as completed and unblock dependecies.""" misspelling of dependencies http://codereview.chromium.org/2827037/diff/22001/23002#newcode586 parallel_emerge:586: # Main control code. Re-write to a main function http://codereview.chromium.org/2827037/diff/22001/23002#newcode588 parallel_emerge:588: PACKAGE, EMERGE_ARGS, BOARD = ParseArgs(sys.argv) I do not think it's good style or necessary to make all these globals http://codereview.chromium.org/2827037/diff/22001/23002#newcode610 parallel_emerge:610: Use standard if name == '__main__' construct |
