[PATCH] python: Remove -builtin flag for SWIG bindings.

[Tobias Mueller]

Hi,

On Wed, Dec 07, 2016 at 03:32:52PM +0100, Justus Winter wrote:
> Tobias Mueller <muelli at cryptobitch.de> writes:
> > The -builtin flag prevents that, though, because Python code
> > will not be compiled.
> 
> I don't quite understand.  Python being compiled sounds at best
> ambiguous.
True.

When run with -builtin, SWIG will not generate Python proxy classes:
<http://www.swig.org/Doc2.0/Python.html#Python_nn29>.
It seems to especially not generate "additional pythoncode":
<http://www.swig.org/Doc2.0/Python.html#Python_nn42>
That, however, can be useful to write parts of the wrapper in Python
rather than C or SWIG.

> Can you please provide an example of what this change will allow us to
> do?
Will send patches.

> > The -py3 flag prevents the SWIG bindings to run under python2
> > when generated without the -builtin flag, because the py3 flag
> > generates python3 code which is incompatible with python2.
> >
> > So we conditionally generate SWIG bindings with -py3.
> 
> Indeed.  That sounds like a good idea in general, independent from the
> '-builtin' issue.
It's not fully independent, because with -builtin, the py3 flag seems to be
irrelevant. The library has been fully functional with python2, after all.
It seems the py3 flag only influences the generated python code.
With the -builtin flag, you don't seem to get any.  That is, the generated
gpg/gpgme.py file consists only of a few functions rather than proxy classes.

$ git diff --stat /tmp/gpgme-with-builtin.py  /tmp/gpgme-no-builtin.py 
 .../{gpgme-with-builtin.py => gpgme-no-builtin.py} | 2894 +++++++++++++++++++-
  1 file changed, 2893 insertions(+), 1 deletion(-)
  

> >  swige = Extension("gpg._gpgme", ["gpgme.i", "helpers.c"],
> > -                  swig_opts = ['-py3', '-builtin', '-threads',
> > -                               '-outdir', 'gpg'] + extra_swig_opts,
> > +                  swig_opts = ['-threads',
> > +                               '-outdir', 'gpg'] + py3 + extra_swig_opts,
> 
> This basically reverts 856bcfe2934237011984fab0bc69800a7c25c34b.  Silly
> past-me did not write his reasons for doing that change in the
> changelog, but I guess I merely wanted to reduce the amount of wrapping
> being done here. 
Right.  The SWIG documentation <http://www.swig.org/Doc2.0/Python.html#Python_nn28>
leaves the impression that -builtin is solely for increasing performance:

    New in SWIG version 2.0.4: The use of Python proxy classes has performance 
    implications that may be unacceptable for a high-performance library. The 
    new -builtin option instructs SWIG to forego the use of proxy classes, and 
    instead create wrapped types as new built-in Python types. When this option 
    is used, the following section ("Proxy classes") does not apply. Details on 
    the use of the -builtin option are in the Built-in Types section.

> But I'm not opposed to reverting this if it has the
> benefits you mentioned.
cool.

Note that when the py3 flag kicks in, you also get interface definitions
(which cause the python2 incompatibility) for the offered functions which
people may appreciate for producing more type-safe code.

> However, in that commit I also had to adapt another piece of code, and
> you didn't revert that chunk.  Could you please investigate this?
> 
856bcfe2934237011984fab0bc69800a7c25c34b changed a call to SWIG_NewPointerObj
to a call to SWIG_Python_NewPointerObj.

The latter seems to be more internal, e.g. it cannot be found in the
documentation <http://www.swig.org/Doc2.0/Python.html>.

It seems to be an implementation detail, because in the generated gpgme_wrap.c
you can find this:

#ifdef SWIGPYTHON_BUILTIN
#define SWIG_NewPointerObj(ptr, type, flags)            SWIG_Python_NewPointerObj(self, ptr, type, flags)
#else
#define SWIG_NewPointerObj(ptr, type, flags)            SWIG_Python_NewPointerObj(NULL, ptr, type, flags)
#endif

So when compiling with -builtin, it expects "self" to be declared.
Because "self" had not declared, compilation fails.
That's probably why 856bcfe2934237011984fab0bc69800a7c25c34b changed the call.

Now I see two options: Either leave the call as is or change it back to
SWIG_NewPointerObj.  Leaving it has the advantage of working regardless whether
the builtin flag is set.  Changing it has the advange of using a documented
function rather than something that looks like a potentially fragile detail to
me.  When also definining a "self" variable before calling SWIG_NewPointerObj,
both benefits should be preserved at the expense of somewhat obscure code with a
stray self variable.  I will send a proposal with changing it back.

Cheers,
  Tobi