Port C extension to Python 3

hannosch commented 7 years ago

After we have a port of the ExtensionClass C extension to Python 3, it should be possible to also port Acquisition's C extension.

@stephan-hof had shown interest in the past but other are most welcome to work on it ;)

stephan-hof commented 7 years ago

Yeah, I have an experimental branch locally where some parts are already ported. Be warned the size of a final patch will be around the same as porting ExtensionClass :(

stephan-hof commented 7 years ago

Hi,

I added a branch https://github.com/zopefoundation/Acquisition/tree/c-extension-py3 which adds python3 support for the C-Extension.

I run the unittests with python2.7 and python3.6 and in both versions they are successfully.
This branch works only with the following branch of ExtensionClass: https://github.com/zopefoundation/ExtensionClass/pull/11

However there are parts which should be mentioned/discussed.

------

aq_inContextOf returns now a bool object (True False) instead of 1 0. I did not like keeping the number, because in python2 there is int/long and in python3 there is only long. So I could either

return always long or
return int in python2 and long in python3.

The first way changes the behaviour in python2 anyway. The second way I dislike, because the function returns different types depending on the python version you are using. By returning a bool there is a consistent return value independent of the python version being used.

-----

More methods of the number protocol are implemented (https://docs.python.org/3/reference/datamodel.html#emulating-numeric-types). Acquisition supported the number protocol till python 1.6. All new special methods since then (python 2.0 and greater), like __iad__, __isub__, ..... were not wrapped at all. Surprisingly nobody noticed this for 17 years. In this branch all these special methods are correctly wrapped. Even the matrix multiplication from python3.5

----

tp_richcompare is not correctly wrapped. This is the current behaviour of tp_richcompare in python2: Instead of calling __eq__, __neq__, __lt__, .. Acquisition calls __cmp__ and converts the return value accordingly. Look at diff_to_bool in Acquistion.c for details. So the python2 version already ignores these special methods and just calls __cmp__. I kept this behaviour for python3 !! Which is pretty awkward, because __cmp__ is not used by python3 any more. The only positive thing is, that existing classes implementing __cmp__ are still working in python3. Full support of tp_richcompare would mean that that special methods like __eq__ would get an Acquisition wrapped self. So in __eq__ you have access to the Acquisition chain. However to get this, we have to copy&paste the implementation of tp_richcompare from cpython. Then we could modify this code to call the special methods with a correctly wrapped self. I don't like copying&pasting code from cpython, since it is so hard to maintain. Even worse the python2 and python3 implementation are very different. Up to now, nobody cared for proper support of tp_richcompare. So I'm reluctant to add all this code for supporting it properly. Note: comparison in python involves a lot of magic and code paths. So it could be that I missed something. I'll dig a bit deeper next weekend.

----

There is other stuff, which are not strictly related to the port. So they don't need to be solved immediately.

The following slots in PySequenceMethods are not wrapped.
- sq_inplace_concat
- sq_inplace_repeat
Implement tp_iternext and call a acquistion wrapped __next__
PEP7 stuff. Acquisition mixes tabs and spaces and a simple :retab does not work. I tried it and got errors in the unittests.
Modernize the C-Code a bit. The code was written ~14 years ago and the C-API of python was not as rich as today. Some constructs can be replaced with functions macros coming from python directly. For example:
- CallMethodO can be replaced with PyObject_CallMethod
- apply_filter could use PyTuple_Pack

Having said all this. I think this branch can be used by other packages depending on Acquisition. So you may have a look at the sprint this week. I'm sorry that I cannot participate.

icemac commented 7 years ago

@stephan-hof Thank you for your porting work. Your description of the changes sounds reasonable. Unfortunately I know way to little of the Python C API to say anything about the code.

hannosch commented 7 years ago

@stephan-hof I've created the #13 PR out of your branch. I've pushed one more commit to it, to avoid the Python 3.3 / setuptools related issues.

stephan-hof commented 7 years ago

@hannosch Thanks for creating the pull request. Should have done it myself.

icemac commented 7 years ago

@hannosch @stephan-hof Does the merge in #13 mean that the C code is now ported to Python 3?

hannosch commented 7 years ago

@icemac Yes, the C code is ported. I've left this issue open, as there's still some minor improvements and code cleanups that could happen before we release a new version of Acquisition.

stephan-hof commented 7 years ago

I'm sorry I pushed the wrong button. Did not want to close it. @hannosch did you see my mail about 'how much' improvements should be done to Acquisition.

hannosch commented 7 years ago

@stephan-hof yeah, I got your mail. Still sitting in my inbox to reply to, once I have more than 5 minutes to spare :)

stephan-hof commented 7 years ago

I made a separate pull request #16 regarding code clean ups. So I see no need to keep this issue open.

zopefoundation / Acquisition