Merge lp:~nejucomo/divmod.org/fix-nevow-setup into lp:divmod.org

Proposed by nejucomo
Status: Needs review
Proposed branch: lp:~nejucomo/divmod.org/fix-nevow-setup
Merge into: lp:divmod.org
Diff against target: 26 lines (+9/-2)
1 file modified
Nevow/setup.py (+9/-2)
To merge this branch: bzr merge lp:~nejucomo/divmod.org/fix-nevow-setup
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Pending
Review via email: mp+176026@code.launchpad.net

Description of the change

This is a bzr branch which should fix the packaging issue which prevents Nevow from being installable in an environment without twisted pre-installed.

Caveats:

* I'm new to bzr, so please double check the bzr revision/merge structure.
* I hope this merge request represents the same change as applying the patch in https://bugs.launchpad.net/nevow/+bug/1091055/comments/5
* I only tested this by running "pip install ." in a new virtualenv (without twisted).
* It uses a regex which will be brittle to changes in nevow/_version.py.
* It uses a regex, so now there's N+1 problems. ;-)

To post a comment you must log in.
Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

This would be an easier change to accept if it included automated test coverage.

Revision history for this message
Daira Hopwood (daira) wrote :

> This would be an easier change to accept if it included automated test
> coverage.

What would constitute an acceptable testing strategy? Note that in order to run tests you have to successfully build, in which case this code was necessarily executed and returned some version (not necessarily the correct one, admittedly). I suppose you can unit-test the function in {{{setup.py}}} though. Or, you could check that the version returned by {{{pkg_resources}}} is the same as the one you get by importing {{{nevow._version}}} (remember to skip the test if {{{pkg_resources}}} is not importable).

Revision history for this message
Tristan Seligmann (mithrandi) wrote :

I think testing the function in setup.py would probably be sufficient in this scenario.

Revision history for this message
nejucomo (nejucomo) wrote :

Where would the automated test code live?

If it's inside Nevow proper, does that mean it would import setup.py in a unit test module?

Should we move the parsing code inside of Nevow, then import that in setup.py? That difficult without re-introducing the problem this patch is trying to solve. It would also be in danger of regressing https://bugs.launchpad.net/nevow/+bug/812537.

Should the setup.py module itself could have unittests?

All of these options sound ridiculous to me. Is there another approach I've missed?

It seems like the right way to test this is to have package infrastructure tests somewhere that test installing packages (and then running the unittests or other verification on the installation).

Does Nevow have such a system? If not, I can try one of the approaches above, and I'd appreciate guidance on which is the least insane.

Revision history for this message
Glyph Lefkowitz (glyph) wrote :

There's no problem with the tests living inside of nevow. The *code* has to live in setup.py to avoid regressing, but there's no problem with importing setup.py as a module to run some functions in it. Why does that seem ridiculous to you?

Revision history for this message
nejucomo (nejucomo) wrote :

> There's no problem with the tests living inside of nevow. The *code* has to
> live in setup.py to avoid regressing, but there's no problem with importing
> setup.py as a module to run some functions in it. Why does that seem
> ridiculous to you?

Perhaps "ridiculous" is too strong. It depends on the expected use case of unittests. Importing "setup" is a problem if the unittests are intended to run from the installed code.

I tend to prefer unittests which install with the application code, such that at any time one may run the test suite for an installed package as a sanity check. For example: "pip install $PKG; trial $PKG"

If, OTOH, we decide unittests can only be run from a revision control checkout (what about unpacked sdists?), then this would not be an issue, except in so far as future maintainers don't realize this policy and violate it.

A middle ground is to have "application" unittests which are installed with the package and can be run on the installed package, and have separate "packaging" unittests.

I think I prefer this "middle ground" approach. Unless someone raises an issue with it, next chance I'll take this approach:

Add a new single python module called something like "nevow_setup_unittests.py" which does not get installed, and place the test code in there, then perhaps tweak "runtests" to pass that to trial. (I'm assuming "runtests" is only for development and is unrelated to the installation.)

I prefer this separate module approach versus a conditional import inside Nevow *application* unittest code because it seems to easy to import the *wrong* setup.py.

I'm curious what other packages have done to address this issue. I know that Tahoe-LAFS, for example has an extremely complex setup.py and importing it has all manner of side effects. I wouldn't be surprised if it modifies sys.path outside of setuptools behavior.

The approach there has been to test all packaging code with test code which lives outside the repository and is currently messily spread about some buildbot configs.

I'd like to see the packaging test code which is specific to a revision live within the repository, yet be separate from application unittest code.

Revision history for this message
nejucomo (nejucomo) wrote :

BTW- as a followup, I just created a fresh virtualenv and ran "pip install twisted && pip install nevow && trial nevow".

The twisted install prior to the nevow install is required because of the ticket this branch attempts to address.

The result is that everything and unittests ran [1], so I believe that importing setup.py in the unittests would be changing a policy, whether that policy is explicit or not.

[1] Unfortunately the test result counts were:

FAILED (skips=28, expectedFailures=2, failures=1, errors=12, successes=711)

Revision history for this message
Glyph Lefkowitz (glyph) wrote :

Attaching your test output and log somewhere would possibly be helpful, since Nevow doesn't have any CI right now...

Revision history for this message
Donald Stufft (dstufft) wrote :

I have no idea about the tests etc but I can at least verify the approach will fix the issues I see with Nevow's setup.py.

Revision history for this message
nejucomo (nejucomo) wrote :

I am dusting this off again, in order to write unittests for the setup.py changes.

Before I begin changing tests, I ran `trial nevow` in the source root directory *without a nevow* install, and see the following error:

{{{
nevow.test.test_url.Serialization.test_rfc1808
===============================================================================
[ERROR]
Traceback (most recent call last):
Failure: twisted.internet.utils._UnexpectedErrorOutput: got stderr: 'Could not create the Java virtual machine.\n'
}}}

Revision history for this message
nejucomo (nejucomo) wrote :

Ah, I misread the error output. It was unrelated to `nevow.test.test_url.Serialization.test_rfc1808` but instead occurred in `nevow.test.test_consolejstest.JSGenerationTestCase.test_generateTestScript`.

I'm now investigating if a js interpreter is needed, and how it is detected. I have node on my $PATH, but it was not detected...

Revision history for this message
Daira Hopwood (daira) wrote :

> Ah, I misread the error output. It was unrelated to
> `nevow.test.test_url.Serialization.test_rfc1808` but instead occurred in
> `nevow.test.test_consolejstest.JSGenerationTestCase.test_generateTestScript`.
>
> I'm now investigating if a js interpreter is needed, and how it is detected.
> I have node on my $PATH, but it was not detected...

I would guess it is looking for a JS implementation written in Java, e.g. Rhino. But that's unrelated to this ticket.

Revision history for this message
Tristan Seligmann (mithrandi) wrote :

Currently the JavaScript tests require the SpiderMonkey interpreter, installed as `js` or `smjs` in your $PATH (iirc). I'm guessing you have another JS intepreter installed as `js`, which is a) not working (producing the "Could not create the Java virtual machine" error), and b) not SpiderMonkey, thus the code that drives it will most likely fail to run the tests anyway.

It's probably not that important to be able to run the JavaScript tests in this case; if SpiderMonkey can't be found, the tests would normally be skipped anyway.

Revision history for this message
Zooko Wilcox-O'Hearn (zooko) wrote :

I've added a comment on https://bugs.launchpad.net/nevow/+bug/812537 saying that this issue is causing Tahoe-LAFS users to be unable to install it.

Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

My intention is now to move Nevow to github (and therefore git) and use Brian Warner's python-versioneer project to remove the need for this Python parsing regexp in order to get the version information in setup.py.

Unmerged revisions

2709. By nejucomo

Modify Nevow/setup.py to not import nevow for the version; instead parse it from the source file with a regex.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Nevow/setup.py'
--- Nevow/setup.py 2009-11-30 01:08:55 +0000
+++ Nevow/setup.py 2013-07-20 20:55:32 +0000
@@ -1,13 +1,20 @@
1#!/usr/bin/python1#!/usr/bin/python
22
3from nevow import __version__ as version3import os
4import re
5
6# Parse the version without importing nevow or twisted:
7with file(os.path.join('nevow', '_version.py'), 'r') as f:
8 match = re.search(r'^version = versions.Version\(.*?, (\d+), (\d+), (\d+)\)$', f.read(), re.MULTILINE)
9 version = '%s.%s.%s' % (match.group(1), match.group(2), match.group(3))
10
11del f, match # Cleanup temporaries used to parse version.
412
5try:13try:
6 import setuptools14 import setuptools
7except ImportError:15except ImportError:
8 setuptools = None16 setuptools = None
917
10import os
11data_files=[]18data_files=[]
12for (dirpath, dirnames, filenames) in os.walk("doc"):19for (dirpath, dirnames, filenames) in os.walk("doc"):
13 if ".svn" in dirnames:20 if ".svn" in dirnames:

Subscribers

People subscribed via source and target branches

to all changes: