On July 13, 2009, Gary Poster wrote:
> On Jul 13, 2009, at 5:06 PM, Francis J. Lacoste wrote:
> > 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!
>
> Yeah, sorry.
>
> >> 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.
>
> It is, but it would break backwards compatibility of the usage of this
> script.  This was a design decision from before I came a long.  If Jim
> were OK with breaking backwards compatibility I'd be happy to go this
> way.
>

I see, it's fine like it is then.

> > === 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.
>
> It is because when buildout uses -vv, it is supposed to show you all
> configuration values, as shown in buildout.txt in the "Predefined
> buildout options" section.  To appear in this output, they need to be
> in the options dict.  Therefore, when appropriate, I switched
> from .get to .setdefault so that the options would get the default.
> It seemed like a reasonable, concise, and idiomatic way of
> accomplishing this.  Otherwise, we'd need something like this.
>
> prefer_final = options.get('prefer-final', 'false')
> options['prefer-final'] = prefer_final
>
> I prefer the setdefault approach, but I'm happy to change.

No, setdefault is fine then.

> >
> >> +    # 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.
>
> That would be nice though, you're right.  this was taken from my quick
> and dirty proof of concept.  If you want me to change it, let me know.

No, that's fine.


>
> My approach (the first) simplifies the code path quite a bit, I
> think.  If you follow what's going on in the second code path, I think
> you might agree.
...

>
> That said, if you and Jim agree on changing to the second approach,
> obviously it's easy enough.

Nah, what is there is fine.


Changes look good.

Thanks a lot.

  review approve

-- 
Francis J. Lacoste
<email address hidden>