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

Add Python 3.8 official support #4092

Merged
merged 21 commits into from Oct 29, 2019
Merged

Add Python 3.8 official support #4092

merged 21 commits into from Oct 29, 2019

Conversation

further-reading
Copy link
Contributor

@further-reading further-reading commented Oct 21, 2019

Fixes #4085

.travis.yml Outdated Show resolved Hide resolved
Co-Authored-By: Mikhail Korobov <kmike84@gmail.com>
@codecov
Copy link

@codecov codecov bot commented Oct 21, 2019

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
Copy link
Contributor

@wRAR wRAR commented Oct 22, 2019

==================================== 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
Copy link
Contributor

@wRAR wRAR commented Oct 22, 2019

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
Copy link
Member

@kmike kmike commented Oct 22, 2019

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
Copy link
Contributor Author

@further-reading further-reading commented Oct 22, 2019

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
Copy link
Contributor

@wRAR wRAR commented Oct 22, 2019

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
Copy link
Contributor Author

@further-reading further-reading commented Oct 27, 2019

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.

.travis.yml Show resolved Hide resolved
Reverting change to 3.8 extra dependency environment.
.travis.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tests/test_downloadermiddleware_httpcache.py Outdated Show resolved Hide resolved
@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 28, 2019

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.

@further-reading further-reading changed the title [WIP] Add Python 3.8 official support Add Python 3.8 official support Oct 28, 2019
tox.ini Outdated Show resolved Hide resolved
pytest.importorskip('leveldb')
except SystemError:
# Happens in python 3.8
pytestmark = pytest.skip("'SystemError: bad call flags' occurs on Python 3.8")
Copy link
Member

@Gallaecio Gallaecio Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Contributor Author

@further-reading further-reading Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}.

Copy link
Member

@Gallaecio Gallaecio Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@further-reading further-reading Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@further-reading further-reading commented Oct 28, 2019

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.

@@ -6,6 +6,7 @@
import email.utils
from contextlib import contextmanager
import pytest
import sys
Copy link
Member

@Gallaecio Gallaecio Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💄 This does not seem to be used below.

wRAR
wRAR approved these changes Oct 29, 2019
@wRAR wRAR merged commit 18b808b into scrapy:master Oct 29, 2019
1 check failed
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.

4 participants