You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Troy Curtis Jr <tr...@apache.org> on 2018/12/16 14:59:36 UTC
Re: [swig-py3][patch] interfacing bytes object instead of str
On Wed, Nov 28, 2018 at 10:40 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Yasuhito FUTATSUKI wrote on Tue, Nov 27, 2018 at 06:50:46 +0900:
> > I've revised typemaps and APIs using svn_stringbuf_t *, then I found
> > almost all those APIs include svn_stream_readline() use svn_stringbuf_t
> > for file contents. So I've modified typemaps again so that those APIs
> > use bytes for svn_stringbuf_t interface.
> >
> > The patch below destined for branches/swig-py affects those API wrappers.
>
> Thanks for the patch, Yasuhito. It looks good on a quick skim. Troy,
> you've
> worked on this branch before; would you perchance be able to review this?
> (If you can, great; but no worries if you can't)
>
Yes the patch is looking good, especially having the svn_stream Python APIs
actually tested! I updated the patch so that it works with the latest
swig-py3 (@r1849027) and attached it.
But there was one item I wanted to talk about related to the patch. I
agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
right general path, but after the patch there is now a "svn_stringbuf_t
*output" typemap that is still using "PyStr" instead of "PyBytes".
Further, the only usage of "svn_stringbuf_t *output" is in
"svn_fs_print_modules" and "svn_ra_print_modules", where the function
parameter is actually an output parameter, so the appropriate typemap is
probably "argout" instead of "in".
Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
your patch and perhaps address the issue in the last paragraph then I can
commit your patch to swig-py3. I'll also start working more on the swig-py3
branch to get it to the point that we can actually get this branch merged
into trunk finally.
Troy
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by fu...@poem.co.jp.
On 12/17/18 18:35, Daniel Shahaf wrote:
> Yasuhito FUTATSUKI wrote on Mon, Dec 17, 2018 at 05:50:03 +0900:
>> On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
>>> On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d....@daniel.shahaf.name>
>>> wrote:
>>>
>>>> Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
>>>>> But there was one item I wanted to talk about related to the patch. I
>>>>> agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
>>>>> right general path,
>
> Would svn_stringbuf_t map better to bytearray? Sorry, that didn't occur
> to me yesterday.
I think if proxy object for svn_stringbuf_t can support buffer protocol,
no need to translate to bytes object for return values, however,
for input values, it is still need to translation to svn_stringbuf_t
with entity copy except proxy objects.
(I already agreed that propety values should be mapped to bytes objects,
so I ommit latter half.)
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yasuhito FUTATSUKI wrote on Mon, Dec 17, 2018 at 05:50:03 +0900:
> On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
> > On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d....@daniel.shahaf.name>
> > wrote:
> >
> > > Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
> > > > But there was one item I wanted to talk about related to the patch. I
> > > > agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
> > > > right general path,
Would svn_stringbuf_t map better to bytearray? Sorry, that didn't occur
to me yesterday.
> > > Are you sure about this? Property values can be binary data (so not
> > > representable as 'str') but are usually represented by «svn_string_t *»;
> > > for example, svn_repos_parse_fns3_t::set_node_property().
> > >
> >
> > Ah yes, I suppose this also has bearing on the discussion in the other
> > thread relating
> > to returning property values. With that in mind, perhaps your suggestion
> > of effectively
> > defaulting to always being Bytes for char * , svn_string_t, and
> > svn_stringbuf_t unless there
> > is a specific circumstance not to makes the most sense as the general
> > principle.
>
> To treat all those values as bytes in wrapper makes the wrapper is lower
> level, keeping raw access like device drivers. On the other hand,
> to make contexts, to make semantic differences in wrapper makes it more
> higher level. So I think those are not the general principle but just a
> policy of the wrapper design.
In general, you're right. The swig bindings are not Pythonic; to use
them requires using C idioms while writing Python. There's certainly
room for higher-level abstractions — I think the javahl and
python-ctypes bindings are examples — but the trade-off is that they
can't be autogenerated like the swig bindings can.
However, all that aside, I think this general discussion doesn't apply
here. The value of the svn_string_t* parameter of svn_repos_parse_fns3_t::set_node_property()
may be *unrepresentable* as a py3 str object. It isn't a question of
C-like idiom versus a Pythonic idiom; the bytes class can can represent
all possible values, the str class cannot.
Consider «svn propset -F /bin/sh foo iota». How would you represent
that value as str?
> I, as a consumer of the Python bindings, prefer the latter if it is
> clear that what are bytes and what are str, however I also agree with
> former if the distinction is too complex for both of consumers and
> developers.
Cheers,
Daniel
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Troy Curtis Jr wrote on Mon, Dec 24, 2018 at 22:20:40 -0500:
> On Mon, Dec 24, 2018 at 1:11 PM Yasuhito FUTATSUKI <fu...@poem.co.jp>
> wrote:
> @@ -1321,7 +1321,7 @@ svn_swig_py_unwrap_string(PyObject *source,
> > void *baton)
> >
>
> Requiring "source" to be a Bytes object, makes this function name be
> incorrect, since giving it a string will cause an error. Probably a
> reasonable place to support both Bytes/str. This is an exported function, so
> changing the name isn't a good option.
We don't promise ABI compatibility for all exported symbols; only for
those that are part of the public API. It's not clear to me that this
function qualifies: is it expected that anyone other than Subversion's
own code will call this function? After all, the header file that
declares it (swigutil_py.h) isn't even installed by install-swig-py.
If it doesn't qualify as a public API function, then we're free to
change its semantics however we wish, with or without renaming it (there
are precedents for both options).
Cheers,
Daniel
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by fu...@poem.co.jp.
On 12/17/18 05:50, Yasuhito FUTATSUKI wrote:
> I found exception error messages isn't unified for
> ('a bytes', 'a bytes object', 'a Bytes object'),
> and nothing but these.
I've updated the patch including fix above (unified to
'a bytes objects') and type casting between signed and
unsigned for len argment in svn_stream_write(),
for swig-py3@1849073
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Troy Curtis Jr <tr...@apache.org>.
On Fri, Dec 28, 2018 at 8:20 AM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:
> On 12/28/18 4:09 AM, Troy Curtis wrote:
> > LGTM, committed as r1849804, but without the unnecessary gymnastics to
> continue supporting str() in those two cases mentioned above. I also
> tweaked the log message to use complete sentences (and removed references
> to the code that I didn't include).
>
> Thank you for correct my broken log message.
>
> As the second step of follow up r1849784, I've fixed #if blocks usage
> in subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c because
> it is obvious and it doesn't contain any functional change.
>
> follow up r189784: fix macro usage to distinguish py2 and py3 context
>
> * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> Include "swigutil_py3c.h" file before define SVN_SWIG_BYTES_FMT to use
> IS_PY3 macro to switch py2 and py3 context
> (add_directory, add_file, apply_textdelta, change_file_prop,
> close_file, parse_fn3_set_revision_property,
> parse_fn3_set_node_property, svn_swig_py_notify_func,
> svn_swig_py_notify_func, svn_swig_py_delta_path_driver_cb_func,
> svn_swig_py_repos_authz_func, svn_swig_py_log_receiver,
> svn_swig_py_client_blame_receiver_func,
> svn_swig_py_changelist_receiver_func,
> svn_swig_py_auth_simple_prompt_func,
> svn_swig_py_config_auth_walk_func,
> ra_callbacks_get_wc_prop, ra_callbacks_push_or_set_wc_prop,
> ra_callbacks_invalidate_wc_props, svn_swig_py_commit_callback,
> reporter_set_path, reporter_link_path,
> wc_diff_callbacks2_file_changed_or_added,
> wc_diff_callbacks2_file_deleted, wc_diff_callbacks2_dir_added,
> wc_diff_callbacks2_dir_props_changed,
> svn_swig_py_config_enumerator2):
> Use IS_PY3 macro instead of comparison of PY_VERSION_HEX
> (add_directory, add_file, apply_textdelta, change_file_prop,
> close_file, parse_fn3_set_revision_property,
> parse_fn3_set_node_property, ra_callbacks_get_wc_prop,
> ra_callbacks_push_or_set_wc_prop, ra_callbacks_invalidate_wc_props):
> Enclose only format argument of Py_CallFunction or Py_CallMethod
> with #if IS_PY3 #else #endif block
>
Great! LGTM, committed as r1850011.
> Cheers,
> --
Yasuhito FUTATSUKI
>
Troy
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 12/28/18 4:09 AM, Troy Curtis wrote:
> LGTM, committed as r1849804, but without the unnecessary gymnastics to continue supporting str() in those two cases mentioned above. I also tweaked the log message to use complete sentences (and removed references to the code that I didn't include).
Thank you for correct my broken log message.
As the second step of follow up r1849784, I've fixed #if blocks usage
in subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c because
it is obvious and it doesn't contain any functional change.
follow up r189784: fix macro usage to distinguish py2 and py3 context
* subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
Include "swigutil_py3c.h" file before define SVN_SWIG_BYTES_FMT to use
IS_PY3 macro to switch py2 and py3 context
(add_directory, add_file, apply_textdelta, change_file_prop,
close_file, parse_fn3_set_revision_property,
parse_fn3_set_node_property, svn_swig_py_notify_func,
svn_swig_py_notify_func, svn_swig_py_delta_path_driver_cb_func,
svn_swig_py_repos_authz_func, svn_swig_py_log_receiver,
svn_swig_py_client_blame_receiver_func,
svn_swig_py_changelist_receiver_func,
svn_swig_py_auth_simple_prompt_func,
svn_swig_py_config_auth_walk_func,
ra_callbacks_get_wc_prop, ra_callbacks_push_or_set_wc_prop,
ra_callbacks_invalidate_wc_props, svn_swig_py_commit_callback,
reporter_set_path, reporter_link_path,
wc_diff_callbacks2_file_changed_or_added,
wc_diff_callbacks2_file_deleted, wc_diff_callbacks2_dir_added,
wc_diff_callbacks2_dir_props_changed,
svn_swig_py_config_enumerator2):
Use IS_PY3 macro instead of comparison of PY_VERSION_HEX
(add_directory, add_file, apply_textdelta, change_file_prop,
close_file, parse_fn3_set_revision_property,
parse_fn3_set_node_property, ra_callbacks_get_wc_prop,
ra_callbacks_push_or_set_wc_prop, ra_callbacks_invalidate_wc_props):
Enclose only format argument of Py_CallFunction or Py_CallMethod
with #if IS_PY3 #else #endif block
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Troy Curtis <tr...@apache.org>.
On 12/27/18 12:42 PM, Yasuhito FUTATSUKI wrote:
> On 12/27/18 1:01 PM, Troy Curtis wrote:
> > The only real functionality changes I'm concerned with on the
> changes are dropping str() conversion on a couple of the API inputs
> since previously providing any object with a suitable __str__ method
> would work, but would now be broken. But a simple way of supporting Py2
> and Py3 isn't a "gimme", so didn't want to block this patch on that.
>
> I see. So I've revise it and str/bytes conversion in unit tests as a first
> step of followup to r1849784.
Oh my, so I've been silly having you look at this. I just realized that
both usages of dropping str() were within the tests! SMH! Since those
usages are completely internal, there is no danger of being provided
some other value. I apologize for the misdirection.
>
> [in subversion/bindings/swig/python/tests]
>
> * client.py (SubversionClientTestCase.test_update4): not to encode
> os.path.sep if it is already bytes object
>
> * core.py (SubversionCoreTestCase.test_stream_write): not to encode
> fname if it is already bytes object
>
> * delta.py (DeltaTestCase.testTxWindowHandler_stream_IF,
> DeltaTestCase.testTxWindowHandler_Stream_IF): not to encode fname if it
> is already bytes object
>
> * fs.py (SubversionFSTestCase.setUp): not to encode self.tmpfile if it
> is already bytes object
>
> * trac/versioncontrol/main.py (Node.__init__): revise change to strip
> str() on r1849784; if path is already bytes, use it for self.path.
> othewise ensure self.path is str, and if it is not bytes (i.e. in case
> of py3), encode it to bytes
>
> * trac/versioncontrol/svn_fs.py (SubversionNode.__init__): don't use
> IS_PY3 condition, just store a unicode string in the exception object.
> - remove unnecessary variable IS_PY3 and therefore unnecessary import
> of sys module
> - (SubversionNode.getproperties): revise change to strip str() on
> r1849784; if value is already bytes, use it for prop[name]. othewise
> ensure value is str, and if it is not bytes (i.e. in case of py3),
> encode it to bytes for consistency of the type of prop[name]
>
> * utils.py (Temper.alloc_empty_dir, Temper.alloc_empty_repo): ensure
> suffix arg for tempfile.mkdtemp() is unicode (for py3 < 3.5)
>
> Cheers,
LGTM, committed as r1849804, but without the unnecessary gymnastics to
continue supporting str() in those two cases mentioned above. I also
tweaked the log message to use complete sentences (and removed
references to the code that I didn't include).
Troy
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 12/27/18 1:01 PM, Troy Curtis wrote:
> The only real functionality changes I'm concerned with on the changes are dropping str() conversion on a couple of the API inputs since previously providing any object with a suitable __str__ method would work, but would now be broken. But a simple way of supporting Py2 and Py3 isn't a "gimme", so didn't want to block this patch on that.
I see. So I've revise it and str/bytes conversion in unit tests as a first
step of followup to r1849784.
[in subversion/bindings/swig/python/tests]
* client.py (SubversionClientTestCase.test_update4): not to encode
os.path.sep if it is already bytes object
* core.py (SubversionCoreTestCase.test_stream_write): not to encode
fname if it is already bytes object
* delta.py (DeltaTestCase.testTxWindowHandler_stream_IF,
DeltaTestCase.testTxWindowHandler_Stream_IF): not to encode fname if it
is already bytes object
* fs.py (SubversionFSTestCase.setUp): not to encode self.tmpfile if it
is already bytes object
* trac/versioncontrol/main.py (Node.__init__): revise change to strip
str() on r1849784; if path is already bytes, use it for self.path.
othewise ensure self.path is str, and if it is not bytes (i.e. in case
of py3), encode it to bytes
* trac/versioncontrol/svn_fs.py (SubversionNode.__init__): don't use
IS_PY3 condition, just store a unicode string in the exception object.
- remove unnecessary variable IS_PY3 and therefore unnecessary import
of sys module
- (SubversionNode.getproperties): revise change to strip str() on
r1849784; if value is already bytes, use it for prop[name]. othewise
ensure value is str, and if it is not bytes (i.e. in case of py3),
encode it to bytes for consistency of the type of prop[name]
* utils.py (Temper.alloc_empty_dir, Temper.alloc_empty_repo): ensure
suffix arg for tempfile.mkdtemp() is unicode (for py3 < 3.5)
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Troy Curtis <tr...@troycurtisjr.com>.
On 12/26/18 11:51 AM, futatuki@poem.co.jp wrote:
> On 12/25/18 12:20, Troy Curtis Jr wrote:
>> On Mon, Dec 24, 2018 at 1:11 PM Yasuhito FUTATSUKI <fu...@poem.co.jp>
>> wrote:
>
>> Awesome! I'm really glad you are helping work on this, helps to keep me
>> motivated as well ;)
>
> I'm glad if I could some help, too :)
I've committed your patch as is at r1849784, so that any of these
enhancements we've discussed can be smaller, more focused patches. I
did tweak some formatting for the log message to better match the
typical guidelines. Mostly spacing and converting to complete sentences.
I also converted the symbol references in python code from
<Class::method> to more the more Pythonic <Class.method> format for
better grepping.
The only real functionality changes I'm concerned with on the changes
are dropping str() conversion on a couple of the API inputs since
previously providing any object with a suitable __str__ method would
work, but would now be broken. But a simple way of supporting Py2 and
Py3 isn't a "gimme", so didn't want to block this patch on that.
>
>> So I wonder, as a guiding principle if we could support either Str or
>> Bytes
>> as an input? That would be akin to most
>> of the standard module interfaces. I think that would be fairly easy to
>> support, and would help ensure that the
>> behavior is much closer to the typical expectation. it would also reduce
>> the number of b"" that are necessary :P Of
>> course, in many cases for the standard library interfaces, the input type
>> is used as a hint to indicate what type to
>> return, which is much more difficult/impossible to fully support in these
>> bindings. That seems like it would provide
>> a way to make using the bindings more familiar, though we'd still have to
>> have the policy of just always returning Bytes
>> objects as output. That input/output disconnect might undo any ease of
>> use
>> advantage than just always using Bytes
>> ...I'm not sure exactly what my opinion on that is...
>
> It is just I wanted to confirm, how deal with inputs :)
>
> I also want that both of Bytes and Str is acceptable input values,
> especially for paths on file system (not Subversion's internal paths).
> So, I'll tweak typemaps for it.
>
>> The patch inline for comments:
>>
>> diff --git a/build/ac-macros/swig.m4 b/build/ac-macros/swig.m4
>>> index d865689de5..c797ae69d8 100644
>>> --- a/build/ac-macros/swig.m4
>>> +++ b/build/ac-macros/swig.m4
>>> @@ -154,7 +154,7 @@ AC_DEFUN(SVN_FIND_SWIG,
>>> ])
>>>
>>> if test "$ac_cv_python_is_py3" = "yes"; then
>>> - SWIG_PY_OPTS="-python -py3"
>>> + SWIG_PY_OPTS="-python -py3 -DPY3"
>>> else
>>> SWIG_PY_OPTS="-python -classic"
>>> fi
>>>
>>
>> We shouldn't define a new preprocessor value here, but rather make use of
>> the existing IS_PY3 value defined by py3c.
>
> It was really needed because IS_PY3 macro symbol is not recognized to
> SWIG, because it is C preprocessers macro, not SWIG macro. I've looked
> for distinguish py2 and py3 on SWIG context, but I couldn't find suitable
> macro. (however this is no more needed)
>
>> diff --git a/subversion/bindings/swig/core.i
>>> b/subversion/bindings/swig/core.i
>>> index e5a234c5e2..c309d48070 100644
>>> --- a/subversion/bindings/swig/core.i
>>> +++ b/subversion/bindings/swig/core.i
>>>
>>
>> [snip]
>>
>> @@ -442,16 +442,18 @@
>>> */
>>> #ifdef SWIGPYTHON
>>> %typemap(in) (const char *data, apr_size_t *len) ($*2_type temp) {
>>> - if (!PyStr_Check($input)) {
>>> + char *tmpdata;
>>> + Py_ssize_t length;
>>> + if (!PyBytes_Check($input)) {
>>> PyErr_SetString(PyExc_TypeError,
>>> - "expecting a string for the buffer");
>>> + "expecting a bytes object for the buffer");
>>> SWIG_fail;
>>> }
>>> - /* Explicitly cast away const from PyStr_AsUTF8AndSize (in
>>> Python 3).
>>> The
>>> - swig generated argument ($1) here will be "char *", but since
>>> its
>>> only
>>> - use is to pass directly into the "const char *" argument of the
>>> wrapped
>>> - function, this is safe to do. */
>>> - $1 = (char *)PyStr_AsUTF8AndSize($input, &temp);
>>>
>>
>>
>>> + if (PyBytes_AsStringAndSize($input, &tmpdata, &length) == -1) {
>>>
>>
>> Why not just use "&$1" here instead of introducing a new temp variable?
>
> It's to avoid C compiler warning to discard 'const' quarifier, but this
> can be also safely accomplished by cast.
>
>> + SWIG_fail;
>>> + }
>>> + temp = ($*2_type)length;
>>> + $1 = tmpdata;
>>> $2 = ($2_ltype)&temp;
>>> }
>>> #endif
>>>
>> @@ -504,8 +506,8 @@
>>> SWIG_fail;
>>> }
>>>
>>> - if (PyStr_Check($input)) {
>>> - const char *value = PyStr_AsString($input);
>>> + if (PyBytes_Check($input)) {
>>> + const char *value = PyBytes_AsString($input);
>>> $1 = apr_pstrdup(_global_pool, value);
>>> }
>>> else if (PyLong_Check($input)) {
>>>
>>
>> Since this function does a whole list of checks and conversions,
>> perhaps it
>> makes sense to support Str here in
>> addition to bytes? Of course in Py2 the "PyStr_Check()" would effectively
>> be performed twice if we did that, but
>> I don't expect that to be a tremendous impact, and with the "cost" of an
>> addition "IS_PY3" preprocessor check,
>> that could be eliminated from the Py2 build.
>
> I Agreed.
>
>>> diff --git a/subversion/bindings/swig/include/svn_global.swg
>>> b/subversion/bindings/swig/include/svn_global.swg
>>> index 191bbdd531..f7364be28f 100644
>>> --- a/subversion/bindings/swig/include/svn_global.swg
>>> +++ b/subversion/bindings/swig/include/svn_global.swg
>>> @@ -31,6 +31,12 @@
>>> #define SVN_DEPRECATED
>>> #endif
>>>
>>> +#ifdef SWIGPYTHON
>>> +%begin %{
>>> +#define SWIG_PYTHON_STRICT_BYTE_CHAR
>>> +%}
>>> +#endif
>>> +
>>> %include typemaps.i
>>> %include constraints.i
>>> %include exception.i
>>> @@ -136,9 +142,15 @@ static PyObject * _global_py_pool = NULL;
>>> /* Python format specifiers. Use Python instead of SWIG to parse
>>> these basic types, because Python reports better error messages
>>> (with correct argument numbers). */
>>> +#if defined(PY3)
>>>
>>
>> This is where we'd want to use py3c's "IS_PY3" value.
>
> including header file py3c/compath.h is done by indirectly as
>
> #ifdef SWIGPYTHON
> %{
> #include "swigutil_py.h"
> #include "swigutil_py3c.h"
> %}
> #endif
>
> however, it is enclosed by %{ %} so it is not defined as SWIG macro
> symbol. and following
>
>>> +%typemap (in, parse="y")
>>> + char *, char const *, char * const, char const * const "";
>>> +#else
>>> %typemap (in, parse="s")
>>> char *, char const *, char * const, char const * const "";
>>> +#endif
>
> is a SWIG context switch. this is why -DPY3 was needed.
> However, if we want to allow both of Bytes and Str for input, `parse'
> directive cannot be used here and must write code here to type check
> and to convert ourself, it isn't needed.
Ah, I see your point!
>
> Rest of all your suggestions convinced me, so I'll start to modify. >
> Cheers,
Thanks!
Troy
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by fu...@poem.co.jp.
On 12/25/18 12:20, Troy Curtis Jr wrote:
> On Mon, Dec 24, 2018 at 1:11 PM Yasuhito FUTATSUKI <fu...@poem.co.jp>
> wrote:
> Awesome! I'm really glad you are helping work on this, helps to keep me
> motivated as well ;)
I'm glad if I could some help, too :)
> So I wonder, as a guiding principle if we could support either Str or Bytes
> as an input? That would be akin to most
> of the standard module interfaces. I think that would be fairly easy to
> support, and would help ensure that the
> behavior is much closer to the typical expectation. it would also reduce
> the number of b"" that are necessary :P Of
> course, in many cases for the standard library interfaces, the input type
> is used as a hint to indicate what type to
> return, which is much more difficult/impossible to fully support in these
> bindings. That seems like it would provide
> a way to make using the bindings more familiar, though we'd still have to
> have the policy of just always returning Bytes
> objects as output. That input/output disconnect might undo any ease of use
> advantage than just always using Bytes
> ...I'm not sure exactly what my opinion on that is...
It is just I wanted to confirm, how deal with inputs :)
I also want that both of Bytes and Str is acceptable input values,
especially for paths on file system (not Subversion's internal paths).
So, I'll tweak typemaps for it.
> The patch inline for comments:
>
> diff --git a/build/ac-macros/swig.m4 b/build/ac-macros/swig.m4
>> index d865689de5..c797ae69d8 100644
>> --- a/build/ac-macros/swig.m4
>> +++ b/build/ac-macros/swig.m4
>> @@ -154,7 +154,7 @@ AC_DEFUN(SVN_FIND_SWIG,
>> ])
>>
>> if test "$ac_cv_python_is_py3" = "yes"; then
>> - SWIG_PY_OPTS="-python -py3"
>> + SWIG_PY_OPTS="-python -py3 -DPY3"
>> else
>> SWIG_PY_OPTS="-python -classic"
>> fi
>>
>
> We shouldn't define a new preprocessor value here, but rather make use of
> the existing IS_PY3 value defined by py3c.
It was really needed because IS_PY3 macro symbol is not recognized to
SWIG, because it is C preprocessers macro, not SWIG macro. I've looked
for distinguish py2 and py3 on SWIG context, but I couldn't find suitable
macro. (however this is no more needed)
> diff --git a/subversion/bindings/swig/core.i
>> b/subversion/bindings/swig/core.i
>> index e5a234c5e2..c309d48070 100644
>> --- a/subversion/bindings/swig/core.i
>> +++ b/subversion/bindings/swig/core.i
>>
>
> [snip]
>
> @@ -442,16 +442,18 @@
>> */
>> #ifdef SWIGPYTHON
>> %typemap(in) (const char *data, apr_size_t *len) ($*2_type temp) {
>> - if (!PyStr_Check($input)) {
>> + char *tmpdata;
>> + Py_ssize_t length;
>> + if (!PyBytes_Check($input)) {
>> PyErr_SetString(PyExc_TypeError,
>> - "expecting a string for the buffer");
>> + "expecting a bytes object for the buffer");
>> SWIG_fail;
>> }
>> - /* Explicitly cast away const from PyStr_AsUTF8AndSize (in Python 3).
>> The
>> - swig generated argument ($1) here will be "char *", but since its
>> only
>> - use is to pass directly into the "const char *" argument of the
>> wrapped
>> - function, this is safe to do. */
>> - $1 = (char *)PyStr_AsUTF8AndSize($input, &temp);
>>
>
>
>> + if (PyBytes_AsStringAndSize($input, &tmpdata, &length) == -1) {
>>
>
> Why not just use "&$1" here instead of introducing a new temp variable?
It's to avoid C compiler warning to discard 'const' quarifier, but this
can be also safely accomplished by cast.
> + SWIG_fail;
>> + }
>> + temp = ($*2_type)length;
>> + $1 = tmpdata;
>> $2 = ($2_ltype)&temp;
>> }
>> #endif
>>
> @@ -504,8 +506,8 @@
>> SWIG_fail;
>> }
>>
>> - if (PyStr_Check($input)) {
>> - const char *value = PyStr_AsString($input);
>> + if (PyBytes_Check($input)) {
>> + const char *value = PyBytes_AsString($input);
>> $1 = apr_pstrdup(_global_pool, value);
>> }
>> else if (PyLong_Check($input)) {
>>
>
> Since this function does a whole list of checks and conversions, perhaps it
> makes sense to support Str here in
> addition to bytes? Of course in Py2 the "PyStr_Check()" would effectively
> be performed twice if we did that, but
> I don't expect that to be a tremendous impact, and with the "cost" of an
> addition "IS_PY3" preprocessor check,
> that could be eliminated from the Py2 build.
I Agreed.
>> diff --git a/subversion/bindings/swig/include/svn_global.swg
>> b/subversion/bindings/swig/include/svn_global.swg
>> index 191bbdd531..f7364be28f 100644
>> --- a/subversion/bindings/swig/include/svn_global.swg
>> +++ b/subversion/bindings/swig/include/svn_global.swg
>> @@ -31,6 +31,12 @@
>> #define SVN_DEPRECATED
>> #endif
>>
>> +#ifdef SWIGPYTHON
>> +%begin %{
>> +#define SWIG_PYTHON_STRICT_BYTE_CHAR
>> +%}
>> +#endif
>> +
>> %include typemaps.i
>> %include constraints.i
>> %include exception.i
>> @@ -136,9 +142,15 @@ static PyObject * _global_py_pool = NULL;
>> /* Python format specifiers. Use Python instead of SWIG to parse
>> these basic types, because Python reports better error messages
>> (with correct argument numbers). */
>> +#if defined(PY3)
>>
>
> This is where we'd want to use py3c's "IS_PY3" value.
including header file py3c/compath.h is done by indirectly as
#ifdef SWIGPYTHON
%{
#include "swigutil_py.h"
#include "swigutil_py3c.h"
%}
#endif
however, it is enclosed by %{ %} so it is not defined as SWIG macro
symbol. and following
>> +%typemap (in, parse="y")
>> + char *, char const *, char * const, char const * const "";
>> +#else
>> %typemap (in, parse="s")
>> char *, char const *, char * const, char const * const "";
>> +#endif
is a SWIG context switch. this is why -DPY3 was needed.
However, if we want to allow both of Bytes and Str for input, `parse'
directive cannot be used here and must write code here to type check
and to convert ourself, it isn't needed.
Rest of all your suggestions convinced me, so I'll start to modify.
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Troy Curtis Jr <tr...@apache.org>.
On Mon, Dec 24, 2018 at 1:11 PM Yasuhito FUTATSUKI <fu...@poem.co.jp>
wrote:
> On 12/17/18 5:50 AM, Yasuhito FUTATSUKI wrote:
> > On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
>
[snip]
> Status update: I've done to make patch that default to map bytes
> from char *, svn_string_t, and svn_stringbuf_t, for swig-py3@1849037.
>
Awesome! I'm really glad you are helping work on this, helps to keep me
motivated as well ;)
So I wonder, as a guiding principle if we could support either Str or Bytes
as an input? That would be akin to most
of the standard module interfaces. I think that would be fairly easy to
support, and would help ensure that the
behavior is much closer to the typical expectation. it would also reduce
the number of b"" that are necessary :P Of
course, in many cases for the standard library interfaces, the input type
is used as a hint to indicate what type to
return, which is much more difficult/impossible to fully support in these
bindings. That seems like it would provide
a way to make using the bindings more familiar, though we'd still have to
have the policy of just always returning Bytes
objects as output. That input/output disconnect might undo any ease of use
advantage than just always using Bytes
...I'm not sure exactly what my opinion on that is...
[ snip ]
Cheers,
> --
> Yasuhito FUTATSUKI
>
The patch inline for comments:
diff --git a/build/ac-macros/swig.m4 b/build/ac-macros/swig.m4
> index d865689de5..c797ae69d8 100644
> --- a/build/ac-macros/swig.m4
> +++ b/build/ac-macros/swig.m4
> @@ -154,7 +154,7 @@ AC_DEFUN(SVN_FIND_SWIG,
> ])
>
> if test "$ac_cv_python_is_py3" = "yes"; then
> - SWIG_PY_OPTS="-python -py3"
> + SWIG_PY_OPTS="-python -py3 -DPY3"
> else
> SWIG_PY_OPTS="-python -classic"
> fi
>
We shouldn't define a new preprocessor value here, but rather make use of
the existing IS_PY3 value defined by py3c.
diff --git a/subversion/bindings/swig/core.i
> b/subversion/bindings/swig/core.i
> index e5a234c5e2..c309d48070 100644
> --- a/subversion/bindings/swig/core.i
> +++ b/subversion/bindings/swig/core.i
>
[snip]
@@ -442,16 +442,18 @@
> */
> #ifdef SWIGPYTHON
> %typemap(in) (const char *data, apr_size_t *len) ($*2_type temp) {
> - if (!PyStr_Check($input)) {
> + char *tmpdata;
> + Py_ssize_t length;
> + if (!PyBytes_Check($input)) {
> PyErr_SetString(PyExc_TypeError,
> - "expecting a string for the buffer");
> + "expecting a bytes object for the buffer");
> SWIG_fail;
> }
> - /* Explicitly cast away const from PyStr_AsUTF8AndSize (in Python 3).
> The
> - swig generated argument ($1) here will be "char *", but since its
> only
> - use is to pass directly into the "const char *" argument of the
> wrapped
> - function, this is safe to do. */
> - $1 = (char *)PyStr_AsUTF8AndSize($input, &temp);
>
> + if (PyBytes_AsStringAndSize($input, &tmpdata, &length) == -1) {
>
Why not just use "&$1" here instead of introducing a new temp variable?
+ SWIG_fail;
> + }
> + temp = ($*2_type)length;
> + $1 = tmpdata;
> $2 = ($2_ltype)&temp;
> }
> #endif
>
@@ -504,8 +506,8 @@
> SWIG_fail;
> }
>
> - if (PyStr_Check($input)) {
> - const char *value = PyStr_AsString($input);
> + if (PyBytes_Check($input)) {
> + const char *value = PyBytes_AsString($input);
> $1 = apr_pstrdup(_global_pool, value);
> }
> else if (PyLong_Check($input)) {
>
Since this function does a whole list of checks and conversions, perhaps it
makes sense to support Str here in
addition to bytes? Of course in Py2 the "PyStr_Check()" would effectively
be performed twice if we did that, but
I don't expect that to be a tremendous impact, and with the "cost" of an
addition "IS_PY3" preprocessor check,
that could be eliminated from the Py2 build.
> diff --git a/subversion/bindings/swig/include/svn_global.swg
> b/subversion/bindings/swig/include/svn_global.swg
> index 191bbdd531..f7364be28f 100644
> --- a/subversion/bindings/swig/include/svn_global.swg
> +++ b/subversion/bindings/swig/include/svn_global.swg
> @@ -31,6 +31,12 @@
> #define SVN_DEPRECATED
> #endif
>
> +#ifdef SWIGPYTHON
> +%begin %{
> +#define SWIG_PYTHON_STRICT_BYTE_CHAR
> +%}
> +#endif
> +
> %include typemaps.i
> %include constraints.i
> %include exception.i
> @@ -136,9 +142,15 @@ static PyObject * _global_py_pool = NULL;
> /* Python format specifiers. Use Python instead of SWIG to parse
> these basic types, because Python reports better error messages
> (with correct argument numbers). */
> +#if defined(PY3)
>
This is where we'd want to use py3c's "IS_PY3" value.
> +%typemap (in, parse="y")
> + char *, char const *, char * const, char const * const "";
> +#else
> %typemap (in, parse="s")
> char *, char const *, char * const, char const * const "";
> +#endif
> %typemap (in, parse="c") char "";
> +
> %typemap (in, fragment=SWIG_As_frag(long)) long
> {
> $1 = ($1_ltype)SWIG_As(long)($input);
>
[snip]
> diff --git a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> index 2301d66770..44ec8662b7 100644
> --- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> +++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
> @@ -547,23 +547,23 @@ static char *make_string_from_ob(PyObject *ob,
> apr_pool_t *pool)
>
Probably a reasonable place to support either Bytes or Str.
{
> if (ob == Py_None)
> return NULL;
> - if (! PyStr_Check(ob))
> + if (! PyBytes_Check(ob))
> {
> - PyErr_SetString(PyExc_TypeError, "not a string");
> + PyErr_SetString(PyExc_TypeError, "not a bytes object");
> return NULL;
> }
> - return apr_pstrdup(pool, PyStr_AsString(ob));
> + return apr_pstrdup(pool, PyBytes_AsString(ob));
> }
> static svn_string_t *make_svn_string_from_ob(PyObject *ob, apr_pool_t
> *pool)
> {
> if (ob == Py_None)
> return NULL;
> - if (! PyStr_Check(ob))
> + if (! PyBytes_Check(ob))
> {
> - PyErr_SetString(PyExc_TypeError, "not a string");
> + PyErr_SetString(PyExc_TypeError, "not a bytes object");
> return NULL;
> }
> - return svn_string_create(PyStr_AsString(ob), pool);
> + return svn_string_create(PyBytes_AsString(ob), pool);
> }
>
>
> @@ -598,7 +598,7 @@ static PyObject *convert_hash(apr_hash_t *hash,
> return NULL;
> }
> /* ### gotta cast this thing cuz Python doesn't use "const" */
> - if (PyDict_SetItemString(dict, (char *)key, value) == -1)
> + if (PyDict_SetItem(dict, PyBytes_FromString((char *)key), value)
> == -1)
> {
> Py_DECREF(value);
> Py_DECREF(dict);
> @@ -623,10 +623,10 @@ static PyObject *convert_svn_string_t(void *value,
> void *ctx,
>
> const svn_string_t *s = value;
>
> - return PyStr_FromStringAndSize(s->data, s->len);
> + return PyBytes_FromStringAndSize(s->data, s->len);
> }
>
> -/* Convert a C string into a Python String object (or a reference to
> +/* Convert a C string into a Python Bytes object (or a reference to
> Py_None if CSTRING is NULL). */
> static PyObject *cstring_to_pystring(const char *cstring)
>
Doesn't returning PyBytes here make this function incorrectly named now? If
this were renamed to
"cstring_to_pybytes" it would be better, since that also matches the
Python2.7 idiom of bytes being alias of str.
And this is a static function, so we are safe to change it from an ABI
standpoint.
> {
> @@ -636,7 +636,7 @@ static PyObject *cstring_to_pystring(const char
> *cstring)
> Py_INCREF(Py_None);
> return retval;
> }
> - return PyStr_FromString(cstring);
> + return PyBytes_FromString(cstring);
> }
>
> static PyObject *convert_svn_client_commit_item3_t(void *value, void *ctx)
>
[snip]
@@ -1321,7 +1321,7 @@ svn_swig_py_unwrap_string(PyObject *source,
> void *baton)
>
Requiring "source" to be a Bytes object, makes this function name be
incorrect, since giving it a string will
cause an error. Probably a reasonable place to support both Bytes/str.
This is an exported function, so changing
the name isn't a good option.
{
> const char **ptr_dest = destination;
> - *ptr_dest = PyStr_AsString(source);
> + *ptr_dest = PyBytes_AsString(source);
>
> if (*ptr_dest != NULL)
> return 0;
> @@ -1440,7 +1440,7 @@ PyObject *svn_swig_py_array_to_list(const
> apr_array_header_t *array)
> for (i = 0; i < array->nelts; ++i)
> {
> PyObject *ob =
> - PyStr_FromString(APR_ARRAY_IDX(array, i, const char *));
> + PyBytes_FromString(APR_ARRAY_IDX(array, i, const char *));
> if (ob == NULL)
> goto error;
> PyList_SET_ITEM(list, i, ob);
> @@ -1748,7 +1748,8 @@ static svn_error_t *delete_entry(const char *path,
>
> /* ### python doesn't have 'const' on the method name and format */
> if ((result = PyObject_CallMethod(ib->editor, (char *)"delete_entry",
> - (char *)"slOO&", path, revision,
> ib->baton,
> + (char *)SVN_SWIG_BYTES_FMT "lOO&",
> + path, revision, ib->baton,
> make_ob_pool, pool)) == NULL)
> {
> err = callback_exception_error();
> @@ -1779,7 +1780,11 @@ static svn_error_t *add_directory(const char *path,
>
> /* ### python doesn't have 'const' on the method name and format */
> if ((result = PyObject_CallMethod(ib->editor, (char *)"add_directory",
> +#if PY_VERSION_HEX >= 0x03000000
> + (char *)"yOylO&", path, ib->baton,
> +#else
> (char *)"sOslO&", path, ib->baton,
> +#endif
>
We definitly want to use the existing "IS_PY3" preprocesser value here. And
yes I think the
definition SVN_SWIG_BYTES_FMT should be changed to use IS_PY3 instead of
the version check as well
(which is the precedence I assume you are following). The same applies to
all the future usages
of PY_VERSION_HEX below as well.
I also think that using SVN_SWIG_BYTES_FMT here would better communicate
purpose. However, I recognize
that that will really chop up the format string, so the "#if #else" seems
reasonable enough. But perhaps we should *only*
include the format string the definition, just to make it more clear what
piece we are doing the conditional compile on?
copyfrom_path, copyfrom_revision,
> make_ob_pool, dir_pool)) == NULL)
> {
> @@ -1810,8 +1815,8 @@ static svn_error_t *open_directory(const char *path,
>
> /* ### python doesn't have 'const' on the method name and format */
> if ((result = PyObject_CallMethod(ib->editor, (char *)"open_directory",
> - (char *)"sOlO&", path, ib->baton,
> - base_revision,
> + (char *)SVN_SWIG_BYTES_FMT "OlO&",
> + path, ib->baton, base_revision,
> make_ob_pool, dir_pool)) == NULL)
> {
> err = callback_exception_error();
>
[snip]
> @@ -2508,10 +2542,10 @@ apr_file_t *svn_swig_py_make_file(PyObject
> *py_file,
> if (py_file == NULL || py_file == Py_None)
> return NULL;
>
>
Since this function is doing type checking between Bytes/File object
anyway, perhaps it'd
be a good place to also support Str?
- if (PyStr_Check(py_file))
> + if (PyBytes_Check(py_file))
> {
> /* input is a path -- just open an apr_file_t */
> - const char* fname = PyStr_AsString(py_file);
> + const char* fname = PyBytes_AsString(py_file);
> apr_err = apr_file_open(&apr_file, fname,
> APR_CREATE | APR_READ | APR_WRITE,
> APR_OS_DEFAULT, pool);
>
[snip]
> @@ -3053,16 +3098,16 @@ svn_error_t *svn_swig_py_get_commit_log_func(const
> char **log_msg,
> *log_msg = NULL;
> err = SVN_NO_ERROR;
> }
> - else if (PyStr_Check(result))
>
If we were going to special-case places that are clearly expected to be
string, this log_func place would
probably one of them.
+ else if (PyBytes_Check(result))
> {
> - *log_msg = apr_pstrdup(pool, PyStr_AsString(result));
> + *log_msg = apr_pstrdup(pool, PyBytes_AsString(result));
> Py_DECREF(result);
> err = SVN_NO_ERROR;
> }
> else
> {
> Py_DECREF(result);
> - err = callback_bad_return_error("Not a string");
> + err = callback_bad_return_error("Not a bytes object");
> }
>
> finished:
>
[snip]
> diff --git
> a/subversion/bindings/swig/python/tests/trac/versioncontrol/main.py
> b/subversion/bindings/swig/python/tests/trac/versioncontrol/main.py
> index 1b8a87c3f4..0d5d0618de 100644
> --- a/subversion/bindings/swig/python/tests/trac/versioncontrol/main.py
> +++ b/subversion/bindings/swig/python/tests/trac/versioncontrol/main.py
> @@ -154,12 +154,12 @@ class Node(object):
> Represents a directory or file in the repository.
> """
>
> - DIRECTORY = "dir"
> - FILE = "file"
> + DIRECTORY = b"dir"
> + FILE = b"file"
>
> def __init__(self, path, rev, kind):
> assert kind in (Node.DIRECTORY, Node.FILE), "Unknown node kind
> %s" % kind
> - self.path = str(path)
> + self.path = path
>
Dropping use of "str()" changes the Py2 interface since in the current
incarnation you could pass a number, or some object which
defines __str__ and it will get used correctly.So I think here we have to
do an isinstanceof check so that we don't try to run "str"
on a bytes object, but otherwise continuing to use str()
> self.rev = rev
> self.kind = kind
>
[snip]
diff --git
> a/subversion/bindings/swig/python/tests/trac/versioncontrol/svn_fs.py
> b/subversion/bindings/swig/python/tests/trac/versioncontrol/svn_fs.py
> index e0aef2df0d..fe92bb70b7 100644
> --- a/subversion/bindings/swig/python/tests/trac/versioncontrol/svn_fs.py
> +++ b/subversion/bindings/swig/python/tests/trac/versioncontrol/svn_fs.py
> @@ -55,9 +55,12 @@ import os.path
> import time
> import weakref
> import posixpath
> +import sys
>
> from svn import fs, repos, core, delta
>
> +IS_PY3 = sys.version_info[0] >= 3
> +
> _kindmap = {core.svn_node_dir: Node.DIRECTORY,
> core.svn_node_file: Node.FILE}
>
> @@ -290,7 +293,7 @@ class SubversionNode(Node):
> def __init__(self, path, rev, authz, scope, fs_ptr):
> self.authz = authz
> self.scope = scope
> - if scope != '/':
> + if scope != b'/':
> self.scoped_path = scope + path
> else:
> self.scoped_path = path
> @@ -300,7 +303,11 @@ class SubversionNode(Node):
> self.root = fs.revision_root(fs_ptr, rev)
> node_type = fs.check_path(self.root, self.scoped_path)
> if not node_type in _kindmap:
> - raise TracError("No node at %s in revision %s" % (path, rev))
> + if IS_PY3:
> + raise TracError("No node at %s in revision %s"
> + % (path.decode('UTF-8'), rev))
> + else:
> + raise TracError("No node at %s in revision %s" % (path,
> rev))
>
Instead of having an explicit IS_PY3 check here, we should just store a
unicode string in the exception object.
> self.created_rev = fs.node_created_rev(self.root,
> self.scoped_path)
> self.created_path = fs.node_created_path(self.root,
> self.scoped_path)
> # Note: 'created_path' differs from 'path' if the last change was
> a copy,
> @@ -323,7 +330,7 @@ class SubversionNode(Node):
> return
> entries = fs.dir_entries(self.root, self.scoped_path)
> for item in entries:
> - path = '/'.join((self.path, item))
> + path = b'/'.join((self.path, item))
> if not self.authz.has_permission(path):
> continue
> yield SubversionNode(path, self._requested_rev, self.authz,
> @@ -350,7 +357,7 @@ class SubversionNode(Node):
> def get_properties(self):
> props = fs.node_proplist(self.root, self.scoped_path)
> for name,value in core._as_list(props.items()):
> - props[name] = str(value) # Make sure the value is a proper
> string
> + props[name] = value
>
Dropping str() here changes the API of the Py2 interface, just like the
other str() drop mentioned above.
> return props
>
> def get_content_length(self):
>
>
[snip]
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 12/17/18 5:50 AM, Yasuhito FUTATSUKI wrote:
> On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
>> Ah yes, I suppose this also has bearing on the discussion in the other
>> thread relating
>> to returning property values. With that in mind, perhaps your suggestion
>> of effectively
>> defaulting to always being Bytes for char * , svn_string_t, and
>> svn_stringbuf_t unless there
>> is a specific circumstance not to makes the most sense as the general
>> principle.
>>
>> Yasuhito, I suppose that means we should probably tweak the typemaps to
>> follow this
>> principle. Have you already started down that path based on the properties
>> discussion?
>
> Yes, but there is almost nothing to output, so you don't need to care :)
> I've also read swig document again, especially about default typemaps,
> and also there is no result :)
Status update: I've done to make patch that default to map bytes
from char *, svn_string_t, and svn_stringbuf_t, for swig-py3@1849037.
buildi system change:
* build/ac-macros/swig.m4: add "-DPY3" option to SWIG_PY_OPTS for py3
build to define symbol to switch typemap between py2 and py3
typemap changes:
* subversion/bindings/swig/core.i
(%typemap(argout) (char *buffer, apr_size_t *len):
map buffer arg to Bytes insted of Str
(%typemap(in) (const char *data, apr_size_t *len) ($*2_type temp):
+ map data arg to Bytes instead of Str
+ passing Py_ssize_t variable as length argment of
PyBytes_AsStringAndSize for for explicit type conversion.
(%typemap(in) const void *value):
map value arg to Bytes instead of Str (for svn_auth_set_parameter())
* subversion/bindings/swig/include/svn_global.swg:
x define SWIG_PYTHON_STRICT_BYTE_CHAR symbol for swig context for
Python3
x switch "in" type typemap for char *, char const *, char * const,
and char const * const "", to use parse parameter "y" for py3 and
"s" for py2
* subversion/bindings/swig/include/svn_string.swg
(%typemap(argout) RET_STRING, %typemap(in) svn_stringbuf_t *,
%typemap(out) svn_stringbuf_t *, %typemap(in) const svn_string_t *,
%typemap(out) svn_string_t *, %typemap(argout) const char **OUTPUT):
map args to Bytes instead of Str
* subversion/bindings/swig/include/svn_swigcompat.swg
(%set_constant(name, value): use PyDict_SetItem and PyBytes_FromString
instead of PyDict_SetItemString, for SWIG <= 1.3.27 (not tested)
* subversion/bindings/swig/include/svn_types.swg
(%typemap(in) const char *MAY_BE_NULL): don't use "z" conversion
format and convert to Bytes if it is not NULL
(%typemap(in) (const char *PTR, apr_size_t LEN): use PyBytes_Check and
PyBytes_AsStringAndSize instead of PyStr_Check and PyStr_AsUTF8AndSize
* subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
(make_string_from_ob(), make_svn_string_from_ob(),
svn_swig_py_make_file(), svn_swig_py_get_commit_log_func()):
use PyBytes_Check and PyBytes_AsString instead of
PyStr_Check and PyStr_AsString
(convert_hash()): use Dict_SetItem and PyBytes_FromString instead of
DictItemString
(convert_svn_string_t(), svn_swig_py_proparray_to_dict(),
ra_callbacks_get_wc_prop()):
use PyBytes_AsStringAndSize instead of PyStr_AsUTF8AndSize
(cstring_to_pystring(), convert_string(),
svn_swig_py_propinheriteditemarray_to_dict(),
svn_swig_py_proparray_to_dict(),
svn_swig_py_locationhash_to_dict(),
svn_swig_py_c_strings_to_list(),
svn_swig_py_changed_path_hash_to_dict(),
svn_swig_py_changed_path2_hash_to_dict(),
svn_swig_py_unwrap_string(),
svn_swig_py_array_to_list()): use PyBytes_FromString instead of
PyStr_FromString
(delete_entry(), add_directory(), open_directory(), change_dir_prop(),
add_file(), open_file(), apply_textdelta(), change_file_prop(),
close_file(), parse_fn3_uuid_record(), parse_fn3_set_revision_property(),
parse_fn3_set_node_property(), parse_fn3_delete_node_property(),
svn_swig_py_notify_func(), svn_swig_py_status_func(),
svn_swig_py_delta_path_driver_cb_func(), svn_swig_py_status_func2(),
svn_swig_py_fs_lock_callback(), svn_swig_py_repos_authz_func(),
svn_swig_py_repos_history_func(), svn_swig_py_proplist_receiver2(),
svn_swig_py_log_receiver(), svn_swig_py_info_receiver_func(),
svn_swig_py_client_blame_receiver_func(),
svn_swig_py_changelist_receiver_func(),
svn_swig_py_auth_gnome_keyring_unlock_prompt_func(),
svn_swig_py_auth_simple_prompt_func(),
svn_swig_py_auth_username_prompt_func(),
svn_swig_py_auth_ssl_server_trust_prompt_func(),
svn_swig_py_auth_ssl_client_cert_prompt_func(),
svn_swig_py_auth_ssl_client_cert_pw_prompt_func(),
svn_swig_py_config_auth_walk_func(), ra_callbacks_get_wc_prop(),
ra_callbacks_push_or_set_wc_prop(), ra_callbacks_invalidate_wc_props(),
svn_swig_py_commit_callback(), svn_swig_py_ra_file_rev_handler_func(),
svn_swig_py_ra_lock_callback(), reporter_set_path(),
reporter_delete_path(), reporter_link_path(),
wc_diff_callbacks2_file_changed_or_added(),
wc_diff_callbacks2_file_deleted(), wc_diff_callbacks2_dir_added(),
wc_diff_callbacks2_dir_deleted(),
wc_diff_callbacks2_dir_props_changed(),
svn_swig_py_config_enumerator2(),
svn_swig_py_config_section_enumerator2()): use 's' for py2 and use 'y'
for py3 in argment format string
(ra_callbacks_push_or_set_wc_prop()): use PyBytes_FromStringAndSize
instead of PyStr_FromStringAndSize
(ra_callbacks_get_client_string()): use PyBytes_FromString instaed of
PyStr_AsString
* subversion/bindings/swig/svn_client.i
(%typemap(argout) apr_array_header_t **props):
use PyBytes_FromStringAndSize() instead of PyStr_FromStringAndSize
helper python codes changes:
* subversion/bindings/swig/python/svn/core.py
(svn_path_compare_paths): not to use cmp for py3
(Stream::read()): chunks is now list of bytes, not list of str
* subversion/bindings/swig/python/svn/fs.py (FileDiff::get_pipe()):
x make sure self.tempfile1 and self.self.tempfile2 are bytes
x pass bytes to header_encoding arg in _svndiff.file_output_unified4()
* subversion/bindings/swig/python/svn/repos.py (Callbacks::__doc__):
treat path as bytes in sample code
* subversion/bindings/swig/python/svn/repos.py
(ChangeCollector::_make_base_path()): treat path as bytes
(ChangeCollector::open_root(): path and basepath should be bytes in
dir_baton
(RevisionChangeCollector::_make_base_path()): treat path as bytes
test unit changes:
* subversion/bindings/swig/python/tests/auth.py: replace all str literals
into bytes literals except docstring and module name
* subversion/bindings/swig/python/tests/checksum.py
(ChecksumTestCases::test_checksum()): check if type of check_val is
bytes instead of str
* subversion/bindings/swig/python/tests/client.py:
x replace all str literals into bytes literals except mode arg of open(),
args to utils.Temper methods, docstrings, and module name
x (SubversionClientTestCase::test_uuid_from_url): check if return value
type client.uuid_from_url() is bytes instead of str
x (SubversionClientTestCase::test_uuid_from_path): check if return value
type client.uuid_from_path() is bytes instead of str
x (SubversionClientTestCase::test_merge_peg3()): open result file in
raw mode
x (SubversionClientTestCase::test_update4()): convert os.path.sep into
bytes if it is str in py3
* subversion/bindings/swig/python/tests/core.py:
x replace all str literals into bytes literals except exception messages,
args to utils.Temper methods, docstring and module name
x add new tests for svn_stream_*()
(new)(SubversionCoreTestCase::test_stream_from_stringbuf()):
test for svn_stream_stringbuf()
(new)(SubversionCoreTestCase::test_stream_read_full()):
test for svn_stream_read_full()
(new)(SubversionCoreTestCase::test_stream_read2()):
test for svn_stream_read2()
(new)(SubversionCoreTestCase::test_stream_write_exception()):
test for svn_stream_write() to raise exception if try to write unicode
(new)(SubversionCoreTestCase::test_stream_write()):
test for svn_stream_write()
(new)(SubversionCoreTestCase::test_stream_readline()):
test for svn_stream_readline()
* subversion/bindings/swig/python/tests/delta.py
(DeltaTestCase::estTxWindowHandler_stream_IF(),
DeltaTestCase::estTxWindowHandler_stream_IF()):
x use bytes literals to make stream by using
svn.core.svn_stream_from_stringbuf()
x use bytes file name to make stream by using
svn.core.svn_stream_from_aprfile2()
* subversion/bindings/swig/python/tests/fs.py
(SubversionFSTestCase::log_message_func(): log_msg_func3 callback now
expect bytes to return
(SubversionFSTestCase::setUp()): pass bytes path and url to
client.import2()
(SubversionFSTestCase::test_diff_repos_paths_internal(),
SubversionFSTestCase::test_diff_repos_paths_external()): pass bytes
path1 to fs.FileDiff::__init__()
* subversion/bindings/swig/python/tests/mergeinfo.py
(SubversionMergeinfoTestCase::TEXT_MERGEINFO1,
SubversionMergeinfoTestCase::TEXT_MERGEINFO2,
SubversionMergeinfoTestCase::MERGEINFO_SRC): use bytes instead of str
(SubversionMergeinfoTestCase::test_mergeinfo_get()):
x pass list of bytes for paths arg to repos.fs_get_mergeinfo()
x replace str in expected_mergeinfo into bytes
* subversion/bindings/swig/python/tests/ra.py
x replace all str literals into bytes literals except assersion message,
args to utils.Temper methods, docstring and module name
x (SubversionRepositoryAccessTestCase::test_get_file): compare file
content as bytes
* subversion/bindings/swig/python/tests/repository.py
x replace all str literals to pass into Subversion API or to compare
with their return values into bytes literals except exception message.
x (SubversionRepositoryTestCase:: test_dump_fs2()): treat dump and
feedback as bytes as they are, and compose expected_feedback as bytes
* subversion/bindings/swig/python/tests/utils.py
(Temper::alloc_empty_dir(),alloc_empty_repo) convert return value of
tempfile.mkdtemp() into bytes
(Temper::file_uri_for_path()): return uri as bytes
* subversion/bindings/swig/python/tests/wc.py
x replace all str literals to pass into Subversion API or to compare
with their return values into bytes literals except exception message.
x (SubversionWorkingCopyTestCase::test_get_adm_dir()): check type as
bytes
x (SubversionWorkingCopyTestCase::test_get_pristine_copy_path(),
test_diff_editor4()): open file as raw mode
x (SubversionWorkingCopyTestCase::test_commit()): use bytes literal
instead of encoded str
* subversion/bindings/swig/python/tests/trac/versioncontrol/main.py
(Node::DIRECTORY, Node::FILE): replace to bytes
(Node::__init__): not to convert path to str
(Node::get_name): treat self.path as bytes
(Changeset::ADD, Changeset::COPY, Changeset::DELETE, Changeset::EDIT,
Changeset::MOVE): replace to bytes
* subversion/bindings/swig/python/tests/trac/versioncontrol/svn_fs.py
(SubversionRepository::__init__(), get_oldest_rev(), youngest_rev(),
next_rev()): treat self.path and self.scope as bytes
(SubversionRepository::normalize_path(), get_node()): treat path as
bytes
(SubversionRepository::get_path_history()): replace 'unknown' Changeset
value to bytes
(SubversionRepository::get_deltas()): replace 'entry' arg as bytes for
repos.svn_repos_dir_delta()
(SubversionNode::__init__()):
x treat self.path and self.scope as bytes
x decode path into str for exception message on py3
(SubversionNode::get_entries()): treat self.path as bytes
(SubversionNode::get_properties()): property values are bytes, not str
(DiffChangeEditor::open_root()): return bytes value
* subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py
x replace all str literals to pass into Subversion API or to compare
with their return values into bytes literals except exception message.
x (REPOS_URL) build REPOS_URL as bytes
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 12/17/18 2:08 AM, Troy Curtis Jr wrote:
> On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
>
>> Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
>>> But there was one item I wanted to talk about related to the patch. I
>>> agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is
>> the
>>> right general path,
>>
>> Are you sure about this? Property values can be binary data (so not
>> representable as 'str') but are usually represented by «svn_string_t *»;
>> for example, svn_repos_parse_fns3_t::set_node_property().
>>
>
> Ah yes, I suppose this also has bearing on the discussion in the other
> thread relating
> to returning property values. With that in mind, perhaps your suggestion
> of effectively
> defaulting to always being Bytes for char * , svn_string_t, and
> svn_stringbuf_t unless there
> is a specific circumstance not to makes the most sense as the general
> principle.
To treat all those values as bytes in wrapper makes the wrapper is lower
level, keeping raw access like device drivers. On the other hand,
to make contexts, to make semantic differences in wrapper makes it more
higher level. So I think those are not the general principle but just a
policy of the wrapper design. I, as a consumer of the Python bindings,
prefer the latter if it is clear that what are bytes and what are str,
however I also agree with former if the distinction is too complex for
both of consumers and developers.
> Yasuhito, I suppose that means we should probably tweak the typemaps to
> follow this
> principle. Have you already started down that path based on the properties
> discussion?
Yes, but there is almost nothing to output, so you don't need to care :)
I've also read swig document again, especially about default typemaps,
and also there is no result :)
>>> but after the patch there is now a "svn_stringbuf_t
>>> *output" typemap that is still using "PyStr" instead of "PyBytes".
>>> Further, the only usage of "svn_stringbuf_t *output" is in
>>> "svn_fs_print_modules" and "svn_ra_print_modules", where the function
>>> parameter is actually an output parameter, so the appropriate typemap is
>>> probably "argout" instead of "in".
I think you are right. And svn_stringbuf_t * type don't have proxy class,
it is no mean to pass string prepend to the result. The consumer of those
APIs can write a code like (using application_pool):
modules += svn.fs.svn_fs_print_modules()
However, this seems to be not only the issue swig-py3 but also trunk.
Actually both of svn.fs.svn_fs_print_module('') and
svn.ra.svn_ra_print_module('') return None type, on FreeBSD package for
Subversion 1.8.8 and 1.11.0, without patch to tweak swig codes, so
it seems those API wrappers are broken.
>>> Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
>>> your patch and perhaps address the issue in the last paragraph then I can
>>> commit your patch to swig-py3.
I found exception error messages isn't unified for
('a bytes', 'a bytes object', 'a Bytes object'),
and nothing but these.
>>> I'll also start working more on the swig-py3
>>> branch to get it to the point that we can actually get this branch merged
>>> into trunk finally.
Oh, thank you!
Cheers,
--
Yasuhito FUTATSUKI
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Troy Curtis Jr <tr...@apache.org>.
On Sun, Dec 16, 2018 at 11:28 AM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
> > But there was one item I wanted to talk about related to the patch. I
> > agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is
> the
> > right general path,
>
> Are you sure about this? Property values can be binary data (so not
> representable as 'str') but are usually represented by «svn_string_t *»;
> for example, svn_repos_parse_fns3_t::set_node_property().
>
Ah yes, I suppose this also has bearing on the discussion in the other
thread relating
to returning property values. With that in mind, perhaps your suggestion
of effectively
defaulting to always being Bytes for char * , svn_string_t, and
svn_stringbuf_t unless there
is a specific circumstance not to makes the most sense as the general
principle.
Yasuhito, I suppose that means we should probably tweak the typemaps to
follow this
principle. Have you already started down that path based on the properties
discussion?
> > but after the patch there is now a "svn_stringbuf_t
> > *output" typemap that is still using "PyStr" instead of "PyBytes".
> > Further, the only usage of "svn_stringbuf_t *output" is in
> > "svn_fs_print_modules" and "svn_ra_print_modules", where the function
> > parameter is actually an output parameter, so the appropriate typemap is
> > probably "argout" instead of "in".
> >
> > Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
> > your patch and perhaps address the issue in the last paragraph then I can
> > commit your patch to swig-py3. I'll also start working more on the
> swig-py3
> > branch to get it to the point that we can actually get this branch merged
> > into trunk finally.
>
> Great. :-)
>
> Cheers,
>
> Daniel
>
Troy
Re: [swig-py3][patch] interfacing bytes object instead of str
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Troy Curtis Jr wrote on Sun, 16 Dec 2018 09:59 -0500:
> But there was one item I wanted to talk about related to the patch. I
> agree that "svn_stringbuf_t" => "bytes" and "svn_string_t" => "str" is the
> right general path,
Are you sure about this? Property values can be binary data (so not
representable as 'str') but are usually represented by «svn_string_t *»;
for example, svn_repos_parse_fns3_t::set_node_property().
> but after the patch there is now a "svn_stringbuf_t
> *output" typemap that is still using "PyStr" instead of "PyBytes".
> Further, the only usage of "svn_stringbuf_t *output" is in
> "svn_fs_print_modules" and "svn_ra_print_modules", where the function
> parameter is actually an output parameter, so the appropriate typemap is
> probably "argout" instead of "in".
>
> Otherwise, the patch LGTM. Yasuhito, if you want to review my changes to
> your patch and perhaps address the issue in the last paragraph then I can
> commit your patch to swig-py3. I'll also start working more on the swig-py3
> branch to get it to the point that we can actually get this branch merged
> into trunk finally.
Great. :-)
Cheers,
Daniel