On 2024/10/14 16:37, Yasuhito FUTATSUKI wrote:
> On 2024/10/14 12:37, Yasuhito FUTATSUKI wrote:
>> It seems it is caused by backward incompatible change of SWIG,
>> even it may be a bug.
>
> It turned out this is not a bug of SWIG, but intentional, for
> the purpose to fix another problem, incompatible change of
> SWIG_Ruby_AppendOutput, SWIG_Python_AppendOutput (and it affect
> the process how we construct result objects).
>
> Thanks,
SWIG 4.3.0-beta1 doesn't work with swig-rb and swig-py bindings, for our use
case that the svn_error_t * return value is not used as a return value on
success and is raised as an exception on error. Also, our %typemap(out) which
uses %append_output for swig-rb has issues.
I created patches and tested with SWIG 3.0.12, 4.2.1, 4.3.0-beta1.
- swig-4.3.0-swig-rb.patch.txt (swig-rb for trunk and 1.14.x)
- swig-4.3.0-swig...@trunk.patch.txt (swig-py for trunk)
- swig-4.3.0-swig...@1.14.x.patch.txt (swig-py for 1.14.x)
Could anybody please test the patches?
--
Jun Omae <jun6...@gmail.com> (大前 潤)
From 002741a54b88661268e2cc901a31835ebd696af4 Mon Sep 17 00:00:00 2001
From: Jun Omae <jun6...@gmail.com>
Date: Tue, 15 Oct 2024 03:46:54 +0900
Subject: [PATCH] Make swig-rb compatible with SWIG 4.3.0.
* subversion/bindings/swig/include/svn_containers.swg
(%typemap(out) apr_hash_t *PROPHASH,
%typemap(out) apr_hash_t *CHANGED_PATH_HASH,
%typemap(out) apr_array_header_t *PROP_LIST,
%typemap(out) apr_array_header_t *PROP_LIST_MAY_BE_NULL):
Set the return value to `$result` rather than using `%append_output` in
`%typemap(out)` for Ruby.
* subversion/bindings/swig/include/svn_types.swg
(%typemap(out) svn_error_t *):
Initialize `$result` with an empty array for the workaround to
`%append_output` incorrectly handling for nil and a list in Ruby.
(%typemap(ret) svn_error_t *):
Use first entry for `$result` when the size of the `$result` list is 1.
* subversion/bindings/swig/ruby/svn/core.rb
(Svn::Core::RangeList.diff):
Revised because multiple values are correctly retrieved from SWIG methods
now.
* subversion/bindings/swig/svn_wc.i
(%typemap(ret) svn_error_t *err):
Added because `%typemap(out) svn_error_t *err` is defined.
---
.../bindings/swig/include/svn_containers.swg | 10 ++++------
.../bindings/swig/include/svn_types.swg | 19 +++++++++++++++++--
subversion/bindings/swig/ruby/svn/core.rb | 2 +-
subversion/bindings/swig/svn_wc.i | 2 ++
4 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/subversion/bindings/swig/include/svn_containers.swg
b/subversion/bindings/swig/include/svn_containers.swg
index 47bc50a92bb8b..7dc3b0dbedea1 100644
--- a/subversion/bindings/swig/include/svn_containers.swg
+++ b/subversion/bindings/swig/include/svn_containers.swg
@@ -310,7 +310,7 @@
%typemap(out) apr_hash_t *PROPHASH
{
- %append_output(svn_swig_rb_apr_hash_to_hash_svn_string($1));
+ $result = svn_swig_rb_apr_hash_to_hash_svn_string($1);
}
#endif
@@ -326,10 +326,8 @@
#ifdef SWIGRUBY
%typemap(out) apr_hash_t *CHANGED_PATH_HASH
{
- VALUE rb_changed_path_hash;
- rb_changed_path_hash =
+ $result =
svn_swig_rb_apr_hash_to_hash_swig_type($1, "svn_log_changed_path_t *");
- %append_output(rb_changed_path_hash);
}
%apply apr_hash_t *CHANGED_PATH_HASH {
@@ -760,7 +758,7 @@
%typemap(out) apr_array_header_t *PROP_LIST
{
- %append_output(svn_swig_rb_prop_apr_array_to_hash_prop($1));
+ $result = svn_swig_rb_prop_apr_array_to_hash_prop($1);
}
%typemap(in) apr_array_header_t *PROP_LIST_MAY_BE_NULL
@@ -778,7 +776,7 @@
%typemap(out) apr_array_header_t *PROP_LIST_MAY_BE_NULL
{
- %append_output($1 ? svn_swig_rb_prop_apr_array_to_hash_prop($1) : Qnil);
+ $result = $1 ? svn_swig_rb_prop_apr_array_to_hash_prop($1) : Qnil;
}
%apply apr_array_header_t *PROP_LIST {
diff --git a/subversion/bindings/swig/include/svn_types.swg
b/subversion/bindings/swig/include/svn_types.swg
index d251e34129f57..07d4c85dec683 100644
--- a/subversion/bindings/swig/include/svn_types.swg
+++ b/subversion/bindings/swig/include/svn_types.swg
@@ -532,14 +532,29 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
#endif
#ifdef SWIGRUBY
-%typemap(out) svn_error_t *
+%typemap(out) svn_error_t * (VALUE *svn_presult = NULL)
{
if ($1) {
svn_swig_rb_destroy_pool(_global_svn_swig_rb_pool);
svn_swig_rb_pop_pool(_global_svn_swig_rb_pool);
svn_swig_rb_handle_svn_error($1);
}
- $result = Qnil;
+ $result = rb_ary_new();
+ svn_presult = &$result;
+}
+
+%typemap(ret) svn_error_t *
+{
+ if (TYPE(*svn_presult) == T_ARRAY) {
+ switch (rb_array_len(*svn_presult)) {
+ case 0:
+ *svn_presult = Qnil;
+ break;
+ case 1:
+ *svn_presult = rb_ary_entry(*svn_presult, 0);
+ break;
+ }
+ }
}
#endif
diff --git a/subversion/bindings/swig/ruby/svn/core.rb
b/subversion/bindings/swig/ruby/svn/core.rb
index 26e5e84d4fba9..ff32926066cc0 100644
--- a/subversion/bindings/swig/ruby/svn/core.rb
+++ b/subversion/bindings/swig/ruby/svn/core.rb
@@ -812,7 +812,7 @@ def initialize(*ranges)
def diff(to, consider_inheritance=false)
result = Core.rangelist_diff(self, to, consider_inheritance)
deleted = result.pop
- added = result
+ added = result.pop
[added, deleted].collect do |result|
self.class.new(*result)
end
diff --git a/subversion/bindings/swig/svn_wc.i
b/subversion/bindings/swig/svn_wc.i
index cee5f7e486727..1d7b461fa008e 100644
--- a/subversion/bindings/swig/svn_wc.i
+++ b/subversion/bindings/swig/svn_wc.i
@@ -242,6 +242,8 @@
{
$result = $1 ? svn_swig_rb_svn_error_to_rb_error($1) : Qnil;
}
+
+%typemap(ret) svn_error_t *err "";
#endif
From 6077352fcc70302bc83de0e3fc995da44afeadee Mon Sep 17 00:00:00 2001
From: Jun Omae <jun6...@gmail.com>
Date: Mon, 14 Oct 2024 18:15:50 +0900
Subject: [PATCH] Make swig-py compatible with SWIG 4.3.0.
* subversion/bindings/swig/include/svn_types.swg
(%typemap(out) svn_error_t *):
Initialize `$result` with an empty list for the workaround to
`%append_output` incorrectly handling for None and a list in Python.
(%typemap(ret) svn_error_t *):
Use first entry for `$result` when the size of the `$result` list is 1.
(%typemap(ret) svn_error_t * SVN_ERR_WITH_ATTRS):
Ditto.
* subversion/bindings/swig/python/svn/fs.py
(svn_fs_commit_txn):
Removed because multiple values are correctly retrieved from SWIG methods
now.
* subversion/bindings/swig/python/tests/client.py
(test_checkout, test_update4):
Check `TypeError` when a NULL pointer passed since SWIG 4.3.0.
* subversion/bindings/swig/python/tests/core.py
(test_svn_rangelist_diff):
Added tests for the workaround for `%append_output` incorrectly handling.
---
.../bindings/swig/include/svn_types.swg | 33 +++++++++++++++----
subversion/bindings/swig/python/svn/fs.py | 28 ----------------
.../bindings/swig/python/tests/client.py | 8 +++--
subversion/bindings/swig/python/tests/core.py | 28 ++++++++++++++++
4 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/subversion/bindings/swig/include/svn_types.swg
b/subversion/bindings/swig/include/svn_types.swg
index 45b2c0d4468b8..d251e34129f57 100644
--- a/subversion/bindings/swig/include/svn_types.swg
+++ b/subversion/bindings/swig/include/svn_types.swg
@@ -435,8 +435,8 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
svn_error_clear($1);
SWIG_fail;
}
- Py_INCREF(Py_None);
- $result = Py_None;
+ Py_XDECREF($result);
+ $result = PyList_New(0);
}
%typemap(out) svn_error_t * SVN_ERR_WITH_ATTRS (apr_status_t apr_err,
@@ -470,11 +470,31 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
SWIG_fail;
}
}
- else
- {
- Py_INCREF(Py_None);
- $result = Py_None;
+ Py_XDECREF($result);
+ $result = PyList_New(0);
+}
+
+%typemap(ret) svn_error_t * {
+ if ($result == NULL) {
+ $result = Py_None;
+ Py_INCREF($result);
+ }
+ else {
+ switch (PyList_Size($result)) {
+ case 0:
+ $result = Py_None;
+ Py_INCREF($result);
+ break;
+ case 1:
+ {
+ PyObject *tmp = $result;
+ $result = PyList_GetItem(tmp, 0);
+ Py_INCREF($result);
+ Py_DECREF(tmp);
+ }
+ break;
}
+ }
}
%typemap(ret) svn_error_t * SVN_ERR_WITH_ATTRS {
@@ -486,6 +506,7 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
Py_XDECREF($result);
SWIG_fail;
}
+ $typemap(ret, svn_error_t *);
}
#endif
diff --git a/subversion/bindings/swig/python/svn/fs.py
b/subversion/bindings/swig/python/svn/fs.py
index 8e75bff10cb28..08439c752a961 100644
--- a/subversion/bindings/swig/python/svn/fs.py
+++ b/subversion/bindings/swig/python/svn/fs.py
@@ -25,34 +25,6 @@
import errno
from libsvn.fs import *
-
-######################################################################
-# Any adjustment of SWIG generated wrapper functions on svn.fs module
-# should be placed before adding alternative names for them.
-
-# fs.commit_txn() should return a 2-tupple of conflict_p and new_rev.
-# However if conflict_p is None, SWIG generated wrapper function
-# libsvn.fs.svn_fs_commit_txn returns an integer value of new_rev.
-# This is a bug of SWIG generated wrapper function, so we fix it here.
-#
-# (There was discussion about this behavior because C API
-# svn_fs_commit_txn() always set NULL to conflict_p if it returns
-# SVN_NO_ERROR and so it seems to be reasonable that fs.commit_txn()
-# returns only rev_new value. However for compatibility, we decided
-# that fs.commit_txn always returns 2-tuple if it does not raises
-# an exception.)
-
-_svn_fs_commit_txn = svn_fs_commit_txn
-
-def svn_fs_commit_txn(*args):
- r"""svn_fs_commit_txn(svn_fs_txn_t txn, apr_pool_t pool) -> svn_error_t"""
- ret = _svn_fs_commit_txn(*args)
- if not isinstance(ret, tuple):
- ret = (None, ret)
- return ret
-
-######################################################################
-
from svn.core import _unprefix_names, Pool, _as_list
_unprefix_names(locals(), 'svn_fs_')
_unprefix_names(locals(), 'SVN_FS_')
diff --git a/subversion/bindings/swig/python/tests/client.py
b/subversion/bindings/swig/python/tests/client.py
index 258d9d3bf5982..ced58ce540b54 100644
--- a/subversion/bindings/swig/python/tests/client.py
+++ b/subversion/bindings/swig/python/tests/client.py
@@ -172,7 +172,9 @@ def test_checkout(self):
path = self.temper.alloc_empty_dir('-checkout')
- self.assertRaises(ValueError, client.checkout2,
+ # TypeError is raised since SWIG 4.3.0
+ self.assertRaises((ValueError, TypeError), r'Received a NULL pointer',
+ client.checkout2,
self.repos_uri, path, None, None, True, True,
self.client_ctx)
@@ -579,7 +581,9 @@ def test_update4(self):
path = self.temper.alloc_empty_dir('-update')
- self.assertRaises(ValueError, client.checkout2,
+ # TypeError is raised since SWIG 4.3.0
+ self.assertRaises((ValueError, TypeError), r'Received a NULL pointer',
+ client.checkout2,
self.repos_uri, path, None, None, True, True,
self.client_ctx)
diff --git a/subversion/bindings/swig/python/tests/core.py
b/subversion/bindings/swig/python/tests/core.py
index aa3bdc9177981..91e7ea9005a30 100644
--- a/subversion/bindings/swig/python/tests/core.py
+++ b/subversion/bindings/swig/python/tests/core.py
@@ -348,6 +348,34 @@ def test_stream_from_stringbuf_unicode(self):
finally:
svn.core.svn_stream_close(stream)
+ def test_svn_rangelist_diff(self):
+ """
+ SWIG incorrectly handles return values when the first %append_output() is
+ invoked with a list instance. svn.core.svn_rangelist_diff() is in the case.
+ We test whether the workaround for it is working.
+ """
+
+ def from_args(start, end, inheritable):
+ instance = svn.core.svn_merge_range_t()
+ instance.start = start
+ instance.end = end
+ instance.inheritable = inheritable
+ return instance
+
+ def to_args(instance):
+ return [instance.start, instance.end, instance.inheritable]
+
+ def map_list(f, iterator):
+ return list(map(f, iterator))
+
+ from_ = [from_args(4, 5, True), from_args(9, 13, True)]
+ to = [from_args(7, 11, True)]
+ rv = svn.core.svn_rangelist_diff(from_, to, True)
+ self.assertIsInstance(rv, (list, tuple))
+ deleted, added = rv
+ self.assertEqual([[7, 9, True]], map_list(to_args, added))
+ self.assertEqual([[4, 5, True], [11, 13, True]],map_list(to_args, deleted))
+
def suite():
return unittest.defaultTestLoader.loadTestsFromTestCase(
From e17b4799d40c0494f7535ba7fad19df944997c2f Mon Sep 17 00:00:00 2001
From: Jun Omae <jun6...@gmail.com>
Date: Mon, 14 Oct 2024 18:15:50 +0900
Subject: [PATCH] Make swig-py compatible with SWIG 4.3.0.
* subversion/bindings/swig/include/svn_types.swg
(%typemap(out) svn_error_t *):
Initialize `$result` with an empty list for the workaround to
`%append_output` incorrectly handling for None and a list in Python.
(%typemap(ret) svn_error_t *):
Use first entry for `$result` when the size of the `$result` list is 1.
(%typemap(ret) svn_error_t * SVN_ERR_WITH_ATTRS):
Ditto.
* subversion/bindings/swig/python/tests/client.py
(test_checkout, test_update4):
Check `TypeError` when a NULL pointer passed since SWIG 4.3.0.
* subversion/bindings/swig/python/tests/core.py
(test_svn_rangelist_diff):
Added tests for the workaround for `%append_output` incorrectly handling.
---
.../bindings/swig/include/svn_types.swg | 27 ++++++++++++++++--
.../bindings/swig/python/tests/client.py | 8 ++++--
subversion/bindings/swig/python/tests/core.py | 28 +++++++++++++++++++
3 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/subversion/bindings/swig/include/svn_types.swg
b/subversion/bindings/swig/include/svn_types.swg
index fa024a2335e41..4d23b07ff88de 100644
--- a/subversion/bindings/swig/include/svn_types.swg
+++ b/subversion/bindings/swig/include/svn_types.swg
@@ -435,8 +435,31 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
svn_error_clear($1);
SWIG_fail;
}
- Py_INCREF(Py_None);
- $result = Py_None;
+ Py_XDECREF($result);
+ $result = PyList_New(0);
+}
+
+%typemap(ret) svn_error_t * {
+ if ($result == NULL) {
+ $result = Py_None;
+ Py_INCREF($result);
+ }
+ else {
+ switch (PyList_Size($result)) {
+ case 0:
+ $result = Py_None;
+ Py_INCREF($result);
+ break;
+ case 1:
+ {
+ PyObject *tmp = $result;
+ $result = PyList_GetItem(tmp, 0);
+ Py_INCREF($result);
+ Py_DECREF(tmp);
+ }
+ break;
+ }
+ }
}
#endif
diff --git a/subversion/bindings/swig/python/tests/client.py
b/subversion/bindings/swig/python/tests/client.py
index 3d91d868427db..3f32c3d31cbd0 100644
--- a/subversion/bindings/swig/python/tests/client.py
+++ b/subversion/bindings/swig/python/tests/client.py
@@ -172,7 +172,9 @@ def test_checkout(self):
path = self.temper.alloc_empty_dir('-checkout')
- self.assertRaises(ValueError, client.checkout2,
+ # TypeError is raised since SWIG 4.3.0
+ self.assertRaises((ValueError, TypeError), r'Received a NULL pointer',
+ client.checkout2,
self.repos_uri, path, None, None, True, True,
self.client_ctx)
@@ -526,7 +528,9 @@ def test_update4(self):
path = self.temper.alloc_empty_dir('-update')
- self.assertRaises(ValueError, client.checkout2,
+ # TypeError is raised since SWIG 4.3.0
+ self.assertRaises((ValueError, TypeError), r'Received a NULL pointer',
+ client.checkout2,
self.repos_uri, path, None, None, True, True,
self.client_ctx)
diff --git a/subversion/bindings/swig/python/tests/core.py
b/subversion/bindings/swig/python/tests/core.py
index e48d493d1a265..26e2963df199f 100644
--- a/subversion/bindings/swig/python/tests/core.py
+++ b/subversion/bindings/swig/python/tests/core.py
@@ -333,6 +333,34 @@ def test_stream_readline(self):
[b'', 1])
svn.core.svn_stream_close(stream)
+ def test_svn_rangelist_diff(self):
+ """
+ SWIG incorrectly handles return values when the first %append_output() is
+ invoked with a list instance. svn.core.svn_rangelist_diff() is in the case.
+ We test whether the workaround for it is working.
+ """
+
+ def from_args(start, end, inheritable):
+ instance = svn.core.svn_merge_range_t()
+ instance.start = start
+ instance.end = end
+ instance.inheritable = inheritable
+ return instance
+
+ def to_args(instance):
+ return [instance.start, instance.end, instance.inheritable]
+
+ def map_list(f, iterator):
+ return list(map(f, iterator))
+
+ from_ = [from_args(4, 5, True), from_args(9, 13, True)]
+ to = [from_args(7, 11, True)]
+ rv = svn.core.svn_rangelist_diff(from_, to, True)
+ self.assertIsInstance(rv, (list, tuple))
+ deleted, added = rv
+ self.assertEqual([[7, 9, True]], map_list(to_args, added))
+ self.assertEqual([[4, 5, True], [11, 13, True]],map_list(to_args, deleted))
+
def suite():
return unittest.defaultTestLoader.loadTestsFromTestCase(
