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

Upgrade PyPy for CI, and test both 3.5 (oldest) and 3.6 (newest) #4504

Merged
merged 28 commits into from Jul 16, 2020

Conversation

Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Apr 16, 2020

No description provided.

@Gallaecio
Copy link
Member Author

Gallaecio commented Apr 16, 2020

@wRAR testPayloadDefaultCiphers fails on PyPy (it unexpectedly works). Any ideas? Maybe we need to change the value of custom_ciphers to something other than 'CAMELLIA256-SHA'?

@wRAR
Copy link
Contributor

wRAR commented Apr 16, 2020

I need to check with Wireshark again what does it actually use to establish the connection (which shouldn't have been established). I think last time it failed to disable TLS1.2+ which we do in the test but I'm not sure.

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #4504 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4504      +/-   ##
==========================================
- Coverage   85.16%   85.14%   -0.02%     
==========================================
  Files         162      162              
  Lines        9748     9763      +15     
  Branches     1437     1441       +4     
==========================================
+ Hits         8302     8313      +11     
- Misses       1186     1189       +3     
- Partials      260      261       +1     
Impacted Files Coverage Δ
scrapy/core/downloader/contextfactory.py 90.00% <0.00%> (-6.67%) ⬇️
scrapy/utils/python.py 84.91% <0.00%> (-0.45%) ⬇️
scrapy/utils/trackref.py 85.71% <0.00%> (+2.85%) ⬆️

@kmike
Copy link
Member

kmike commented May 7, 2020

hey! What caused pypy tests to pass?

@@ -37,8 +38,8 @@ matrix:
install:
- |
if [ "$TOXENV" = "pypy3" ]; then
export PYPY_VERSION="pypy3.5-5.9-beta-linux_x86_64-portable"
Copy link
Member

@kmike kmike May 7, 2020

Choose a reason for hiding this comment

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

Do you think "Installing Scrapy" section should be updated, with the new minimal pypy version?

Copy link
Member Author

@Gallaecio Gallaecio May 7, 2020

Choose a reason for hiding this comment

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

Makes sense.

Once we fix the remaining issue, though, we should probably try and find the lowest version where tests pass.

Copy link
Member

@kmike kmike May 18, 2020

Choose a reason for hiding this comment

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

I'm not sure we should care too much about supporting old pypy versions; I think it is for advanced users to run Scrapy on pypy, and they could install a recent version anyways, to get best performance & compatibility. That should be fine to pick some version which is fresh enough, and doesn't cause us problems. Of course, knowing a specific pypy version is better, but I'd not put too much effort in figuring it out - if that's easy, let's do it, otherwise - don't bother.

Copy link
Member Author

@Gallaecio Gallaecio Jul 1, 2020

Choose a reason for hiding this comment

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

PyPy 5.9 continues to work.

I’ve also moved the FAQ entry about supported Python versions to install.rst, to avoid duplication. The previous information was accurate, though, so if you have any second thoughts about the documentation changes, we can revert that commit and leave the documentation as is.

@Gallaecio
Copy link
Member Author

Gallaecio commented May 7, 2020

hey! What caused pypy tests to pass?

They didn’t, because of some Travis CI downtime it just seemed that way.

@Gallaecio Gallaecio changed the title Upgrade PyPy for CI, and test both 3.5 (oldest) and 3.6 (newest) [WIP] Upgrade PyPy for CI, and test both 3.5 (oldest) and 3.6 (newest) May 22, 2020
@Gallaecio
Copy link
Member Author

Gallaecio commented Jun 17, 2020

mitmproxy does not officially support PyPy, so I guess we better skip tests that require it on PyPy.

@Gallaecio
Copy link
Member Author

Gallaecio commented Jun 18, 2020

I wonder if we should mark the 2 remaining issues as expected failure and create separate issues for them, to move this along.

@Gallaecio Gallaecio changed the title [WIP] Upgrade PyPy for CI, and test both 3.5 (oldest) and 3.6 (newest) Upgrade PyPy for CI, and test both 3.5 (oldest) and 3.6 (newest) Jul 1, 2020
# which was fixed in Cython 0.26, released on 2017-06-19, and used to
# generate the C headers of lxml release tarballs published since then, the
# first of which was:
'lxml>=4.0.0',
Copy link
Member Author

@Gallaecio Gallaecio Jul 1, 2020

Choose a reason for hiding this comment

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

It is technically possible for someone to install an older version if they re-generate the C headers with Cython 0.26+. If they are willing to do that, however, they can also change this number here locally for a Scrapy version supporting that.

kmike
kmike approved these changes Jul 1, 2020
Copy link
Member

@kmike kmike left a comment

👍

wRAR
wRAR approved these changes Jul 16, 2020
kmike
kmike approved these changes Jul 16, 2020
Copy link
Member

@kmike kmike left a comment

👍 👍

@kmike kmike merged commit d29bec6 into scrapy:master Jul 16, 2020
2 checks passed
@kmike
Copy link
Member

kmike commented Jul 16, 2020

Thanks @Gallaecio!

@noviluni
Copy link
Member

noviluni commented Aug 18, 2020

@Gallaecio, from what I read in the last comment here: #2333, as this has been merged, you should probably check that PR again

wRAR added a commit that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants