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

Make aiopg installable in PyPy environment #361

Closed
wants to merge 10 commits into from

Conversation

avanov
Copy link

@avanov avanov commented Jul 29, 2017

No description provided.

@asvetlov
Copy link
Member

Please enable PyPy in travis config to make sure that library is really functional on PyPy.

@avanov
Copy link
Author

avanov commented Jul 29, 2017

Is there a list of pypy versions available on Travis CI, or do they follow the same naming scheme as pyenv does? I test the package against pypy3.5-5.8.0 locally.

@asvetlov
Copy link
Member

@codecov
Copy link

codecov bot commented Jul 29, 2017

Codecov Report

Merging #361 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   93.49%   93.55%   +0.06%     
==========================================
  Files          23       26       +3     
  Lines        3550     3570      +20     
  Branches      205      209       +4     
==========================================
+ Hits         3319     3340      +21     
+ Misses        186      184       -2     
- Partials       45       46       +1
Impacted Files Coverage Δ
tests/test_sa_engine.py 98.93% <100%> (+0.04%) ⬆️
tests/test_extended_types.py 100% <100%> (ø) ⬆️
aiopg/utils.py 81.61% <100%> (+0.41%) ⬆️
tests/test_cursor.py 100% <100%> (ø) ⬆️
aiopg/psycopg2_compat/__init__.py 100% <100%> (ø)
aiopg/pool.py 98.89% <100%> (ø) ⬆️
tests/test_connection.py 98.21% <100%> (-0.01%) ⬇️
tests/conftest.py 66.66% <100%> (+1.2%) ⬆️
aiopg/psycopg2_compat/extras.py 100% <100%> (ø)
tests/test_sa_connection.py 100% <100%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb3d37c...8535cbe. Read the comment docs.

@avanov
Copy link
Author

avanov commented Jul 29, 2017

Ok, currently builds fail on PyPy because of the presence of psycopg2 in requirements.txt. What is the reason for having psycopg2 pinned in requirements.txt?

aiopg/utils.py Outdated
# that imports psycopg2 will use psycopg2cffi
if PY_IMPL == 'PyPy':
from psycopg2cffi import compat
compat.register()
Copy link
Member

Choose a reason for hiding this comment

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

I have never used psycopg2cffi, but what happened if compat.register() called two times? or after first import of psycopg2 in other module?

Copy link
Author

@avanov avanov Jul 30, 2017

Choose a reason for hiding this comment

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

@jettify it imports psycopg2cffi and makes it available under psycopg2 name in sys.modules (see https://github.com/chtd/psycopg2cffi/blob/master/psycopg2cffi/compat.py)

As long asaiopg.utils is imported in the main package namespace before psycopg2, all subsequent occurences of psycopg2 will be mapped to already imported psycopg2cffi (including relative import cases that go through the main aiopg namespace anyway).

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.
Thanks @jettify for pointing on.
Maybe better to explicitly import either psycopg2 or psycopg2cffi?
Doing this we'll never catch an import order problem.

Copy link
Author

Choose a reason for hiding this comment

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

@asvetlov do you mean you would prefer psycopg2_compat module with sth like this inside?

# aiopg/psycopg2_compat

if PY_IMPL == 'PyPy':
    from psycopg2cffi import *
else:
    from psycopg2 import *

Copy link
Member

@asvetlov asvetlov Jul 30, 2017

Choose a reason for hiding this comment

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

Yes, please do

@avanov
Copy link
Author

avanov commented Jul 30, 2017

@asvetlov shall I remove psycopg2 from requirements.txt ?

@jettify
Copy link
Member

jettify commented Jul 30, 2017

We use requirements.txt to track updated with pyup bot, so can be removed, I believe.

@avanov
Copy link
Author

avanov commented Aug 2, 2017

The test failures for CPython environments are now consistent with #362 due to changes introduced to psycopg2 between version 2.6.2 and 2.7.3. TravisCI now fetches the most recent psycopg2 version during the python setup.py install build step.

Copy link

@Daenyth Daenyth left a comment

Choose a reason for hiding this comment

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

This would be a great feature to have!

@steven-upside
Copy link

I see that this commit has been approved, are there plans to merge this anytime soon? PyPy + aiopg would be a great combination!

@asvetlov
Copy link
Member

asvetlov commented Nov 22, 2017

  1. PR was approved by arbitrary github user, not aiopg committer. Basically it is an another form to say "I like it, +1"
  2. Tests are still fails, the Pull Request is not ready for merging yet.

@avanov
Copy link
Author

avanov commented Nov 22, 2017

@steven-upside so yes, I am waiting for a response from psycopg2cffi on the issue above, and on another one at chtd/psycopg2cffi#85 , after which I think I could finalise this PR.

@aio-libs aio-libs deleted a comment from CLAassistant Nov 21, 2020
@avanov
Copy link
Author

avanov commented May 15, 2021

closing this as no longer relevant

@avanov avanov closed this May 15, 2021
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.

5 participants