Make Kiva compileable with Swig 4

[jvkersch]

Fixes #360.

The Swig wrappers used a historic construct (using _swig_setattr instead of direct attribute assignment) that broke with the relase of Swig 4 (which removed _swig_setattr).

There are a lot of historical oddities with the Swig wrappers (as is well-known); I didn't touch anything beyond the strictly necessary, expecting the Swig layer to go away with #414.

[aaronayres35]

LGTM (although it may be beneficial to have a pair of more SWIG-savvy eyes on this), thank you for this PR @jvkersch !

In an environment with swig 4 installed, if I comment out this line:

enable/setup.py

Line 454 in 6e1c93e

verify_swig_version()

and I run pip install -e . on master things seems to work, however when running the test suite many tests fail with the NameError: name '_swig_setattr' is not defined.

Doing the same on this branch all the tests pass! 🎉 🙌 (aside from a couple failing with ModuleNotFoundError: No module named 'celiagg', but that is orthogonal - they should be skipped if missing celiagg).

I did some digging in the swig release notes / changelog but struggled to find specific mentions about this. These PRs seem potentially relevant:
swig/swig#718
swig/swig#1261
swig/swig@4301fa5

TBH I do not have a great understanding on why _swig_setattr used to be needed.

Once this is merged, a follow up PR can be added to remove the recently added verify_swig_version in setup.py.

Additionally, given we have dropped support for python 2, I believe we should be passing the -py3 flag as a swig option in setup.py? see swig/swig#695

[aaronayres35]

I think this is relevant to this?: swig/swig#1261
changelog entry for the above PR:
"""
2018-07-31: wsfulton
[Python] #1293 Overloaded C++ function wrappers now raise a TypeError instead
of NotImplementedError when the types passed are incorrect. This change means
there is now consistency with non-overloaded function wrappers which have always
raised TypeError when the incorrect types are passed. The error message remains
the same and is for example now:

          TypeError: Wrong number or type of arguments for overloaded function 'f'.
            Possible C/C++ prototypes are:
              f(int)
              f(char const *)

        instead of:

          NotImplementedError: Wrong number or type of arguments for overloaded function 'f'.
            Possible C/C++ prototypes are:
              f(int)
              f(char const *)

        *** POTENTIAL INCOMPATIBILITY ***

"""

[jwiggins]

I'm basically 👍 on this. Glad the change wasn't complicated.

[jwiggins]

Thanks for the update.

[jvkersch]

@jwiggins @aaronayres35 @rahulporuri Thanks for the reviews. I restored the comment that was erroneously removed -- I don't plan on dig into the Swig code any more.

The next thing to do would be to remove the check for the version of Swig (to be clear, I think we should still check for Swig). I can do that in this PR or in a follow-up PR.

[jwiggins]

I'd prefer a follow-up PR for removing the version check

[rahulporuri]

LGTM with one small question/comment.

[rahulporuri]

Just FTR swig/swig#1261. That's definitely one resource to help people understand why this change is needed - not sure if it's the best resource.

[jvkersch]

@rahulporuri Did you want me to make a change here, or is your comment to provide additional clarification?

[rahulporuri]

... is your comment to provide additional clarification?

just additional clarification, for the human-lizard hybrid developers who will revisit this PR in the future for debugging purposes.

[jwiggins]

for the human-lizard hybrid developers

Waitaminnit... what do you know?

[rahulporuri]

I'd prefer a follow-up PR for removing the version check

Same here. Let's get this merged so that I can revert the changes to the branch protections 😅 😬