Code review comment for ~mpontillo/uvtool:dev-env-setup-and-hacking-instructions

On Thu, Jan 05, 2017 at 05:45:13PM -0000, Mike Pontillo wrote:
> (1) How do I do manual testing?
> (2) How do I do automated testing?
> a. How do I run the unit tests?
> b. Is there a larger integration test or CI system I should be mindful of?
> (3) How do I create a package to test with?

Thank you for enumerating these. This is a great list. I agree that we
need to be able to answer these questions quickly for any contributor,
and so and we can use the list to check against any proposed solution.

> If you want to address (2) you really want to have an automated way to
> get everything going, so that on a fresh image you can easily test all
> the things you need to test. I think it's good to have dev-env-setup
> activities written in code, so that they can later be automated.

Agreed, with the caveat that if it isn't uvtool-specific, it shouldn't
be in uvtool's tree. Instead there should be a community convention and
community tooling (eg. setuptools or virtualenv), and we should be using
that.

> This change doesn't address (3). Beyond running 'apt-get source', I
> don't have a clue where the debian/ directory is for uvtool, etc.

It's in the Debian packaging branch. Having a separate packaging is the
conventional thing to do, so I think it should suffice to point to it in
a HACKING.md.

> That aside, let me break down the changes so you understand the
> purpose for each one before you make a decision.
>
> * As a new contributor to a Python project, my first instinct was to
> look around in the setup.py and requirements.txt for what I would
> need to install from pypi to get going. I soon realized all that was
> missing

OK. This is a reasonable expectation.

> - Solution: create a Makefile, to pique the interest of curious
> developers. This came somewhat from my experience working on MAAS,
> where there are some similarities to uvtool: it is a Python
> project, but it isn't based on dependencies from pypi; rather, it
> is based on dependencies from the Ubuntu archive. (In MAAS it is
> quite difficult to get going, and having the `make` targets really
> helps new developers. If something is going wrong, we can just say
> "did you run `make install-dependencies`?)

Here's where I think my opinion diverges. The right way to fix this is
to directly fix your original missing observation. There should be a
requirements.txt and the dependencies should be put into pypi. That's
the correct way to fix your use case above, IMHO.

> - Suggestion: lose the script and just put the `sudo apt-get
> install -yu ...` in the Makefile. That way the list of dependencies
> for development and testing can be maintained in one place (as
> opposed to having to document it in the HACKING guide, and then
> repeating the same steps when writing automation scripts).

I think this is moot, as I don't think there should be a Makefile.
Because that's not part of the Python community's convention, and this
is a pure Python project.

> [...]

> * As a new contributor to uvtool, I might ask myself: given the lack
> of a `virtualenv` requirement and the need to test in the context of
> the actual Ubuntu system, how do I actually run this thing without
> creating a package and installing it? (Now, you and I have some
> experience with this, and could figure out quickly that we just need
> to set up the PATH and PYTHONPATH appropriately. But we have a lot of
> experience with this sort of thing. $RANDOM_NEW_CONTRIBUTOR might
> appreciate a little more help.)
> - Solution: Create a script (similar to how `virtualenv` works) to
> automatically set up the development sandbox.
> - Side benefit: there is no additional thinking required when you
> go to automate tests. Just ". contrib/develop" and you're done.

My alternate solution: explain the one-liner in HACKING.md.

> The other thing I would point out is, there is nothing about the
> `develop` script that is really specific to uvtool in particular.

Agreed.

> As
> long as it works, there is no code to maintain in there, and it can
> only lower the barrier to entry for new contributors.

I disagree. The code isn't in some upstream Python build tooling
project, so we have to maintain it. If bugs are found, we have to fix
them. If conventions change, we have to update our use of them.

Contrary to your opinion I think it actually raises the barrier for new
contributors, because on seeing the script (or reference to the script
in HACKING.md) new contributors are immediately given a black box.
Contributors need things to be transparent, so have to examine the
script to make it so. Better to not have the script at all, or for the
script to be somewhere in an upstream tool that the entire community
uses and is familiar with.

Asking contributors to run the single line "PYTHONPATH=. bin/..." or
"setup.py test" or whatever is however immediately transparent. The
former because it's a one-liner that does nothing special and should be
immediately understandable to a Python developer, and the latter because
it uses setuptools, which should also be immediately understandable to a
Python developer.

As it isn't uvtool-specific, it does not belong in uvtool. As it's a
general problem, it should be solved in a general way with generic
tooling that the whole community can use. This may be setuptools or
virtualenv or something else. If a Python community member's
expectations are broken because uvtool's tree detracts from convention,
then we should fix that. And I'll accept patches and encourage that side
of things being fixed.

Robie