On Sat, Dec 30, 2017 at 12:49 PM Branko Čibej <[email protected]> wrote:
> On 30.12.2017 16:11, Troy Curtis Jr wrote:
> >
> >
> > On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > Good morning Troy,
> >
> > [email protected] <mailto:[email protected]> wrote on
> > Fri, Dec 22, 2017 at 03:52:59 -0000:
> > > On branch swig-py3: Correctly manage swig objects with Python
> > new style classes.
> > >
> > > * build/ac-macros/swig.m4
> > > (SVN_FIND_SWIG):
> > > Request that swig generate new style classes, even for Python
> > 2, to reduce
> > > differences with Python 3.
> > >
> > > Modified: subversion/branches/swig-py3/build/ac-macros/swig.m4
> > > URL:
> >
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/build/ac-macros/swig.m4?rev=1818995&r1=1818994&r2=1818995&view=diff
> > >
> >
>
> ==============================================================================
> > > --- subversion/branches/swig-py3/build/ac-macros/swig.m4 (original)
> > > +++ subversion/branches/swig-py3/build/ac-macros/swig.m4 Fri Dec
> > 22 03:52:59 2017
> > > @@ -143,7 +143,7 @@ AC_DEFUN(SVN_FIND_SWIG,
> > > if test "$ac_cv_python_is_py3" = "yes"; then
> > > SWIG_PY_OPTS="-python -py3"
> > > else
> > > - SWIG_PY_OPTS="-python -classic"
> > > + SWIG_PY_OPTS="-python"
> > > fi
> >
> > What are the compatibility implications of this change? I
> > couldn't find any
> > documentation for the "-classic" option.
> >
> > > * subversion/bindings/swig/include/proxy.py
> > > (__getattr__): Replace __getattr__ with __getattribute__ to
> > correctly
> > > intercept attribute access, even when swig uses descriptors
> > for new style
> > > classes (which are the only type available in Python 3).
> >
> > Could you help me understand what's going on here and why it's
> > correct? I've
> > read the documentation of object.__getattr__ and
> > object.__getattribute__¹ but I
> > don't follow how the fact that the class into which this code is
> > included
> > became a new-style class (rather than a classic class) brought on
> > the need to
> > migrate from __getattr__ to __getattribute__, nor why the order of
> > precedence
> > of lookup (first __dict__, then object.__getattribute__, then
> > _swig_getattr) is
> > correct.
> >
> > I'm asking because I'm trying to review the branch (to +1 its
> > merge) and this
> > is one of the open questions I have.
> >
> > ¹
> >
> https://docs.python.org/3/reference/datamodel.html#object.__getattr__
> > et seq.
> >
> >
> > Sure thing. This is the change that took me so long to find, and then
> > fix the right way, so I certainly understand the non-obviousness of
> > it. Perhaps some portion of this explanation could go in an
> > appropriate place in the code for future reference? Though much of it
> > only applies to the motivation for the change and probably wouldn't be
> > beneficial just given the final implementation.
> >
> > Giving swig the '-classic' option forces it to generate only classic
> > classes. Without that flag it will produce code that works as either
> > an old style or new style class, dependent on a try/except block
> > around 'import object'. In practice, this means that for any Python
> > past 2.2 (when new-style classes were introduced), the classes are in
> > the new style and inherit from 'object'.
>
> This all makes sense and seems nice on the surface, but I'm not sure we
> can just change the behaviour of the bindings from old-style to
> new-style classes in a Python 2.x build. There are enough subtle
> differences in behaviour between the two that existing scripts could
> break after an upgrade of the bindings.
>
> Python 3.x has only new-style (or rather, even-newer-style) classes and
> there's no backward-compatibility consideration, since our bindings
> currently don't work with Python3.
>
>
That is a reasonable concern. I definitely preferred the cleaner single
implementation, but honestly the code necessary to continue to use classic
classes in python 2 is not large. I've attached a working patch for
reference/discussion. It is a bit more code and some conditional
definitions, but perhaps it is the more preferred course to take?
[[[
On branch swig-py3: Go back to using classic classes for Python 2 swig
bindings.
Add some additional clarifying comments for the reasons behind overriding
__getattr__ and __getattribute__.
* build/ac-macros/swig.m4
(SVN_FIND_SWIG): Add the '-classic' flag to swig when python 2 is
detected.
* subversion/bindings/swig/include/proxy.py
(_retrieve_swig_value): Factor out metadata retrieval from
__getattribute__ to a new function.
(__getattribute__): Only define __getattribute__ for new style classes.
(__getattr__): Add back implementation for classic classes.
]]]
Troy
Index: build/ac-macros/swig.m4
===================================================================
--- build/ac-macros/swig.m4 (revision 1819610)
+++ build/ac-macros/swig.m4 (working copy)
@@ -155,7 +155,7 @@
if test "$ac_cv_python_is_py3" = "yes"; then
SWIG_PY_OPTS="-python -py3"
else
- SWIG_PY_OPTS="-python"
+ SWIG_PY_OPTS="-python -classic"
fi
dnl Sun Forte adds an extra space before substituting APR_INT64_T_FMT
Index: subversion/bindings/swig/include/proxy.py
===================================================================
--- subversion/bindings/swig/include/proxy.py (revision 1819610)
+++ subversion/bindings/swig/include/proxy.py (working copy)
@@ -12,42 +12,68 @@
if "_is_valid" in self.__dict__:
assert self.__dict__["_is_valid"](), "Variable has already been deleted"
- def __getattribute__(self, name):
- """Manage access to all attributes of this object."""
+ def _retrieve_swig_value(self, name, value):
+ # If we got back a different object than we have cached, we need to copy
+ # all our metadata into it, so that it looks identical to the one
+ # originally set.
+ members = self.__dict__.get('_members')
+ if members is not None and name in members:
+ _copy_metadata_deep(value, members[name])
- # Start by mimicing __getattr__ behavior: immediately return __dict__ or
- # items directly present in __dict__
- mydict = object.__getattribute__(self, '__dict__')
- if name == "__dict__":
- return mydict
+ # Verify that the new object is good
+ _assert_valid_deep(value)
- if name in mydict:
- return mydict[name]
+ return value
- object.__getattribute__(self, 'assert_valid')()
+ # SWIG classes generated with -classic do not define this variable,
+ # so set it to 0 when it doesn't exist
+ try: _newclass
+ except NameError: _newclass = 0
- try:
- value = object.__getattribute__(self, name)
- except AttributeError:
- value = _swig_getattr(self,
- object.__getattribute__(self, '__class__'),
- name)
+ # Attribute access must be intercepted in order ensure that objects coming
+ # from read attribute access match those that are set with write
+ # attribute access. Specifically the metadata, such as associated apr_pool
+ # object, should match the originally assigned object.
+ #
+ # For classic classes it is enough to use __getattr__ to intercept swig
+ # derived attributes. However, with new style classes SWIG makes use of
+ # descriptors which mean that __getattr__ is never called. Therefore,
+ # __getattribute__ must be used for the interception.
- # If we got back a different object than we have, we need to copy all our
- # metadata into it, so that it looks identical
- try:
- members = object.__getattribute__(self, '_members')
- if name in members:
- _copy_metadata_deep(value, members[name])
- # Verify that the new object is good
- except AttributeError:
- pass
+ if _newclass:
+ def __getattribute__(self, name):
+ """Manage access to all attributes of this object."""
- # Verify that the new object is good
- _assert_valid_deep(value)
+ # Start by mimicing __getattr__ behavior: immediately return __dict__ or
+ # items directly present in __dict__
+ mydict = object.__getattribute__(self, '__dict__')
- return value
+ if name == "__dict__":
+ return mydict
+ if name in mydict:
+ return mydict[name]
+
+ object.__getattribute__(self, 'assert_valid')()
+
+ try:
+ value = object.__getattribute__(self, name)
+ except AttributeError:
+ value = _swig_getattr(self,
+ object.__getattribute__(self, '__class__'),
+ name)
+
+ fn = object.__getattribute__(self, '_retrieve_swig_value')
+ return fn(name, value)
+ else:
+ def __getattr__(self, name):
+ """Get an attribute from this object"""
+ self.assert_valid()
+
+ value = _swig_getattr(self, self.__class__, name)
+
+ return self._retrieve_swig_value(name, value)
+
def __setattr__(self, name, value):
"""Set an attribute on this object"""
self.assert_valid()
