Code review comment for lp:~bjornt/lazr-js/buildoutification

On Oct 8, 2009, at 9:24 AM, Björn Tillenius wrote:

> On Wed, Oct 07, 2009 at 08:11:57PM -0000, Gary Poster wrote:
>> Review: Needs Fixing
>> Hi Bjorn. This is great to see.
>>
>> I think we can simplify the setup. I don't know if we need as many
>> of
>> the tricks that Launchpad uses. Here's a diff that removes a lot of
>> them. This should be regarded as a conversation starter, rather
>> than as
>> a concrete suggestion. I'll call out the ideas here, and then you
>> can
>> see them concretely in the diff. I've marked the items that I
>> strongly
>> think are valid with three stars (***).
>
> Right, I was meaning for this MP to be the start of a conversation, so
> that we had a diff we could look at. Many of the things I simply took
> from LP.

Cool.

>> - Launchpad keeps a separate, shared download-cache and eggs. That
>> makes sense if the build is happening on a machine that does not
>> allow
>> network access, or if we want to prevent being exposed to PyPI
>> service
>> interruptions, or if we want to use custom distributions. On the
>> other
>> hand, if we remove that, we can have a simpler build. If developers
>> want shared eggs, they can set up a global cache
>> (https://dev.launchpad.net/HackingLazrLibraries#Global%20Cache).
>> This
>> change affects .bzrignore, and the Makefile (including the bin/
>> buildout
>> target).
>
> There is some value in always using a download-cache.

Agreed. There are other ways to get some of the same value (a repo
managed by Canonical/Launchpad would give us the benefit of protection
from PyPI outages, for instance). But still agreed.

> The main reason is
> would be to help people remember adding things to the download-cache
> branch, whenever they add a new dependencies. We do have want to run
> tests using PQM or buildbot, and those machines won't have net access.

buildbot has net access, but PQM doesn't. Sometimes developers don't
have net access, also, and a download-cache lets them be productive
then.

> Although, I said I wanted to worry about this after landing this
> branch
> (since for LP to use it it doesn't matter), so I guess I could
> remove it
> for now, and we can decide what to do later.

I don't have a strong opinion about it. I just want to point out some
possible simplifications for your consideration.

...

>> - If we change the Makefile to use -S whenever it calls Python, we
>> don't need my custom releases of buildout that handle a system Python
>> (which Jim still has on his queue to review), so we can just use
>> released versions of the various packages. (On the other hand, if
>> Jim
>> approves my changes, we could arguably just get rid of the Makefile
>> entirely and instruct people to run ``python bootstrap.py &&
>> bin/buildout && bin/build`` or something to get the build.)
>
> So, I think this change sounds good, but could you expand a bit more
> on
> that the difference between 'python -S bin/build' and 'bin/build'
> is? I
> don't quite understand why the former is better.

Because not running site.py (python -S) means that site-packages are
not included, which means that we don't get complications from using a
system Python with site-packages, which is what my zc.buildout branch
addresses. By using -S, we just make the problem go away, simply.

An alternative solution is to tell people to use a clean Python, with
an empty site-packages. That's what Jim recommends for zc.buildout
generally.

>
>
>> - Including ez_setup.py was another artifact of wanting to be able to
>> build on a machine that had no outside net access. If we don't
>> need to
>> support that, we can remove it (and the reference in setup.py).
>
> I think we need to support this to have the tests run in a buildbot
> (which doesn't have net access), right?

buildbot has net access. PQM doesn't. developers sometimes don't.
IMO, it's the same basic decision as the download-cache. If you want
one, you want the other; if you don't one, don't have the other.

> If you agree with all of these thoughts, you can take the diff as is.
>> Otherwise, hopefully it will still give us something to talk about
>> it.
>> If you just change the starred items, and push back convincingly on
>> the
>> rest, that will probably be fine.
>
> I've applied parts of the diff. I'm attaching the changes I've done so
> far. Changing the Makefile using -S sounds reasonable, but I'd like to
> understand why before doing it.
>
> I propose pushing the decision of whether to force the use a
> download-cache for later, after this branch has landed, so that we can
> get something landed now, and worry about the details (which don't
> matter for LP's use) later.

I think that will work fine.

Gary