Skip to content
This repository was archived by the owner on Jun 1, 2022. It is now read-only.

Python 3 support #12

Merged
merged 6 commits into from
Mar 21, 2014
Merged

Python 3 support #12

merged 6 commits into from
Mar 21, 2014

Conversation

zackw
Copy link
Contributor

@zackw zackw commented Mar 21, 2014

Please consider these changes, which collectively add Python 3 support. The bulk of the work is modernizing CAPI use to the >=2.6 standard; the only Py3-specific changes are related to str/unicode and the different signature for module initialization.

@oschwald
Copy link
Member

Thanks for the patch. Beyond just adding Python 3 support, it looks like a worthwhile clean-up of the code. Before merging, we need to discuss internally whether we are ready to drop Python 2.5 support, but I suspect that we are.

@zackw
Copy link
Contributor Author

zackw commented Mar 21, 2014

2.6 is the oldest Python I can conveniently test, but I suspect 2.5 will
either still work, or require only trivial changes. Most of the "new" stuff
I used is documented as present since the object revamp in 2.2.

If you do still need 2.5 internally and my changes don't work as is, I can
try to find time to set up a 2.5 environment and fix it, but I can't
promise any timeframe.

@oschwald
Copy link
Member

I just tried building it on 2.5 using pyenv and received the following:

/usr/bin/gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I/home/greg/.pyenv/versions/2.5.6/include/python2.5 -c py_GeoIP.c -o build/temp.linux-x86_64-2.5/py_GeoIP.o
py_GeoIP.c:662:5: warning: implicit declaration of function ‘PyVarObject_HEAD_INIT’ [-Wimplicit-function-declaration]
     PyVarObject_HEAD_INIT(NULL, 0)
     ^
py_GeoIP.c:663:5: error: initializer element is not constant
     "GeoIP.GeoIP",               /*tp_name*/
     ^
py_GeoIP.c:663:5: error: (near initialization for ‘GeoIP_GeoIPType.ob_refcnt’)
py_GeoIP.c:663:5: error: expected ‘}’ before string constant
py_GeoIP.c: In function ‘GeoIP_populate_module’:
py_GeoIP.c:762:5: warning: implicit declaration of function ‘PyModule_AddIntMacro’ [-Wimplicit-function-declaration]
     CHECK(PyModule_AddIntMacro(m, GEOIP_STANDARD));
     ^
py_GeoIP.c: At top level:
py_GeoIP.c:47:1: warning: ‘GeoIP_GeoIP_init’ defined but not used [-Wunused-function]
 GeoIP_GeoIP_init(PyObject *self, PyObject *args, PyObject *kwargs)
 ^
py_GeoIP.c:79:13: warning: ‘GeoIP_GeoIP_dealloc’ defined but not used [-Wunused-function]
 static void GeoIP_GeoIP_dealloc(PyObject *self)
             ^
py_GeoIP.c:565:20: warning: ‘GeoIP_GeoIP_methods’ defined but not used [-Wunused-variable]
 static PyMethodDef GeoIP_GeoIP_methods[] = {
                    ^
py_GeoIP.c:651:20: warning: ‘GeoIP_GeoIP_getsets’ defined but not used [-Wunused-variable]
 static PyGetSetDef GeoIP_GeoIP_getsets[] = {
                    ^
error: command '/usr/bin/gcc' failed with exit status 1

@oschwald
Copy link
Member

I got some free time to look into that failure and fixed it in the branch greg/py3. We will review the rest of the changes and hopefully merge this.

zackw added 2 commits March 21, 2014 15:43
Also flip one piece of compatibility logic around, so the ifdefs at
the top of the file consistently define new stuff in terms of old stuff
when necessary, allowing the bulk of the code to be written new-style.
@zackw
Copy link
Contributor Author

zackw commented Mar 21, 2014

Thanks for the pointer to pyenv; that makes backward compatibility testing much much easier.

I just pushed another batch of changes which should fix the py2.5 failures and also the CI failures (all of the CI failures appear to be because of -Wmissing-field-initializers, which is not in any of my pythons' default CFLAGS). Now works for me with 2.5, 2.6, 2.7, 3.3, and 3.4.

oschwald added a commit that referenced this pull request Mar 21, 2014
@oschwald oschwald merged commit 5cb489c into maxmind:master Mar 21, 2014
@oschwald
Copy link
Member

Everything looks great. Thanks! 👍

@oschwald
Copy link
Member

I released this to PyPI.

@zackw
Copy link
Contributor Author

zackw commented Mar 21, 2014

Awesome! Thanks for the quick turnaround.

@oschwald
Copy link
Member

I realized that non-ascii strings were broken in Python 3 with these changes as they are encoded in Latin1 by default. I fixed this in #13 and will release that once someone here reviews it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants