Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8965033:
Adds Python package management to depot_tools. (Closed)
DescriptionAdds Python package management to depot_tools.
This CL introduces package_management.py, a module for managing third-party Python packages in a site directory within depot_tools. A future CL will integrate this with depot_tools, causing the packages to be downloaded and installed on first run after that CL lands. Once in place presubmit_canned_checks will be extended to make use of the locally installed version of pylint.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120225
Patch Set 1 #Patch Set 2 : . #
Total comments: 37
Patch Set 3 : Addressed review comments. #
Total comments: 9
Patch Set 4 : Addressed nits and pylint messages. #Patch Set 5 : Update pylint blacklist. #
Messages
Total messages: 12 (1 generated)
Would you mind taking a quick look, thinking more about design/approach? Before pushing further I want to make sure this seems sane to everyone. (Unittests to follow.)
This looks pretty sane to me, though M-A will need to comment on how appropriate this is for depot_tools. I'd probably use logging instead of printing, but again M-A's preference wins. http://codereview.chromium.org/8965033/diff/6001/package_management.py File package_management.py (right): http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcode71 package_management.py:71: SITE_DIR = os.path.join(ROOT_DIR, 'site-packages-py%s' % sys.version[0:3]) format from sys.version_info, perhaps? http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcode93 package_management.py:93: perform an endrun around setuptools for the purpose of bootstrapping. Its nit: endrun->end-run? http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:292: def CreateOrAddSiteDirectory(path): Maybe name EnsureSiteDirectory?
http://codereview.chromium.org/8965033/diff/6001/.gitignore File .gitignore (right): http://codereview.chromium.org/8965033/diff/6001/.gitignore#newcode24 .gitignore:24: /depot_tools_testing_lib/_rietveld This was wrong btw. http://codereview.chromium.org/8965033/diff/6001/.gitignore#newcode31 .gitignore:31: /.subversion These are from your home dir, I don't understand. http://codereview.chromium.org/8965033/diff/6001/package_management.py File package_management.py (right): http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:140: """Makes a safe name or safe version safe to use in a file name. Stylistic note; I'm not a fan of functions with a longer docstring that then implementation, especially when the implementation is a simple re.sub() call. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:227: # If the path where the object was downloaded to does not match the That may happen? http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:233: raise Error, sys.exc_info()[1], sys.exc_info()[2] Use the new format; raise Error(sys.exc_info()[1], sys.exc_info()[2]) or whatever want to achieve. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:244: path: the path to add. A path is a path, don't document that. Silly style reviewers. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:351: sys.stdout = string_stdout No need to name the variables, just sys.stdout = cStringIO.StringIO() http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:354: function(*args, **kwargs) return function(*args, **kwargs) http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:358: return remove http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:386: raise InstallError, sys.exc_info()[1], sys.exc_info()[2] same everywhere http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:403: result = subprocess.call([sys.executable, __file__, pycode]) return not subprocess.call(... http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:438: """Bootstraps the runtime with setuptools. It'd be nice if it was documented that it's meant to be run from a child process. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:510: stat = os.stat(path) return os.stat(path).st_mtime http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:609: exec('import %s' % package_name) __import__(package_name) http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:614: exec('result = ModuleIsFromPackage(%s, SITE_DIR)' % package_name) ?? result = ModuleIsFromPackage(package_name, SITE_DIR) because you want a pointer to package_name as a module? Note that the exec call above redefines package_name from a string into a module. That's confusing. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:631: result = False result = 0 http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:633: return 0 if result else 1 return result and have the function return relevant return code? Note that I don't really mind either way so you can ignore that.
PTAnotherL http://codereview.chromium.org/8965033/diff/6001/.gitignore File .gitignore (right): http://codereview.chromium.org/8965033/diff/6001/.gitignore#newcode24 .gitignore:24: /depot_tools_testing_lib/_rietveld On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > This was wrong btw. Do you want it removed? http://codereview.chromium.org/8965033/diff/6001/.gitignore#newcode31 .gitignore:31: /.subversion On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > These are from your home dir, I don't understand. Hmm... they *should* be from my home directory but they somehow showed up in my /src/depot_tools. Weird. Removed. http://codereview.chromium.org/8965033/diff/6001/package_management.py File package_management.py (right): http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcode71 package_management.py:71: SITE_DIR = os.path.join(ROOT_DIR, 'site-packages-py%s' % sys.version[0:3]) On 2011/12/17 15:26:03, Ruðrugis wrote: > format from sys.version_info, perhaps? Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcode93 package_management.py:93: perform an endrun around setuptools for the purpose of bootstrapping. Its On 2011/12/17 15:26:03, Ruðrugis wrote: > nit: endrun->end-run? Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:140: """Makes a safe name or safe version safe to use in a file name. On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > Stylistic note; > > I'm not a fan of functions with a longer docstring that then implementation, > especially when the implementation is a simple re.sub() call. Noted ;) http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:227: # If the path where the object was downloaded to does not match the On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > That may happen? There's some mention of caching; if a particular file has already been downloaded via urllib, and is redownloaded, it will return the path to the original download (so say the docs). It is very doubtful (somebody else in this process would have had to have downloaded the same file previously), but apparently possible. Per the docs: "If the URL points to a local file, or a valid cached copy of the object exists, the object is not copied." Come to think of it, there's no need for us to make a copy only to delete it later on. I'll simply return the path given me by urlretrieve. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:233: raise Error, sys.exc_info()[1], sys.exc_info()[2] On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > Use the new format; > raise Error(sys.exc_info()[1], sys.exc_info()[2]) > > or whatever want to achieve. That's not quite equivalent. I want to recast the error type, but keep the same value and traceback. This is the only way I know how to do that. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:244: path: the path to add. On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > A path is a path, don't document that. > > Silly style reviewers. Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:292: def CreateOrAddSiteDirectory(path): On 2011/12/17 15:26:03, Ruðrugis wrote: > Maybe name EnsureSiteDirectory? Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:351: sys.stdout = string_stdout On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > No need to name the variables, just > sys.stdout = cStringIO.StringIO() Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:354: function(*args, **kwargs) On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > return function(*args, **kwargs) Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:358: return On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > remove Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:403: result = subprocess.call([sys.executable, __file__, pycode]) On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > return not subprocess.call(... Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:438: """Bootstraps the runtime with setuptools. On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > It'd be nice if it was documented that it's meant to be run from a child > process. Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:609: exec('import %s' % package_name) On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > __import__(package_name) Well, you learn something new everyday! Done. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:614: exec('result = ModuleIsFromPackage(%s, SITE_DIR)' % package_name) On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > ?? > result = ModuleIsFromPackage(package_name, SITE_DIR) > > because you want a pointer to package_name as a module? Note that the exec call > above redefines package_name from a string into a module. That's confusing. That's the intended result. Made more clear by using __import__, and keeping a pointer to the actual module. http://codereview.chromium.org/8965033/diff/6001/package_management.py#newcod... package_management.py:633: return 0 if result else 1 On 2011/12/19 21:04:23, Marc-Antoine Ruel wrote: > return result and have the function return relevant return code? > > Note that I don't really mind either way so you can ignore that. For consistency, I made all functions return True/False success status, and have the 'translation' happen in Main(). I'll keep it this way with a little comment added here.
lgtm with a few nits. http://codereview.chromium.org/8965033/diff/12001/.gitignore File .gitignore (right): http://codereview.chromium.org/8965033/diff/12001/.gitignore#newcode4 .gitignore:4: # Ignore scripts that are generated by Mac/Linux bootstrapping. FYI, it's Windows bootstrapping. They should probably disappear. I don't mind about the comment. http://codereview.chromium.org/8965033/diff/12001/package_management.py File package_management.py (right): http://codereview.chromium.org/8965033/diff/12001/package_management.py#newcode9 package_management.py:9: PyPi repository. It is intended to work with any version of Python from 2.4 Don't claim to support 2.4 unless you plan to support it. I don't. It'll just work with whatever python version we embed, and maybe other version but I don't want to guarantee that. So just remove this sentence. http://codereview.chromium.org/8965033/diff/12001/package_management.py#newco... package_management.py:160: if extension[0] != '.': if not extension.startswith('.'): http://codereview.chromium.org/8965033/diff/12001/package_management.py#newco... package_management.py:214: """Adds the provided path to the head of PYTHONPATH and sys.path.""" path = os.path.abspath(path) http://codereview.chromium.org/8965033/diff/12001/package_management.py#newco... package_management.py:416: except: Not a fan. (IOError, OSError) would probably be fine? http://codereview.chromium.org/8965033/diff/12001/package_management.py#newco... package_management.py:424: raise Error raise Error()
Oops, this was languishing in backlog. Committing. http://codereview.chromium.org/8965033/diff/12001/.gitignore File .gitignore (right): http://codereview.chromium.org/8965033/diff/12001/.gitignore#newcode4 .gitignore:4: # Ignore scripts that are generated by Mac/Linux bootstrapping. On 2012/01/09 02:34:15, Marc-Antoine Ruel wrote: > FYI, it's Windows bootstrapping. They should probably disappear. I don't mind > about the comment. Removed comment, consolidated with rest of bootstrapping ignores. http://codereview.chromium.org/8965033/diff/12001/package_management.py File package_management.py (right): http://codereview.chromium.org/8965033/diff/12001/package_management.py#newcode9 package_management.py:9: PyPi repository. It is intended to work with any version of Python from 2.4 On 2012/01/09 02:34:15, Marc-Antoine Ruel wrote: > Don't claim to support 2.4 unless you plan to support it. I don't. It'll just > work with whatever python version we embed, and maybe other version but I don't > want to guarantee that. So just remove this sentence. I tested it on 2.4, assuming that depot_tools still supported 2.4 (I thought I read that somewhere). Anyhow, removed the comment. http://codereview.chromium.org/8965033/diff/12001/package_management.py#newco... package_management.py:416: except: On 2012/01/09 02:34:15, Marc-Antoine Ruel wrote: > Not a fan. (IOError, OSError) would probably be fine? Not strictly needed. Removed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisha@chromium.org/8965033/18001
Presubmit check for 8965033-18001 failed and returned exit status 1. warning: code.google.com certificate with fingerprint e9:92:78:b1:ec:f5:64:c6:fc:fb:0c:5b:ee:e0:71:18:6c:e9:42:28 not verified (check hostfingerprints or web.cacerts config setting) Running presubmit commit checks ... ************* Module package_management W0612:204,13:Package.DownloadEgg: Unused variable 'headers' E1101:235,2:AddSiteDirectory: Module 'site' has no 'addsitedir' member W0612:327,36:InstallPackage: Unused variable 'e' W0612:396,4:BootstrapSetupTools: Unused variable 'setuptools' W0702:449,2:_GetModTime: No exception type(s) specified Checking out rietveld... Running push-basic.sh Running save-description-on-failure.sh Running basic.sh Running upload-stale.sh Running upload-local-tracking-branch.sh Running patch.sh Running submit-from-new-dir.sh Running post-dcommit-hook-test.sh Running hooks.sh Running abandon.sh Running upstream.sh ** Presubmit Messages ** You should install python 2.5 and run ln -s $(which python2.5) python. A great place to put this symlink is in depot_tools. Otherwise, you break depot_tools on python 2.5, you get to keep the pieces. ** Presubmit ERRORS ** Fix pylint errors first. Presubmit checks took 374.1s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisha@chromium.org/8965033/27007
Change committed as 120225
Message was sent while issue was closed.
jslv.lrge@gmail.com changed reviewers: + jslv.lrge@gmail.com
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
