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

cffi port #29

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@boytm
Contributor

boytm commented May 4, 2016

Is this directory structure is what you want?
Currently travis-ci pypy build fail on run "tests.py", show message:

ImportError: No module named _pycares_cffi .

But previous step "python setup.py install " show success. Any suggestion?

Show outdated Hide outdated pycares/_cfficore/__init__.py
PYCARES_ADDRTTL_SIZE = 256
class Error(StandardError):

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

StandardError is gone from Python 3, make it inherit from Exception.

@saghul

saghul May 5, 2016

Owner

StandardError is gone from Python 3, make it inherit from Exception.

@@ -62,7 +62,7 @@ pycares_methods[] = {
#ifdef PYCARES_PYTHON3
static PyModuleDef pycares_module = {
PyModuleDef_HEAD_INIT,
"pycares", /*m_name*/
"_core", /*m_name*/

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

this should be pycares._core

@saghul

saghul May 5, 2016

Owner

this should be pycares._core

This comment has been minimized.

@boytm

boytm May 6, 2016

Contributor

I see tornado speedups module use no prefix in python 3. https://github.com/tornadoweb/tornado/blob/master/tornado/speedups.c#L37
And I see python3 source code, it seems it calculate module.name automatically if last name of _Py_PackageContext is equal with this setting name
https://github.com/python/cpython/blob/master/Objects/moduleobject.c#L165

@boytm

boytm May 6, 2016

Contributor

I see tornado speedups module use no prefix in python 3. https://github.com/tornadoweb/tornado/blob/master/tornado/speedups.c#L37
And I see python3 source code, it seems it calculate module.name automatically if last name of _Py_PackageContext is equal with this setting name
https://github.com/python/cpython/blob/master/Objects/moduleobject.c#L165

This comment has been minimized.

@saghul

saghul May 6, 2016

Owner

Tests pass, so we can look into that later :-)

@saghul

saghul May 6, 2016

Owner

Tests pass, so we can look into that later :-)

This comment has been minimized.

@boytm

boytm May 11, 2016

Contributor

I debug with python 3.5.1 and confirm this behavior.

Breakpoint 2 at 0x478153: file Objects/moduleobject.c, line 166.
(gdb) p *module
$10 = {m_base = {ob_base = {ob_refcnt = 1, ob_type = 0x841bc0}, m_init = 0, m_index = 18, m_copy = 0x0}, m_name = 0x7ffff0897849 "_core", m_doc = 0x0, m_size = -1, m_methods = 0x7ffff0a9b220, m_slots = 0x0, m_traverse = 0, m_clear = 0, m_free = 0}
(gdb) p _Py_PackageContext
$11 = 0x7ffff7e8b120 "pycares._core"

@boytm

boytm May 11, 2016

Contributor

I debug with python 3.5.1 and confirm this behavior.

Breakpoint 2 at 0x478153: file Objects/moduleobject.c, line 166.
(gdb) p *module
$10 = {m_base = {ob_base = {ob_refcnt = 1, ob_type = 0x841bc0}, m_init = 0, m_index = 18, m_copy = 0x0}, m_name = 0x7ffff0897849 "_core", m_doc = 0x0, m_size = -1, m_methods = 0x7ffff0a9b220, m_slots = 0x0, m_traverse = 0, m_clear = 0, m_free = 0}
(gdb) p _Py_PackageContext
$11 = 0x7ffff7e8b120 "pycares._core"

Show outdated Hide outdated setup.py
)]
# cffi module
kwargs['setup_requires'] = ["cffi>=1.5.0"]

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

do this only if the implementation is "PyPy" and fail otherwise

@saghul

saghul May 5, 2016

Owner

do this only if the implementation is "PyPy" and fail otherwise

This comment has been minimized.

@boytm

boytm May 9, 2016

Contributor

Do you mean:

if (platform.python_implementation() == 'PyPy' or
        os.environ.get('PYCARES_CFFI', '0') != '0'):
    # build cffi module, only when PyPy or environment flags enable
else:
    # build original C ext, otherwise
@boytm

boytm May 9, 2016

Contributor

Do you mean:

if (platform.python_implementation() == 'PyPy' or
        os.environ.get('PYCARES_CFFI', '0') != '0'):
    # build cffi module, only when PyPy or environment flags enable
else:
    # build original C ext, otherwise

This comment has been minimized.

@saghul

saghul May 9, 2016

Owner

Exactly :-)

@saghul

saghul May 9, 2016

Owner

Exactly :-)

Show outdated Hide outdated pycares/_cfficore/__init__.py
@@ -0,0 +1,667 @@
#!/usr/bin/env python

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

drop the shebang

@saghul

saghul May 5, 2016

Owner

drop the shebang

Show outdated Hide outdated pycares/_cfficore/pycares_build.py
@@ -0,0 +1,698 @@
#!/usr/bin/env python

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

drop the shebang

@saghul

saghul May 5, 2016

Owner

drop the shebang

Show outdated Hide outdated pycares/_cfficore/__init__.py
#!/usr/bin/env python
from _pycares_cffi import ffi as _ffi, lib as _lib
import _cffi_backend

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

unused import?

@saghul

saghul May 5, 2016

Owner

unused import?

This comment has been minimized.

@boytm

boytm May 6, 2016

Contributor

Note that some bundler tools that try to find all modules used by a project, like PyInstaller, will miss _cffi_backend in the out-of-line mode because your program contains no explicit import cffi or import _cffi_backend. You need to add _cffi_backend explicitly (as a “hidden import” in PyInstaller, but it can also be done more generally by adding the line import _cffi_backend in your main program).

https://cffi.readthedocs.io/en/latest/cdef.html

@boytm

boytm May 6, 2016

Contributor

Note that some bundler tools that try to find all modules used by a project, like PyInstaller, will miss _cffi_backend in the out-of-line mode because your program contains no explicit import cffi or import _cffi_backend. You need to add _cffi_backend explicitly (as a “hidden import” in PyInstaller, but it can also be done more generally by adding the line import _cffi_backend in your main program).

https://cffi.readthedocs.io/en/latest/cdef.html

This comment has been minimized.

@saghul

saghul May 6, 2016

Owner

the plase add a comment so I don't accidentally remove it one day,

@saghul

saghul May 6, 2016

Owner

the plase add a comment so I don't accidentally remove it one day,

Show outdated Hide outdated pycares/_cfficore/__init__.py
@@ -0,0 +1,667 @@
#!/usr/bin/env python
from _pycares_cffi import ffi as _ffi, lib as _lib

This comment has been minimized.

@saghul

saghul May 5, 2016

Owner

where is this _pycares_cffi thing?

@saghul

saghul May 5, 2016

Owner

where is this _pycares_cffi thing?

This comment has been minimized.

@boytm

boytm May 6, 2016

Contributor

pycares/_cfficore/pycares_build.py will automatic generate this _pycares_cffi.so module by call cff.FFI.set_source()

@boytm

boytm May 6, 2016

Contributor

pycares/_cfficore/pycares_build.py will automatic generate this _pycares_cffi.so module by call cff.FFI.set_source()

This comment has been minimized.

@saghul

saghul May 6, 2016

Owner

as a top level module? it should do that inside the pycares/ namespace perhaps?

@saghul

saghul May 6, 2016

Owner

as a top level module? it should do that inside the pycares/ namespace perhaps?

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 5, 2016

Owner

I don't understand where _pycares_cffi is supposed to be.

Owner

saghul commented May 5, 2016

I don't understand where _pycares_cffi is supposed to be.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 5, 2016

Owner

I think the tests are failing on CPython 3 because of the broken name, that should be an easy fix.

Owner

saghul commented May 5, 2016

I think the tests are failing on CPython 3 because of the broken name, that should be an easy fix.

@boytm

This comment has been minimized.

Show comment
Hide comment
@boytm

boytm May 6, 2016

Contributor

cffi have two mode:

  • in ABI mode generate a .py file in order to reduce C function definiton parser time ,
  • in API mode call the compiler to generate a .so module. And _pycares_cffi is this .so file and named by FFI.set_source()
Contributor

boytm commented May 6, 2016

cffi have two mode:

  • in ABI mode generate a .py file in order to reduce C function definiton parser time ,
  • in API mode call the compiler to generate a .so module. And _pycares_cffi is this .so file and named by FFI.set_source()
@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 6, 2016

Owner

Ah, gotcha, thanks.

Owner

saghul commented May 6, 2016

Ah, gotcha, thanks.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 6, 2016

Owner

The Python 3 build seems fixed now. I'll also try to run the code here locally, to see what's up with PyPy.

Owner

saghul commented May 6, 2016

The Python 3 build seems fixed now. I'll also try to run the code here locally, to see what's up with PyPy.

boytm added some commits May 6, 2016

Fix appveyor python 3.3-3.5 build, pass the VC compiler path which fo…
…und by setuptools to Makefile.msvc. Because appveyor run_with_env.cmd only set VC environment for python x64 2.7-3.4 only. And the current c-ares library for python x86 3.3-3.4 is built by VC2008 which is default case in deps\c-ares\vcbuild.bat, but pycares._core is built by VC2010, so the final dll is linked with two msvcrt.dll library.
To fix Python35 build. We use setuptools to build c-ares library inst…
…ead of old vcbuild.bat which use a different version VC compiler. But currently only windows and linux platforms is support.
@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 24, 2016

Owner

Oh, I got a notification from AppVeyor that all tests pass! Fantastic! I like the approach to building c-ares with setuptools. I'll look into merging this tonight and ironing out any remaining issues (on OSX for example) and also enabling OSX on Travis.

Owner

saghul commented May 24, 2016

Oh, I got a notification from AppVeyor that all tests pass! Fantastic! I like the approach to building c-ares with setuptools. I'll look into merging this tonight and ironing out any remaining issues (on OSX for example) and also enabling OSX on Travis.

Show outdated Hide outdated setup_cares.py
cares_sources += ['deps/c-ares/src/windows_port.c',
'deps/c-ares/src/ares_platform.c']
self.compiler.add_include_dir(os.path.join(self.cares_dir, 'src/config_win32'))
#-D_WIN32_WINNT=0x0600

This comment has been minimized.

@saghul

saghul May 24, 2016

Owner

do we need to set this?

@saghul

saghul May 24, 2016

Owner

do we need to set this?

This comment has been minimized.

@boytm

boytm May 25, 2016

Contributor

The original c-ares Makefile define "--std=gnu89 -D_WIN32_WINNT=0x0600" for MinGW, but I can built it without these flags on MinGW GCC 4.9.
So I‘ll delete this.

@boytm

boytm May 25, 2016

Contributor

The original c-ares Makefile define "--std=gnu89 -D_WIN32_WINNT=0x0600" for MinGW, but I can built it without these flags on MinGW GCC 4.9.
So I‘ll delete this.

self.compiler.define_macro('_FILE_OFFSET_BITS', 64)
if sys.platform.startswith('linux'):
self.compiler.add_include_dir(os.path.join(self.cares_dir, 'src/config_linux'))

This comment has been minimized.

@saghul

saghul May 24, 2016

Owner

can you add the include dirs also for osx and freebsd?

@saghul

saghul May 24, 2016

Owner

can you add the include dirs also for osx and freebsd?

This comment has been minimized.

@boytm

boytm May 24, 2016

Contributor

yes, I can add it according to the original Makefile . but I have no osx to verify it.

@boytm

boytm May 24, 2016

Contributor

yes, I can add it according to the original Makefile . but I have no osx to verify it.

This comment has been minimized.

@saghul

saghul May 24, 2016

Owner

please do, I can test it.

@saghul

saghul May 24, 2016

Owner

please do, I can test it.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 24, 2016

Owner

@boytm I'm tidying up the commits here: https://github.com/saghul/pycares/tree/cffi

You already did 98% of it. Thanks a lot! Will ping you back once it lands.

Owner

saghul commented May 24, 2016

@boytm I'm tidying up the commits here: https://github.com/saghul/pycares/tree/cffi

You already did 98% of it. Thanks a lot! Will ping you back once it lands.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul May 25, 2016

Owner

Ok, done! Landed in the following commits:

b64d53f cffi: fixup FreeBSD support
9d3deb3 cffi: fix OSX support
7511d0e core: add CFFI core implementation
baab29e core: reorganize package
c3e2d1e ci: add Python 3.5 to AppVeyor
a3c4e7f build: refactor building bundled c-ares
33b242d ci: use PyPy 5 in Travis CI
92afcf1 test: add Python 3.5 to tox.ini
962646c test: moved test file to tests/tests.py

Thanks a ton for your work! I'll make a 2.0 release tonight. 🎉

Owner

saghul commented May 25, 2016

Ok, done! Landed in the following commits:

b64d53f cffi: fixup FreeBSD support
9d3deb3 cffi: fix OSX support
7511d0e core: add CFFI core implementation
baab29e core: reorganize package
c3e2d1e ci: add Python 3.5 to AppVeyor
a3c4e7f build: refactor building bundled c-ares
33b242d ci: use PyPy 5 in Travis CI
92afcf1 test: add Python 3.5 to tox.ini
962646c test: moved test file to tests/tests.py

Thanks a ton for your work! I'll make a 2.0 release tonight. 🎉

@saghul saghul closed this May 25, 2016

@saghul saghul referenced this pull request May 25, 2016

Closed

cffi port #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment