Code review comment for lp://staging/~gary/zc.buildout/support-system-python

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>