You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@red-bean.com> on 2020/07/28 14:34:36 UTC
Python bindings API confusion
Hey, all.
I'm writing some code that performs commits via the Subversion Python
bindings, and I'm struggling to understand some things I see there.
In the svn_fs.i interface file, there's this block of code:
/* -----------------------------------------------------------------------
Fix the return value for svn_fs_commit_txn(). If the conflict result is
NULL, then %append_output() is passed Py_None, but that goofs up
because that is *also* the marker for "I haven't started assembling a
multi-valued return yet" which means the second return value (new_rev)
will not cause a 2-tuple to be manufactured.
The answer is to explicitly create a 2-tuple return value.
FIXME: Do the Perl and Ruby bindings need to do something similar?
*/
#ifdef SWIGPYTHON
%typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
/* this is always Py_None */
Py_DECREF($result);
/* build the result tuple */
$result = Py_BuildValue("zi", *$1, (long)*$2);
}
#endif
This reads and claims to behave exactly as I'd expect, given the dual
return values of svn_fs_commit_txn (and svn_repos_fs_commit_txn which wraps
it). And since this interface file is included from svn_repos.i, I would
expect those typemaps to apply also to svn_repos_fs_commit_txn, which has
matching parameter types and names.
But this isn't how the code appears to work in practice. A successful
commit gets back from svn.repos.fs_commit_txn not a 2-tuple, but just the
newly created revision number. Moreover, if my commit succeeds but the
post-commit hook fails, svn.repos.fs_commit_txn() raises an Exception,
masking the return of the newly created revision number altogether. Now,
the masked revision number thing I can understand -- it's hard to do in
Python what we promise to do in C (which is to return an svn_error_t but
still set the new_rev return value). But the 2-tuple thing I have no
explanation for. Any ideas?
-- Mike
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
I'm sorry, but I'd like to make a correction.
On 2020/12/26 20:02, Yasuhito FUTATSUKI wrote:
> [[[
> Index: subversion/bindings/swig/python/svn/fs.py
> ===================================================================
> --- subversion/bindings/swig/python/svn/fs.py (revision 1884802)
> +++ subversion/bindings/swig/python/svn/fs.py (working copy)
> @@ -24,6 +24,14 @@
> ######################################################################
>
> from libsvn.fs import *
> +
> +# For API compatibility we should replace wrapper function entity before
> +# adding alternative names.
> +_svn_fs_commit_txn = svn_fs_commit_txn
> +def svn_fs_commit_txn(*args) -> "char const **, svn_revnum_t *":
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
To keep Python 2 compatibility, it is better to remove annotation.
This function definition was originally taken from SWIG generated module
file for Python 3, subversion/bindings/swig/python/fs.py, and then
I modified to adjust return value. It was a mistake.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/31 8:59, Yasuhito FUTATSUKI wrote:
<snip>
> During writing the test above I also found that on swig-py for Python 3
> wrapper functions using svn_string_t * or svn_stringbuf_t * for in
> input arguments don't accept str objects for their values. This was
> my overlook, so I'll fix it. However, this is another topic.
Fix it in 1885370.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/31 8:59, Yasuhito FUTATSUKI wrote:
> On 2020/12/28 19:10, Daniel Shahaf wrote:
>> Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
>>> On 2020/12/26 3:38, Nathan Hartman wrote:
>>>
>>>> But the solution you suggest above, adding a new_rev attribute to the
>>>> exception and leaving the return value as it was, sounds like a good way to
>>>> avoid the incompatibility and also avoid the API bump.
>>>
>>> I had written such a patch on typemaps at first because it was straight
>>> forward. Although I removed it after r1880967 is commited, I can write
>>> such a code again and I'll do if it is needed.
>>>
>>> Alternately, here is an easier way to do it. The patch below is a code
>>> that replace wrapper function entity on top of the svn.fs module
>>> (not tested, and more appropriate comment is needed in the code).
>
> <snip>
>
>>> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
>>> in above patch, then it will satisfy the Daniel's proposal.
>>
>> I agree with Nathan that it'd be confusing for a Python
>> svn_fs_commit_txn2() to exist when a C function of the same name does
>> not, even if the C API skips a revv number in consideration of swig-py.
>> So, if we'd like to introduce a simplified API, I'd vote to name it
>> something else.
>
> Then I'll commit patch amended only for restore Python 2 support,
> without renaming to svn_fs_commit_txn2, if Mike agrees because he
> already wrote some code using svn.fs.commit_txn().
> (attached swig-py-fs_commit_txn-revert-return-value-type-patch.txt)
I got a reply from Mike off the list with non negative answer. So I think
there is no objection to restore the behavior on return value type of
svn.fs.commit_txn(), yet, at least.
However as a result of reconsideration of how it should be done, now
I don't think this patch wasn't sufficieant. If C API svn_fs_commit_txn()
returns SVN_NO_ERROR but conflict_p is not NULL by accident,
svn.fs.commit_txn() with this patch returns (None, (conflict, new_revision))
and this is obiously incorrect. So I added the check for it.
Also, I rerote the comment, then commit it in 1885007.
> I also wrote tests for svn.fs.commit_txn, both for return value and
> attributes in SubversionException, by porting tests for C API into Python.
> (attached patch swig-py-fs_commit_txn-test-patch.txt; tested on Python
> 2.7.18 and 3.9.0 on FreeBSD 12)
I added to more assertion to check if attributes exist in exception,
and commit it in 1885008.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/28 19:10, Daniel Shahaf wrote:
> Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
>> On 2020/12/26 3:38, Nathan Hartman wrote:
>>
>>> But the solution you suggest above, adding a new_rev attribute to the
>>> exception and leaving the return value as it was, sounds like a good way to
>>> avoid the incompatibility and also avoid the API bump.
>>
>> I had written such a patch on typemaps at first because it was straight
>> forward. Although I removed it after r1880967 is commited, I can write
>> such a code again and I'll do if it is needed.
>>
>> Alternately, here is an easier way to do it. The patch below is a code
>> that replace wrapper function entity on top of the svn.fs module
>> (not tested, and more appropriate comment is needed in the code).
<snip>
>> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
>> in above patch, then it will satisfy the Daniel's proposal.
>
> I agree with Nathan that it'd be confusing for a Python
> svn_fs_commit_txn2() to exist when a C function of the same name does
> not, even if the C API skips a revv number in consideration of swig-py.
> So, if we'd like to introduce a simplified API, I'd vote to name it
> something else.
Then I'll commit patch amended only for restore Python 2 support,
without renaming to svn_fs_commit_txn2, if Mike agrees because he
already wrote some code using svn.fs.commit_txn().
(attached swig-py-fs_commit_txn-revert-return-value-type-patch.txt)
I also wrote tests for svn.fs.commit_txn, both for return value and
attributes in SubversionException, by porting tests for C API into Python.
(attached patch swig-py-fs_commit_txn-test-patch.txt; tested on Python
2.7.18 and 3.9.0 on FreeBSD 12)
During writing the test above I also found that on swig-py for Python 3
wrapper functions using svn_string_t * or svn_stringbuf_t * for in
input arguments don't accept str objects for their values. This was
my overlook, so I'll fix it. However, this is another topic.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yasuhito FUTATSUKI wrote on Sat, 26 Dec 2020 11:02 +00:00:
> On 2020/12/26 3:38, Nathan Hartman wrote:
>
> > But the solution you suggest above, adding a new_rev attribute to the
> > exception and leaving the return value as it was, sounds like a good way to
> > avoid the incompatibility and also avoid the API bump.
>
> I had written such a patch on typemaps at first because it was straight
> forward. Although I removed it after r1880967 is commited, I can write
> such a code again and I'll do if it is needed.
>
> Alternately, here is an easier way to do it. The patch below is a code
> that replace wrapper function entity on top of the svn.fs module
> (not tested, and more appropriate comment is needed in the code).
>
> [[[
> Index: subversion/bindings/swig/python/svn/fs.py
> ===================================================================
> --- subversion/bindings/swig/python/svn/fs.py (revision 1884802)
> +++ subversion/bindings/swig/python/svn/fs.py (working copy)
> @@ -24,6 +24,14 @@
> ######################################################################
>
> from libsvn.fs import *
> +
> +# For API compatibility we should replace wrapper function entity before
> +# adding alternative names.
> +_svn_fs_commit_txn = svn_fs_commit_txn
> +def svn_fs_commit_txn(*args) -> "char const **, svn_revnum_t *":
> + r"""svn_fs_commit_txn(svn_fs_txn_t txn, apr_pool_t pool) -> svn_error_t"""
> + return None, _svn_fs_commit_txn(*args)
> +
> from svn.core import _unprefix_names, Pool, _as_list
> _unprefix_names(locals(), 'svn_fs_')
> _unprefix_names(locals(), 'SVN_FS_')
>
> ]]]
>
> Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
> in above patch, then it will satisfy the Daniel's proposal.
I agree with Nathan that it'd be confusing for a Python
svn_fs_commit_txn2() to exist when a C function of the same name does
not, even if the C API skips a revv number in consideration of swig-py.
So, if we'd like to introduce a simplified API, I'd vote to name it
something else.
Cheers,
Daniel
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/26 3:38, Nathan Hartman wrote:
> But the solution you suggest above, adding a new_rev attribute to the
> exception and leaving the return value as it was, sounds like a good way to
> avoid the incompatibility and also avoid the API bump.
I had written such a patch on typemaps at first because it was straight
forward. Although I removed it after r1880967 is commited, I can write
such a code again and I'll do if it is needed.
Alternately, here is an easier way to do it. The patch below is a code
that replace wrapper function entity on top of the svn.fs module
(not tested, and more appropriate comment is needed in the code).
[[[
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 1884802)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -24,6 +24,14 @@
######################################################################
from libsvn.fs import *
+
+# For API compatibility we should replace wrapper function entity before
+# adding alternative names.
+_svn_fs_commit_txn = svn_fs_commit_txn
+def svn_fs_commit_txn(*args) -> "char const **, svn_revnum_t *":
+ r"""svn_fs_commit_txn(svn_fs_txn_t txn, apr_pool_t pool) -> svn_error_t"""
+ return None, _svn_fs_commit_txn(*args)
+
from svn.core import _unprefix_names, Pool, _as_list
_unprefix_names(locals(), 'svn_fs_')
_unprefix_names(locals(), 'SVN_FS_')
]]]
Of course, we can use svn_fs_commit_txn2 instead of _svn_fs_commit_txn
in above patch, then it will satisfy the Daniel's proposal.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Dec 25, 2020 at 12:00 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
>
> I may be more than a bit late for this, but can we avoid the
> incompatibility in the first place? I.e., avoid requiring API users to
> check svn.core.SVN_VER_MINOR before interpreting an API's return value?
> Adding a new_rev attribute to the exception is a welcome change — it
> addresses Nathan's point (1) — but I question whether changing «return
> (None, foo)» to «return foo» (plus or minus a singleton tuple around it)
> justifies breaking compatibility. This change sounds like it should be
> part of a proper API bump, namely, provide a svn.fs.commit_txn2(),
> keeping svn.fs.commit_txn() as-is.
>
> (Implementation-wise, it may be easier to implement svn.fs.commit_txn2()
> as a wrapper around .commit_txn() than the other way around. When we
> next revv the C API, we can skip a revv number in consideration of the
> bindings, and call the revved API svn_fs_commit_txn3(), deprecating
> svn_fs_commit_txn().)
I thought about suggesting an API bump for the Python wrapper but I didn't
suggest it because I thought it might be confusing to have
svn.fs.commit_txn2() be a wrapper for svn_fs_commit_txn(), especially if
there's ever a svn_fs_commit_txn2().
But the solution you suggest above, adding a new_rev attribute to the
exception and leaving the return value as it was, sounds like a good way to
avoid the incompatibility and also avoid the API bump.
Cheers,
Nathan
Re: Python bindings API confusion
Posted by Branko Čibej <br...@apache.org>.
On 25.12.2020 18:00, Daniel Shahaf wrote:
>> In addition, we introduce a framework to add attributes to
>> 'SubversionException' to return additional values to the caller in
>> situations such as above. (This new feature is mentioned for
>> completeness but is not part of the errata.)
> "the erratum"
Using the plural is correct in this case, as there is more than one
erratum being discussed.
Re: Python bindings API confusion
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Wed, Dec 23, 2020 at 00:09:46 -0500:
> On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
> >
> > On 2020/08/29 2:49, C. Michael Pilato wrote:
> > >>> Committed revision 1880967.
> > >>>
> > >>> I made only minor comment/formatting changes.
> > >>
> > >> Shouldn't the change be documented somewhere for users of the
> > >> bindings? (E.g., release notes, API errata, API documentation…)
> > >>
> > >
> > > Yes, I think that makes sense. The release notes for sure. As for API
> > > docs/errata ... do we even have such a thing for the bindings?
> >
> > I'm very sorry for my late reply.
> >
> > This change affects all programs using return value of svn.fs.commit_txn.
> > So I think api-errata is needed.
> >
> > As you all and I know my English is too bad, however I wrote it as a
> > starting point. Please refine or rewrite if the errata is needed.
>
> Thank you for remembering to do this.
>
> I rewrote it differently, because I was not familiar with the context
> of r1880967, so I think this was helpful as an exercise for me to
> learn more about how the C APIs and Python bindings interact.
>
> Please review and tell me what you think... (Maybe some of the facts
> are wrong!)
>
> [[[
> svn.fs.commit_txn is a Python wrapper for the C API svn_fs_commit_txn.
>
> Like other C APIs, svn_fs_commit_txn uses its return value to indicate
> success or error. In addition, it returns two more values by pointer
> arguments: 'conflict' and 'new_rev'. These are mutually exclusive:
> either the commit succeeds, indicated by a valid revision number in
> 'new_rev' or the commit fails, with details in 'conflict'. The API's
> return value, taken together with these two additional pieces of
> information, tell the full story of the function's success or failure.
>
> In particular, if a commit succeeds but an error occurs in post-commit
> processing, the API will return an error but still indicate the
> successful commit by returning a valid revision number in 'new_rev'.
>
> When the C API returns successfully, the Python wrapper returns the
> two values 'conflict' and 'new_rev' in a 2-tuple. If the C API returns
> an error, the Python wrapper raises an exception.
>
> Unfortunately, this suffers the following problems:
>
> (1) An exception prevents the wrapper from returning the 2-tuple, so
> the caller cannot know whether a successful commit occurred or not.
>
> (2) When there is no exception, 'conflict' is always None, defeating
> the purpose of trying to return it in a 2-tuple.
>
> (3) This is inconsistent with svn.repos.commit_txn, which only returns
> a 'new_rev' singleton on success.
>
> For the 1.15 release, svn.fs.commit_txn is made consistent with
> svn.repos.commit_txn in returning a 'new_rev' singleton on success.
"returning a singleton" sounds like it returns a one-element tuple, as in
«return tuple([foo])», as opposed to just «return foo».
> In addition, we introduce a framework to add attributes to
> 'SubversionException' to return additional values to the caller in
> situations such as above. (This new feature is mentioned for
> completeness but is not part of the errata.)
"the erratum"
> API users who relied on the previous 2-tuple return value should
> adjust their code to process the 'new_rev' singleton on successful
> return.
I may be more than a bit late for this, but can we avoid the
incompatibility in the first place? I.e., avoid requiring API users to
check svn.core.SVN_VER_MINOR before interpreting an API's return value?
Adding a new_rev attribute to the exception is a welcome change — it
addresses Nathan's point (1) — but I question whether changing «return
(None, foo)» to «return foo» (plus or minus a singleton tuple around it)
justifies breaking compatibility. This change sounds like it should be
part of a proper API bump, namely, provide a svn.fs.commit_txn2(),
keeping svn.fs.commit_txn() as-is.
(Implementation-wise, it may be easier to implement svn.fs.commit_txn2()
as a wrapper around .commit_txn() than the other way around. When we
next revv the C API, we can skip a revv number in consideration of the
bindings, and call the revved API svn_fs_commit_txn3(), deprecating
svn_fs_commit_txn().)
Unrelated: could the log message of r1880967 be extended to make it
clear (in the top summary) that the revision comprises not only internal
infrastructure changes but also user-visible changes?
Thanks,
Daniel
> ]]]
>
> Thoughts?
Re: Python bindings API confusion (+ svnbook swig-py examples not
py3 compatible)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, 28 Aug 2020 13:49 -0400:
> On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
> > C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:
> > > Sending subversion/bindings/swig/include/svn_types.swg
> > > Sending
> > subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> > > Sending
> > subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
> > > Sending subversion/bindings/swig/svn_fs.i
> > > Sending subversion/bindings/swig/svn_repos.i
> > > Transmitting file data .....done
> > > Committing transaction...
> > > Committed revision 1880967.
> > >
> > > I made only minor comment/formatting changes.
> >
> > Shouldn't the change be documented somewhere for users of the
> > bindings? (E.g., release notes, API errata, API documentation…)
> >
>
> Yes, I think that makes sense. The release notes for sure. As for API
> docs/errata ... do we even have such a thing for the bindings?
We have errata in notes/api-errata/, and the release notes could point
to them.
Documentation… I think there's just the README files in the source,
release notes of past minor lines, and the example code in
#svn.developer.usingapi.codesamples in the book?
Cheers,
Daniel
P.S. The example code in that section of the book won't work in
Python 3 (due to using py2-only "print" and "except" syntaxes). We
should probably add some text explaining the example code is py2 but
the bindings support py3 as well (as explained in the 1.l4 release
notes), or better yet, port the examples to py3.
Re: Python bindings API confusion
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Dec 23, 2020 at 1:02 PM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
> The last paragraph is for "== Impact on API Users ==" section, I
> think. (Yes, my draft also missed this mark of the section.)
>
> I think the error in this erratum is (3) only, and the fix might
> be a change type of return value of svn.repos.commit_txn. This was
> caused by r845217, which fixed the return type of svn.fs.commit_txn
> to 2-tuple and wasn't applied to svn.repos.commit_txn. So the
> explanation of these API is only for justification of the fix.
> Also as the target of this api-errata are users of those API,
> I think that so detailed explanation of APIs itself does not need.
>
> However, as a document for these Python wrapper APIs, this
> explanation is very useful.
Thanks.
Feel free to cut out the parts that are excessive for the errata.
Maybe one of us will add the longer explanation to the API docs later.
Cheers,
Nathan
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/12/23 14:09, Nathan Hartman wrote:
> On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
>>
>> On 2020/08/29 2:49, C. Michael Pilato wrote:
>>>>> Committed revision 1880967.
>>>>>
>>>>> I made only minor comment/formatting changes.
>>>>
>>>> Shouldn't the change be documented somewhere for users of the
>>>> bindings? (E.g., release notes, API errata, API documentation…)
>>>>
>>>
>>> Yes, I think that makes sense. The release notes for sure. As for API
>>> docs/errata ... do we even have such a thing for the bindings?
>>
>> I'm very sorry for my late reply.
>>
>> This change affects all programs using return value of svn.fs.commit_txn.
>> So I think api-errata is needed.
>>
>> As you all and I know my English is too bad, however I wrote it as a
>> starting point. Please refine or rewrite if the errata is needed.
>
> Thank you for remembering to do this.
>
> I rewrote it differently, because I was not familiar with the context
> of r1880967, so I think this was helpful as an exercise for me to
> learn more about how the C APIs and Python bindings interact.
>
> Please review and tell me what you think... (Maybe some of the facts
> are wrong!)
>
> [[[
>
> svn.fs.commit_txn is a Python wrapper for the C API svn_fs_commit_txn.
>
> Like other C APIs, svn_fs_commit_txn uses its return value to indicate
> success or error. In addition, it returns two more values by pointer
> arguments: 'conflict' and 'new_rev'. These are mutually exclusive:
> either the commit succeeds, indicated by a valid revision number in
> 'new_rev' or the commit fails, with details in 'conflict'. The API's
> return value, taken together with these two additional pieces of
> information, tell the full story of the function's success or failure.
>
> In particular, if a commit succeeds but an error occurs in post-commit
> processing, the API will return an error but still indicate the
> successful commit by returning a valid revision number in 'new_rev'.
>
> When the C API returns successfully, the Python wrapper returns the
> two values 'conflict' and 'new_rev' in a 2-tuple. If the C API returns
> an error, the Python wrapper raises an exception.
>
> Unfortunately, this suffers the following problems:
>
> (1) An exception prevents the wrapper from returning the 2-tuple, so
> the caller cannot know whether a successful commit occurred or not.
>
> (2) When there is no exception, 'conflict' is always None, defeating
> the purpose of trying to return it in a 2-tuple.
>
> (3) This is inconsistent with svn.repos.commit_txn, which only returns
> a 'new_rev' singleton on success.
>
> For the 1.15 release, svn.fs.commit_txn is made consistent with
> svn.repos.commit_txn in returning a 'new_rev' singleton on success.
>
> In addition, we introduce a framework to add attributes to
> 'SubversionException' to return additional values to the caller in
> situations such as above. (This new feature is mentioned for
> completeness but is not part of the errata.)
>
> API users who relied on the previous 2-tuple return value should
> adjust their code to process the 'new_rev' singleton on successful
> return.
>
> ]]]
>
> Thoughts?
The last paragraph is for "== Impact on API Users ==" section, I
think. (Yes, my draft also missed this mark of the section.)
I think the error in this erratum is (3) only, and the fix might
be a change type of return value of svn.repos.commit_txn. This was
caused by r845217, which fixed the return type of svn.fs.commit_txn
to 2-tuple and wasn't applied to svn.repos.commit_txn. So the
explanation of these API is only for justification of the fix.
Also as the target of this api-errata are users of those API,
I think that so detailed explanation of APIs itself does not need.
However, as a document for these Python wrapper APIs, this
explanation is very useful.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Dec 22, 2020 at 10:13 AM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> On 2020/08/29 2:49, C. Michael Pilato wrote:
> >>> Committed revision 1880967.
> >>>
> >>> I made only minor comment/formatting changes.
> >>
> >> Shouldn't the change be documented somewhere for users of the
> >> bindings? (E.g., release notes, API errata, API documentation…)
> >>
> >
> > Yes, I think that makes sense. The release notes for sure. As for API
> > docs/errata ... do we even have such a thing for the bindings?
>
> I'm very sorry for my late reply.
>
> This change affects all programs using return value of svn.fs.commit_txn.
> So I think api-errata is needed.
>
> As you all and I know my English is too bad, however I wrote it as a
> starting point. Please refine or rewrite if the errata is needed.
Thank you for remembering to do this.
I rewrote it differently, because I was not familiar with the context
of r1880967, so I think this was helpful as an exercise for me to
learn more about how the C APIs and Python bindings interact.
Please review and tell me what you think... (Maybe some of the facts
are wrong!)
[[[
svn.fs.commit_txn is a Python wrapper for the C API svn_fs_commit_txn.
Like other C APIs, svn_fs_commit_txn uses its return value to indicate
success or error. In addition, it returns two more values by pointer
arguments: 'conflict' and 'new_rev'. These are mutually exclusive:
either the commit succeeds, indicated by a valid revision number in
'new_rev' or the commit fails, with details in 'conflict'. The API's
return value, taken together with these two additional pieces of
information, tell the full story of the function's success or failure.
In particular, if a commit succeeds but an error occurs in post-commit
processing, the API will return an error but still indicate the
successful commit by returning a valid revision number in 'new_rev'.
When the C API returns successfully, the Python wrapper returns the
two values 'conflict' and 'new_rev' in a 2-tuple. If the C API returns
an error, the Python wrapper raises an exception.
Unfortunately, this suffers the following problems:
(1) An exception prevents the wrapper from returning the 2-tuple, so
the caller cannot know whether a successful commit occurred or not.
(2) When there is no exception, 'conflict' is always None, defeating
the purpose of trying to return it in a 2-tuple.
(3) This is inconsistent with svn.repos.commit_txn, which only returns
a 'new_rev' singleton on success.
For the 1.15 release, svn.fs.commit_txn is made consistent with
svn.repos.commit_txn in returning a 'new_rev' singleton on success.
In addition, we introduce a framework to add attributes to
'SubversionException' to return additional values to the caller in
situations such as above. (This new feature is mentioned for
completeness but is not part of the errata.)
API users who relied on the previous 2-tuple return value should
adjust their code to process the 'new_rev' singleton on successful
return.
]]]
Thoughts?
Nathan
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/08/29 2:49, C. Michael Pilato wrote:
> On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
>> C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:
>>> Sending subversion/bindings/swig/include/svn_types.swg
>>> Sending
>> subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
>>> Sending
>> subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
>>> Sending subversion/bindings/swig/svn_fs.i
>>> Sending subversion/bindings/swig/svn_repos.i
>>> Transmitting file data .....done
>>> Committing transaction...
>>> Committed revision 1880967.
>>>
>>> I made only minor comment/formatting changes.
>>
>> Shouldn't the change be documented somewhere for users of the
>> bindings? (E.g., release notes, API errata, API documentation…)
>>
>
> Yes, I think that makes sense. The release notes for sure. As for API
> docs/errata ... do we even have such a thing for the bindings?
I'm very sorry for my late reply.
This change affects all programs using return value of svn.fs.commit_txn.
So I think api-errata is needed.
As you all and I know my English is too bad, however I wrote it as a
starting point. Please refine or rewrite if the errata is needed.
[[[
API ERRATA -- $Id$
Root Cause of Errata: design error of wrapper function
Library(s) Affected: (SWIG Python bindings)svn.fs
Function(s) Affected: (SWIG Python bindings)svn.fs.commit_txn
New Behavior in: 1.15
Related Issues: n/a
== Details ==
The C API svn_fs_commit_txn has two pointer arguments to return values,
'conflict' and 'new_rev'. However 'conflict' is always NULL unless
svn_fs_commit_txn cause error, and as Python Wrapper function raises
error if C API returns error state, the return value of Python Wrapper
function svn.fs.commit_txn returned 2-tupple of None and 'new_rev' value
since 1.14.
This behavior is quite useless and inconsistent with the behavior
of svn.repos.commit_txn, the wrapper of svn_repos_commit_txn, which
has also two pointer arguments 'conflict' and 'new_rev' but only
returns 'new_rev' singleton.
For the 1.15 release, svn.fs.commit_txn returns 'new_rev' singleton
if it doesn't raise exception. In addtion, we introduce framework
to add attributes to 'SubversionException' to return values to caller
if argments for output in C API are useful when the API returns an error
(however this new feature actually is not a part of the errata).
If an API user expects that it returns a 2-tuple will get some exceptions
when try to retrieve its element.
All API users should review their treatments of return value of the API.
]]]
I also worry but couldn't wrote description of this change for API docs.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
On Fri, Aug 21, 2020 at 7:10 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:
> > Sending subversion/bindings/swig/include/svn_types.swg
> > Sending
> subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> > Sending
> subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
> > Sending subversion/bindings/swig/svn_fs.i
> > Sending subversion/bindings/swig/svn_repos.i
> > Transmitting file data .....done
> > Committing transaction...
> > Committed revision 1880967.
> >
> > I made only minor comment/formatting changes.
>
> Shouldn't the change be documented somewhere for users of the
> bindings? (E.g., release notes, API errata, API documentation…)
>
Yes, I think that makes sense. The release notes for sure. As for API
docs/errata ... do we even have such a thing for the bindings?
-- Mike
Re: Python bindings API confusion
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Tue, 18 Aug 2020 10:23 -0400:
> Sending subversion/bindings/swig/include/svn_types.swg
> Sending subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> Sending subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
> Sending subversion/bindings/swig/svn_fs.i
> Sending subversion/bindings/swig/svn_repos.i
> Transmitting file data .....done
> Committing transaction...
> Committed revision 1880967.
>
> I made only minor comment/formatting changes.
Shouldn't the change be documented somewhere for users of the
bindings? (E.g., release notes, API errata, API documentation…)
Cheers,
Daniel
Re: Python bindings API confusion
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
Sending subversion/bindings/swig/include/svn_types.swg
Sending subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
Sending subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.h
Sending subversion/bindings/swig/svn_fs.i
Sending subversion/bindings/swig/svn_repos.i
Transmitting file data .....done
Committing transaction...
Committed revision 1880967.
I made only minor comment/formatting changes.
On Sun, Aug 2, 2020 at 9:36 AM Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
wrote:
> On 2020/07/31 10:19, Yasuhito FUTATSUKI wrote:
> > On 2020/07/30 22:20, C. Michael Pilato wrote:
> >> The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
> >> there are two issues I have with it.
> >>
> >> First, unless I'm mistaken, there's no way to get the conflict_p back as
> >> part of the 2-tuple return value -- if there's a conflict,
> >> svn.repos.fs_commit_txn() is going to raise an Exception. So we're
> >> basically returning a 2-tuple whose first value is always None. We
> might
> >> as well preserve the current behavior and return the newly committed
> >> revision as a singleton. (Which also means we don't break compatibility
> >> with prior versions.)
> >
> > How do you think about this issue on svn.fs.commit_txn()? It is also
> > always (None, new_rev) if it doesn't raise Exception, since r845217,
> > Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
> > however, I took priority of compatibility. (Though I think no one
> > ever used this Python wrapper function in practical program....)
> >
> > Putting aside this compatibility issue, my preference is:
> >
> > if error is not occured:
> > if *conflict_p is NULL:
> > return *new_rev as a singletone
> > else:
> > # a foolproof. we can detect an internal error by doing this.
> return 2-tuple (*conflict_p, *new_rev)
> > else:
> > # error is occred
> > raise exception with values of *conflict_p and *new_rev in the
> exception
> >
> > So I prefer to make this change and announce it. Note that we have
> > an option that we'll change behavor only in Python 3 with preserve
> > the behavor in Python 2.
>
> I've revised the patch, with this applying to both of Python 2 and Python
> 3.
>
> >> Also, I don't love that we've used "conflict_p" and "new_rev" as the
> >> variables names within the SubversionException. Anything we do here
> will
> >> be non-obvious and require documentation, so I'd rather give these
> things
> >> meaningful names ("conflict_path" and "new_revision"). But as a
> concept,
> >> I'd say this part works well.
> >
> > Using argment names of C API have a merit that if we will implement such
> > special wrappers requires attirbutes of SubversionException, we just say
> > implement it, without attributes names. Although my patch uses
> > "conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
> > and "$2_name" (in the case of svn.fs.commit_txn).
> >
> > Of course, as there is no typemaps to be applied to multiple C APIs,
> > we should implement them one by one, so its trouble will be only a bit
> > that even we will name new attribute.
> In this patch, I used "$1_name" yet.
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/31 10:19, Yasuhito FUTATSUKI wrote:
> On 2020/07/30 22:20, C. Michael Pilato wrote:
>> The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
>> there are two issues I have with it.
>>
>> First, unless I'm mistaken, there's no way to get the conflict_p back as
>> part of the 2-tuple return value -- if there's a conflict,
>> svn.repos.fs_commit_txn() is going to raise an Exception. So we're
>> basically returning a 2-tuple whose first value is always None. We might
>> as well preserve the current behavior and return the newly committed
>> revision as a singleton. (Which also means we don't break compatibility
>> with prior versions.)
>
> How do you think about this issue on svn.fs.commit_txn()? It is also
> always (None, new_rev) if it doesn't raise Exception, since r845217,
> Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
> however, I took priority of compatibility. (Though I think no one
> ever used this Python wrapper function in practical program....)
>
> Putting aside this compatibility issue, my preference is:
>
> if error is not occured:
> if *conflict_p is NULL:
> return *new_rev as a singletone
> else:
> # a foolproof. we can detect an internal error by doing this.
return 2-tuple (*conflict_p, *new_rev)
> else:
> # error is occred
> raise exception with values of *conflict_p and *new_rev in the exception
>
> So I prefer to make this change and announce it. Note that we have
> an option that we'll change behavor only in Python 3 with preserve
> the behavor in Python 2.
I've revised the patch, with this applying to both of Python 2 and Python 3.
>> Also, I don't love that we've used "conflict_p" and "new_rev" as the
>> variables names within the SubversionException. Anything we do here will
>> be non-obvious and require documentation, so I'd rather give these things
>> meaningful names ("conflict_path" and "new_revision"). But as a concept,
>> I'd say this part works well.
>
> Using argment names of C API have a merit that if we will implement such
> special wrappers requires attirbutes of SubversionException, we just say
> implement it, without attributes names. Although my patch uses
> "conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
> and "$2_name" (in the case of svn.fs.commit_txn).
>
> Of course, as there is no typemaps to be applied to multiple C APIs,
> we should implement them one by one, so its trouble will be only a bit
> that even we will name new attribute.
In this patch, I used "$1_name" yet.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/30 22:20, C. Michael Pilato wrote:
> The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
> there are two issues I have with it.
>
> First, unless I'm mistaken, there's no way to get the conflict_p back as
> part of the 2-tuple return value -- if there's a conflict,
> svn.repos.fs_commit_txn() is going to raise an Exception. So we're
> basically returning a 2-tuple whose first value is always None. We might
> as well preserve the current behavior and return the newly committed
> revision as a singleton. (Which also means we don't break compatibility
> with prior versions.)
How do you think about this issue on svn.fs.commit_txn()? It is also
always (None, new_rev) if it doesn't raise Exception, since r845217,
Feb 2003. I also don't like this current behavor of svn.fs.commit_txn(),
however, I took priority of compatibility. (Though I think no one
ever used this Python wrapper function in practical program....)
Putting aside this compatibility issue, my preference is:
if error is not occured:
if *conflict_p is NULL:
return *new_rev as a singletone
else:
# a foolproof. we can detect an internal error by doing this.
return tupple (*conflict_p, *new_rev)
else:
# error is occred
raise exception with values of *conflict_p and *new_rev in the exception
So I prefer to make this change and announce it. Note that we have
an option that we'll change behavor only in Python 3 with preserve
the behavor in Python 2.
> Also, I don't love that we've used "conflict_p" and "new_rev" as the
> variables names within the SubversionException. Anything we do here will
> be non-obvious and require documentation, so I'd rather give these things
> meaningful names ("conflict_path" and "new_revision"). But as a concept,
> I'd say this part works well.
Using argment names of C API have a merit that if we will implement such
special wrappers requires attirbutes of SubversionException, we just say
implement it, without attributes names. Although my patch uses
"conflict_p" and "new_rev" as a C string literal, it can be "$1_name"
and "$2_name" (in the case of svn.fs.commit_txn).
Of course, as there is no typemaps to be applied to multiple C APIs,
we should implement them one by one, so its trouble will be only a bit
that even we will name new attribute.
> Does this change anything about the svn_fs_commit_txn() interface?
Nothing in the current patch, except the exception also have those
attribute.
>
> -- Mike
>
> On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
> wrote:
>
>> On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
>>> On 2020/07/29 23:43, C. Michael Pilato wrote:
>>>> Makes sense to me as a way to get access to all the things that can be
>>>> returned in an errorful situation.
>>>
>>> Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
>>> attributes to SubversionException instance on svn_fs.commit_txn and
>>> svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
>>> tuple of (None, int), for a consistency.
>>>
>>> I've tested this method on other API with dummy attribute, but I didn't
>>> test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
>>>
>>> Could you please review and try it ?
>>>
>>> Thanks, --
>>> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>>
>> Ah, I sent the patch before including a commit message. Here it is.
>> [[[
>> swig-py: Allow SubversionException to add attributes
>>
>> [in subversion/bindings/swig/]
>>
>> * include/svn_types.swg
>> (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
>> distinct those C APIs which return value by using pointer args when
>> error is occured.
>> (typemap(out) svn_error_t_with_attrs *): New typemap.
>>
>> * python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
>> (svn_swig_py_build_svn_exception): New function.
>> (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
>> get the exception class and to make the instance of it from subversion
>> error.
>>
>> * svn_fs.i
>> (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
>> - Add attribute "conflict_p" and "new_rev" to SubversionException
>> instance when the exception is occured.
>> - Fix the conversion format to build return value on Python 3
>> "z" conversion makes str when conflict_p is not NULL, although
>> it is always NULL in this context. So this is foolproof.
>> (svn_fs_commit_txn):
>> New fake function declaration for SWIG to make a wrapper. Ignore
>> the real one.
>>
>> * svn_repos.i
>> (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
>> svn_revnum_t *new_rev)):
>> New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
>> and modified argment number.
>> (svn_repos_fs_commit_txn):
>> New fake function declaration for SWIG to make a wrapper. Ignore
>> the real one.
>>
>> Found by: cmpilato
>> ]]]
>>
>>
>> Thanks,
>> --
>> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>>
>
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
The patch seems to work pretty well (for svn_repos_fs_commit_txn), but
there are two issues I have with it.
First, unless I'm mistaken, there's no way to get the conflict_p back as
part of the 2-tuple return value -- if there's a conflict,
svn.repos.fs_commit_txn() is going to raise an Exception. So we're
basically returning a 2-tuple whose first value is always None. We might
as well preserve the current behavior and return the newly committed
revision as a singleton. (Which also means we don't break compatibility
with prior versions.)
Also, I don't love that we've used "conflict_p" and "new_rev" as the
variables names within the SubversionException. Anything we do here will
be non-obvious and require documentation, so I'd rather give these things
meaningful names ("conflict_path" and "new_revision"). But as a concept,
I'd say this part works well.
Does this change anything about the svn_fs_commit_txn() interface?
-- Mike
On Thu, Jul 30, 2020 at 5:00 AM Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
wrote:
> On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
> > On 2020/07/29 23:43, C. Michael Pilato wrote:
> >> Makes sense to me as a way to get access to all the things that can be
> >> returned in an errorful situation.
> >
> > Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
> > attributes to SubversionException instance on svn_fs.commit_txn and
> > svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
> > tuple of (None, int), for a consistency.
> >
> > I've tested this method on other API with dummy attribute, but I didn't
> > test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
> >
> > Could you please review and try it ?
> >
> > Thanks, --
> > Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>
> Ah, I sent the patch before including a commit message. Here it is.
> [[[
> swig-py: Allow SubversionException to add attributes
>
> [in subversion/bindings/swig/]
>
> * include/svn_types.swg
> (svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
> distinct those C APIs which return value by using pointer args when
> error is occured.
> (typemap(out) svn_error_t_with_attrs *): New typemap.
>
> * python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
> (svn_swig_py_build_svn_exception): New function.
> (svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
> get the exception class and to make the instance of it from subversion
> error.
>
> * svn_fs.i
> (typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
> - Add attribute "conflict_p" and "new_rev" to SubversionException
> instance when the exception is occured.
> - Fix the conversion format to build return value on Python 3
> "z" conversion makes str when conflict_p is not NULL, although
> it is always NULL in this context. So this is foolproof.
> (svn_fs_commit_txn):
> New fake function declaration for SWIG to make a wrapper. Ignore
> the real one.
>
> * svn_repos.i
> (typemap(argout) (const char **conflict_p, svn_repos_t *repos,
> svn_revnum_t *new_rev)):
> New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
> and modified argment number.
> (svn_repos_fs_commit_txn):
> New fake function declaration for SWIG to make a wrapper. Ignore
> the real one.
>
> Found by: cmpilato
> ]]]
>
>
> Thanks,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/30 17:47, Yasuhito FUTATSUKI wrote:
> On 2020/07/29 23:43, C. Michael Pilato wrote:
>> Makes sense to me as a way to get access to all the things that can be
>> returned in an errorful situation.
>
> Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
> attributes to SubversionException instance on svn_fs.commit_txn and
> svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
> tuple of (None, int), for a consistency.
>
> I've tested this method on other API with dummy attribute, but I didn't
> test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
>
> Could you please review and try it ?
>
> Thanks, --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Ah, I sent the patch before including a commit message. Here it is.
[[[
swig-py: Allow SubversionException to add attributes
[in subversion/bindings/swig/]
* include/svn_types.swg
(svn_error_t_with_attrs): New C macro. An alias of svn_error_t to
distinct those C APIs which return value by using pointer args when
error is occured.
(typemap(out) svn_error_t_with_attrs *): New typemap.
* python/libsvn_swig_py/swigutil_py.h, python/libsvn_swig_py/swigutil_py.c
(svn_swig_py_build_svn_exception): New function.
(svn_swig_py_svn_exception): Use svn_swig_py_build_svn_exception to
get the exception class and to make the instance of it from subversion
error.
* svn_fs.i
(typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev)):
- Add attribute "conflict_p" and "new_rev" to SubversionException
instance when the exception is occured.
- Fix the conversion format to build return value on Python 3
"z" conversion makes str when conflict_p is not NULL, although
it is always NULL in this context. So this is foolproof.
(svn_fs_commit_txn):
New fake function declaration for SWIG to make a wrapper. Ignore
the real one.
* svn_repos.i
(typemap(argout) (const char **conflict_p, svn_repos_t *repos,
svn_revnum_t *new_rev)):
New typemap. Brought from typemap for svn_fs_commit_txn in svn_fs.i,
and modified argment number.
(svn_repos_fs_commit_txn):
New fake function declaration for SWIG to make a wrapper. Ignore
the real one.
Found by: cmpilato
]]]
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/29 23:43, C. Michael Pilato wrote:
> Makes sense to me as a way to get access to all the things that can be
> returned in an errorful situation.
Okey, I implemented it. The attached patch add "conflict_p" and "new_rev"
attributes to SubversionException instance on svn_fs.commit_txn and
svn_repos.fs_commit_txn. Also, svn_repos.fs_commit_txn now returns
tuple of (None, int), for a consistency.
I've tested this method on other API with dummy attribute, but I didn't
test on those svn_fs.commit_txn and svn_repos.fs_commit_txn.
Could you please review and try it ?
Thanks, --
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by "C. Michael Pilato" <cm...@red-bean.com>.
Makes sense to me as a way to get access to all the things that can be
returned in an errorful situation.
On Wed, Jul 29, 2020 at 7:17 AM Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
wrote:
> On 2020/07/29 7:45, Yasuhito FUTATSUKI wrote:
> > On 2020/07/29 1:02, Yasuhito FUTATSUKI wrote:
> >> On 2020/07/28 23:34, C. Michael Pilato wrote:
> >>> Hey, all.
> >>>
> >>> I'm writing some code that performs commits via the Subversion Python
> >>> bindings, and I'm struggling to understand some things I see there.
> >>>
> >>> In the svn_fs.i interface file, there's this block of code:
> >>>
> >>> /*
> -----------------------------------------------------------------------
> >>> Fix the return value for svn_fs_commit_txn(). If the conflict
> result is
> >>> NULL, then %append_output() is passed Py_None, but that goofs up
> >>> because that is *also* the marker for "I haven't started assembling
> a
> >>> multi-valued return yet" which means the second return value
> (new_rev)
> >>> will not cause a 2-tuple to be manufactured.
> >>>
> >>> The answer is to explicitly create a 2-tuple return value.
> >>>
> >>> FIXME: Do the Perl and Ruby bindings need to do something similar?
> >>> */
> >>> #ifdef SWIGPYTHON
> >>> %typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
> >>> /* this is always Py_None */
> >>> Py_DECREF($result);
> >>> /* build the result tuple */
> >>> $result = Py_BuildValue("zi", *$1, (long)*$2);
> >>> }
> >>> #endif
> >>
> >> Ah, it returns a tuple of str and int, not a tuple of bytes and int.
> >
> > I made a patch addressing to it, as attached
> > swig-py-fix-svn_fs_commit-patch.txt.
> > (Only affect on Python 3, no function change on Python 2.7)
> >
> >>> This reads and claims to behave exactly as I'd expect, given the dual
> >>> return values of svn_fs_commit_txn (and svn_repos_fs_commit_txn which
> wraps
> >>> it). And since this interface file is included from svn_repos.i, I
> would
> >>> expect those typemaps to apply also to svn_repos_fs_commit_txn, which
> has
> >>> matching parameter types and names.
> >>
> >> It seems it isn't match because svn_repos_fs_commit_txn have extra
> >> argument "svn_repos_t *repos" between them.
> >>
> >>> But this isn't how the code appears to work in practice. A successful
> >>> commit gets back from svn.repos.fs_commit_txn not a 2-tuple, but just
> the
> >>> newly created revision number. Moreover, if my commit succeeds but the
> >>> post-commit hook fails, svn.repos.fs_commit_txn() raises an Exception,
> >>> masking the return of the newly created revision number altogether.
> Now,
> >>> the masked revision number thing I can understand -- it's hard to do in
> >>> Python what we promise to do in C (which is to return an svn_error_t
> but
> >>> still set the new_rev return value). But the 2-tuple thing I have no
> >>> explanation for. Any ideas?
> >>
> >> I think it is need one more typemap for it in svn_repos.i like
> >>
> >> [[[
> >> #ifdef SWIGPYTHON
> >> %typemap(argout) (const char **conflict_p, svn_repos_t *repos,
> svn_revnum_t *new_rev) {
> >> /* this is always Py_None */
> >> Py_DECREF($result);
> >> /* build the result tuple */
> >> $result = Py_BuildValue("zi", *$1, (long)*$3);
> >> }
> >> #endif
> >> ]]]
> >
> > Also I made a patch for it, based on fixed typemap for svn_fs_commit_txn,
> > as attached swig-py-fix-svn_repos_fs_commit-patch.txt
> >
> > Difference on SWIG generated subversion/bindings/swig/python/svn_repos.c
> > between before and after this patch is below. (SWIG 3.0.12, for Python 3)
> > [[[
> > --- subversion/bindings/swig/python/svn_repos.c.before_patch
> 2020-07-29 05:45:35.626521000 +0900
> > +++ subversion/bindings/swig/python/svn_repos.c 2020-07-29
> 06:41:51.220682000 +0900
> > @@ -12126,23 +12126,16 @@
> > resultobj = Py_None;
> > }
> > {
> > - PyObject *s;
> > - if (*arg1 == NULL) {
> > - Py_INCREF(Py_None);
> > - s = Py_None;
> > - }
> > - else {
> > - s = PyBytes_FromString(*arg1);
> > - if (s == NULL)
> > - SWIG_fail;
> > - }
> > - resultobj = SWIG_Python_AppendOutput(resultobj, s);
> > - }
> > - if (SWIG_IsTmpObj(res3)) {
> > - resultobj = SWIG_Python_AppendOutput(resultobj,
> SWIG_From_long((*arg3)));
> > - } else {
> > - int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN | 0 ) : 0
> ;
> > - resultobj = SWIG_Python_AppendOutput(resultobj,
> SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_long, new_flags));
> > + /* this is always Py_None */
> > + Py_DECREF(resultobj);
> > + /* build the result tuple */
> > + resultobj = Py_BuildValue(
> > + #if PY_VERSION_HEX >= 0x03000000
> > + "yi",
> > + #else
> > + "zi",
> > + #endif
> > + *arg1, (long)*arg3);
> > }
> > {
> > Py_XDECREF(_global_py_pool);
> > ]]]
> >
> > I couldn't make test cases for them, so I didn't test, yet.
>
> I looked over svn_fs_commit_txn() C API, then I reconsidered how the
> Python wapper function should be.
>
> If svn_fs_commit_txn() returns NULL (i.e. the Python wrapper function
> can return without exception), the *conflict_p is always NULL.
> So I think it is no mean except compatibility that Python wrapper
> returns it as None in a tuple.
>
> It is better that when an exception is occur, the wrapper API returns
> conflict_p and new_rev as attributes of SubversionException object,
> if we can make it so.
>
> Any thoughts?
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/29 7:45, Yasuhito FUTATSUKI wrote:
> On 2020/07/29 1:02, Yasuhito FUTATSUKI wrote:
>> On 2020/07/28 23:34, C. Michael Pilato wrote:
>>> Hey, all.
>>>
>>> I'm writing some code that performs commits via the Subversion Python
>>> bindings, and I'm struggling to understand some things I see there.
>>>
>>> In the svn_fs.i interface file, there's this block of code:
>>>
>>> /* -----------------------------------------------------------------------
>>> Fix the return value for svn_fs_commit_txn(). If the conflict result is
>>> NULL, then %append_output() is passed Py_None, but that goofs up
>>> because that is *also* the marker for "I haven't started assembling a
>>> multi-valued return yet" which means the second return value (new_rev)
>>> will not cause a 2-tuple to be manufactured.
>>>
>>> The answer is to explicitly create a 2-tuple return value.
>>>
>>> FIXME: Do the Perl and Ruby bindings need to do something similar?
>>> */
>>> #ifdef SWIGPYTHON
>>> %typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
>>> /* this is always Py_None */
>>> Py_DECREF($result);
>>> /* build the result tuple */
>>> $result = Py_BuildValue("zi", *$1, (long)*$2);
>>> }
>>> #endif
>>
>> Ah, it returns a tuple of str and int, not a tuple of bytes and int.
>
> I made a patch addressing to it, as attached
> swig-py-fix-svn_fs_commit-patch.txt.
> (Only affect on Python 3, no function change on Python 2.7)
>
>>> This reads and claims to behave exactly as I'd expect, given the dual
>>> return values of svn_fs_commit_txn (and svn_repos_fs_commit_txn which wraps
>>> it). And since this interface file is included from svn_repos.i, I would
>>> expect those typemaps to apply also to svn_repos_fs_commit_txn, which has
>>> matching parameter types and names.
>>
>> It seems it isn't match because svn_repos_fs_commit_txn have extra
>> argument "svn_repos_t *repos" between them.
>>
>>> But this isn't how the code appears to work in practice. A successful
>>> commit gets back from svn.repos.fs_commit_txn not a 2-tuple, but just the
>>> newly created revision number. Moreover, if my commit succeeds but the
>>> post-commit hook fails, svn.repos.fs_commit_txn() raises an Exception,
>>> masking the return of the newly created revision number altogether. Now,
>>> the masked revision number thing I can understand -- it's hard to do in
>>> Python what we promise to do in C (which is to return an svn_error_t but
>>> still set the new_rev return value). But the 2-tuple thing I have no
>>> explanation for. Any ideas?
>>
>> I think it is need one more typemap for it in svn_repos.i like
>>
>> [[[
>> #ifdef SWIGPYTHON
>> %typemap(argout) (const char **conflict_p, svn_repos_t *repos, svn_revnum_t *new_rev) {
>> /* this is always Py_None */
>> Py_DECREF($result);
>> /* build the result tuple */
>> $result = Py_BuildValue("zi", *$1, (long)*$3);
>> }
>> #endif
>> ]]]
>
> Also I made a patch for it, based on fixed typemap for svn_fs_commit_txn,
> as attached swig-py-fix-svn_repos_fs_commit-patch.txt
>
> Difference on SWIG generated subversion/bindings/swig/python/svn_repos.c
> between before and after this patch is below. (SWIG 3.0.12, for Python 3)
> [[[
> --- subversion/bindings/swig/python/svn_repos.c.before_patch 2020-07-29 05:45:35.626521000 +0900
> +++ subversion/bindings/swig/python/svn_repos.c 2020-07-29 06:41:51.220682000 +0900
> @@ -12126,23 +12126,16 @@
> resultobj = Py_None;
> }
> {
> - PyObject *s;
> - if (*arg1 == NULL) {
> - Py_INCREF(Py_None);
> - s = Py_None;
> - }
> - else {
> - s = PyBytes_FromString(*arg1);
> - if (s == NULL)
> - SWIG_fail;
> - }
> - resultobj = SWIG_Python_AppendOutput(resultobj, s);
> - }
> - if (SWIG_IsTmpObj(res3)) {
> - resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_From_long((*arg3)));
> - } else {
> - int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN | 0 ) : 0 ;
> - resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_long, new_flags));
> + /* this is always Py_None */
> + Py_DECREF(resultobj);
> + /* build the result tuple */
> + resultobj = Py_BuildValue(
> + #if PY_VERSION_HEX >= 0x03000000
> + "yi",
> + #else
> + "zi",
> + #endif
> + *arg1, (long)*arg3);
> }
> {
> Py_XDECREF(_global_py_pool);
> ]]]
>
> I couldn't make test cases for them, so I didn't test, yet.
I looked over svn_fs_commit_txn() C API, then I reconsidered how the
Python wapper function should be.
If svn_fs_commit_txn() returns NULL (i.e. the Python wrapper function
can return without exception), the *conflict_p is always NULL.
So I think it is no mean except compatibility that Python wrapper
returns it as None in a tuple.
It is better that when an exception is occur, the wrapper API returns
conflict_p and new_rev as attributes of SubversionException object,
if we can make it so.
Any thoughts?
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/29 1:02, Yasuhito FUTATSUKI wrote:
> On 2020/07/28 23:34, C. Michael Pilato wrote:
>> Hey, all.
>>
>> I'm writing some code that performs commits via the Subversion Python
>> bindings, and I'm struggling to understand some things I see there.
>>
>> In the svn_fs.i interface file, there's this block of code:
>>
>> /* -----------------------------------------------------------------------
>> Fix the return value for svn_fs_commit_txn(). If the conflict result is
>> NULL, then %append_output() is passed Py_None, but that goofs up
>> because that is *also* the marker for "I haven't started assembling a
>> multi-valued return yet" which means the second return value (new_rev)
>> will not cause a 2-tuple to be manufactured.
>>
>> The answer is to explicitly create a 2-tuple return value.
>>
>> FIXME: Do the Perl and Ruby bindings need to do something similar?
>> */
>> #ifdef SWIGPYTHON
>> %typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
>> /* this is always Py_None */
>> Py_DECREF($result);
>> /* build the result tuple */
>> $result = Py_BuildValue("zi", *$1, (long)*$2);
>> }
>> #endif
>
> Ah, it returns a tuple of str and int, not a tuple of bytes and int.
I made a patch addressing to it, as attached
swig-py-fix-svn_fs_commit-patch.txt.
(Only affect on Python 3, no function change on Python 2.7)
>> This reads and claims to behave exactly as I'd expect, given the dual
>> return values of svn_fs_commit_txn (and svn_repos_fs_commit_txn which wraps
>> it). And since this interface file is included from svn_repos.i, I would
>> expect those typemaps to apply also to svn_repos_fs_commit_txn, which has
>> matching parameter types and names.
>
> It seems it isn't match because svn_repos_fs_commit_txn have extra
> argument "svn_repos_t *repos" between them.
>
>> But this isn't how the code appears to work in practice. A successful
>> commit gets back from svn.repos.fs_commit_txn not a 2-tuple, but just the
>> newly created revision number. Moreover, if my commit succeeds but the
>> post-commit hook fails, svn.repos.fs_commit_txn() raises an Exception,
>> masking the return of the newly created revision number altogether. Now,
>> the masked revision number thing I can understand -- it's hard to do in
>> Python what we promise to do in C (which is to return an svn_error_t but
>> still set the new_rev return value). But the 2-tuple thing I have no
>> explanation for. Any ideas?
>
> I think it is need one more typemap for it in svn_repos.i like
>
> [[[
> #ifdef SWIGPYTHON
> %typemap(argout) (const char **conflict_p, svn_repos_t *repos, svn_revnum_t *new_rev) {
> /* this is always Py_None */
> Py_DECREF($result);
> /* build the result tuple */
> $result = Py_BuildValue("zi", *$1, (long)*$3);
> }
> #endif
> ]]]
Also I made a patch for it, based on fixed typemap for svn_fs_commit_txn,
as attached swig-py-fix-svn_repos_fs_commit-patch.txt
Difference on SWIG generated subversion/bindings/swig/python/svn_repos.c
between before and after this patch is below. (SWIG 3.0.12, for Python 3)
[[[
--- subversion/bindings/swig/python/svn_repos.c.before_patch 2020-07-29 05:45:35.626521000 +0900
+++ subversion/bindings/swig/python/svn_repos.c 2020-07-29 06:41:51.220682000 +0900
@@ -12126,23 +12126,16 @@
resultobj = Py_None;
}
{
- PyObject *s;
- if (*arg1 == NULL) {
- Py_INCREF(Py_None);
- s = Py_None;
- }
- else {
- s = PyBytes_FromString(*arg1);
- if (s == NULL)
- SWIG_fail;
- }
- resultobj = SWIG_Python_AppendOutput(resultobj, s);
- }
- if (SWIG_IsTmpObj(res3)) {
- resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_From_long((*arg3)));
- } else {
- int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN | 0 ) : 0 ;
- resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_long, new_flags));
+ /* this is always Py_None */
+ Py_DECREF(resultobj);
+ /* build the result tuple */
+ resultobj = Py_BuildValue(
+ #if PY_VERSION_HEX >= 0x03000000
+ "yi",
+ #else
+ "zi",
+ #endif
+ *arg1, (long)*arg3);
}
{
Py_XDECREF(_global_py_pool);
]]]
I couldn't make test cases for them, so I didn't test, yet.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Python bindings API confusion
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2020/07/28 23:34, C. Michael Pilato wrote:
> Hey, all.
>
> I'm writing some code that performs commits via the Subversion Python
> bindings, and I'm struggling to understand some things I see there.
>
> In the svn_fs.i interface file, there's this block of code:
>
> /* -----------------------------------------------------------------------
> Fix the return value for svn_fs_commit_txn(). If the conflict result is
> NULL, then %append_output() is passed Py_None, but that goofs up
> because that is *also* the marker for "I haven't started assembling a
> multi-valued return yet" which means the second return value (new_rev)
> will not cause a 2-tuple to be manufactured.
>
> The answer is to explicitly create a 2-tuple return value.
>
> FIXME: Do the Perl and Ruby bindings need to do something similar?
> */
> #ifdef SWIGPYTHON
> %typemap(argout) (const char **conflict_p, svn_revnum_t *new_rev) {
> /* this is always Py_None */
> Py_DECREF($result);
> /* build the result tuple */
> $result = Py_BuildValue("zi", *$1, (long)*$2);
> }
> #endif
Ah, it returns a tuple of str and int, not a tuple of bytes and int.
> This reads and claims to behave exactly as I'd expect, given the dual
> return values of svn_fs_commit_txn (and svn_repos_fs_commit_txn which wraps
> it). And since this interface file is included from svn_repos.i, I would
> expect those typemaps to apply also to svn_repos_fs_commit_txn, which has
> matching parameter types and names.
It seems it isn't match because svn_repos_fs_commit_txn have extra
argument "svn_repos_t *repos" between them.
> But this isn't how the code appears to work in practice. A successful
> commit gets back from svn.repos.fs_commit_txn not a 2-tuple, but just the
> newly created revision number. Moreover, if my commit succeeds but the
> post-commit hook fails, svn.repos.fs_commit_txn() raises an Exception,
> masking the return of the newly created revision number altogether. Now,
> the masked revision number thing I can understand -- it's hard to do in
> Python what we promise to do in C (which is to return an svn_error_t but
> still set the new_rev return value). But the 2-tuple thing I have no
> explanation for. Any ideas?
I think it is need one more typemap for it in svn_repos.i like
[[[
#ifdef SWIGPYTHON
%typemap(argout) (const char **conflict_p, svn_repos_t *repos, svn_revnum_t *new_rev) {
/* this is always Py_None */
Py_DECREF($result);
/* build the result tuple */
$result = Py_BuildValue("zi", *$1, (long)*$3);
}
#endif
]]]
but I didn't tried yet.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>