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

Python bindings wrapper fails for DiagnosticsSetOn with python 3 #156

Open
chrispbradley opened this issue Jul 12, 2018 · 3 comments
Open

Comments

@chrispbradley
Copy link
Member

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
Copy link
Contributor

hsorby commented Jul 15, 2018

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
Copy link
Member Author

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
Copy link
Contributor

hsorby commented Jul 16, 2018

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.

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

No branches or pull requests

2 participants