Support for Python 3 #4

george-hopkins · 2017-01-09T13:43:03Z

This PR implements support for Python 3. As I'm not very familiar with whole code base, I would be glad to receive some feedback if everything still works as expected. All unit tests pass on 2.7, 3.3 and 3.5. In addition the decoding was tested against an other implementation.

Wait for ~~Support for Python 3 zooko/pyutil#4~~ Support for Python 3 tpltnt/pyutil#1 to get merged and released (ports pyutil to Python 3)

george-hopkins · 2017-01-09T13:50:45Z

@daira Thanks for reviewing! I updated the code accordingly. Please feel free to comment if you find any further issues.

It seems that the comments don't show up here as they were made before this PR was created.

ThomasWaldmann

note: I am not a project member, just potentially interested in zfec, should it support python 3.x some day.

ThomasWaldmann · 2017-03-20T17:09:17Z

.travis.yml

  - "2.6"
+  - "2.7"
+  - "3.3"
+  - "3.5"


no 3.4 and 3.6?

ThomasWaldmann · 2017-03-20T17:09:55Z

README.rst

@@ -261,7 +261,8 @@ Dependencies

 A C compiler is required.  To use the Python API or the command-line tools a
 Python interpreter is also required.  We have tested it with Python v2.4,
-v2.5, v2.6, and v2.7.  For the Haskell interface, GHC >= 6.8.1 is required.
+v2.5, v2.6, v2.7, v3.3 and v3.5.  For the Haskell interface, GHC >= 6.8.1 is


as travis is only testing >= 2.6, I am asking myself whether the code has recently been tested on 2.4 and 2.5 (considering they are quite dead since long). this is just a general comment, not related to your PR.

update: code in this PR can't work on 2.4 / 2.5 (see below).

and there is no reason to support these dead versions today, so just remove all references from docs etc.

ThomasWaldmann · 2017-03-20T17:10:18Z

setup.py

+    "Programming Language :: Python :: 3.3",
+    "Programming Language :: Python :: 3.4",
+    "Programming Language :: Python :: 3.5",
+    "Programming Language :: Python :: 3.6",


here we have 3.4 and 3.6. \o/

ThomasWaldmann · 2017-03-20T17:17:10Z

zfec/_version.py

@@ -8,6 +8,8 @@
 # This file is released into the public domain. Generated by
 # versioneer-0.15 (https://github.com/warner/python-versioneer)

+from __future__ import print_function
+


https://docs.python.org/2/library/__future__.html

if i understand that correctly, this will require python >= 2.6.0a2.

so, guess it's time to drop everything < 2.6.

and i'ld also drop 2.6 asap, more and more tools in the python world make troubles on 2.6, so even testing it is sometimes not that easy.

ThomasWaldmann · 2017-03-20T17:24:55Z

zfec/cmdline_zunfec.py

+try:
+    unicode = unicode
+except NameError:
+    unicode = str


this is a bit unusual. the recommended style of a port that works on python 3 and 2 is that code should look as in python3 (or somehow close) and/or be close to python3 semantics.

that makes it easier for the future (when nobody uses 2 any more and at some time, support for 2 will get removed).

I guess most people use something like this:

text_type = str # python 3 text_type = unicode # python 2

also, I'ld put such stuff into a _compat.py module, so it can be imported from there to anywhere it is needed.

same applies for:

bytes_type = bytes # python 3 bytes_type = str # python 2

also, we shouldn't be afraid to use six for this sort of compatibility stuff, it's pretty small and used by a lot of other packages (including Tahoe)

ThomasWaldmann · 2017-03-20T17:28:09Z

zfec/easyfec.py

@@ -33,7 +33,7 @@ def encode(self, data):
            reconstruct the input data
        """
        chunksize = div_ceil(len(data), self.fec.k)
-        l = [ data[i*chunksize:(i+1)*chunksize] + "\x00" * min(chunksize, (((i+1)*chunksize)-len(data))) for i in range(self.fec.k) ]
+        l = [ data[i*chunksize:(i+1)*chunksize] + b"\x00" * min(chunksize, (((i+1)*chunksize)-len(data))) for i in range(self.fec.k) ]


byte literals also will require 2.6.

ThomasWaldmann · 2017-03-20T17:32:29Z

zfec/test/test_zfec.py

+try:
+    from cStringIO import StringIO
+except ImportError:
+    from io import BytesIO as StringIO


rather use py3 names:

from cStringIO import StringIO as BytesIO # py2 from io import BytesIO # py3

ThomasWaldmann · 2017-03-20T17:34:51Z

zfec/test/test_zfec.py


    def test_bad_args_construct_decoder(self):
        try:
            zfec.Decoder(-1, -1)
-        except zfec.Error, e:
+        except zfec.Error as e:


"except ... as ..." requires 2.6.

ThomasWaldmann · 2017-03-20T17:37:40Z

zfec/test/test_zfec.py

@@ -286,40 +293,40 @@ def _help_test_filefec(self, teststr, k, m, numshs=None):
            tempdir.shutdown()

    def test_filefec_all_shares(self):
-        return self._help_test_filefec("Yellow Whirled!", 3, 8)
+        return self._help_test_filefec(b"Yellow Whirled!", 3, 8)


maybe prettier than using b"..." at every place here would be to change self._help_test_filefec() to convert to bytes_type (which needs to be set up in a similar way as text_type, str for py2, bytes for py3).

ThomasWaldmann · 2017-03-20T17:41:56Z

.travis.yml

  - "2.6"
+  - "2.7"
+  - "3.3"


btw, 3.3 is (as everything < 3.4) a slightly crappy python 3 version.
so, considering it's 2017 already, I'ld consider if it makes sense to start supporting it now.
everybody and his dog has 3.4+ nowadays and 3.4 has quite some important features and fixes for stuff still missing/broken in 3.3.

ThomasWaldmann · 2017-03-20T17:48:03Z

zfec/filefec.py

@@ -251,7 +253,7 @@ def decode_from_files(outf, infiles, verbose=False):
        m = nm
        if not (k is None or k == nk):
            raise CorruptedShareFilesError("Share files were corrupted -- share file %r said that k was %s but another share file previously said that k was %s" % (f.name, nk, k,))
-        if k > len(infiles):
+        if not (k is None or k <= len(infiles)):


this looks like a bugfix (not related to py3 porting), so likely it should live in a separate commit or even separate PR.

The extra check is needed for Python 3 because int and None can't be compared anymore (unorderable types).

Good point. It's probably still worth putting this in a separate commit (which should appear before the general port-to-py3 commits), since it fixes a non-py3-specific problem (comparing None against an integer doesn't make sense in py2 either, even if it's not treated as an error)

george-hopkins · 2017-03-24T18:59:31Z

@ThomasWaldmann Thank you for putting so much effort in reviewing this pull-request! In retrospect, it might be more sensible to ditch Python 2 support altogether. This would simplify maintenance quite a bit.

@zooko, @vu3rdd What are your views on this proposal? I'm happy to address the issues mentioned as soon as possible if there is an interest in this PR in general.

ThomasWaldmann · 2017-03-25T00:02:49Z

@george-hopkins 2.7.x and 3.4+ are quite reasonable choices right now. YMMV. :)

copyninja · 2017-07-15T07:43:10Z

zfec/filefec.py

        else:
            if padlen > 0:
                outf.truncate(byteswritten - padlen)
            return # Done.
    if verbose:
-        print
-        print "Done!"
+        print()


I think this is done to get "Done!" printed on new line and can be replaced with single statement like below

print("\nDone!")

warner · 2017-07-15T23:32:27Z

Yay for py3 support! I'd second @ThomasWaldmann suggestion to go with 2.7 and >=3.3 or >=3.4. I'm particularly keen to maintain 2.7 support because Tahoe-LAFS is still 2.7-only, and I think Tahoe remains the largest customer of zfec.

warner · 2017-07-16T01:34:04Z

I'll definitely land this once it's ready. Some ideas for a rewritten branch:

start with a commit that removes support for py2.4/2.5 (which I think just means changing setup.py's metadata claims)
then a commit that switches from absolute imports to relative imports
then one to fix that compare-int-vs-None problem
then some commits to replace the py3-incompatible syntax issues: print functions, "except X,y" -> "except X as y". This is a good place to add from __future__ import lines
then a commit to add six and maybe change some of the bytes/unicodes to use it. Also things like changing import cStringIO to use the six.moves equivalent
then fix the rest of the py3 incompatibilities
then finally add py3 to setup.py and travis and tox

The general idea is the tree should build and test correctly after every commit. But I'll be ok if it doesn't actually achieve that, as long as the sequence is fairly easy to follow,

That travis line that installs pyutil from a git repo.. what's that for? I want to get rid of that. Do we need a new pyutil release with something fixed? What do we need pyutil for anyways? (I vaguely remember talking with zooko a long while back about maybe splitting the CLI utilities out of zfec and into a separate package, because I seem to remember that pyutil was only needed for the CLI tools).

george-hopkins · 2017-07-16T15:46:04Z

@warner Thank you for your feedback! I'm happy to incorporate the suggested changes if the PR has a chance of getting merged.

As for pyutil: As pyutil needs to be ported to Python 3 as well, I created zooko/pyutil#4. If you can help to get it merged, that would be great! As soon as it's merged, we can get rid of the include from my personal repository here (used it to see if Travis succeeds on all versions).

sbellem · 2017-10-17T17:38:24Z

May we just know whether there are near-term plans to move this PR forward and eventually merge it?

I could provide some help if needed.

tpltnt · 2018-01-11T09:57:28Z

I am waiting for @george-hopkins to do the PR for Python 3 support against my fork so I can build and push to pypi. This would remove the major blocker and move the Python 3 port of zfec forward.

tpltnt · 2018-01-12T17:29:32Z

I am happy to announce pyutil version 3.0.0 now supports Python 3 thanks to @george-hopkins . The module is already published on pypi.

george-hopkins · 2018-01-12T20:32:20Z

With the new release of pyutil (thanks to @tpltnt!) the patch is now ready to be reviewed. The updated patch already includes some fixes for the issues mentioned earlier.

tpltnt · 2018-01-12T21:16:30Z

The code looks good to me.

nimamox · 2018-02-02T00:59:53Z

Has it been merged to the main branch? I couldn't use pip3 to install zfec.
EDIT: I get an error during compilation on macOS from @george-hopkins fork.

$ pip3 install git+https://github.com/george-hopkins/zfec
...
    zfec/_fecmodule.c:633:12: warning: incompatible integer to pointer conversion assigning to 'PyObject *' (aka 'struct _object *') from 'int' [-Wint-conversion]
        module = Py_InitModule3("_fec", fec_functions, fec__doc__);
               ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    zfec/_fecmodule.c:635:7: error: non-void function 'init_fec' should return a value [-Wreturn-type]
          return;
          ^
    33 warnings and 5 errors generated.
    error: command 'clang' failed with exit status 1

warner · 2018-02-02T07:33:14Z

Looks good to me, local tests pass on macOS. There's a problem with extending tox to include py3 (the "trialcoverage" dependency doesn't work on py3), but we can fix that later. Will land momentarily. Thanks everyone!

george-hopkins · 2018-02-03T09:16:46Z

@warner Thank you for merging and the clean up you had to do before the release!

@nimamox I kept my porting efforts in a separate branch "python3". However, with the new release you can simply install it with pip install zfec.

george-hopkins force-pushed the python3 branch from 4af7a43 to bc88820 Compare January 9, 2017 13:43

george-hopkins mentioned this pull request Jan 9, 2017

Implement error-correcting OP defragmenter daniestevez/free-outernet#3

Merged

FelixSchwarz mentioned this pull request Mar 20, 2017

evaluate redundancy / error correction options borgbackup/borg#225

Open

ThomasWaldmann reviewed Mar 20, 2017

View reviewed changes

copyninja reviewed Jul 15, 2017

View reviewed changes

This was referenced Aug 25, 2017

[python3 support] Investigate what is needed amiller/HoneyBadgerBFT#25

Closed

python3 compatibility #1

Closed

sbellem mentioned this pull request Oct 31, 2017

Support for Python 3 zooko/pyutil#4

Closed

george-hopkins added 6 commits January 12, 2018 21:07

Fix comparison between incompatible types

0441751

Prepare for Python 3

6445443

Drop support for Python <2.7

7f3a7d3

Implement support for Python 3

89188d0

Port extension to Python 3

cd8e1e4

Speed up random string generation

4f046fa

george-hopkins force-pushed the python3 branch from bc88820 to 4f046fa Compare January 12, 2018 20:14

george-hopkins changed the title ~~[WIP] Support for Python 3~~ Support for Python 3 Jan 12, 2018

warner merged commit 4f046fa into tahoe-lafs:master Feb 2, 2018

george-hopkins deleted the python3 branch February 3, 2018 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Python 3 #4

Support for Python 3 #4

george-hopkins commented Jan 9, 2017 •

edited

Loading

george-hopkins commented Jan 9, 2017

ThomasWaldmann left a comment •

edited

Loading

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

warner Jul 16, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

ThomasWaldmann Mar 20, 2017

george-hopkins Mar 24, 2017

warner Jul 16, 2017

george-hopkins commented Mar 24, 2017

ThomasWaldmann commented Mar 25, 2017

copyninja Jul 15, 2017

warner commented Jul 15, 2017

warner commented Jul 16, 2017

george-hopkins commented Jul 16, 2017

sbellem commented Oct 17, 2017 •

edited

Loading

tpltnt commented Jan 11, 2018

tpltnt commented Jan 12, 2018

george-hopkins commented Jan 12, 2018

tpltnt commented Jan 12, 2018

nimamox commented Feb 2, 2018 •

edited

Loading

warner commented Feb 2, 2018

george-hopkins commented Feb 3, 2018

Support for Python 3 #4

Support for Python 3 #4

Conversation

george-hopkins commented Jan 9, 2017 • edited Loading

george-hopkins commented Jan 9, 2017

ThomasWaldmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

george-hopkins commented Mar 24, 2017

ThomasWaldmann commented Mar 25, 2017

Choose a reason for hiding this comment

warner commented Jul 15, 2017

warner commented Jul 16, 2017

george-hopkins commented Jul 16, 2017

sbellem commented Oct 17, 2017 • edited Loading

tpltnt commented Jan 11, 2018

tpltnt commented Jan 12, 2018

george-hopkins commented Jan 12, 2018

tpltnt commented Jan 12, 2018

nimamox commented Feb 2, 2018 • edited Loading

warner commented Feb 2, 2018

george-hopkins commented Feb 3, 2018

george-hopkins commented Jan 9, 2017 •

edited

Loading

ThomasWaldmann left a comment •

edited

Loading

sbellem commented Oct 17, 2017 •

edited

Loading

nimamox commented Feb 2, 2018 •

edited

Loading