Merge lp://staging/~bellini666/storm/py3 into lp://staging/storm

A critical part of landing this on trunk is a test suite working under both python2.7 and python3. I haven't succeeded here (using a fresh Xenial container).

The mechanical python2->python3 bits seem fine.

I'm in no way qualified to review storm/cextensions.c , so will be relying on the tests here unless I can get some alternative eyeballs to have a look.

I'm unsure about the changes from explicit large ints to plain ints (1L -> 1). I haven't looked closely into what the tests that needed this change were actually testing, and how this change can be valid under both py2 and py3.

I've setup a clean Xenial container, and setup a dev environment to run the test suite (we can only land this if the tests pass under both python2 and python3).

dev/ubuntu-deps needs updating (or an alternative mechanism for getting developer dependencies. Add these packages: python-future python3-future python-dev python3-dev python3-mysqldb python3-fixtures python3-psycopg2 python3-testresources python3-transaction python3-twisted python3-zope.component python3-zope.security python3-setuptools

'make check' uses the default python2, and is running 0 tests. Something has broken with test discovery, and this needs to be fixed.

'make check PYTHON=python3' uses python3, and is failing to build:

$ make check PYTHON=python3
STORM_CEXTENSIONS=0 python3 setup.py test
running test
running egg_info
writing dependency_links to storm.egg-info/dependency_links.txt
writing top-level names to storm.egg-info/top_level.txt
writing storm.egg-info/PKG-INFO
reading manifest file 'storm.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching 'ez_setup.py'
writing manifest file 'storm.egg-info/SOURCES.txt'
running build_ext
copying build/lib.linux-x86_64-3.5/storm/cextensions.cpython-35m-x86_64-linux-gnu.so -> storm
Traceback (most recent call last):
  File "setup.py", line 57, in <module>
    [Extension("storm.cextensions", ["storm/cextensions.c"])])
  File "/usr/lib/python3.5/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib/python3.5/distutils/dist.py", line 955, in run_commands
    self.run_command(cmd)
  File "/usr/lib/python3.5/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/usr/lib/python3/dist-packages/setuptools/command/test.py", line 159, in run
    self.with_project_on_sys_path(self.run_tests)
  File "/usr/lib/python3/dist-packages/setuptools/command/test.py", line 140, in with_project_on_sys_path
    func()
  File "/usr/lib/python3/dist-packages/setuptools/command/test.py", line 180, in run_tests
    testRunner=self._resolve_as_ep(self.test_runner),
  File "/usr/lib/python3.5/unittest/main.py", line 93, in __init__
    self.parseArgs(argv)
  File "/usr/lib/python3.5/unittest/main.py", line 123, in parseArgs
    self._do_discovery([])
  File "/usr/lib/python3.5/unittest/main.py", line 228, in _do_discovery
    self.test = loader.discover(self.start, self.pattern, self.top)
  File "/usr/lib/python3.5/unittest/loader.py", line 341, in discover
    tests = list(self._find_tests(start_dir, pattern))
  File "/usr/lib/python3.5/unittest/loader.py", line 398, in _find_tests
    full_pa...

I will assume this patch and will send a new version addressing this issues (and some other issues I found with cextensions).

Any news on this? I would really like to see a Python 3 compatible release of storm.

The branch worked for me after two additional fixes. The first one is related to Zope security proxies. For some reason I do not really understand I had to define a '__len__' method for result and reference sets to be able to pass security proxied sets to the 'list' or 'tuple' constructor (although iterating over the sets worked).

The second fix affects the C extensions which raised a 'TypeError' for compile methods that pass the 'join' parameter, e.g. 'storm.expr.compile_compound_oper()'. I am not a C programmer and never wrote a Python C extension. But after reading a few lines of the C API docs I found that the error arises from an invalid 'S' format character in the 'PyArg_ParseTupleAndKeywords()' call inside 'Compile__call__()'. Replacing the 'S' with an 'U' made the C extensions work for me:

--- cextensions.c.ORIG 2018-07-06 08:46:29.000000000 +0200
+++ cextensions.c 2018-07-05 14:37:02.000000000 +0200
@@ -1682,7 +1682,7 @@

     join = default_compile_join;

- if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|OSbb", kwlist,
+ if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|OUbb", kwlist,
                                      &expr, &state, &join, &raw, &token)) {
         return NULL;
     }

Thanks for doing this work.

Status report: I've been breaking this down into more reviewable pieces and sending small MPs for each (in some cases doing the work independently, and in some cases borrowing from this MP and giving credit where appropriate). I also switched to tox for running tests, which makes it much easier to run a suitable test matrix. There's still quite a bit to go, but I've got a fair amount merged already and I plan to continue.

> Thanks for doing this work.
>
> Status report: I've been breaking this down into more reviewable pieces and
> sending small MPs for each (in some cases doing the work independently, and in
> some cases borrowing from this MP and giving credit where appropriate). I
> also switched to tox for running tests, which makes it much easier to run a
> suitable test matrix. There's still quite a bit to go, but I've got a fair
> amount merged already and I plan to continue.

I don't like the fact that the work I've done is being authored by someone else, and I'm loosing the right to say that I contributed to the project. Sends a very sad message to future contributors...

At least this project is finally seeing an update to python3.

Huh, wait, I'm not claiming credit for your work; where I've committed things with my authorship they have been actually different in substantive ways (e.g. for int/long handling I took more care to ensure that the tests were still meaningful on Python 2 while not being syntax errors on Python 3, and for print function conversion I added a bunch of __future__ imports to make it more robust. By contrast, the string exception fix I landed just now (admittedly a small change) I cherry-picked directly from your branch and the commit has your authorship on it. I expect there to be more of the latter. You will absolutely not lose the right to say that you contributed to the project, and I'm sorry for giving that impression.

If I could have fixed up this branch and landed it then I absolutely would have done, but it was just too much to digest, particularly given the poor testing situation described by Stuart. Since most of the comments made on this branch were over a year and a half ago, it seemed likely that you weren't going to have time to engage with them, and attempts by other people to do so also seemed to have stalled. There are also aspects of this branch that I'm not sure I agree with and may want to do differently. On the other hand there are certainly parts of your changes where you seem to know better than I and I expect to be trying to land your changes rather directly, such as the virtual subclass handling or the fleshing out of special object methods.

The approach I'm taking of breaking the branch down into small pieces, separating the large but essentially-mechanical changes from the ones that are non-trivial and require thought so that reviewers can skim-read the former and think hard about the latter, means that most of your work can actually land even if we choose to do some parts of it differently. I would very much encourage future contributors (including you, if you choose to contribute further) to take the approach of many small merge proposals rather than one large one.

Regarding the main issue, ok, I understood your motivations.

I would just disagree with the many small merge proposals in this situation specifically. Here are my thoughts about that:

1) The code is actually broken in small pieces. Each change is in its own commit, there's not a single large commit doing the whole "python2 -> python3" migration.

2) Having said that, knowing that there's a dependency between each of those changes in a lot of the times, dividing them into small merge proposals would produce a lot of divergences, specially after code reviews.

3) Testing those would be a nightmare since to test with python3, I cannot just test the unicode migration without doing other one if that needs to be done in the same piece of code.

4) Because storm uses bzr instead of git, there would be no way to open a lot of merge proposals, each one for each subsequent commit, and as long as they were merged in sequence, it would be possible to mimic what you are trying to acomplish.

5) Lastly, the purpose of a merge proposal is to merge a "feature" which was developed in a branch containing a sequence of commits. There are features which can be broken into parts, but "migrate python2 to python3" IMO is something atomic.

But again, that is my opinion and maybe your and the other maintainers of the project will disagree with me. Either way I just wanted to point it out.

OK, I indeed do disagree with you on this and it seems worth explaining why. Point-by-point:

1) I understand that, indeed, but merge proposals are the unit of code review and 3000+-line merge proposals are typically viewed as rather indigestible. It is true that it's possible to go and look at the individual commits, but it's then difficult to comment on those individually. (Some of this is admittedly due to shortcomings in the Launchpad code review system, but you work with what you've got.) It also puts maintainers in a difficult position when they disagree with something near the start of your sequence of commits: what are they to do when they give you feedback and you don't respond for over 18 months, which is something that does happen from time to time (and indeed happened in this case)? They can't easily merge the rest of it and finish off the last bits in some other way, because you've linearised it all.

2) Lots of the changes aren't interdependent and can be proposed in parallel without causing this kind of problem. This is exactly what I've been doing. If you propose the non-interdependent changes in a form that can be tested separately, then you make reviewers' lives easier because they know they can merge the uncontroversial ones separately and reduce the overall size of what's left; that's much harder with a single linearised branch because later commits might well rely on earlier ones and not be cherry-pickable.

3) It's true that it's less easy to test the whole assembly until you're quite far along. On the other hand this merge proposal wasn't fully tested with Python 3 anyway since it fails tests in the way that Stuart pointed out in August 2017 (I proposed https://code.launchpad.net/~cjwatson/storm/zope-interface-class-decorators/+merge/368445 to fix the particular test failures they quoted), and so there may still be other problems that we haven't reached yet; I understand that you tested with stoq, which is useful, but our first concern is to ensure that our own test suite passes. And in practice for Python 3 porting one often starts by working through many well-understood standard changes, ensuring that each one continues to pass tests on Python 2, and then seeing what's left once the codebase is in a testable state. This strategy works well enough when filing many small merge proposals, since they're of a size where the feedback loop between proposer and reviewer can be reasonably short.

4) You can use prerequisite branches on merge proposals if you like. See my comment at the end of 1) for the practical problems caused by this, though.

5) This is a reasonable philosophical disagreement, but I do find my approach works better in practice. I've done quite a lot of other Python 3 porting work this way.

In any case, I'm certainly grateful for the work you've done on this and will explicitly credit you in the NEWS file regardless of the exact balance of specific commits that end up with your name on them, as well as others who've contributed here.

This is now effectively landed (with some commits from this branch, and some commits prepared separately) in Storm 0.21. Thanks!