launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Review: Needs Fixing

Looks good. Lots of comments, mostly trivial. However, setup.py is
broken, so Needs Fixing.


[1]

+You will need to manually install Postgres 9.1 (postgresql-9.1),
+, RabbitMQ (rabbitmq-server), python-dev and make::

s/^, //


[2]

+Additionally, you probably want to install the following python libraries
+for development convenience::

s/probably want/need/, at least for python-lxml.


[3]

+possible. You'll need to add the dependency to the
+``allowed-eggs-from-site-packages`` option in the ``buildout.cfg`` file. (And
+don't forget to add the version to ``versions.cfg`` as we run with
+``allowed-picked-version`` set to false.)

Might be worth mentioning that it also needs to be added to setup.py.

It would be nice if we could auto-generate versions.cfg from setup.py,
or vice-versa. Actually, doesn't buildout use dependency information
from setup.py?


[4]

+If it is a development-only depency (i.e. only needed for the test suite, or

s/depency/dependency/


[5]

+buildout_versions_file = versions.cfg

Grr, everything else uses hyphens. /me emails buildout-versions
author.


[6]

+download-cache = download-cache

/^download-cache/d

We've chosen to *not* define this in <branch>/buildout.cfg; we're
using ~/.buildout/default.cfg to use a shared cache. This is much
easier than following the Launchpad model while MaaS's deps are small
enough.


[7]

+allowed-eggs-from-site-packages =
+    Django
+    South
...

I know it's trivial, but the rest of the file uses 2 spaces for
indentation :)


[8]

+    # Convenient developer dependencies
+    Jinja2
+    Pygments

Are these dependencies of Sphinx? They're not direct dependencies I
think, so can they be left out here?


[9]

+    install_requires=[
+        'setuptools',
+        'Django' = 1.3.1,

This doesn't work. This does:

        'Django == 1.3.1',


[10]

+    extra_requires=dict(

s/extra_requires/extras_require/

Also, there's a missing brace at the end of setup.py.


[11]

+# Automatic versions in Precise

Is this something that buildout-versions provides?


[12]

We could structure versions.cfg like so:

  [versions]
  <= versions-run
     versions-dev
     versions-doc
     versions-auto

  [versions-run]
  Django = ...

  [versions-dev]
  lxml = ...

  [version-doc]
  Sphinx = ...

  [versions-auto]
  buildout-versions = ...

Then we could pull these out in setup.py:

  versions = ConfigParser.ConfigParser()
  versions.read("versions.cfg")

  def gen_deps(section):
      for option in versions.options(section):
          value = versions.get(section, option)
          if options[-1] in "<!>":
              yield b"%s=%s" % (option, value)
          else:
              yield b"%s==%s" % (option, value)

  deps = lambda section: list(gen_deps(section))

  ...
      install_requires=(
          deps("versions-run") +
          deps("versions-auto")),
  ...
      extras_require={
          b'doc': deps("versions-doc"),
          b'tests': deps("versions-run"),
          },

And, after using buildout-versions to update versions.cfg, we should
probably merge the automatically added things into their appropriate
section in versions.cfg.


[13]

Does buildout-versions help us notice dependencies we no longer need?

-- 
https://code.launchpad.net/~flacoste/maas/buildout-site-packages/+merge/94628
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~flacoste/maas/buildout-site-packages into lp:maas.