Initial version of what will become the best installer in the world - ever

Review Request #1696 — Created June 28, 2010 and discarded

Information

rb-fairy
master

Reviewers

Initial version of installer with example json file structure for ubuntu that shows working source, easy_install and apt type installs.
All manual - there might be a slight problem with lucene which needs a patch (one more command in source type install for it).
MA
mike_conley
  1. Mario:
    
    Nice work.
    
    I've mostly tackled style nit-picks here, since I had my knuckles rapped for similar issues.
    
    Basically it comes down to this:
    -imports should be alphabetical (in fact, it seems to be:  if there's an unordered list of things, you might as well alphabetize)
    -4 spaces per indentation, not 8.
    -In most cases, pad if, if/else, and for blocks with spaces above and below.
    
    Once those are fixed up, I'll take another look.
    
    Thanks,
    
    -Mike
    1. Thank you Mike for all your comments, I'll make sure to fix those - I am well aware that the code must conform to PEP8,
      thus having 4 spaces, and certainly not mix tabs and spaces. I will also fix trailing whitespaces.
  2. input.json (Diff revision 1)
     
     
    Hm - I still think it's weird to have an array of single entry dicts like this.
    
    Didn't we agree upon:
    
    "database": {
      "mysql": {
       ...
       }
      "pqsql": {
       ...
       }
       ...etc?
    }
    1. An array is afaik an only way to guarantee the same order every time your parse it - that way I can easily select first entry as default, which is mysql.
  3. install.py (Diff revision 1)
     
     
     
     
     
     
     
     
    From what I gather, we want these in alphabetical order, so:
    
    import os
    import platform
    ...
    
  4. install.py (Diff revision 1)
     
     
    Other style nitpick - we want 4 spaces per indentation, not 8.
  5. install.py (Diff revision 1)
     
     
    typo: "an" -> "and"
  6. install.py (Diff revision 1)
     
     
    space after this
  7. install.py (Diff revision 1)
     
     
    space before this
  8. install.py (Diff revision 1)
     
     
    space before this
  9. install.py (Diff revision 1)
     
     
    space after this
  10. install.py (Diff revision 1)
     
     
    space before this
  11. install.py (Diff revision 1)
     
     
    space before this
  12. install.py (Diff revision 1)
     
     
    space before and after this
  13. install.py (Diff revision 1)
     
     
    space before and after this
  14. install.py (Diff revision 1)
     
     
    space before this
  15. install.py (Diff revision 1)
     
     
    space before this
  16. 
      
MA
  1. 
      
    1. Matthew, thanks for the reviews - you rock. I'll review your remarks, and apply as necessary.
  2. install.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Personally I put them in order of lowest level to highest level, so common stuff at the top, and more specialized stuff at the bottom .. old habit .. it also happily turns out to make it look nice line length wise :)
    
    import os
    import sys
    import shutil
    import platform
    import simplejson as json
    from types import ListType
    from subprocess import Popen, PIPE
    
    But really no-one cares .. it's just whatever you're comfortable with and whatever makes it easy to read later; alphabetical is good too.
  3. install.py (Diff revision 1)
     
     
    According to good old guido's python style guide, class is preceded with two blank lines. That also goes for tabs characters, they should all be 4 spaces I reckon:
    
    http://www.python.org/dev/peps/pep-0008/
  4. install.py (Diff revision 1)
     
     
    'file' is a reserved keyword. I wouldn't use it for variable nor attribute names myself.
    
    Also I see that the file is actually a filename so probably I'd call it fileName :) 
  5. install.py (Diff revision 1)
     
     
    No need for the brackets there:
    
    self.arch, self.distro = self._get_osarch()
    
    is fine
  6. install.py (Diff revision 1)
     
     
     
     
     
     
    No need for the i = 0 nor i = i + 1. Just use enumerate:
    
    for i, o in enumerate(options):
        ...
    
    does the same thing
  7. install.py (Diff revision 1)
     
     
    Use += ..
    
    opt_str += "[%d] %s\n" % (i, o)
    
    is the same result
  8. install.py (Diff revision 1)
     
     
    Another good += candidate
  9. install.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Personally I don't like the redundant use of the valid variable .. it might be OK with some people, but I'd prefer something like:
    
    while 1:
       do stuff
       if done:
           break
    
    sort of style
  10. install.py (Diff revision 1)
     
     
    I'd add a try..except around the 'int' conversion, and only need to convert it once .. maybe something like:
    
                try:
                    choice = int(choice)
                except:
                    continue  # Basically the choice is not an int, just ignore it .. probably should print some insult or something :P
    			if choice in range(0, len(options)):
    				break
    		return choice
    
  11. install.py (Diff revision 1)
     
     
    evil line with whitespace and nothing on it  EVIL!
  12. install.py (Diff revision 1)
     
     
     
    use .items() instead of .keys():
    
    for key, pkg in depmap.items():
  13. install.py (Diff revision 1)
     
     
     
    You can just add lists together, no need for the second loop. Either:
    
    options += opt.keys()
    
    or:
    
    options.extend(opt.keys())
  14. install.py (Diff revision 1)
     
     
     
     
     
     
     
     
    This is just another option, I like this style myself, but some may see it as a bit freaky:
    
    
                    descr = {
                        'database': 'Choose database:',
                        'web_server': 'Choose web server:'
                    }.get(key, 'Not set!')
  15. install.py (Diff revision 1)
     
     
    re-route not reroot.. haha, I bet I'm starting to annoy now
  16. install.py (Diff revision 1)
     
     
     
     
    You can use a list comprehension here:
    
    pkg_list = [pkg['pkg_name'] for pkg in self.pkgmgr_list]
    
    
  17. install.py (Diff revision 1)
     
     
     
    self._source_install() already loops through self.source_list(), no need to call it once for every item.
  18. install.py (Diff revision 1)
     
     
    Might want to capture stderr too here .. just incase
  19. install.py (Diff revision 1)
     
     
    I'd use a try: finally: block here
    
            oldDir = os.getcwd()
    		os.mkdir('/tmp/reviewboard_installer')
            try:
    
    ...
    
    
            finally:
                os.chdir(oldDir)
                shutil.rmtree('/tmp/reviewboard_installer')
    
  20. 
      
MA
MA
MA
david
  1. 
      
  2. input.json (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    I'm not exactly sure what the semantics of "an array of dicts of a single element" are here. Is there a particular meaning?
    
    If not, I'd like to see it just be one dict:
    
    {
        "mysql" : { ... },
        "pgsql" : { ... },
        ...
    }
  3. input.json (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
    Indent is 4 spaces.
  4. install.py (Diff revision 4)
     
     
    You don't need the parens here.
  5. install.py (Diff revision 4)
     
     
    Can you catch only ValueError?
  6. install.py (Diff revision 4)
     
     
    Since you're doing a numeric comparison, it'd be a lot easier to use >= 0 and < len(options).
    
    The way you've written this it's generating a list and then checking every element.
  7. install.py (Diff revision 4)
     
     
     
     
     
     
     
    I wonder if instead of having 3 lists and checking the string for each one, if it might be cleaner to have a dict of method name : list
    
    {
        'pkgmgr': [],
        'easy_install': [],
        'source': []
    }
    
    and then you could just do self.pkgs[method].append(pkg)
  8. install.py (Diff revision 4)
     
     
     
    I'm not sure it's valid to assume that 64-bit == x86_64
  9. install.py (Diff revision 4)
     
     
    After this, you should probably check p1.returncode to make sure that it returned 0 and not an error.
  10. install.py (Diff revision 4)
     
     
    Same here.
  11. install.py (Diff revision 4)
     
     
    And again...
  12. install.py (Diff revision 4)
     
     
    Can you explicitly pass in the mode as 0700? By default os.mkdir() uses 0777, which is a pretty big security risk when this is (presumably) running as a privileged user.
  13. install.py (Diff revision 4)
     
     
    Hmm, so your installer has a dependency on wget? And it's assuming that everything's a .tar.gz?
  14. 
      
MA
Review request changed
Change Summary:
Took into account a lot of comments from previous reviews and irc talks. Error handling is now much better.
Versions stuff and description are only provisional, and will be implemented properly later in the process
- we might switch to description being hard-coded later on, but as I designed json files it made sense
to put them in there, at least temporarily.
MA
  1. 
      
  2. install.py (Diff revision 5)
     
     
    class File1(object):
  3. install.py (Diff revision 5)
     
     
     
    I'd just add a function comment in here:
    
    """Loads our settings from the provided json config file"""
  4. install.py (Diff revision 5)
     
     
     
    Redundant function ?
  5. install.py (Diff revision 5)
     
     
     
    redundant function ?
  6. 
      
MA
  1. 
      
  2. install.py (Diff revision 5)
     
     
    It'd be nice to have a nice comment here explaining what the function does, perhaps with an example output too..
  3. install.py (Diff revision 5)
     
     
     
     
     
     
     
     
    You can get rid of the extra 'i' variable by using enumerate:
    
    
                default_num = -1
                for (k, v), i in self.depmap[cat]['options'].items():
                    opt_str += "[%d] %s: %s\n" % (i, v['name'], v['description'])
                    if k == self.depmap[cat]['default']:
                        default_num = i
    
  4. install.py (Diff revision 5)
     
     
    I'd probably go with:
    
    choice = raw_input(opt_str).strip()
    
    Just to remove extra spaces
  5. 
      
MA
  1. 
      
  2. install.py (Diff revision 5)
     
     
    In my code I make all top level classes children of object:
    
    class File2(object):
    
    This makes it a 'new style class' rather than an 'old style class'
    
    http://www.python.org/doc/newstyle/
  3. install.py (Diff revision 5)
     
     
     
    Function comments should be in the form of a string. That way autodoc tools like Sphinx pick it up well. And they can compile all of your todos into a single page and cool stuff like that
  4. install.py (Diff revision 5)
     
     
    evil trailing whitespace
  5. install.py (Diff revision 5)
     
     
     
     
    dep = self.depmap[pkg]
    pkgver = dep['default']
    pkgdata = dep[pkgver]
    method = pkgdata['method']
    if method == 'pkgmgr':
    ...
    # Just less lookups .. nit picking .. sorry
    
    If you wanted to be really pythony, you could switch the if statements to:
    
    myList = {'pkgmgr': self.pkgmgr_list, 'easy_install', self.easy_list}.get(method, self.source_list)
    myList.append(pkg)
    
    in fact it might be a neat idea to init that dictionary of list names to actual lists in the __init__:
    
    self.name2list = {'pkgmgr': self.pkgmgr_list, 'easy_install', self.easy_list}
    
    then you could use it where ever with:
    
    myList = self.name2list.get(methodName, self.source_list)
  6. install.py (Diff revision 5)
     
     
     
    I probably would move the file reading out to a 'process' or 'read' method .. just a personal preference, and keeps with how most python classes are .. you init a 'reader' object, then you fill with some file..
  7. install.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This whole block looks similar to above, I'm sure you could probably merge them into a single reusable func. Possibly a base class or utility class sort of thing
  8. install.py (Diff revision 5)
     
     
     
     
    could be done with enumerate as above
    
    for (k, v), i in enumerate(self.depmap.items()):
    ...
  9. install.py (Diff revision 5)
     
     
     
    either:
    
    deps = self.depmap[self.version]['dependencies']['requirements'][:]
    
    or:
    
    deps = []
    deps.extend(self.depmap[self.version]['dependencies']['requirements'])
  10. install.py (Diff revision 5)
     
     
     
    deps.extend(self.depmap[self.version]['dependencies']['optional'])
    
    or possibly:
    
    deps += self.depmap[self.version]['dependencies']['optional']
  11.