Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Port calibre to python 3 #870

Closed
wants to merge 64 commits into from
Closed

WIP: Port calibre to python 3 #870

wants to merge 64 commits into from

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Sep 3, 2018

What works:

  • Starting the calibre GUI
  • Calibre C extensions
  • Adding a book

What doesn't work:

  • Recipies
  • Everything else
  • Python 2 compatibility

Anyway, this pull request is just here to get some early feedback. There's a lot of work that is left to do, like manually testing as many code paths as possible, as well as porting recipies to use MechanicalSoup or something like that instead of mechanize.

My goal is to have this code work the same on python2 and on python3, with the goal of eventually dropping python2 support late 2019 or early 2020.

To bootstrap this version, you'll need to install the following pip packages:

apsw==3.9.2.post1
dukpy==0.3
Elements==0.2.0
feedparser==5.2.1
html2text==2018.1.9
html5-parser==0.4.5
netifaces==0.10.7

and delete all recipes except ajc.recipie

This was done using lib2to3.fixes.fix_numliterals

Python 2 supports 0o, so this is fine.
This is compatible with python 2 & 3, and was done using

sed -i -e 's/from future_builtins import \(.*\)$/from six.moves import \1/g' *.py src/**/*.py setup/**/*.py
This is using a fixer plugin that I wrote, and can be found at
https://gist.github.com/7088e5543b65b1dd1320a822c9cb2ef5
I'm not sure why this was using a byte string--this dict contains
regular strings.
Done using libmodernize.fixes.fix_imports_six
Replace with six.moves using

sed -i -e 's/from cStringIO import StringIO/from six.moves import StringIO/g' -e 's/cStringIO.StringIO/StringIO/g' -e 's/import cStringIO/from six.moves import StringIO/g' *.py src/**/*.py setup/**/*.py recipes/**/*.py

and some manual touch-up
This was done using the regex ([^']\s*)\bur'([\x01-\x26\x28-\xff]+)'
Otherwise we'd try to read an unassigned variable
@aimylios
Copy link
Contributor

One of the tests fails for me on a Python 3 build of Calibre (revision abd9e6b):

test_palmdoc_compression (calibre.ebooks.compression.palmdoc.find_tests.<locals>.Test) ... ERROR [0.0004 s]

======================================================================
ERROR: test_palmdoc_compression (calibre.ebooks.compression.palmdoc.find_tests.<locals>.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/aimylios/calibre-3.45.0/src/calibre/ebooks/compression/palmdoc.py", line 92, in test_palmdoc_compression
    self.assertEqual(py_compress_doc(test), x)
  File "/home/aimylios/calibre-3.45.0/src/calibre/ebooks/compression/palmdoc.py", line 51, in py_compress_doc
    och = ord(ch)
TypeError: ord() expected string of length 1, but int found

----------------------------------------------------------------------
Ran 316 tests in 44.733s

FAILED (errors=1, skipped=10)
get_categories: item Unknown is not in authors list!

I'm pretty sure all of the tests passed when I did the same exercise about a month ago. It might thus be related to the changes merged on 26th of May (f2e0181 and b842fe7).

@eli-schwartz
Copy link
Contributor

In the second commit you reference, we started collecting a unittest for that function. It was probably always subtly broken -- in python3, a b'' string is a bytearray and indexing it returns a number, not a b'' string.

@miasma
Copy link

miasma commented Jul 14, 2019

Tried the Arch git build from AUR. The latest version fails to build with

test_prints_nothing_if_no_errors (calibre.db.cli.tests.PrintCheckLibraryResultsTest) ... ok [5e-05 s]

======================================================================
FAIL: test_bonjour (calibre.srv.tests.loop.LoopTest)
Test advertising via BonJour
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/makepkg/calibre-git/src/calibre/src/calibre/srv/tests/loop.py", line 129, in test_bonjour
    self.assertIsNotNone(info)
AssertionError: unexpectedly None

======================================================================
FAIL: test_searching (calibre.db.tests.reading.ReadingTest)
Test searching returns the same data for both backends
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/makepkg/calibre-git/src/calibre/src/calibre/db/tests/reading.py", line 338, in test_searching
    ans, nr, query))
AssertionError: Items in the second set but not the first:
2 : Old result: set([1]) != New result: set([1, 2]) for search: date:9/6/2011

----------------------------------------------------------------------
Ran 316 tests in 41.428s

FAILED (failures=2, skipped=9)

python-zeroconf is installed at least.

@eli-schwartz
Copy link
Contributor

The package has dependencies on python-zeroconf so that should always be available anyway. ;)

On the topic of your errors, I do not get either error. However, note that the first error will unpredictably occur on both python2 and python3, so I usually just try rerunning the testsuite and it often succeeds again.

Not sure why the second error would happen.

@eli-schwartz
Copy link
Contributor

FWIW, I did update the binary repository mentioned in the calibre-git AUR package details. So you can get the python3 build as a prebuilt package.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Jul 14, 2019

@kovidgoyal I wonder if you've noticed a curious oddity that I am noticing? Seems like when running the python2 and python3 tests in quick succession, regardless of how often test_bonjour/test_websocket_perf flakes out on python2, it seems to be much more reliably succeeding on python3.

I just ran the tests 5 times in a row, and on python2 it succeeded once, failed 4 times. On python3, it succeeded all 5 times.

EDIT: in 20 runs, python3 failed 3 times, all three with an entirely different error:

======================================================================
FAIL: test_http_basic (calibre.srv.tests.http.TestHTTP)
Test basic HTTP protocol conformance
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/calibre-git/src/calibre/src/calibre/srv/tests/http.py", line 286, in test_http_basic
    self.ae(r.read(), ('%d' % i).encode('ascii'))
AssertionError: b'3' != b'1'

----------------------------------------------------------------------
Ran 316 tests in 38.083s

FAILED (failures=1, skipped=10)

@kovidgoyal
Copy link
Owner

The bonjour test is utterly reliable for me on both pythons. The http_basic python3 test failure I do see from time to time, have to investigate that as it indicates pielining is out of order which is a pretty bad bug.

@kovidgoyal
Copy link
Owner

Actually given that pipelining is not really used by browsers or even
curl anymore, maybe best to just drop that test.

@eli-schwartz
Copy link
Contributor

As of #1040 we are finally done porting unicode_check -- the setup.py helper which checks if all files correctly import __future__.unicode_literals.

So that should be one source of potential incompatibilities fixed. Also, all other future imports have been activated for the relevant touched files, especially division (which needed quite a few fixes along the way).

@eli-schwartz
Copy link
Contributor

Now that #1042 is merged, I think all ebook format conversions should work. Or at least not crash obviously. (I roundtripped this format and tested it converted without tracebacks, and tried opening it with calibre itself, so that says something at least -- but who actually uses TCR anyway. ;))

@kovidgoyal
Copy link
Owner

Yeah there are not a lot of low hanging fruit left. Now one just has to get betas out and have people test them and report bugs. And plugin developers use the betas to port their plugins. But it is going to have to wait until after the webengine migration is done and calibre 4 is released.

@eli-schwartz
Copy link
Contributor

So... since calibre seems to be more or less running fairly stable so far...

With the calibre 4.4.0 release, I've uploaded a split package to the official archlinux [community] repository. calibre now depends on calibre-common, which contains all python-agnostic code, and calibre and calibre-python3 can now be installed together. Which one you use when you try to run the command line tools depends on the symlinks that were created by calibre-alternatives set [2|3].

While I anticipate that people will still want to use python2, simply because plugins are a very important part of the ecosystem, people now have the option to install the python3 version and try it out. Hopefully this will help us discover any more lingering bugs.

@kovidgoyal
Copy link
Owner

Look at the py3 branch -- I am working on getting binary builds done
with python3. Once that is done, hopefully the plugin developers can
work on porting their plugins using those builds.

@eli-schwartz
Copy link
Contributor

Looking forward to it. :)

@kovidgoyal
Copy link
Owner

FYI binary builds are available as described here: https://www.mobileread.com/forums/showthread.php?t=325721

@escwartz: Feel free to post in that thread if you have tips for porting plugins to python 3.

@pevik
Copy link

pevik commented Mar 24, 2020

Hm, it'd be great if python3 port was finished, because new distros already dropped python2 but you require it for setup.py bootstrap (I know I can download precompiled calibre or compile PyQt5 for python2 myself). So what is missing?

@eli-schwartz
Copy link
Contributor

@pevik

You want this for OpenSUSE, correct? You can import Fedora 31's specfile here: https://src.fedoraproject.org/rpms/calibre/blob/f31/f/calibre.spec

Debian likewise builds with python3, which "mostly works" albeit not all plugins are ported.

Gentoo and Arch Linux provide the option to use either python2 or python3.

I'm not positive what the porting stats are for https://plugins.calibre-ebook.com/, unlike the pyqt4 -> pyqt5 move there isn't an obviously easy way to heuristically prove that a plugin has been ported... the dates seem to indicate many many plugins were not ported yet, though I know a couple of my favorite ones were. (We cannot just "finish" the porting by declaration and make all those work.)

@pevik
Copy link

pevik commented Mar 25, 2020

@eli-schwartz No, for Debian, which has PyQt5 uninstallable for python2 in Debian testing release (it has python2 itself). Also PyQt5 does not provide builds in pip for python2 any more. But thanks for info.
Understand, to port 136 plugins is a lot work (even some of them are already ported).

@eli-schwartz
Copy link
Contributor

Oh, I was just looking at your membership in @SUSE :p

Debian sid/Bullseye builds calibre using python3 (it must, since python2 is being forcibly removed), Debian stable/Buster builds calibre using python2. What is the problem exactly?

/cc @norbusan

@norbusan
Copy link
Contributor

Yes, Debian/buster is the last with Python2.
Debian/testing and sid are in the process of removing Python2, lots of packages have already disappeared.
Thus, we need to build Calibre on with Python3 on sid/testing.

@pevik
Copy link

pevik commented Mar 25, 2020

I use both distros and want calibre package was working in both. As I wrote I can use precompiled package or compile also PyQt5, but that's not ideal.

Debian: I reported Debian package [1], where you put many suggestions [2]. Unfortunately I cannot fix it as I haven't forced to learn Debian packaging (I'm not a Debian developer, but that could be solved via [3] or even to send a patch in a bug tracker).

openSUSE: There is updated package in [4]. I'll test the bug in it. Fixing openSUSE package would be for me much easier than Debian package.

Out of curiosity for python3 port: esting your py3 branch (that's the one which it looks to be working (2 test fails: test_unrar and test_get., but that's minor issue). So now we're waiting on plugins being ported?

[1] https://bugs.launchpad.net/calibre/+bug/1850835
[2] https://bugs.launchpad.net/calibre/+bug/1850835/comments/7
[3] https://mentors.debian.net/
[4] https://build.opensuse.org/package/show/Documentation:Tools/calibre

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Mar 25, 2020

Note that most or possibly all of my suggestions from https://bugs.launchpad.net/calibre/+bug/1850835/comments/7 are more or less obsolete. @norbusan has done some great work to improve calibre packaging on Debian, so as far as I know the latest versions should be in pretty good shape!

Where is test_unrar and test_get failing and with what errors? The former might be failing due to licensing issues preventing your distribution from providing the libunrar software, and thus, the python3-unrardll module which links to it.

@ofek
Copy link

ofek commented Mar 25, 2020

Hey quick question: have we considered using the officially Qt backed https://www.qt.io/qt-for-python (https://pypi.org/project/PySide2/)

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 25, 2020 via email

@eli-schwartz
Copy link
Contributor

My sole experience with PySide2 has been it crashing because for subtle reasons it didn't support python 3.8 yet after our distro python 3.8 mass rebuild. PyQt just worked...

@ofek
Copy link

ofek commented Mar 25, 2020

Might be related to note here https://wiki.qt.io/Qt_for_Python

5.14 will not work on Windows with Python 3.8.0, please use Python 3.8.1 or greater.

Which doesn't surprise me, nearly every initial .0 Python release has had a few issues

@eli-schwartz
Copy link
Contributor

No, it was actually https://bugs.archlinux.org/task/64514 and it was solved by "wait for qt 5.14" which was months after python 3.8, and the bug was in fact solved by PySide, so this has nothing to do with "nearly every initial .0 Python release has had a few issues".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants