Add Python 3.8 official support

[further-reading]

Fixes #4085

[codecov]

Codecov Report

Merging #4092 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4092      +/-   ##
==========================================
+ Coverage   85.68%   85.82%   +0.13%     
==========================================
  Files         165      165              
  Lines        9734     9847     +113     
  Branches     1463     1463              
==========================================
+ Hits         8341     8451     +110     
- Misses       1136     1139       +3     
  Partials      257      257

Impacted Files	Coverage Δ
scrapy/extensions/httpcache.py	95.56% <100%> (+0.04%)	⬆️
scrapy/utils/decorators.py	78.57% <0%> (-1.43%)	⬇️
scrapy/extensions/statsmailer.py	29.16% <0%> (-1.27%)	⬇️
scrapy/contracts/__init__.py	83.33% <0%> (-0.41%)	⬇️
scrapy/spidermiddlewares/offsite.py	100% <0%> (ø)	⬆️
scrapy/spidermiddlewares/depth.py	100% <0%> (ø)	⬆️
scrapy/spidermiddlewares/httperror.py	100% <0%> (ø)	⬆️
scrapy/core/spidermw.py	100% <0%> (ø)	⬆️
scrapy/extensions/corestats.py	100% <0%> (ø)	⬆️
scrapy/downloadermiddlewares/robotstxt.py	100% <0%> (ø)	⬆️
... and 57 more

[wRAR]

==================================== ERRORS ====================================
________ ERROR collecting tests/test_downloadermiddleware_httpcache.py _________
tests/test_downloadermiddleware_httpcache.py:157: in <module>
    class LeveldbStorageTest(DefaultStorageTest):
tests/test_downloadermiddleware_httpcache.py:159: in LeveldbStorageTest
    pytest.importorskip('leveldb')
E   SystemError: bad call flags
________ ERROR collecting tests/test_downloadermiddleware_httpcache.py _________
tests/test_downloadermiddleware_httpcache.py:157: in <module>
    class LeveldbStorageTest(DefaultStorageTest):
tests/test_downloadermiddleware_httpcache.py:159: in LeveldbStorageTest
    pytest.importorskip('leveldb')
E   SystemError: bad call flags

Hmm?

[wRAR]

So it looks like the latest leveldb (released in 2016) doesn't support Python 3.8, here is a patch in Fedora (which we obviously cannot use): https://src.fedoraproject.org/rpms/python-leveldb/c/57adbb30b4d4c1ee58976eaa8ca5b3cea9c516ab?branch=master

Scrapy uses leveldb for scrapy.extensions.httpcache.LeveldbCacheStorage. The module itself is only in the test requirements, the docs tell the user to install the module manually to use LeveldbCacheStorage.

Maybe we should skip the test for 3.8, at least for now, and document the problem at least in the test sources.

[kmike]

Hey! I think it'd be nice to fix it before declaring Python 3.8 support. Switch to another leveldb wrapper, e.g. https://github.com/wbolster/plyvel? The advantage of the old package is that it bundles leveldb (so it is easier to install), but it seems plyvel bundles leveldb as well (though not always) since v1.0.1:

Provide binary packages (manylinux1 wheels) for Linux.
These wheel packages have the LevelDB library embedded.

Another option would be to clone a repo to scrapy organization, fix an issue there, then release - we've done this for https://github.com/scrapy/pypydispatcher to get pypy support for a less maintained package. Though I'd just switch.

In this PR, to make it bounded, we can

• mark test as xfail (run=False),
• keep Python 3.8 on CI
• remove one of the -extra-deps environments
• remove Python 3.8 from setup.py for now?

[further-reading]

Thanks a lot for the help! I've come upon another issue where trying to run tox.ini is getting an error trying to load Twisted.

ERROR: Could not find a version that satisfies the requirement Twisted!=18.4.0 (from -c tests/constraints.txt (line 1)) (from versions: none)
ERROR: No matching distribution found for Twisted!=18.4.0 (from -c tests/constraints.txt (line 1))

I'll have a deeper look into that and your other feedback as soon as I can!

[wRAR]

The Debian leveldb maintainer just replied to the Py3.8 bug that they don't want leveldb in Debian anymore and prefer Plyvel instead.

[further-reading]

Hey! I've added xfails now to handle the issue with leveldb in python 3.8.

To give some additional info - the issue is likely similar to this one https://bugs.python.org/msg348804 which is caused by a python 3.8 change to the C-API that generates SystemError: bad call flags when some modules are imported. To incorporate skipping properly I added a try/except to the class LeveldbStorageTest object when it tries to import leveldb and then approrpiate xfails to the tests it would run on.

From what I understand the pytest.importorskip('leveldb') attribute in the class should be handling this - its purpose seems to be for skipping tests in a class if the module fails to import? Looking at the code in pytest it triggers only on ImportError, so I'll look into adding a feature there to have it trigger on SystemErrors too.

[Gallaecio]

We also need this: https://github.com/further-reading/scrapy/pull/1

I think that’s all. The remaining documentation changes (mentioning 3.8 support and LevelDB storage backend deprecation) belong to the news.rst documentation file, which is filled in a separate pull request soon before a release.

[Gallaecio]

Is the assignment needed?

Suggested change

	pytestmark = pytest.skip("'SystemError: bad call flags' occurs on Python 3.8")
	pytest.skip("'SystemError: bad call flags' occurs on Python 3.8")

[further-reading]

Yes - travis CI called it out specifically in the last build https://travis-ci.org/scrapy/scrapy/jobs/603880139?utm_medium=notification&utm_source=github_status

Using pytest.skip outside of a test is not allowed. To decorate a test function, use the @pytest.mark.skip or @pytest.mark.skipif decorators instead, and to skip a module use `pytestmark = pytest.mark.{skip,skipif}.

[Gallaecio]

This is getting complex.

I think you may be able to move the try-except to something like https://docs.pytest.org/en/latest/xunit_setup.html#method-and-function-level-setup-teardown, but feel free to go back to your initial approach of decorators based on the running Python version.

[further-reading]

I'm not sure how using a setup method is gonna make it less complex? You mean making something like or is there another approach I'm missing?

class LeveldbStorageTest(DefaultStorageTest):
    storage_class = 'scrapy.extensions.httpcache.LeveldbCacheStorage'

    @classmethod
    def setup_class(cls, *args, **kwargs):
        test_class = cls(*args, **kwargs)
        try:
            test_class.leveldb = pytest.importorskip('leveldb')
        except SystemError:
            test_class.pytestmark = pytest.mark.skip("'SystemError: bad call flags' occurs on Python >= 3.8")

        return test_class

Right now I have it refactored to a single check localized to the specific subclass where the problem arises, with clear commenting/logging when it triggers. Reverting back to the multiple decorators or incorporating a setup method seems like it'd be adding complexity as I am adding adding checks to multiple parts of the code or I am having to incorporate additional syntax to mimic the current behavior.

[further-reading]

For the latest travis test - The failure is happening in the python 3.6 enviornment and seems unrelated to the changes I made. https://travis-ci.org/scrapy/scrapy/jobs/603892627?utm_medium=notification&utm_source=github_status I've seen this test fail randomly a few times only to work fine on an additional run.

In this latest commit the py3.8 tests are working on travis.

[Gallaecio]

💄 This does not seem to be used below.