On July 11, 2009, Gary Poster wrote: > Gary Poster has proposed merging lp:~gary/zc.buildout/support-system-python > into lp:~gary/zc.buildout/trunk. > > Requested reviews: > Francis J. Lacoste (flacoste) > > I have made a number of changes to zc.buildout in the > gary-support-system-python branch in zope.org's svn repository. They all > support Launchpad or Landscape's use of Buildout with a system Python, > directly or indirectly. > Wow, this was a lot of changes! From my cursory understanding, these changes looks good. I have a bunch of questions / suggestions. So from the Launchpad point of view, it seems to satisfy our use case. Thanks a lot, this will make zc.buildout a lot more robust for our use-case. > +# We have to manually parse our options rather than using one of the stdlib > +# tools because we want to pass the ones we don't recognize along to > +# zc.buildout.buildout.main. Wouldn't it have been possible to use a '--' marker to delimit the bootstrap options from the buildout ones: ./bootstrap.py --eggs eggs -- ZC.buildout options follows. That's a common idiom I think. > +match_equals = re.compile(r'(%s)=(\S*)' % ('|'.join(configuration),)).match By using this regex, you preclude any paths with spaces in them. I know many people avoid them because a lot of UNIX programs break on them, but I'm sure you'll encounter a lot of Mac OS X users that have spaces in some of their paths :-) > +# defaults > +tmpeggs = None The above comment doesn't make sense to me. > +if (configuration['--download-base'] and > + not configuration['--download-base'].endswith('/')): > + # download base needs a trailing slash to make the world happy > + configuration['--download-base'] += '/' > + I know this is something specific to our coding conventions, but comments are sentences needing a capital and a period ;-) > +if configuration['--version']: > + configuration['--version'] = '==' + configuration['--version'] I don't understand the reason for that code? I'm guessing it's because you are going to pass it over to setuptools? Maybe add a comment? > === modified file 'src/zc/buildout/buildout.py' > - prefer_final = options.get('prefer-final', 'false') > + prefer_final = options.setdefault('prefer-final', 'false') Is there any reason for the switch to setdefault? If it's because you are reusing options in other places, then I guess that's fine. Otherwise, I know some people find using setdefault() with it's return value a little confusing to read. > + options = self['buildout'] > + > + # Get a base working set for our distributions that corresponds to the > + # stated desires in the configuration. > + distributions = ['setuptools', 'zc.buildout'] You have trailing white space on that previous line. > === modified file 'src/zc/buildout/buildout.txt' I like the merging of the sections into the general option list. It might make them harder to find from the table of contents though. > === modified file 'src/zc/buildout/easy_install.py' > + > +def _get_system_packages(executable): > + """return a pair of the standard lib and site packages for the executable. > + """ > + # We want to get a list of the site packages, which is not easy. The > + # canonical way to do this is to use distutils.sysconfig.get_python_lib(), > + # but that only returns a single path, which does not reflect reality for > + # many system Pythons, which have multiple additions. Instead, we start > + # Python with -S, which does not import site.py and set up the extra paths > + # like site-packages or (Ubuntu/Debian) dist-packages and python- support. > + # We then compare that sys.path with the normal one. The set of the normal > + # one minus the set of the ones in ``python -S`` is the set of packages > + # that are effectively site-packages. > + def get_sys_path(clean=False): > + cmd = [executable, "-c", > + "import sys, os;" > + "print repr([os.path.normpath(p) for p in sys.path])"] > + if clean: > + cmd.insert(1, '-S') > + _proc = subprocess.Popen( > + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) > + stdout, stderr = _proc.communicate(); > + if _proc.returncode: > + raise RuntimeError( > + 'error trying to get system packages:\n%s' % (stderr,)) > + res = eval(stdout) Woa, eval! You like playing with fire! I would simply have use writelines(), readlines()... but heh, if the executable is compromised... all things are loose. I guess there is no danger here. > @@ -650,34 +732,52 @@ > self._maybe_add_setuptools(ws, dist) > > # OK, we have the requested distributions and they're in the working > - # set, but they may have unmet requirements. We'll simply keep > - # trying to resolve requirements, adding missing requirements as they > - # are reported. > - # > - # Note that we don't pass in the environment, because we want > + # set, but they may have unmet requirements. We'll resolve these > + # requirements. This is code modified from > + # pkg_resources.WorkingSet.resolve. We can't reuse that code directly > + # because we have to constrain our requirements (see > + # versions_section_ignored_for_dependency_in_favor_of_site_packages in > + # zc.buildout.tests). Wouldn't it make tracking divergence easier if you moved the "code modified from" into a separate function? > + # > + requirements.reverse() # set up the stack > + processed = {} # set of processed requirements > + best = {} # key -> dist > + # > + # Note that we don't use the existing environment, because we want > # to look for new eggs unless what we have is the best that > # matches the requirement. > - while 1: > - try: > - ws.resolve(requirements) > - except pkg_resources.DistributionNotFound, err: > - [requirement] = err > - requirement = self._constrain(requirement) > - if dest: > - logger.debug('Getting required %r', str(requirement)) > - else: > - logger.debug('Adding required %r', str(requirement)) > - _log_requirement(ws, requirement) > - > - for dist in self._get_dist(requirement, ws, self._always_unzip > - ): > - > - ws.add(dist) > - self._maybe_add_setuptools(ws, dist) > - except pkg_resources.VersionConflict, err: > - raise VersionConflict(err, ws) > - else: > - break > + env = pkg_resources.Environment(ws.entries) > + while requirements: > + # process dependencies breadth-first > + req = self._constrain(requirements.pop(0)) > + if req in processed: > + # Ignore cyclic or redundant dependencies > + continue > + dist = best.get(req.key) > + if dist is None: > + # Find the best distribution and add it to the map > + dist = ws.by_key.get(req.key) > + if dist is None: > + try: > + dist = best[req.key] = env.best_match(req, ws) > + except pkg_resources.VersionConflict, err: > + raise VersionConflict(err, ws) > + if dist is None: > + if dest: dest? Is it a typo (of dist) or something else? (I don't see a dest in the context.) > + logger.debug('Getting required %r', str(req)) > + else: > + logger.debug('Adding required %r', str(req)) > + _log_requirement(ws, req) > + for dist in self._get_dist(req, > + ws, self._always_unzip): > + ws.add(dist) > + self._maybe_add_setuptools(ws, dist) > + if dist not in req: > + # Oops, the "best" so far conflicts with a dependency > + raise VersionConflict( > + pkg_resources.VersionConflict(dist, req), ws) > + requirements.extend(dist.requires(req.extras)[::-1]) > + processed[req] = True > @@ -969,7 +1121,7 @@ > if relative_paths: > relative_paths = os.path.normcase(relative_paths) > sname = os.path.normcase(os.path.abspath(sname)) > - spath = ',\n '.join( > + spath = ',\n '.join( > [_relativitize(os.path.normcase(path_item), sname, relative_paths) > for path_item in path] > ) > @@ -977,7 +1129,7 @@ > for i in range(_relative_depth(relative_paths, sname)): > rpsetup += "base = os.path.dirname(base)\n" > else: > - spath = repr(path)[1:-1].replace(', ', ',\n ') > + spath = repr(path)[1:-1].replace(', ', ',\n ') > rpsetup = '' > return spath, rpsetup Is the change in indentation intentional? > === modified file 'src/zc/buildout/tests.py' > @@ -2533,6 +2789,8 @@ > def create_sample_eggs(test, executable=sys.executable): > write = test.globs['write'] > dest = test.globs['sample_eggs'] > + site_packages = test.globs['tmpdir']('site_packages') > + test.globs['site_packages'] = site_packages > tmp = tempfile.mkdtemp() > try: > write(tmp, 'README.txt', '') > @@ -2549,6 +2807,8 @@ > % (i, c1) > ) > zc.buildout.testing.sdist(tmp, dest) > + if i==1: > + zc.buildout.testing.sys_install(tmp, site_packages) > > write( > tmp, 'setup.py', > @@ -2578,6 +2838,8 @@ > " zip_safe=True, version='0.%s%s')\n" % (i, c1) > ) > zc.buildout.testing.bdist_egg(tmp, executable, dest) > + if i==3: > + zc.buildout.testing.sys_install(tmp, site_packages) > I don't understand the above changes. > @@ -2652,6 +2914,49 @@ > test.globs['sample_eggs']) > test.globs['update_extdemo'] = lambda : add_source_dist(test, 1.5) > zc.buildout.testing.install_develop('zc.recipe.egg', test) > + # most tests don't need this set up, and it takes some time, so we just > + # make it available as a convenience. Sentence starts with a capital :-) -- Francis J. Lacoste