Code review comment for lp:~bjornt/lazr-js/buildoutification
Revision history for this message
| Björn Tillenius (bjornt) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
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.
> - 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:/
> change affects .bzrignore, and the Makefile (including the bin/buildout
> target).
There is some value in always using a download-cache. 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.
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 see any reason to specify the BUILDOUT_CFG in the Makefile.
> There's only one.
Right, this was copied from LP; removed.
> - ``make clean`` might as well just trash the bin directory. ***
Right, makes sense; done.
> - We can just use the interpreter option for zc.recipe.egg to produce
> the py file, and remove the z3c.recipe.
> buildout-templates directory). We made the py file in Launchpad like
> that because Launchpad's Makefile wanted to use the -t option. (I have
> a branch I'm working on for Launchpad that switches it to using the
> interpreter option also.) ***
Indeed, this is simpler, this was another copy from LP. I've changed it
to use the interpreter option and removed the z3c.recipe.
dependency.
> - The entry points maybe ought to be in the setup.py file, so someone
> can use it without buildout. Maybe. Anyway, might as well put it in
> the expected place. ***
Yep, having them in setup.py is better, I didn't know that it was
enough. Fixed.
> - No need to specify extra-paths in buildout.cfg. You are already
> doing the right thing in setup.py. ***
Yes. It was necessary at one point, but now it's not needed anymore, so
I've removed it.
> - 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.
> - 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?
> 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.
--
Björn Tillenius | https:/