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

Adds _PYPY to twisted.python.compat #223

Closed
wants to merge 9 commits into from

Conversation

rodrigc
Copy link
Contributor

@rodrigc rodrigc commented Jun 26, 2016

@codecov-io
Copy link

codecov-io commented Jun 26, 2016

Current coverage is 88.48%

Merging #223 into trunk will decrease coverage by 1.63%

@@              trunk       #223   diff @@
==========================================
  Files           821        821           
  Lines        143608     143614      +6   
  Methods           0          0           
  Messages          0          0           
  Branches      13205      13206      +1   
==========================================
- Hits         129419     127082   -2337   
- Misses        11947      14144   +2197   
- Partials       2242       2388    +146   

Powered by Codecov. Last updated by 20df2cf...487b49c

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 26, 2016

@markrwilliams I am trying to merge your patch, but the codecov checks which were recently made mandatory are preventing me from doing the merge. See: #223 , in the section Some checks were not successful

Your changes are legit, so there is nothing on your side.

@adiroiban @glyph I think we need to add pypy to the list of coverage reports that are uploaded to codecov. I don't know how to do that. Can you help?

@@ -262,8 +261,8 @@ def __reduce__(self):

def __getstate__(self):
log.msg( "WARNING: serializing ephemeral %s" % self )
import gc
if '__pypy__' not in sys.builtin_module_names:
if _PYPY:
Copy link
Member

Choose a reason for hiding this comment

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

pretty sure this condition is backwards, should be if not _PYPY

Copy link
Member

Choose a reason for hiding this comment

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

you are right :(

@adiroiban
Copy link
Member

this is ugly, but yes, we need to add a coverage report for pypy.

I am trying to get coverage report for pypi....see twisted-infra/braid#209

I would say that if someone cares about Twisted on PyPy, the work should start by fixing the current tests to make PyPy a supported platform.

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 26, 2016

@adiroiban At the Twisted Meetup in San Francisco, @markrwilliams mentioned that he was interested in fixing the tests that are breaking on pypy, so he is working on it.

@markrwilliams
Copy link
Member

That's exactly what I'm aiming to do :) I've got set of changes that (seem
to) make all tests pass on PyPy. I'm trying to break them up into
individual patches/PRs to make review easier.

If it makes getting coverage working easier I can push them as one
patch/PR. The diff is +150/-35 so it's not huge.

On Sun, Jun 26, 2016 at 1:09 PM, Adi Roiban notifications@github.com
wrote:

this is ugly, but yes, we need to add a coverage report for pypy.

I am trying to get coverage report for pypi....see twisted-infra/braid#209
twisted-infra/braid#209

I would say that if someone cares about Twisted on PyPy, the work should
start by fixing the current tests to make PyPy a supported platform.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#223 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAq-9y7MoL2yh0-4TWf0aYrKWT7Q1MKWks5qPtx3gaJpZM4I-hDk
.

@rodrigc
Copy link
Contributor Author

rodrigc commented Jun 26, 2016

@markrwilliams hold on and wait for the pypy coverage buildbot to be set up.
Splitting up into small patches is a good approach. That is what I have been doing for Python3 stuff, and it has worked out well.

@markrwilliams
Copy link
Member

@rodrigc you got it :)

FWIW I think addressing @alex's comment will restore coverage and hopefully solve the mergability problem.

@adiroiban
Copy link
Member

I am trying to build pypy with travis-ci ... and have it in the allow_failures

@markrwilliams
Copy link
Member

@adiroiban ok! thank you!!

@adiroiban
Copy link
Member

The PyPy Travis-CI part has this ticket https://twistedmatrix.com/trac/ticket/8505 ... hope it will be easy :)

@adiroiban
Copy link
Member

See #226 ... I am stuck as it looks like coverage is not working on PyPy.

@markrwilliams please take a look and see if you can help. Thanks!

I am a simple py2.7 gui and I have never used the fancy py3 or pypy versions :)

@markrwilliams
Copy link
Member

@adiroiban good news - i can reproduce the coverage failure on my laptop. i'll dig in and see if i can figure things out. thanks!!

@adiroiban
Copy link
Member

@markrwilliams thanks! Please send your feedback to #226 so that we can have it fixed in trunk.

@rodrigc rodrigc force-pushed the 8503-markrwilliams-pypy-indicator branch from 1ae0792 to d1441e2 Compare June 29, 2016 09:38
@rodrigc
Copy link
Contributor Author

rodrigc commented Jul 8, 2016

@glyph @adiroiban Can you override codecov and merge this? codecov is blocking this,
but only because the PyPy builder which can upload coverage results to codecov is not set up yet.
This is blocking @markrilliams from moving forward with PyPy work.

@rodrigc
Copy link
Contributor Author

rodrigc commented Jul 8, 2016

Closing this due to this review feedback: https://twistedmatrix.com/trac/ticket/8503#comment:7

@rodrigc rodrigc closed this Jul 8, 2016
@rodrigc
Copy link
Contributor Author

rodrigc commented Jul 9, 2016

@glyph based on your latest feedback, can you merge this PR? codecov is blocking it.
This PR will help lay the foundation to get a working Pypy buildbot

@rodrigc rodrigc reopened this Jul 9, 2016
@rodrigc
Copy link
Contributor Author

rodrigc commented Jul 18, 2016

I would have preferred this to get merged instead of #221 .
But #221 was merged, so closing this.

@rodrigc rodrigc closed this Jul 18, 2016
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.

None yet

5 participants