OpenCMISS / iron

Source code repository for OpenCMISS-Iron
9 stars 62 forks source link

Python bindings wrapper fails for DiagnosticsSetOn with python 3 #156

Open chrispbradley opened 6 years ago

chrispbradley commented 6 years ago

The (python) bindings for the DiagnosticsSetOn routine is something like

iron.DiagnosticsSetOn(iron.DiagnosticTypes.FROM,[1,2,3,4,5],"Diagnostics",["routineName1","routineName2"])

with the last argument being a list of routine names. This worked for python 2 but it now doesn't work with python 3. The problem comes from the python wrapper code. The problem code in iron_python_wrapper.c is something like

len6 = PyObject_Length(obj3);
for (i6 =0; i6 < len6; i6++) {
  o6 = PySequence_GetItem(obj3,i6);
  if (!PyString_Check(o6)) {
    Py_XDECREF(o6);
    PyErr_SetString(PyExc_ValueError,"Expected a sequence of strings");
    return NULL;
  } 

Under python 3 the PyString_Check fails and so we get an "Expected a sequence of strings" error message.

In python 3 the strings are a bit different as the PyString calls no longer exist and are replaced with either PyByte or PyUnicode calls. SWIG tries to maintain python 2/3 compatibility by doing

/ Compatibility macros for Python 3 /

if PY_VERSION_HEX >= 0x03000000

...

define PyString_Check(name) PyBytes_Check(name)

...

However, what we really need is PyUnicode_Check as we are dealing with strings rather than raw bytes.

I suggest doing something like

if PY_VERSION_HEX >= 0x03000000

if(!PyUnicode_Check(o6) {

else

if(!PyString_Check(o6) {

endif

Any comments?

hsorby commented 6 years ago

There should be a typemap that can be applied by SWIG saving the need for doing this directly. I would try the typemap first and then fall back to this.

chrispbradley commented 6 years ago

Yes, I was going to change the typemap in https://github.com/OpenCMISS/iron/blob/develop/bindings/python/iron.i Is this what you mean?

hsorby commented 6 years ago

Yes, partly. I was actually thinking of trying to use cstring.i to do this work for us instead of using our own implementation.

So maybe a bit more work than the above proposal but it would end up with less coding (and simpler coding) on our side.