launchpad-reviewers team mailing list archive

["Francis J. Lacoste" <francis.lacoste@xxxxxxxxxx>]

On 12-02-24 06:41 PM, Gavin Panella wrote:
> Review: Needs Fixing
> 
> Looks good. Lots of comments, mostly trivial. However, setup.py is
> broken, so Needs Fixing.

Thanks for the review, I've fixed everything you asked for.


> 
> 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?

It does, but versions.cfg is applied on top of those. The general rule
of setup.py is that you don't pin version unless your package won't work
with any other version, whereas we use version pinning in buildout for
repeatable builds.


> 
> [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.

Well, ok. But I disagree, it's not easier than the Launchpad model if
you have a lot of different projects managed by buildout. In which case,
you end up with a massive eggs and download-cache where it's very hard
to know what comes from where.


> 
> [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?

They are, but they still need to be added here otherwise buildout will
install them from PyPI.

> 
> 
> [11]
> 
> +# Automatic versions in Precise
> 
> Is this something that buildout-versions provides?
> 

Unfortunately, no, buildout-versions doesn't pick up the dependency
coming from system python. I need to report a bug (probably submit a
fix) upstream. The future version of buildout (2.0) actually drop the
system eggs support that Gary added. It seems that nobody but us is
using that and supporting it requires isn't trivial, so upstream decided
to drop it.

> 
> [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.
> 

I've reordered the file versions.cfg file as you suggested, but I'm not
sure that generating the setup.py dependency from versions.cfg is such a
good idea. This will basically mean that the version will be pinned like
in buildout, which is good for repeatable builds, but not so much for
general use. Given that we are not going to release that package on
PyPI, maybe that's kind of moot (and mean that setup.py isn't that
interesting either).

> 
> [13]
> 
> Does buildout-versions help us notice dependencies we no longer need?
> 

Unfortunately, no. But it 's easy enough to remove all automatic
dependencies information and run ./bin/buildout -v Only the one actually
in use will be written to the file.


-- 
Francis J. Lacoste
francis.lacoste@xxxxxxxxxx


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.