Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6005004:
WIP Chromite supporting "LEGACY" mode, as requested by Anush. (Closed)
|
Created:
10 years ago by diandersAtChromium Modified:
9 years, 6 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
DescriptionWIP Chromite supporting "LEGACY" mode, as requested by Anush.
This version of chromite supports the following features:
1. Can be run from inside or outside the chroot.
2. CWD can be anything when chromite is run.
3. The chromite.sh script can be installed to the root of the
checkout (using repo's copyfile mechanism) so we can run
without needing to be in src/scripts
4. Current commands are build, clean, distclean, ebuild,
emerge, equery, portageq, shell, workon.
5. Build spec files can be named on command line using any
substring.
6. Build commands can be named on command line using any
prefix.
7. Menu driven for easy discovery.
8. Chroot specs can be specified for any command with --chroot,
which must be the first thing on the command line (so it's
not interpreted as the arg to sub commands). By default, the
'chroot.spec' is used.
BUG=chromium-os:10556
TEST=Ran chromite_unittest.py; played w/ various commands.
Patch Set 1 #
Total comments: 1
Patch Set 2 : Reverted accidentally checked in file. #Patch Set 3 : Renamed chromite to have .py and added symlink. #Patch Set 4 : Use Info instead of printing to stderr. #
Total comments: 68
Patch Set 5 : Adress sosa's code review, except for unit tests. #Patch Set 6 : Fixed pylint errors. #Patch Set 7 : First pass at unit tests, plus other small fixes. #Patch Set 8 : Major changes after Anush review; fixed bug creating chroot. #Patch Set 9 : Fixed unit tests w/ new chromite changes. #Patch Set 10 : Removed 'HOST' option when looking for chroot specs. #
Total comments: 70
Patch Set 11 : Fixes for sosa's code review comments. #
Total comments: 2
Patch Set 12 : More sosa feedback; put dummy sections in specs for anush; fixed unit tests. #
Messages
Total messages: 22 (0 generated)
Anush, Here's a first pass on the 'LEGACY' support to Chromite. PTAL. I'm sure that there's a lot more that can be done, but hopefully this at least sets up a baseline. Since there's a lot of code here, I'm happy to sit down and walk through it with you. Unless that happens this afternoon, it will have to wait until Jan 4th.
Few drive-by comments (I can do a fuller review if you want). Please add unit tests. If you don't know, you may never (as we've seen with many other py code). Check-in chromite.py and symlink chromite to chromite.py so it can be imported. Do not use print >> sys.stderr ... Use Info or Warn instead. Why no bug or test? http://codereview.chromium.org/6005004/diff/1/chromite/chromite.sh File chromite/chromite.sh (right): http://codereview.chromium.org/6005004/diff/1/chromite/chromite.sh#newcode15 chromite/chromite.sh:15: exec $MY_DIR/src/scripts/chromite/chromite "$@" I find this kinda awkward. Why in the root ?
Also, I'd run gpylint on this and where applicable modify your code to somewhat conform to it.
Chris, Thanks! Feel free to do a more thorough code review. There's a lot of code, so I'm happy to walk you through it too. * Yes, you're right. I need to add unit tests. I will do that before officially submitting. * I can do the chromite.py w/ chromite as a symlink. I didn't do that because I didn't create this file, but now's the chance to fix it, right! ;) Done. * No "print >> sys.stderr". Done, except in case of menus/prompts. * No bug number for this--it was requested by Anush. I can make a bug if you want. I did adhoc testing, but have not done formal testing yet since this is a bit of a WIP. * gpylint conforms pretty well. There were a few places I couldn't make it happy. I'll talk to you and see if you have any ideas of what I can do to address those that are left. On 2010/12/22 22:02:09, sosa wrote: > Also, I'd run gpylint on this and where applicable modify your code to somewhat > conform to it.
I opened 10556 for this work. Chris - please do give it a full review. Thanks On Wed, Dec 22, 2010 at 2:23 PM, <dianders@chromium.org> wrote: > Chris, > > Thanks! Feel free to do a more thorough code review. There's a lot of > code, so > I'm happy to walk you through it too. > > * Yes, you're right. I need to add unit tests. I will do that before > officially submitting. > > * I can do the chromite.py w/ chromite as a symlink. I didn't do that > because I > didn't create this file, but now's the chance to fix it, right! ;) Done. > > * No "print >> sys.stderr". Done, except in case of menus/prompts. > > * No bug number for this--it was requested by Anush. I can make a bug if > you > want. I did adhoc testing, but have not done formal testing yet since this > is a > bit of a WIP. > > * gpylint conforms pretty well. There were a few places I couldn't make it > happy. I'll talk to you and see if you have any ideas of what I can do to > address those that are left. > > > On 2010/12/22 22:02:09, sosa wrote: > >> Also, I'd run gpylint on this and where applicable modify your code to >> > somewhat > >> conform to it. >> > > > > > http://codereview.chromium.org/6005004/ >
Haven't had a chance to look at TextMenu yet. But here is my initial thoughts. Also a meta thought: Why even bother doing chroot dirs? Do ppl ever do this? Would you expect them to hand edit these specs in order to specify chroot? If anything you'd prob add it to options parser rather than the config http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode9 chromite/chromite.py:9: import base64 Maybe sort ascii ... Capitals first, lowercase second. Preference though http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode11 chromite/chromite.py:11: import cPickle as pickle it's preferred not to rename. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode25 chromite/chromite.py:25: _SRCROOT_PATH = os.path.realpath(os.path.join(_CHROMITE_PATH, can't you always do this? Just just use the else. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode27 chromite/chromite.py:27: sys.path.insert(0, os.path.join(_CHROMITE_PATH, 'lib')) Can't you do some gooey goodness with __init__.py files? Scottz was using this methodology http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode39 chromite/chromite.py:39: _USAGE = 'usage: %prog [options] [cmd [build.spec]]' Define these in your main http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode45 chromite/chromite.py:45: # A list of paths that we'll search through to find spec files. Why are these globals? You use both of these once. Just define where they are used. Also why are these arrays? Will we ever want more than one? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode59 chromite/chromite.py:59: _DEFAULT_BUILD_SPEC = """ Why are these defined here vs legacy.spc etc http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode92 chromite/chromite.py:92: def _IsInsideChroot(): Already defined in chromite lib http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:129: def _FindCommand(cmd_name): Def good candidate for unit test. Mock out TextMenu call http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:157: if cmd_name in this_cmd: regex is better. "in" does any substring ... not any prefix. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:167: Die('No commands matched: "%s". ' I would consider throwing exceptions rather than dying here. Exceptions are preferred in Python http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:175: choice = text_menu.TextMenu(possible_choices, 'Which chromite command:', neat! http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:191: 2. The full name of a spec file somewhere in the spec search path I think it'd be better to combine 2 and 3 into one ... any arbitrary prefix is a subset of an exact match. This also cleans up your logic. Just search for prefix using regex. Don't even bother trying to optimize for case 2 since it needlessly complicates this method. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:246: continue add line after http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:260: options[this_spec_name] = file_path You could probably merge the majority of this function and the one above into one function ... takes array of strings + descriptions + prefix ... if 1 -> return string with prefix o/w menu -> then return selection http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:306: arguments = list(arguments) [] is a shorthand for list() is it not? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:316: Die('OK, cancelling...') Maybe you should add a extra option that does exit rather than having Ctrl+c a normal part of the routine http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:331: Die('OK, cancelling...') same http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:350: default_build_spec = _DEFAULT_BUILD_SPEC % dict(name=build_spec_name) {} not dict http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:354: chroot_spec = build_config.get('LEGACY', 'chroot_spec') erm why read specified build spec but legacy chroot spec? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:359: default_chroot_spec = _DEFAULT_CHROOT_SPEC % dict(name=chroot_spec_name) {} http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:392: chroot_dir = _GetChrootAbsDir(chroot_config) use DoesChrootExist() http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:402: cmd = './make_chroot --chroot="%s" %s' % ( Why are you setting --chroot here if we always use the default in this CL? Also why not just put --chroot option in make_chroot_flags? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:415: Die(str(e)) don't catch it http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:449: resume_state = base64.b64encode(resume_state) Can you store state to a temp file? tempfile.mkstemp. I'm worried these command lines might get too long http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) How do you --force? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:508: Die(str(e)) ditto on the die here http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:591: """Clean out built packages for a board. Wait, what is this for? When would we want to do this but not do setup_board --force? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:662: if not os.path.isdir(chroot_dir): This isn't necessary. Make_chroot --delete handles this http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:710: 2 lines here
I recall seeing a document describing the goals of chromite a few months ago and went back to check it after this CL but saw it hadn't really been updated. It would be really nice to have a current version of the document circulated detailing the goals, tradeoffs and plan for chromite. It would also help with this and other CL's being reviewed for chromite as they could reference the document to provide information on the intent and goals of the CL's.
On Thu, Dec 23, 2010 at 9:42 AM, <kliegs@chromium.org> wrote: > I recall seeing a document describing the goals of chromite a few months ago > and > went back to check it after this CL but saw it hadn't really been updated. > It > would be really nice to have a current version of the document circulated > detailing the goals, tradeoffs and plan for chromite. That document still holds for the goals of chromite. However we are trying to decouple independent/non-controversial small pieces to progress without getting into "coloring the bikeshed" with individual preferences on workflow just yet. This CL is the chromite wrapper/shell to provide the ability to wrap the _existing_ steps into a config/data driven entity _without_ changing any of the existing workflows. It will not replace any of the existing workflows right now and when it does it will be reviewed to suit the use cases we want to support. Right now it just enables us to boot strap the Build System test suite and also provide a formal way for the scripts to be run inside/outside the chroot (instead of each script relying on the restart_in_chroot hacks). It will also formalize the numerous calls to enter_chroot with a program to run, and provide for the ability to check/warn on supported/deprecated options. > > It would also help with this and other CL's being reviewed for chromite as > they > could reference the document to provide information on the intent and goals > of > the CL's. "chromite" shouldn't require any special treatment - Like every other initiative small CLs could be explained in the CL or in the issue tracker and larger ones should be documented in design docs and reviewed. We are documenting each of the larger initiatives undertaken and coming up with respective design docs and will get them reviewed as appropriate. Please do review the code and provide any feedback and feel free to ping Doug or me if you need further clarificaiton on this CL. Thanks Anush > > http://codereview.chromium.org/6005004/ >
On Wed, Dec 22, 2010 at 5:52 PM, <sosa@chromium.org> wrote: > Haven't had a chance to look at TextMenu yet. But here is my initial > thoughts. > > Also a meta thought: > > Why even bother doing chroot dirs? Do ppl ever do this? Would you expect > them > to hand edit these specs in order to specify chroot? Yup. I use this a lot especially when I attempt to upgrade portage packages or portage itself etc. If anything you'd prob > add > it to options parser rather than the config It would be ideal to reduce the number of options passed in, since it will grow over time. > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py > File chromite/chromite.py (right): > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode9 > chromite/chromite.py:9: import base64 > Maybe sort ascii ... Capitals first, lowercase second. Preference > though > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode11 > chromite/chromite.py:11: import cPickle as pickle > it's preferred not to rename. > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode25 > chromite/chromite.py:25: _SRCROOT_PATH = > os.path.realpath(os.path.join(_CHROMITE_PATH, > can't you always do this? Just just use the else. > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode27 > chromite/chromite.py:27: sys.path.insert(0, os.path.join(_CHROMITE_PATH, > 'lib')) > Can't you do some gooey goodness with __init__.py files? Scottz was > using this methodology > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode39 > chromite/chromite.py:39: _USAGE = 'usage: %prog [options] [cmd > [build.spec]]' > Define these in your main > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode45 > chromite/chromite.py:45: # A list of paths that we'll search through to > find spec files. > Why are these globals? You use both of these once. Just define where > they are used. > > Also why are these arrays? Will we ever want more than one? > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode59 > chromite/chromite.py:59: _DEFAULT_BUILD_SPEC = """ > Why are these defined here vs legacy.spc etc > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode92 > chromite/chromite.py:92: def _IsInsideChroot(): > Already defined in chromite lib > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:129: def _FindCommand(cmd_name): > Def good candidate for unit test. Mock out TextMenu call > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:157: if cmd_name in this_cmd: > regex is better. "in" does any substring ... not any prefix. > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:167: Die('No commands matched: "%s". ' > I would consider throwing exceptions rather than dying here. Exceptions > are preferred in Python > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:175: choice = text_menu.TextMenu(possible_choices, > 'Which chromite command:', > neat! > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:191: 2. The full name of a spec file somewhere in > the spec search path > I think it'd be better to combine 2 and 3 into one ... any arbitrary > prefix is a subset of an exact match. This also cleans up your logic. > Just search for prefix using regex. Don't even bother trying to > optimize for case 2 since it needlessly complicates this method. > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:246: continue > add line after > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:260: options[this_spec_name] = file_path > You could probably merge the majority of this function and the one above > into one function ... takes array of strings + descriptions + prefix ... > if 1 -> return string with prefix o/w menu -> then return selection > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:306: arguments = list(arguments) > [] is a shorthand for list() is it not? > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:316: Die('OK, cancelling...') > Maybe you should add a extra option that does exit rather than having > Ctrl+c a normal part of the routine > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:331: Die('OK, cancelling...') > same > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:350: default_build_spec = _DEFAULT_BUILD_SPEC % > dict(name=build_spec_name) > {} not dict > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:354: chroot_spec = build_config.get('LEGACY', > 'chroot_spec') > erm why read specified build spec but legacy chroot spec? > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:359: default_chroot_spec = _DEFAULT_CHROOT_SPEC % > dict(name=chroot_spec_name) > {} > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:392: chroot_dir = _GetChrootAbsDir(chroot_config) > use DoesChrootExist() > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:402: cmd = './make_chroot --chroot="%s" %s' % ( > Why are you setting --chroot here if we always use the default in this > CL? Also why not just put --chroot option in make_chroot_flags? > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:415: Die(str(e)) > don't catch it > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:449: resume_state = base64.b64encode(resume_state) > Can you store state to a temp file? tempfile.mkstemp. I'm worried > these command lines might get too long > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) > How do you --force? > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:508: Die(str(e)) > ditto on the die here > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:591: """Clean out built packages for a board. > Wait, what is this for? When would we want to do this but not do > setup_board --force? > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:662: if not os.path.isdir(chroot_dir): > This isn't necessary. Make_chroot --delete handles this > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:710: > 2 lines here > > http://codereview.chromium.org/6005004/ >
Did these things: * Addressed sosa's code review (still need to add unit test) * Added 'shell' command (you can use 'sh' prefix if 'shell' is too long). * Made 'distclean' and 'shell' take chroot specs instead of build specs. * Changed it so that you need to specify a prefix of a build command to match, not just any substring. * pylint cleanups on text_menu. Still to do: * Double-check with Anush that this still is really working like he wanted it to. * Unit tests. * Other commands: emerge, ebuild, equery, portageq, workon. You can take a look now, or wait until the "Still to do" stuff is done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode9 chromite/chromite.py:9: import base64 No. Current order matches Google python conventions: "Within each grouping, imports should be sorted lexicographically, ignoring case, according to each module's full package path." http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode11 chromite/chromite.py:11: import cPickle as pickle On 2010/12/23 01:52:27, sosa wrote: > it's preferred not to rename. Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode25 chromite/chromite.py:25: _SRCROOT_PATH = os.path.realpath(os.path.join(_CHROMITE_PATH, On 2010/12/23 01:52:27, sosa wrote: > can't you always do this? Just just use the else. Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode27 chromite/chromite.py:27: sys.path.insert(0, os.path.join(_CHROMITE_PATH, 'lib')) ...or, I can just not be completely stupid and change these to things like 'from lib import text_menu'. Original code was looking at "../lib", which is why the complexity was originally there. There had also been talk about copying chromite.py to an alternate location, but decided instead of use chromite.sh wrapper. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode39 chromite/chromite.py:39: _USAGE = 'usage: %prog [options] [cmd [build.spec]]' On 2010/12/23 01:52:27, sosa wrote: > Define these in your main Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode45 chromite/chromite.py:45: # A list of paths that we'll search through to find spec files. Moved to where they are used; they are lists in anticipation of the need to support specs that live in private overlays. Added a comment to that effect. On 2010/12/23 01:52:27, sosa wrote: > Why are these globals? You use both of these once. Just define where they are > used. > > Also why are these arrays? Will we ever want more than one? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode59 chromite/chromite.py:59: _DEFAULT_BUILD_SPEC = """ Done. Moved to specs/build/_defaults and specs/chroot/_defaults. Since I could put documentation in those file more cleanly, removed some of the extra cruft from the spec files so we only specify things that we care about. On 2010/12/23 01:52:27, sosa wrote: > Why are these defined here vs legacy.spc etc http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcode92 chromite/chromite.py:92: def _IsInsideChroot(): On 2010/12/23 01:52:27, sosa wrote: > Already defined in chromite lib Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:157: if cmd_name in this_cmd: Changed to startswith(). You're right that prefix is better than substring here. On 2010/12/23 01:52:27, sosa wrote: > regex is better. "in" does any substring ... not any prefix. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:167: Die('No commands matched: "%s". ' I don't think so in this case. Exceptions are better in weird error conditions. ...but in directly in response to a user action, they are confusing. Saying "./chromite implode' should clearly tell the user that there is no implode command, not display an ugly stack trace. Let me know if you feel strongly about making it an exception and we can talk. On 2010/12/23 01:52:27, sosa wrote: > I would consider throwing exceptions rather than dying here. Exceptions are > preferred in Python http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:191: 2. The full name of a spec file somewhere in the spec search path No. I really wanted a substring for finding the spec. In particular, I want to match "x86-mario" when the user passes "mario". Going to leave it how it is. On 2010/12/23 01:52:27, sosa wrote: > I think it'd be better to combine 2 and 3 into one ... any arbitrary prefix is a > subset of an exact match. This also cleans up your logic. Just search for > prefix using regex. Don't even bother trying to optimize for case 2 since it > needlessly complicates this method. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:246: continue On 2010/12/23 01:52:27, sosa wrote: > add line after Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:260: options[this_spec_name] = file_path I'd rather not merge them. It is true that there are similarities, but there are also subtle differences that would complicate the merged function. To me, TextMenu() seems the right level of abstraction. On 2010/12/23 01:52:27, sosa wrote: > You could probably merge the majority of this function and the one above into > one function ... takes array of strings + descriptions + prefix ... if 1 -> > return string with prefix o/w menu -> then return selection http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:306: arguments = list(arguments) You are saying to do this instead? arguments = arguments[:] That certainly does work if arguments is a list. However, if incoming arguments happens to be something else (tuple, numpy array, generator, ...) then it might not do what you want. I have gotten in the habit of copying lists with the list constructor to be flexible in what type of sequence I take in (just as I would be flexible if I didn't copy/futz). http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:316: Die('OK, cancelling...') On 2010/12/23 01:52:27, sosa wrote: > Maybe you should add a extra option that does exit rather than having Ctrl+c a > normal part of the routine Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:331: Die('OK, cancelling...') On 2010/12/23 01:52:27, sosa wrote: > same Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:350: default_build_spec = _DEFAULT_BUILD_SPEC % dict(name=build_spec_name) On 2010/12/23 01:52:27, sosa wrote: > {} not dict Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:354: chroot_spec = build_config.get('LEGACY', 'chroot_spec') This is the chroot spec specified in the LEGACY section of the build spec. Maybe it shouldn't belong in there. I'm not 100% sure I understand Anush's thoughts on how this will evolve into the final Chromite design. He proposed things in various LEGACY sections, and I thought this was what he meant. I'll check. On 2010/12/23 01:52:27, sosa wrote: > erm why read specified build spec but legacy chroot spec? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:359: default_chroot_spec = _DEFAULT_CHROOT_SPEC % dict(name=chroot_spec_name) On 2010/12/23 01:52:27, sosa wrote: > {} Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:392: chroot_dir = _GetChrootAbsDir(chroot_config) Added ability for _GetChrootAbsDir() to check and removed _DoesChrootExist(). Good call. On 2010/12/23 01:52:27, sosa wrote: > use DoesChrootExist() http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:402: cmd = './make_chroot --chroot="%s" %s' % ( Boards can specify alternate chroots. The --chroot is not included in the make_chroot_flags because it needs to be common between the make_chroot_flags and enter_chroot_flags. On 2010/12/23 01:52:27, sosa wrote: > Why are you setting --chroot here if we always use the default in this CL? Also > why not just put --chroot option in make_chroot_flags? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:415: Die(str(e)) On 2010/12/23 01:52:27, sosa wrote: > don't catch it Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:449: resume_state = base64.b64encode(resume_state) I researched this and it looks like I have 2 megs for command line on my system (getconf ARG_MAX); but agree that temp file is cleaner and more portable. Changed to use tempfile.NamedTemporaryFile(). On 2010/12/23 01:52:27, sosa wrote: > Can you store state to a temp file? tempfile.mkstemp. I'm worried these > command lines might get too long http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) You don't. You do a ./chromite clean first. This isn't quite as ideal (doesn't background the rm -rf), but hopefully is sufficient? On 2010/12/23 01:52:27, sosa wrote: > How do you --force? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:508: Die(str(e)) On 2010/12/23 01:52:27, sosa wrote: > ditto on the die here Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:591: """Clean out built packages for a board. You would do this instead of setup_board --force. This is similar to most other workflows (Makefiles, VC++, etc) where clean is a separate step and not an option to the standard build. However, if we want to keep the existing "1 step" process, we could probably add an option to build so you could do something like "./chromite build mario --clean" (or --force). What do you think? On 2010/12/23 01:52:27, sosa wrote: > Wait, what is this for? When would we want to do this but not do setup_board > --force? http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:662: if not os.path.isdir(chroot_dir): Yes, but it is a nicety to do this check before giving the dire warning. I will delete it if you want me to, though. I suppose one option would be add the dire warning to "make_chroot --delete --warn", then I could avoid the duplication. On 2010/12/23 01:52:27, sosa wrote: > This isn't necessary. Make_chroot --delete handles this http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:710: On 2010/12/23 01:52:27, sosa wrote: > 2 lines here Done.
...on second thought, maybe best to hold off on reviewing stuff. To add 'emerge', 'ebuild', ... commands I'll have to change the command-line processing yet again. Sigh. I'll try to find something more flexible.
OK folks, I think I'm about done with working on this. I think this implements
everything that Anush was requesting.
anush: Please try using this and let me know anything you want changed. You are
the one who understands the "future direction" of chromite, so please take extra
steps to make sure that design/implementation choices won't cause headaches.
sosa: I've hopefully addressed your code review comments, but also changed /
added a whole lot of code. I'm happy to walk through stuff with you. I added
unit tests for two functions. If all looks good, I will add a few more unit
tests in a future checkin.
kliegs: Take a look!
There are definitely lots of things I did to try to be 'smart'. Some of those
might be good, and some might be bad. I'd love to hear what you think, and I
can certainly take out pieces that don't work. Sometimes extra 'smartness' can
be annoying or just too darn complex.
Unless there's anything seriously wrong with this, it would be nice to push so
you guys can start playing with it.
Things to try:
./chromite
./chromite build mario
./chromite shell
./chromite build
./chromite build x86
./chromite workon mario start
./chromite workon host start
./chromite workon mario list --all
./chromtie workon start # Common "OOPS"
./chromite equery host belongs /usr/bin/sudo
./chromite shell echo 'HI'
./chromite shell -- echo -n 'HI'
./chromite shell ABC=def -- bash -c 'echo $ABC'
./chromite --help
./chromite clean --help
./chromite workon mario --help # Usage is wrong
Nearly all commands can be run inside or outside the chroot. The only exception
is commands that modify the chroot ("build host", "distclean", ...).
On 2011/01/06 00:56:41, diandersAtChromium wrote:
> ...on second thought, maybe best to hold off on reviewing stuff. To add
> 'emerge', 'ebuild', ... commands I'll have to change the command-line
processing
> yet again. Sigh. I'll try to find something more flexible.
This is a LOT of code. Can you specifically subdivision which of the reviewers you want to look at what? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode10 chromite/chromite.py:10: import cPickle as cPickle redundant?
I suspect I'm going to be unpopular but I have to ask what the actual design goals/requirements are for this. The only document I've seen is an old one. And this just looks like its adding extra bits to my command line for everything I want to do. Why does everything have to have ./chromite in front of it? Why can't I just run 'build' or 'build <host>'? Why can't I just run equery directly? What benefit is all of this providing? (And yes - I'm sure some of these have been answered in hallway conversations or smaller email threads. But it would really help me at least in reviewing this to understand the large picture rather than just specific code bits) On Mon, Jan 10, 2011 at 1:38 PM, <sosa@chromium.org> wrote: > This is a LOT of code. Can you specifically subdivision which of the > reviewers > you want to look at what? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py > > File chromite/chromite.py (right): > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode10 > chromite/chromite.py:10: import cPickle as cPickle > redundant? > > > http://codereview.chromium.org/6005004/ >
Major comments: 1) Modularize-code. I suggest classes for all your commands and suggested some methods within them. Take a look at the buildbot code for an example. I think they did a good job with it. 2) Split this up into a few files. 3) I really don't like the strict ordering of args. There must be some better way to do it. Also, if I type --help at any time in an arg list, I should be able to know all the help i need for what I have. 4) I don't really think we should wrap non-build commands. I'm not a big fan of wrapping portage methods. Developers should REALLY understand these. Let's not abstract too much from them. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:167: Die('No commands matched: "%s". ' I can go either way. On 2011/01/06 00:23:23, diandersAtChromium wrote: > I don't think so in this case. Exceptions are better in weird error conditions. > ...but in directly in response to a user action, they are confusing. Saying > "./chromite implode' should clearly tell the user that there is no implode > command, not display an ugly stack trace. > > Let me know if you feel strongly about making it an exception and we can talk. > > On 2010/12/23 01:52:27, sosa wrote: > > I would consider throwing exceptions rather than dying here. Exceptions are > > preferred in Python > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:306: arguments = list(arguments) I meant arguments = [arguments]. Are these not equivalent? On 2011/01/06 00:23:23, diandersAtChromium wrote: > You are saying to do this instead? > arguments = arguments[:] > > That certainly does work if arguments is a list. However, if incoming arguments > happens to be something else (tuple, numpy array, generator, ...) then it might > not do what you want. I have gotten in the habit of copying lists with the list > constructor to be flexible in what type of sequence I take in (just as I would > be flexible if I didn't copy/futz). http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:354: chroot_spec = build_config.get('LEGACY', 'chroot_spec') ping @anush On 2011/01/06 00:23:23, diandersAtChromium wrote: > This is the chroot spec specified in the LEGACY section of the build spec. > Maybe it shouldn't belong in there. I'm not 100% sure I understand Anush's > thoughts on how this will evolve into the final Chromite design. He proposed > things in various LEGACY sections, and I thought this was what he meant. I'll > check. > > On 2010/12/23 01:52:27, sosa wrote: > > erm why read specified build spec but legacy chroot spec? > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) Difference between make_chroot --replace and setup_board --force? There will be an outcry! On 2011/01/06 00:23:23, diandersAtChromium wrote: > You don't. You do a ./chromite clean first. This isn't quite as ideal (doesn't > background the rm -rf), but hopefully is sufficient? > > On 2010/12/23 01:52:27, sosa wrote: > > How do you --force? > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:591: """Clean out built packages for a board. That is nice! On 2011/01/06 00:23:23, diandersAtChromium wrote: > You would do this instead of setup_board --force. This is similar to most other > workflows (Makefiles, VC++, etc) where clean is a separate step and not an > option to the standard build. However, if we want to keep the existing "1 step" > process, we could probably add an option to build so you could do something like > "./chromite build mario --clean" (or --force). What do you think? > > On 2010/12/23 01:52:27, sosa wrote: > > Wait, what is this for? When would we want to do this but not do setup_board > > --force? > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:662: if not os.path.isdir(chroot_dir): The question is fine. I specifically meant the os.path.isdir check / Die messg On 2011/01/06 00:23:23, diandersAtChromium wrote: > Yes, but it is a nicety to do this check before giving the dire warning. I will > delete it if you want me to, though. I suppose one option would be add the dire > warning to "make_chroot --delete --warn", then I could avoid the duplication. > > On 2010/12/23 01:52:27, sosa wrote: > > This isn't necessary. Make_chroot --delete handles this > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode49 chromite/chromite.py:49: _CHROOT_SPEC_SEARCH_PATH = [ can be overriden in unittest i.e. chromite._COMMAND_HANDLERS = [teststuffffff] http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode63 chromite/chromite.py:63: May wanna consider making a Class for commands and baking some common logic like vars in there. I think it might be good to use classes for commands like the buildbots so that you can do some fun inheritance stuff, etc. It'll also make it easier to change their implementations later. Also this file is getting big. May wanna consider breaking out commands into their own module. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode78 chromite/chromite.py:78: """ is this a strict requirement? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode88 chromite/chromite.py:88: ] Seems weird to me to be combining this functionality here. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:152: reverse ... if/else...just do if choice. Good to avoid "is None" http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:198: menu of all spec files in the spec path. NOTE: Only possible if ...maybe may wanna make plural in anticipation as well :) http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:218: if is_chroot_spec: What should happen with subtrees? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:233: This seems bad. Maybe disallow. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:249: # Skip if this spec file doesn't contain our substring. We are _always_ This will almost never happen because of host special case. Not even sure if it's worth optimizing. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:257: continue Same as before. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:311: Sort of weird to assume it was a cancel here. Either switch to exceptions or move this Die into _FindSpec http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:327: # Find the spec given the name... Why the last part? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:374: True if the chroot appears to exist; False if it appears not to exist. Again might be nice to class / objectize commands. Have a pre-start / precondtion stage ... have some NAME that you print out, before running, cleanup, etc. Commonize src/scripts etc. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:439: chroot_config: A SafeConfigParser representing the config for the chroot. build host? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:479: This just runs the setup_board command with the proper args, if needed. Shouldn't you close the state_file before using it? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:598: """ you have to type in caps? Generally it's lowercase and any prefix i.e. y,n,ye,no,yes and uppercase http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:606: if not _DoesChrootExist(chroot_config): it'll be weird if any of these asserts happen. it'll be unclear that the dev has a bad config. Maybe all these sorta checks should go into some CONFIG checker class or something rather than as part of the cmds. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:641: RunCommand(args) again with the YES http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:726: typo http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:750: Why are you skipping 1? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:753: ? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:779: Why oh why are we wrapping these things???? I don't know if agree with this. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:862: Why not use this rather than randomly have some explicit other ones? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:953: There's two ways to do this??? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:990: I think you should be able to do some better stuff with options parser to make the help more useful. It doesn't seem that using --help will ever be useful. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:995: sys.argv[1]? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:1015: Why is there some strict ordering here :( http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.py File chromite/chromite_unittest.py (right): http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... chromite/chromite_unittest.py:113: def testCaseInsensitiveBuild(self): Up to you but you may want to mock out the "known" commands so that these unittests aren't fragile to the names of the commands. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... chromite/chromite_unittest.py:146: # Create our mox and stub out function calls used by _FindSpec()... It almost seems for the setup and teardown's you should move the comments right below the docstrings to the docstring.
On Mon, Jan 10, 2011 at 8:44 PM, <sosa@chromium.org> wrote: > Major comments: > > 1) Modularize-code. I suggest classes for all your commands and suggested > some > methods within them. Take a look at the buildbot code for an example. I > think > they did a good job with it. > > 2) Split this up into a few files. > > 3) I really don't like the strict ordering of args. There must be some > better > way to do it. Also, if I type --help at any time in an arg list, I should > be > able to know all the help i need for what I have. > > 4) I don't really think we should wrap non-build commands. I'm not a big > fan > of wrapping portage methods. Developers should REALLY understand these. > Let's > not abstract too much from them. > For developers who need to it is always there. We are trying to make the chroot transparent with this, to enable developers who have no idea of what a chroot is and the dance required to build an image - we have developers working on P0s stalled because of this. (btw, emerge-<board> itself is a wrapper to emerge with args) > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py > > File chromite/chromite.py (right): > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:167: Die('No commands matched: "%s". ' > I can go either way. > > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > >> I don't think so in this case. Exceptions are better in weird error >> > conditions. > >> ...but in directly in response to a user action, they are confusing. >> > Saying > >> "./chromite implode' should clearly tell the user that there is no >> > implode > >> command, not display an ugly stack trace. >> > > Let me know if you feel strongly about making it an exception and we >> > can talk. > > On 2010/12/23 01:52:27, sosa wrote: >> > I would consider throwing exceptions rather than dying here. >> > Exceptions are > >> > preferred in Python >> > > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:306: arguments = list(arguments) > I meant arguments = [arguments]. Are these not equivalent? > > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > >> You are saying to do this instead? >> arguments = arguments[:] >> > > That certainly does work if arguments is a list. However, if incoming >> > arguments > >> happens to be something else (tuple, numpy array, generator, ...) then >> > it might > >> not do what you want. I have gotten in the habit of copying lists >> > with the list > >> constructor to be flexible in what type of sequence I take in (just as >> > I would > >> be flexible if I didn't copy/futz). >> > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:354: chroot_spec = build_config.get('LEGACY', > 'chroot_spec') > ping @anush I think Doug has deprecated the use of the word LEGACY here. > > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > >> This is the chroot spec specified in the LEGACY section of the build >> > spec. > >> Maybe it shouldn't belong in there. I'm not 100% sure I understand >> > Anush's > >> thoughts on how this will evolve into the final Chromite design. He >> > proposed > >> things in various LEGACY sections, and I thought this was what he >> > meant. I'll > >> check. >> > > On 2010/12/23 01:52:27, sosa wrote: >> > erm why read specified build spec but legacy chroot spec? >> > > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) > Difference between > > make_chroot --replace > and setup_board --force? > > There will be an outcry! > > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > >> You don't. You do a ./chromite clean first. This isn't quite as >> > ideal (doesn't > >> background the rm -rf), but hopefully is sufficient? >> > > On 2010/12/23 01:52:27, sosa wrote: >> > How do you --force? >> > > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:591: """Clean out built packages for a board. > That is nice! > > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > >> You would do this instead of setup_board --force. This is similar to >> > most other > >> workflows (Makefiles, VC++, etc) where clean is a separate step and >> > not an > >> option to the standard build. However, if we want to keep the >> > existing "1 step" > >> process, we could probably add an option to build so you could do >> > something like > >> "./chromite build mario --clean" (or --force). What do you think? >> > > On 2010/12/23 01:52:27, sosa wrote: >> > Wait, what is this for? When would we want to do this but not do >> > setup_board > >> > --force? >> > > > > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... > chromite/chromite.py:662: if not os.path.isdir(chroot_dir): > The question is fine. I specifically meant the os.path.isdir check / > Die messg > > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > >> Yes, but it is a nicety to do this check before giving the dire >> > warning. I will > >> delete it if you want me to, though. I suppose one option would be >> > add the dire > >> warning to "make_chroot --delete --warn", then I could avoid the >> > duplication. > > On 2010/12/23 01:52:27, sosa wrote: >> > This isn't necessary. Make_chroot --delete handles this >> > > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode49 > chromite/chromite.py:49: _CHROOT_SPEC_SEARCH_PATH = [ > can be overriden in unittest i.e. > > chromite._COMMAND_HANDLERS = [teststuffffff] > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode63 > chromite/chromite.py:63: > May wanna consider making a Class for commands and baking some common > logic like vars in there. I think it might be good to use classes for > commands like the buildbots so that you can do some fun inheritance > stuff, etc. It'll also make it easier to change their implementations > later. > > Also this file is getting big. May wanna consider breaking out commands > into their own module. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode78 > chromite/chromite.py:78: """ > is this a strict requirement? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode88 > chromite/chromite.py:88: ] > Seems weird to me to be combining this functionality here. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:152: > reverse ... if/else...just do if choice. Good to avoid "is None" > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:198: menu of all spec files in the spec path. > NOTE: Only possible if > ...maybe may wanna make plural in anticipation as well :) > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:218: if is_chroot_spec: > What should happen with subtrees? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:233: > This seems bad. Maybe disallow. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:249: # Skip if this spec file doesn't contain our > substring. We are _always_ > This will almost never happen because of host special case. Not even > sure if it's worth optimizing. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:257: continue > Same as before. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:311: > Sort of weird to assume it was a cancel here. Either switch to > exceptions or move this Die into _FindSpec > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:327: # Find the spec given the name... > Why the last part? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:374: True if the chroot appears to exist; False if > it appears not to exist. > Again might be nice to class / objectize commands. Have a pre-start / > precondtion stage ... have some NAME that you print out, before running, > cleanup, etc. Commonize src/scripts etc. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:439: chroot_config: A SafeConfigParser representing > the config for the chroot. > build host? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:479: This just runs the setup_board command with > the proper args, if needed. > Shouldn't you close the state_file before using it? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:598: """ > you have to type in caps? Generally it's lowercase and any prefix i.e. > y,n,ye,no,yes and uppercase > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:606: if not _DoesChrootExist(chroot_config): > it'll be weird if any of these asserts happen. it'll be unclear that > the dev has a bad config. > > Maybe all these sorta checks should go into some CONFIG checker class or > something rather than as part of the cmds. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:641: RunCommand(args) > again with the YES > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:726: > typo > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:750: > Why are you skipping 1? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:753: > ? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:779: > Why oh why are we wrapping these things???? I don't know if agree with > this. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:862: > Why not use this rather than randomly have some explicit other ones? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:953: > There's two ways to do this??? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:990: > I think you should be able to do some better stuff with options parser > to make the help more useful. It doesn't seem that using --help will > ever be useful. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:995: > sys.argv[1]? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:1015: > Why is there some strict ordering here :( > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.py > File chromite/chromite_unittest.py (right): > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... > chromite/chromite_unittest.py:113: def testCaseInsensitiveBuild(self): > Up to you but you may want to mock out the "known" commands so that > these unittests aren't fragile to the names of the commands. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... > chromite/chromite_unittest.py:146: # Create our mox and stub out > function calls used by _FindSpec()... > It almost seems for the setup and teardown's you should move the > comments right below the docstrings to the docstring. > > > http://codereview.chromium.org/6005004/ >
I honestly think if we have developers who don't know what the chroot is, they should learn what the chroot is. On Tue, Jan 11, 2011 at 10:46 AM, Anush Elangovan(அனுஷ்) <anush@chromium.org> wrote: > > > On Mon, Jan 10, 2011 at 8:44 PM, <sosa@chromium.org> wrote: >> >> Major comments: >> >> 1) Modularize-code. I suggest classes for all your commands and >> suggested some >> methods within them. Take a look at the buildbot code for an example. I >> think >> they did a good job with it. >> >> 2) Split this up into a few files. >> >> 3) I really don't like the strict ordering of args. There must be some >> better >> way to do it. Also, if I type --help at any time in an arg list, I should >> be >> able to know all the help i need for what I have. >> >> 4) I don't really think we should wrap non-build commands. I'm not a big >> fan >> of wrapping portage methods. Developers should REALLY understand these. >> Let's >> not abstract too much from them. > > For developers who need to it is always there. We are trying to make the > chroot transparent with this, to enable developers who have no idea of what > a chroot is and the dance required to build an image - we have developers > working on P0s stalled because of this. (btw, emerge-<board> itself is a > wrapper to emerge with args) > >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py >> File chromite/chromite.py (right): >> >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... >> chromite/chromite.py:167: Die('No commands matched: "%s". ' >> I can go either way. >> >> On 2011/01/06 00:23:23, diandersAtChromium wrote: >>> >>> I don't think so in this case. Exceptions are better in weird error >> >> conditions. >>> >>> ...but in directly in response to a user action, they are confusing. >> >> Saying >>> >>> "./chromite implode' should clearly tell the user that there is no >> >> implode >>> >>> command, not display an ugly stack trace. >> >>> Let me know if you feel strongly about making it an exception and we >> >> can talk. >> >>> On 2010/12/23 01:52:27, sosa wrote: >>> > I would consider throwing exceptions rather than dying here. >> >> Exceptions are >>> >>> > preferred in Python >> >> >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... >> chromite/chromite.py:306: arguments = list(arguments) >> I meant arguments = [arguments]. Are these not equivalent? >> >> On 2011/01/06 00:23:23, diandersAtChromium wrote: >>> >>> You are saying to do this instead? >>> arguments = arguments[:] >> >>> That certainly does work if arguments is a list. However, if incoming >> >> arguments >>> >>> happens to be something else (tuple, numpy array, generator, ...) then >> >> it might >>> >>> not do what you want. I have gotten in the habit of copying lists >> >> with the list >>> >>> constructor to be flexible in what type of sequence I take in (just as >> >> I would >>> >>> be flexible if I didn't copy/futz). >> >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... >> chromite/chromite.py:354: chroot_spec = build_config.get('LEGACY', >> 'chroot_spec') >> ping @anush > > I think Doug has deprecated the use of the word LEGACY here. > >> >> On 2011/01/06 00:23:23, diandersAtChromium wrote: >>> >>> This is the chroot spec specified in the LEGACY section of the build >> >> spec. >>> >>> Maybe it shouldn't belong in there. I'm not 100% sure I understand >> >> Anush's >>> >>> thoughts on how this will evolve into the final Chromite design. He >> >> proposed >>> >>> things in various LEGACY sections, and I thought this was what he >> >> meant. I'll >>> >>> check. >> >>> On 2010/12/23 01:52:27, sosa wrote: >>> > erm why read specified build spec but legacy chroot spec? >> >> >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... >> chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) >> Difference between >> >> make_chroot --replace >> and setup_board --force? >> >> There will be an outcry! >> >> On 2011/01/06 00:23:23, diandersAtChromium wrote: >>> >>> You don't. You do a ./chromite clean first. This isn't quite as >> >> ideal (doesn't >>> >>> background the rm -rf), but hopefully is sufficient? >> >>> On 2010/12/23 01:52:27, sosa wrote: >>> > How do you --force? >> >> >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... >> chromite/chromite.py:591: """Clean out built packages for a board. >> That is nice! >> >> On 2011/01/06 00:23:23, diandersAtChromium wrote: >>> >>> You would do this instead of setup_board --force. This is similar to >> >> most other >>> >>> workflows (Makefiles, VC++, etc) where clean is a separate step and >> >> not an >>> >>> option to the standard build. However, if we want to keep the >> >> existing "1 step" >>> >>> process, we could probably add an option to build so you could do >> >> something like >>> >>> "./chromite build mario --clean" (or --force). What do you think? >> >>> On 2010/12/23 01:52:27, sosa wrote: >>> > Wait, what is this for? When would we want to do this but not do >> >> setup_board >>> >>> > --force? >> >> >> >> http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... >> chromite/chromite.py:662: if not os.path.isdir(chroot_dir): >> The question is fine. I specifically meant the os.path.isdir check / >> Die messg >> >> On 2011/01/06 00:23:23, diandersAtChromium wrote: >>> >>> Yes, but it is a nicety to do this check before giving the dire >> >> warning. I will >>> >>> delete it if you want me to, though. I suppose one option would be >> >> add the dire >>> >>> warning to "make_chroot --delete --warn", then I could avoid the >> >> duplication. >> >>> On 2010/12/23 01:52:27, sosa wrote: >>> > This isn't necessary. Make_chroot --delete handles this >> >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode49 >> chromite/chromite.py:49: _CHROOT_SPEC_SEARCH_PATH = [ >> can be overriden in unittest i.e. >> >> chromite._COMMAND_HANDLERS = [teststuffffff] >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode63 >> chromite/chromite.py:63: >> May wanna consider making a Class for commands and baking some common >> logic like vars in there. I think it might be good to use classes for >> commands like the buildbots so that you can do some fun inheritance >> stuff, etc. It'll also make it easier to change their implementations >> later. >> >> Also this file is getting big. May wanna consider breaking out commands >> into their own module. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode78 >> chromite/chromite.py:78: """ >> is this a strict requirement? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode88 >> chromite/chromite.py:88: ] >> Seems weird to me to be combining this functionality here. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:152: >> reverse ... if/else...just do if choice. Good to avoid "is None" >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:198: menu of all spec files in the spec path. >> NOTE: Only possible if >> ...maybe may wanna make plural in anticipation as well :) >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:218: if is_chroot_spec: >> What should happen with subtrees? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:233: >> This seems bad. Maybe disallow. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:249: # Skip if this spec file doesn't contain our >> substring. We are _always_ >> This will almost never happen because of host special case. Not even >> sure if it's worth optimizing. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:257: continue >> Same as before. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:311: >> Sort of weird to assume it was a cancel here. Either switch to >> exceptions or move this Die into _FindSpec >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:327: # Find the spec given the name... >> Why the last part? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:374: True if the chroot appears to exist; False if >> it appears not to exist. >> Again might be nice to class / objectize commands. Have a pre-start / >> precondtion stage ... have some NAME that you print out, before running, >> cleanup, etc. Commonize src/scripts etc. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:439: chroot_config: A SafeConfigParser representing >> the config for the chroot. >> build host? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:479: This just runs the setup_board command with >> the proper args, if needed. >> Shouldn't you close the state_file before using it? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:598: """ >> you have to type in caps? Generally it's lowercase and any prefix i.e. >> y,n,ye,no,yes and uppercase >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:606: if not _DoesChrootExist(chroot_config): >> it'll be weird if any of these asserts happen. it'll be unclear that >> the dev has a bad config. >> >> Maybe all these sorta checks should go into some CONFIG checker class or >> something rather than as part of the cmds. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:641: RunCommand(args) >> again with the YES >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:726: >> typo >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:750: >> Why are you skipping 1? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:753: >> ? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:779: >> Why oh why are we wrapping these things???? I don't know if agree with >> this. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:862: >> Why not use this rather than randomly have some explicit other ones? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:953: >> There's two ways to do this??? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:990: >> I think you should be able to do some better stuff with options parser >> to make the help more useful. It doesn't seem that using --help will >> ever be useful. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:995: >> sys.argv[1]? >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... >> chromite/chromite.py:1015: >> Why is there some strict ordering here :( >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.py >> File chromite/chromite_unittest.py (right): >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... >> chromite/chromite_unittest.py:113: def testCaseInsensitiveBuild(self): >> Up to you but you may want to mock out the "known" commands so that >> these unittests aren't fragile to the names of the commands. >> >> >> http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... >> chromite/chromite_unittest.py:146: # Create our mox and stub out >> function calls used by _FindSpec()... >> It almost seems for the setup and teardown's you should move the >> comments right below the docstrings to the docstring. >> >> http://codereview.chromium.org/6005004/ > >
Responded to sosa's comments (4 in patch set 4, 30 in patch set 10) and uploaded some fixes requested by Chris. The bigger changes requested by sosa (split into files, add command superclass) were addressed with TODOs. I will do these after moving chromite to the new home that anush has prepared for it. The strict ordering of certain args is somewhat inevitable with anything that has lots of subcommands. Git is a good example. I will try to make 'chromite --help' work better in a future change (TODO is in the code for that). The wrapping of non-build commands was discussed between sosa and anush (and mentioned in sosa's most recent comment). Sounds like sosa is OK with what's there, except shell. sosa: one advantage of 'chromite shell' over 'enter_chroot' is that it works with chroot spec files. These spec files could specify alternate commands for entering the chroot. I'm not sure how useful that is at the moment, but could be useful if something changes in the future? Also: once chromite is in the path, you'll be able to do chromite shell from any directory (you don't need to be in src/scripts). Hoping for a LGTM in this so I can at least get something basic committed... http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:306: arguments = list(arguments) Not equivalent. In [1]: l = range(10) In [2]: list(l) Out[2]: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] In [3]: [l] Out[3]: [[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]] On 2011/01/11 04:44:22, sosa wrote: > I meant arguments = [arguments]. Are these not equivalent? > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > > You are saying to do this instead? > > arguments = arguments[:] > > > > That certainly does work if arguments is a list. However, if incoming > arguments > > happens to be something else (tuple, numpy array, generator, ...) then it > might > > not do what you want. I have gotten in the habit of copying lists with the > list > > constructor to be flexible in what type of sequence I take in (just as I would > > be flexible if I didn't copy/futz). > http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:485: board_dir = _GetBoardDir(build_config) On 2011/01/11 04:44:22, sosa wrote: > Difference between > > make_chroot --replace > and setup_board --force? > > There will be an outcry! > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > > You don't. You do a ./chromite clean first. This isn't quite as ideal > (doesn't > > background the rm -rf), but hopefully is sufficient? > > > > On 2010/12/23 01:52:27, sosa wrote: > > > How do you --force? > > > Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:591: """Clean out built packages for a board. On 2011/01/11 04:44:22, sosa wrote: > That is nice! > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > > You would do this instead of setup_board --force. This is similar to most > other > > workflows (Makefiles, VC++, etc) where clean is a separate step and not an > > option to the standard build. However, if we want to keep the existing "1 > step" > > process, we could probably add an option to build so you could do something > like > > "./chromite build mario --clean" (or --force). What do you think? > > > > On 2010/12/23 01:52:27, sosa wrote: > > > Wait, what is this for? When would we want to do this but not do setup_board > > > --force? > > > Done. http://codereview.chromium.org/6005004/diff/11001/chromite/chromite.py#newcod... chromite/chromite.py:662: if not os.path.isdir(chroot_dir): On 2011/01/11 04:44:22, sosa wrote: > The question is fine. I specifically meant the os.path.isdir check / Die messg > > On 2011/01/06 00:23:23, diandersAtChromium wrote: > > Yes, but it is a nicety to do this check before giving the dire warning. I > will > > delete it if you want me to, though. I suppose one option would be add the > dire > > warning to "make_chroot --delete --warn", then I could avoid the duplication. > > > > On 2010/12/23 01:52:27, sosa wrote: > > > This isn't necessary. Make_chroot --delete handles this > > > Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode10 chromite/chromite.py:10: import ConfigParser On 2011/01/10 18:38:27, sosa wrote: > redundant? Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode49 chromite/chromite.py:49: _CHROOT_SPEC_SEARCH_PATH = [ Good point! Hmmm, at the moment though, I am going to leave the existing tests the way they are, though (since I actually do want to test that a build command exists and that sh maps to the shell command). When I add a few more unit tests, I will add one that does this. Added a TODO in chromite_unittest.py. On 2011/01/11 04:44:22, sosa wrote: > can be overriden in unittest i.e. > > chromite._COMMAND_HANDLERS = [teststuffffff] http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode63 chromite/chromite.py:63: Added TODO for class for commands. Splitting things into multiple files will wait until chromite moves to its own repo, which will happen after this pushes in a separate CL. On 2011/01/11 04:44:22, sosa wrote: > May wanna consider making a Class for commands and baking some common logic like > vars in there. I think it might be good to use classes for commands like the > buildbots so that you can do some fun inheritance stuff, etc. It'll also make it > easier to change their implementations later. > > Also this file is getting big. May wanna consider breaking out commands into > their own module. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode78 chromite/chromite.py:78: """ I haven't tested with targets with whitespace. I could imagine that passing --board=blah might not work with whitespace. I added a TODO to check this in a better way. On 2011/01/11 04:44:22, sosa wrote: > is this a strict requirement? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcode88 chromite/chromite.py:88: ] On 2011/01/11 04:44:22, sosa wrote: > Seems weird to me to be combining this functionality here. Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:152: No. Better habit is to test for None if you know that object is specified to be None in a certain case. From PEP-8: Also, beware of writing "if x" when you really mean "if x is not None" Google style guide does not contradict as far as I know. I believe "is None" has the side effect of being slightly faster (not that it matters in this case). On 2011/01/11 04:44:22, sosa wrote: > reverse ... if/else...just do if choice. Good to avoid "is None" http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:198: menu of all spec files in the spec path. NOTE: Only possible if No. Search path is used in the singular as far as I know. According to the google: http://www.google.com/search?sclient=psy&hl=en&q=define:search+path On 2011/01/11 04:44:22, sosa wrote: > ...maybe may wanna make plural in anticipation as well :) http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:218: if is_chroot_spec: They are not searched. Only the directory itself. Is there a compelling reason that subtrees should be searched? On 2011/01/11 04:44:22, sosa wrote: > What should happen with subtrees? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:233: On 2011/01/11 04:44:22, sosa wrote: > This seems bad. Maybe disallow. Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:249: # Skip if this spec file doesn't contain our substring. We are _always_ No. This is an extremely common case. I use it all the time: chromite build mario On 2011/01/11 04:44:22, sosa wrote: > This will almost never happen because of host special case. Not even sure if > it's worth optimizing. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:257: continue No. On 2011/01/11 04:44:22, sosa wrote: > Same as before. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:311: On 2011/01/11 04:44:22, sosa wrote: > Sort of weird to assume it was a cancel here. Either switch to exceptions or > move this Die into _FindSpec Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:327: # Find the spec given the name... Why what last part? See doctests below for usage. On 2011/01/11 04:44:22, sosa wrote: > Why the last part? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:374: True if the chroot appears to exist; False if it appears not to exist. Added TODO. On 2011/01/11 04:44:22, sosa wrote: > Again might be nice to class / objectize commands. Have a pre-start / > precondtion stage ... have some NAME that you print out, before running, > cleanup, etc. Commonize src/scripts etc. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:439: chroot_config: A SafeConfigParser representing the config for the chroot. On 2011/01/11 04:44:22, sosa wrote: > build host? Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:479: This just runs the setup_board command with the proper args, if needed. No. That will delete it. On 2011/01/11 04:44:22, sosa wrote: > Shouldn't you close the state_file before using it? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:598: """ Requiring exactly 'YES' was on purpose. Maybe that's a little too up-tight. Changing to allow any capitalization of 'yes', but not just 'y'. If people get too annoyed by it, we will make it looser. On 2011/01/11 04:44:22, sosa wrote: > you have to type in caps? Generally it's lowercase and any prefix i.e. > y,n,ye,no,yes and uppercase http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:606: if not _DoesChrootExist(chroot_config): Generally I try to use assert fails for program errors or for errors that are never going to come up in real life (but I'd still rather have an assert fail than something bad happen). The comment above talks about this, though make it's not clear enough. Many of these asserts indicate that someone has mucked with _GetBoardDir(), which will always return a non-blank absolute path with the target name in it. That is a program errors. Only two of these could be triggered by a bad user config: blank target name and target name with a '*' in it. The '*' case is unlikely to happen (who would put a * in their target name???) and really won't cause any badness. Maybe I should just remove it. The blank target case probably should be checked elsewhere. Added a TODO for now. On 2011/01/11 04:44:22, sosa wrote: > it'll be weird if any of these asserts happen. it'll be unclear that the dev > has a bad config. > > Maybe all these sorta checks should go into some CONFIG checker class or > something rather than as part of the cmds. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:641: RunCommand(args) On 2011/01/11 04:44:22, sosa wrote: > again with the YES Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:726: On 2011/01/11 04:44:22, sosa wrote: > typo Done. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:750: I'm skipping argument 0. From raw_argv desc, this list includes the command's name. That shouldn't be passed to the parse_args function. On 2011/01/11 04:44:22, sosa wrote: > Why are you skipping 1? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:753: Canonical way to make pylint and other automated checkers happy. From pylint docs, you can see that pylint will not (by default) complain about the variables "_" and "dummy" being unused. Thus, a good way to document that you have a variable that is unused is to assign it to _. See "dummy-variables-rgx" in pylint. This is probably more common in lua, where the standard language docs encourage it. An example from the lua docs: http://www.lua.org/pil/20.3.html ...however, I believe it is also the convension in python. This stackoverflow question might also be interesting: http://stackoverflow.com/questions/1739514/as-variable-name-in-python On 2011/01/11 04:44:22, sosa wrote: > ? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:779: See discussion at _CmdShell(). On 2011/01/11 04:44:22, sosa wrote: > Why oh why are we wrapping these things???? I don't know if agree with this. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:862: I think what you mean here is why do we have things like "emerge" as a special chromite command. Why not just let user get at emerge using the generic "shell" syntax. Sounds like you worked this out with Anush (from your recent comment in this code review). I think Chris's point was that the emerge, portage, etc commands don't seem to "fit" the rest of chromite. Also: using them really requires knowledge of the chroot (chroot paths, etc), so maybe don't belong in chromite. Only advantage I can see of wrapping commands like emerge and portage is that they can refer to build specs. On 2011/01/11 04:44:22, sosa wrote: > Why not use this rather than randomly have some explicit other ones? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:953: Yes, at the moment. Distclean was introduced before 'host' was a target. Once host was a target, it seemed to make sense to have 'clean host' do distclean, but I'm not sure. There is a comment above asking "Does that mean we should get rid of distclean"? I would love your opinion on which of the three things we should do: 1. Make 'clean host' an error condition 2. Remove 'distclean'. This assumes 'clean host' is discoverable enough. 3. Allow either 'clean host' or 'distclean'. I am fine with any of the three and would love to get outside opinions. On 2011/01/11 04:44:22, sosa wrote: > There's two ways to do this??? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:990: Added a TODO for this. On 2011/01/11 04:44:22, sosa wrote: > I think you should be able to do some better stuff with options parser to make > the help more useful. It doesn't seem that using --help will ever be useful. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:995: This is a shorthand for: if len(sys.argv) >= 1 and sys.argv[1] == '--help'. ...if you think it's too "clever" and less readable, I will change the syntax. On 2011/01/11 04:44:22, sosa wrote: > sys.argv[1]? http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:1015: Talked verbally about this. Hard to have arbitrary ordering with subcommands. On 2011/01/11 04:44:22, sosa wrote: > Why is there some strict ordering here :( http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.py File chromite/chromite_unittest.py (right): http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... chromite/chromite_unittest.py:113: def testCaseInsensitiveBuild(self): Added TODO for this. On 2011/01/11 04:44:22, sosa wrote: > Up to you but you may want to mock out the "known" commands so that these > unittests aren't fragile to the names of the commands. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite_unittest.... chromite/chromite_unittest.py:146: # Create our mox and stub out function calls used by _FindSpec()... Not going to in this case, though I can see your point. I'd rather keep this comment outside the docstring since this function would also be the place to put other setup code. In that case, I would still like to describe each code block in comments (not the docstring). On 2011/01/11 04:44:22, sosa wrote: > It almost seems for the setup and teardown's you should move the comments right > below the docstrings to the docstring.
Nits. LGTM once you address them.a On a side note, if I don't respond in a day and I'm not OOO, your review probably fell off my radar or I thought you didn't actually respond with code so please feel free to ping me :) http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:218: for dir_path in search_path: Nothing except if someone adds a directory that ends in .spec something weird might happen. On 2011/01/12 00:04:45, diandersAtChromium wrote: > They are not searched. Only the directory itself. Is there a compelling reason > that subtrees should be searched? > > On 2011/01/11 04:44:22, sosa wrote: > > What should happen with subtrees? > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:249: if spec_name and len(options) == 1: We're talking about milliseconds where something is interactive already. I often choose the slightly slower code when it comes to interactive code if it makes the code easier to read. It's up to you though. On 2011/01/12 00:04:45, diandersAtChromium wrote: > No. This is an extremely common case. I use it all the time: chromite build > mario > > On 2011/01/11 04:44:22, sosa wrote: > > This will almost never happen because of host special case. Not even sure if > > it's worth optimizing. > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:257: if choice is None: Hater :). On 2011/01/12 00:04:45, diandersAtChromium wrote: > No. > > On 2011/01/11 04:44:22, sosa wrote: > > Same as before. > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:598: if answer != 'YES': As a note, you are mimic'ing behavior already existing. Usually it's best to conform to the old style if it exists, but I'm ok I guess with making it more restrictive. On 2011/01/12 00:04:45, diandersAtChromium wrote: > Requiring exactly 'YES' was on purpose. Maybe that's a little too up-tight. > Changing to allow any capitalization of 'yes', but not just 'y'. If people get > too annoyed by it, we will make it looser. > > On 2011/01/11 04:44:22, sosa wrote: > > you have to type in caps? Generally it's lowercase and any prefix i.e. > > y,n,ye,no,yes and uppercase > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:753: _ = options Um, why not just do (_, argv) = parser blah then? On 2011/01/12 00:04:45, diandersAtChromium wrote: > Canonical way to make pylint and other automated checkers happy. From pylint > docs, you can see that pylint will not (by default) complain about the variables > "_" and "dummy" being unused. Thus, a good way to document that you have a > variable that is unused is to assign it to _. See "dummy-variables-rgx" in > pylint. > > This is probably more common in lua, where the standard language docs encourage > it. An example from the lua docs: http://www.lua.org/pil/20.3.html > > ...however, I believe it is also the convension in python. This stackoverflow > question might also be interesting: > http://stackoverflow.com/questions/1739514/as-variable-name-in-python > > On 2011/01/11 04:44:22, sosa wrote: > > ? > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:948: _DoDistClean(chroot_config, options.yes) I think we should have one way to do it. http://codereview.chromium.org/6005004/diff/51001/chromite/chromite.py#newcod... chromite/chromite.py:532: # We'll put CWD as src/scripts when running the command. Since everyone Nit: may be simpler to be an array and prepend/ append if clean_first. Here and same code above.
Addressed sosa's latest comments in code and responded in code review. Added blank sections to board specs for Anush (so it's easier to add stuff to them). Re-fixed unit tests, which I realized I had broken in the previous patch. They pass cleanly now. As per Anush, I will *NOT* be pushing this, but will instead be putting it (as is) to the new "chromite" repo. In that CL, I will reference this code review for anyone that wants history. http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py File chromite/chromite.py (right): http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:218: for dir_path in search_path: Ah, good point! Skipping all things where os.path.isfile() returns False. On 2011/01/13 19:07:04, sosa wrote: > Nothing except if someone adds a directory that ends in .spec something weird > might happen. > > On 2011/01/12 00:04:45, diandersAtChromium wrote: > > They are not searched. Only the directory itself. Is there a compelling > reason > > that subtrees should be searched? > > > > On 2011/01/11 04:44:22, sosa wrote: > > > What should happen with subtrees? > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:249: if spec_name and len(options) == 1: Not sure I understand. This code path avoids presenting the user with a menu of 1 item. It's not about speed, but avoiding the weird UI. Added a clearer comment. On 2011/01/13 19:07:04, sosa wrote: > We're talking about milliseconds where something is interactive already. I > often choose the slightly slower code when it comes to interactive code if it > makes the code easier to read. It's up to you though. > > On 2011/01/12 00:04:45, diandersAtChromium wrote: > > No. This is an extremely common case. I use it all the time: chromite build > > mario > > > > On 2011/01/11 04:44:22, sosa wrote: > > > This will almost never happen because of host special case. Not even sure > if > > > it's worth optimizing. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:598: if answer != 'YES': OK, will mimic other behavior. Done. On 2011/01/13 19:07:04, sosa wrote: > As a note, you are mimic'ing behavior already existing. Usually it's best to > conform to the old style if it exists, but I'm ok I guess with making it more > restrictive. > > On 2011/01/12 00:04:45, diandersAtChromium wrote: > > Requiring exactly 'YES' was on purpose. Maybe that's a little too up-tight. > > Changing to allow any capitalization of 'yes', but not just 'y'. If people > get > > too annoyed by it, we will make it looser. > > > > On 2011/01/11 04:44:22, sosa wrote: > > > you have to type in caps? Generally it's lowercase and any prefix i.e. > > > y,n,ye,no,yes and uppercase > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... chromite/chromite.py:753: _ = options Could do that. Somehow, I thought it was clearer in this case, but I guess not. I will change to your suggested syntax. On 2011/01/13 19:07:04, sosa wrote: > Um, why not just do (_, argv) = parser blah then? > > On 2011/01/12 00:04:45, diandersAtChromium wrote: > > Canonical way to make pylint and other automated checkers happy. From pylint > > docs, you can see that pylint will not (by default) complain about the > variables > > "_" and "dummy" being unused. Thus, a good way to document that you have a > > variable that is unused is to assign it to _. See "dummy-variables-rgx" in > > pylint. > > > > This is probably more common in lua, where the standard language docs > encourage > > it. An example from the lua docs: http://www.lua.org/pil/20.3.html > > > > ...however, I believe it is also the convension in python. This stackoverflow > > question might also be interesting: > > http://stackoverflow.com/questions/1739514/as-variable-name-in-python > > > > On 2011/01/11 04:44:22, sosa wrote: > > > ? > > > http://codereview.chromium.org/6005004/diff/51001/chromite/chromite.py#newcod... chromite/chromite.py:532: # We'll put CWD as src/scripts when running the command. Since everyone On 2011/01/13 19:07:04, sosa wrote: > Nit: may be simpler to be an array and prepend/ append if clean_first. > > Here and same code above. Done.
SGTM, LGTM On Thu, Jan 13, 2011 at 1:26 PM, <dianders@chromium.org> wrote: > Addressed sosa's latest comments in code and responded in code review. > > Added blank sections to board specs for Anush (so it's easier to add stuff > to > them). > > Re-fixed unit tests, which I realized I had broken in the previous patch. > They > pass cleanly now. > > > As per Anush, I will *NOT* be pushing this, but will instead be putting it > (as > is) to the new "chromite" repo. In that CL, I will reference this code > review > for anyone that wants history. > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py > File chromite/chromite.py (right): > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:218: for dir_path in search_path: > Ah, good point! Skipping all things where os.path.isfile() returns > False. > > On 2011/01/13 19:07:04, sosa wrote: >> >> Nothing except if someone adds a directory that ends in .spec > > something weird >> >> might happen. > >> On 2011/01/12 00:04:45, diandersAtChromium wrote: >> > They are not searched. Only the directory itself. Is there a > > compelling >> >> reason >> > that subtrees should be searched? >> > >> > On 2011/01/11 04:44:22, sosa wrote: >> > > What should happen with subtrees? >> > > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:249: if spec_name and len(options) == 1: > Not sure I understand. This code path avoids presenting the user with a > menu of 1 item. It's not about speed, but avoiding the weird UI. > > Added a clearer comment. > > On 2011/01/13 19:07:04, sosa wrote: >> >> We're talking about milliseconds where something is interactive > > already. I >> >> often choose the slightly slower code when it comes to interactive > > code if it >> >> makes the code easier to read. It's up to you though. > >> On 2011/01/12 00:04:45, diandersAtChromium wrote: >> > No. This is an extremely common case. I use it all the time: > > chromite build >> >> > mario >> > >> > On 2011/01/11 04:44:22, sosa wrote: >> > > This will almost never happen because of host special case. Not > > even sure >> >> if >> > > it's worth optimizing. >> > > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:598: if answer != 'YES': > OK, will mimic other behavior. Done. > > On 2011/01/13 19:07:04, sosa wrote: >> >> As a note, you are mimic'ing behavior already existing. Usually it's > > best to >> >> conform to the old style if it exists, but I'm ok I guess with making > > it more >> >> restrictive. > >> On 2011/01/12 00:04:45, diandersAtChromium wrote: >> > Requiring exactly 'YES' was on purpose. Maybe that's a little too > > up-tight. >> >> > Changing to allow any capitalization of 'yes', but not just 'y'. If > > people >> >> get >> > too annoyed by it, we will make it looser. >> > >> > On 2011/01/11 04:44:22, sosa wrote: >> > > you have to type in caps? Generally it's lowercase and any prefix > > i.e. >> >> > > y,n,ye,no,yes and uppercase >> > > > > http://codereview.chromium.org/6005004/diff/41001/chromite/chromite.py#newcod... > chromite/chromite.py:753: _ = options > Could do that. Somehow, I thought it was clearer in this case, but I > guess not. I will change to your suggested syntax. > > On 2011/01/13 19:07:04, sosa wrote: >> >> Um, why not just do (_, argv) = parser blah then? > >> On 2011/01/12 00:04:45, diandersAtChromium wrote: >> > Canonical way to make pylint and other automated checkers happy. > > From pylint >> >> > docs, you can see that pylint will not (by default) complain about > > the >> >> variables >> > "_" and "dummy" being unused. Thus, a good way to document that you > > have a >> >> > variable that is unused is to assign it to _. See > > "dummy-variables-rgx" in >> >> > pylint. >> > >> > This is probably more common in lua, where the standard language > > docs >> >> encourage >> > it. An example from the lua docs: http://www.lua.org/pil/20.3.html >> > >> > ...however, I believe it is also the convension in python. This > > stackoverflow >> >> > question might also be interesting: >> > > > http://stackoverflow.com/questions/1739514/as-variable-name-in-python >> >> > >> > On 2011/01/11 04:44:22, sosa wrote: >> > > ? >> > > > > http://codereview.chromium.org/6005004/diff/51001/chromite/chromite.py#newcod... > chromite/chromite.py:532: # We'll put CWD as src/scripts when running > the command. Since everyone > On 2011/01/13 19:07:04, sosa wrote: >> >> Nit: may be simpler to be an array and prepend/ append if > > clean_first. > >> Here and same code above. > > Done. > > http://codereview.chromium.org/6005004/ >
FYI: committed as: http://codereview.chromium.org/6343001/ - cros_build_lib stuff http://codereview.chromium.org/6334003/ - text Menu http://codereview.chromium.org/6308007/ - main chromite change. I will mark this change as abandoned. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
