You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jun Omae <ju...@gmail.com> on 2019/11/13 13:22:49 UTC
[Patch] Support building with SWIG 4 on Python 3.x
Hi,
I tried to support building SWIG 4 on Python 3.x, and post patch.
Since SWIG 4, -classic and -modern options have been dropped. As the result,
SWIG generates new-style classes which are using property() instead of
__swig_[gs]etmethods__.
In attached patch, SWIG would generate new-style classes without
__swig_[gs]etmethods__ when SWIG 3.x is used (-modern option would be used),
in the same as SWIG 4.x.
Verified by check-swig-py with:
- Python 3.[5678] with SWIG 1.3.40, 2.0.12, 3.0.12 on Ubuntu 16.04 amd64
- Python 2.7 with SWIG 3.0.12 and 4.0.1 on Ubuntu 16.04 amd64
[[[
Support building SWIG 4 on Python 3.x.
* build/ac-macros/swig.m4 (SVN_FIND_SWIG):
Allow building with SWIG 4+, and add -modern option when Python 3 and
SWIG 3.x are detected.
* subversion/bindings/swig/include/proxy.py
Use _get_instance_attr and _set_instance_attr.
* subversion/bindings/swig/include/proxy.swg
(_get_instance_attr): New function to get an instance attribute
without metadata for new-style and old-style classes.
(_set_instance_attr): New function to set an instance attribute for
new-style and old-style classes.
]]]
--
Jun Omae <ju...@gmail.com> (大前 潤)
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/11/15 16:37, Branko Čibej wrote:
> On 15.11.2019 08:10, Yasuhito FUTATSUKI wrote:
>> On 2019/11/15 14:04, Nathan Hartman wrote:
>>> On Thu, Nov 14, 2019 at 8:18 AM Jun Omae <ju...@gmail.com> wrote:
>>>
>>>> (Posting to dev list, again...)
>>>>
>>>> On Thu, Nov 14, 2019 at 1:47 AM Julian Foad <ju...@apache.org>
>>>> wrote:
>>>>> Perhaps someone could commit this if there are no other concerns with
>>>>> it, and adjust INSTALL at the same time?
>>>>
>>>> Updated proposed patch, including a change of
>>>> subversion/bindings/swig/INSTALL.
>>>>
>>>
>>> Hi all,
>>>
>>> Jun, thank you for this patch.
>>>
>>> I would like to get this committed quickly if there are no
>>> objections, but
>>> this is not my area of expertise.
>>>
>>> Brane, do I understand correctly that you are satisfied with the
>>> results of
>>> your review/test?
>>
>> I also tested make check-swig-py on FreeBSD, with Python 2.7/3.7 and
>> SWIG 2.0.0/2.0.12/3.0.9/3.0.10/3.0.12/4.0.1 combination. Of course,
>> combination of Python 2.7 + SWIG 4.0.1 and combination of
>> Python 3.7 + SWIG < 3.0.10 are blocked by .check_swig_py target :)
>> Other combinations are passed the test.
>>
>> I considered if we can move classic style versus new style class
>> conditional
>> from run time to SWIG code generation time only, but it is not just
>> problem in this patch only, but also in current code.
>>
>>> In particular, the main question I have is with regards to the -modern
>>> option being added for SWIG 3.x .. <4 with Py3. If I understand
>>> correctly,
>>> this changes the way SWIG will generate accessors (properties vs
>>> getters/setters). Would this break any downstream code? (And if so,
>>> is that
>>> acceptable?)
>>
>> There is no Python 3 application depending on it, because we have not yet
>> released SWIG Python bindings that supports Python 3.
>>
>>> Also I don't fully understand the last part of the patch: is it creating
>>> replacements for the aforementioned getters/setters?
>>
>> No, they are only helpers to create them to absorb difference how to
>> access
>> attributes between combination of Python/SWIG versions. I think this is
>> a private part how we implement proxy objects for C data structures, and
>> not for expose to be used from applications.
>
> +1.
>
> While the modern/classic distinction is potentially visible from Python
> 2 code (especially when using metaclasses with swig-py bindings), I
> consider that very much a corner case. With the upcoming EOL of Python
> 2, this difference is quickly becoming irrelevant.
Moreover, swig-py bindings for Python 2 always use classic style for it,
if we can decide not to support SWIG 4+ for Python 2. In this case,
the distinction is needed only for Python 2/3 dual support in
applications, but it is equivalent to Python 2/3 distinction in such
case.
> We should add a note
> about that to the 1.14 release notes, but otherwise, I'm all for
> committing this patch.
I think it is also need some description on somewhere about other
difference between py2/py3 bindings, mapping from/to char * type in
C structures (and somethings else if they exist), but it is not a
reason to block to commit the patch.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Nov 15, 2019 at 2:37 AM Branko Čibej <br...@apache.org> wrote:
> > On 2019/11/15 14:04, Nathan Hartman wrote:
> >> On Thu, Nov 14, 2019 at 8:18 AM Jun Omae <ju...@gmail.com> wrote:
> >>> Updated proposed patch, including a change of
> >>> subversion/bindings/swig/INSTALL.
>
> While the modern/classic distinction is potentially visible from Python
> 2 code (especially when using metaclasses with swig-py bindings), I
> consider that very much a corner case. With the upcoming EOL of Python
> 2, this difference is quickly becoming irrelevant. We should add a note
> about that to the 1.14 release notes, but otherwise, I'm all for
> committing this patch.
Committed in r1869853.
Jun, thank you once more for this patch.
Thanks to all for reviewing!
Cheers,
Nathan
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Branko Čibej <br...@apache.org>.
On 15.11.2019 08:10, Yasuhito FUTATSUKI wrote:
> On 2019/11/15 14:04, Nathan Hartman wrote:
>> On Thu, Nov 14, 2019 at 8:18 AM Jun Omae <ju...@gmail.com> wrote:
>>
>>> (Posting to dev list, again...)
>>>
>>> On Thu, Nov 14, 2019 at 1:47 AM Julian Foad <ju...@apache.org>
>>> wrote:
>>>> Perhaps someone could commit this if there are no other concerns with
>>>> it, and adjust INSTALL at the same time?
>>>
>>> Updated proposed patch, including a change of
>>> subversion/bindings/swig/INSTALL.
>>>
>>
>> Hi all,
>>
>> Jun, thank you for this patch.
>>
>> I would like to get this committed quickly if there are no
>> objections, but
>> this is not my area of expertise.
>>
>> Brane, do I understand correctly that you are satisfied with the
>> results of
>> your review/test?
>
> I also tested make check-swig-py on FreeBSD, with Python 2.7/3.7 and
> SWIG 2.0.0/2.0.12/3.0.9/3.0.10/3.0.12/4.0.1 combination. Of course,
> combination of Python 2.7 + SWIG 4.0.1 and combination of
> Python 3.7 + SWIG < 3.0.10 are blocked by .check_swig_py target :)
> Other combinations are passed the test.
>
> I considered if we can move classic style versus new style class
> conditional
> from run time to SWIG code generation time only, but it is not just
> problem in this patch only, but also in current code.
>
>> In particular, the main question I have is with regards to the -modern
>> option being added for SWIG 3.x .. <4 with Py3. If I understand
>> correctly,
>> this changes the way SWIG will generate accessors (properties vs
>> getters/setters). Would this break any downstream code? (And if so,
>> is that
>> acceptable?)
>
> There is no Python 3 application depending on it, because we have not yet
> released SWIG Python bindings that supports Python 3.
>
>> Also I don't fully understand the last part of the patch: is it creating
>> replacements for the aforementioned getters/setters?
>
> No, they are only helpers to create them to absorb difference how to
> access
> attributes between combination of Python/SWIG versions. I think this is
> a private part how we implement proxy objects for C data structures, and
> not for expose to be used from applications.
+1.
While the modern/classic distinction is potentially visible from Python
2 code (especially when using metaclasses with swig-py bindings), I
consider that very much a corner case. With the upcoming EOL of Python
2, this difference is quickly becoming irrelevant. We should add a note
about that to the 1.14 release notes, but otherwise, I'm all for
committing this patch.
-- Brane
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/11/15 14:04, Nathan Hartman wrote:
> On Thu, Nov 14, 2019 at 8:18 AM Jun Omae <ju...@gmail.com> wrote:
>
>> (Posting to dev list, again...)
>>
>> On Thu, Nov 14, 2019 at 1:47 AM Julian Foad <ju...@apache.org> wrote:
>>> Perhaps someone could commit this if there are no other concerns with
>>> it, and adjust INSTALL at the same time?
>>
>> Updated proposed patch, including a change of
>> subversion/bindings/swig/INSTALL.
>>
>
> Hi all,
>
> Jun, thank you for this patch.
>
> I would like to get this committed quickly if there are no objections, but
> this is not my area of expertise.
>
> Brane, do I understand correctly that you are satisfied with the results of
> your review/test?
I also tested make check-swig-py on FreeBSD, with Python 2.7/3.7 and
SWIG 2.0.0/2.0.12/3.0.9/3.0.10/3.0.12/4.0.1 combination. Of course,
combination of Python 2.7 + SWIG 4.0.1 and combination of
Python 3.7 + SWIG < 3.0.10 are blocked by .check_swig_py target :)
Other combinations are passed the test.
I considered if we can move classic style versus new style class conditional
from run time to SWIG code generation time only, but it is not just
problem in this patch only, but also in current code.
> In particular, the main question I have is with regards to the -modern
> option being added for SWIG 3.x .. <4 with Py3. If I understand correctly,
> this changes the way SWIG will generate accessors (properties vs
> getters/setters). Would this break any downstream code? (And if so, is that
> acceptable?)
There is no Python 3 application depending on it, because we have not yet
released SWIG Python bindings that supports Python 3.
> Also I don't fully understand the last part of the patch: is it creating
> replacements for the aforementioned getters/setters?
No, they are only helpers to create them to absorb difference how to access
attributes between combination of Python/SWIG versions. I think this is
a private part how we implement proxy objects for C data structures, and
not for expose to be used from applications.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Nov 14, 2019 at 8:18 AM Jun Omae <ju...@gmail.com> wrote:
> (Posting to dev list, again...)
>
> On Thu, Nov 14, 2019 at 1:47 AM Julian Foad <ju...@apache.org> wrote:
> > Perhaps someone could commit this if there are no other concerns with
> > it, and adjust INSTALL at the same time?
>
> Updated proposed patch, including a change of
> subversion/bindings/swig/INSTALL.
>
Hi all,
Jun, thank you for this patch.
I would like to get this committed quickly if there are no objections, but
this is not my area of expertise.
Brane, do I understand correctly that you are satisfied with the results of
your review/test?
In particular, the main question I have is with regards to the -modern
option being added for SWIG 3.x .. <4 with Py3. If I understand correctly,
this changes the way SWIG will generate accessors (properties vs
getters/setters). Would this break any downstream code? (And if so, is that
acceptable?)
Also I don't fully understand the last part of the patch: is it creating
replacements for the aforementioned getters/setters?
Thanks,
Nathan
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Jun Omae <ju...@gmail.com>.
(Posting to dev list, again...)
On Thu, Nov 14, 2019 at 1:47 AM Julian Foad <ju...@apache.org> wrote:
> Nathan wrote in another thread:
> > Note: If the patch contributed by Jun Omae a little earlier is
> > committed, the text regarding SWIG 4.0.0+ in INSTALL will need to be
> > adjusted accordingly. I have not changed it because the patch has not
> > been committed yet.
>
> Perhaps someone could commit this if there are no other concerns with
> it, and adjust INSTALL at the same time?
>
> - Julian
Updated proposed patch, including a change of subversion/bindings/swig/INSTALL.
[[[
Support building with SWIG 4 on Python 3.x
* build/ac-macros/swig.m4 (SVN_FIND_SWIG):
Allow building with SWIG 4+, and add -modern option when Python 3 and
SWIG 3.x are detected.
* subversion/bindings/swig/INSTALL
(BUILDING SWIG BINDINGS FOR SVN ON UNIX, Step 1):
Update supported SWIG versions for Python 3 bindings.
* subversion/bindings/swig/include/proxy.py
Use _get_instance_attr and _set_instance_attr.
* subversion/bindings/swig/include/proxy.swg
(_get_instance_attr): New function to get an instance attribute
without metadata for new-style and old-style classes.
(_set_instance_attr): New function to set an instance attribute for
new-style and old-style classes.
]]]
--
Jun Omae <ju...@gmail.com> (大前 潤)
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
>>> On 13.11.2019 14:22, Jun Omae wrote:
>>>> I tried to support building SWIG 4 on Python 3.x, and post patch.
>>>>
>>>> Since SWIG 4, -classic and -modern options have been dropped. As the
>>>> result,
>>>> SWIG generates new-style classes which are using property() instead of
>>>> __swig_[gs]etmethods__.
>>>>
>>>> In attached patch, SWIG would generate new-style classes without
>>>> __swig_[gs]etmethods__ when SWIG 3.x is used (-modern option would be
>>>> used),
>>>> in the same as SWIG 4.x.
>>>>
>>>> Verified by check-swig-py with:
>>>>
>>>> - Python 3.[5678] with SWIG 1.3.40, 2.0.12, 3.0.12 on Ubuntu 16.04 amd64
>>>> - Python 2.7 with SWIG 3.0.12 and 4.0.1 on Ubuntu 16.04 amd64
[...]
> Works with Python 3.7 and Swig 4.0.1 on macOS. I also tested swig-rb and
> swig-pl, just to make sure. I didn't test Python 2, but the buildbots
> will do that when the patch is committed.
Looks good.
Nathan wrote in another thread:
> Note: If the patch contributed by Jun Omae a little earlier is
> committed, the text regarding SWIG 4.0.0+ in INSTALL will need to be
> adjusted accordingly. I have not changed it because the patch has not
> been committed yet.
Perhaps someone could commit this if there are no other concerns with
it, and adjust INSTALL at the same time?
- Julian
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Branko Čibej <br...@apache.org>.
On 13.11.2019 15:05, Branko Čibej wrote:
> On 13.11.2019 14:52, Branko Čibej wrote:
>> On 13.11.2019 14:22, Jun Omae wrote:
>>> Hi,
>>>
>>> I tried to support building SWIG 4 on Python 3.x, and post patch.
>>>
>>> Since SWIG 4, -classic and -modern options have been dropped. As the
>>> result,
>>> SWIG generates new-style classes which are using property() instead of
>>> __swig_[gs]etmethods__.
>>>
>>> In attached patch, SWIG would generate new-style classes without
>>> __swig_[gs]etmethods__ when SWIG 3.x is used (-modern option would be
>>> used),
>>> in the same as SWIG 4.x.
>>>
>>> Verified by check-swig-py with:
>>>
>>> - Python 3.[5678] with SWIG 1.3.40, 2.0.12, 3.0.12 on Ubuntu 16.04 amd64
>>> - Python 2.7 with SWIG 3.0.12 and 4.0.1 on Ubuntu 16.04 amd64
>> Can you please update your patch so that it applies to the current
>> trunk? The current version causes conflicts on all three files that it
>> modifies.
>
> Oops. Actually, it's 'svn patch --strip 1' that fails, even though that
> should be the correct invocation. Without '--strip 1', it works, as does
> plain 'patch -p1'. That's not so good.
Works with Python 3.7 and Swig 4.0.1 on macOS. I also tested swig-rb and
swig-pl, just to make sure. I didn't test Python 2, but the buildbots
will do that when the patch is committed.
-- Brane
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Branko Čibej <br...@apache.org>.
On 13.11.2019 14:52, Branko Čibej wrote:
> On 13.11.2019 14:22, Jun Omae wrote:
>> Hi,
>>
>> I tried to support building SWIG 4 on Python 3.x, and post patch.
>>
>> Since SWIG 4, -classic and -modern options have been dropped. As the
>> result,
>> SWIG generates new-style classes which are using property() instead of
>> __swig_[gs]etmethods__.
>>
>> In attached patch, SWIG would generate new-style classes without
>> __swig_[gs]etmethods__ when SWIG 3.x is used (-modern option would be
>> used),
>> in the same as SWIG 4.x.
>>
>> Verified by check-swig-py with:
>>
>> - Python 3.[5678] with SWIG 1.3.40, 2.0.12, 3.0.12 on Ubuntu 16.04 amd64
>> - Python 2.7 with SWIG 3.0.12 and 4.0.1 on Ubuntu 16.04 amd64
> Can you please update your patch so that it applies to the current
> trunk? The current version causes conflicts on all three files that it
> modifies.
Oops. Actually, it's 'svn patch --strip 1' that fails, even though that
should be the correct invocation. Without '--strip 1', it works, as does
plain 'patch -p1'. That's not so good.
-- Brane
Re: [Patch] Support building with SWIG 4 on Python 3.x
Posted by Branko Čibej <br...@apache.org>.
On 13.11.2019 14:22, Jun Omae wrote:
> Hi,
>
> I tried to support building SWIG 4 on Python 3.x, and post patch.
>
> Since SWIG 4, -classic and -modern options have been dropped. As the
> result,
> SWIG generates new-style classes which are using property() instead of
> __swig_[gs]etmethods__.
>
> In attached patch, SWIG would generate new-style classes without
> __swig_[gs]etmethods__ when SWIG 3.x is used (-modern option would be
> used),
> in the same as SWIG 4.x.
>
> Verified by check-swig-py with:
>
> - Python 3.[5678] with SWIG 1.3.40, 2.0.12, 3.0.12 on Ubuntu 16.04 amd64
> - Python 2.7 with SWIG 3.0.12 and 4.0.1 on Ubuntu 16.04 amd64
Can you please update your patch so that it applies to the current
trunk? The current version causes conflicts on all three files that it
modifies.
Thanks,
-- Brane