You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Shivani Poddar <sh...@gmail.com> on 2012/12/06 23:40:48 UTC
[PATCH] Missing Binding for svn_checksum_t
Hi,
I have generated the attached [PATCH] as a part of the contribution i
wanted to make towards the subversion's swig-py project as a part of the
OPW program.
Having chosen the Python Bindings Problem, i tried learning all the
prerequisites for me to become able to contribute even in the mildest of
substance.
Following a strong support from the mentors and co-mentors i however
managed to grasp many concepts (swig,typemapping,python-c api's,avn) in
the course of last week and am posting the typemap i have written to map
the function svn_checksum_t in /subversion/include/svn_checksum.h , the
typemaps i look to put into the
/subversion/bindings/swig/include/svn_checksum.swg file.
Log Message:
subversion/bindings/swig/include
(svn_checksum_t.swg) : Generated a typemap.
(svn_checksum_kind_t,svn_checksum_create,svn_checksum_clear): typemapped
these functions
Suggested by: Stefan Sperling <st...@elego.de>
Ben Reser <br...@fornix.brain.org>
Daniel Shahaf <~d...@apache>
I hope this PATCH is incorporated and i am able to contribute substantially
to the subversion community.
Regards,
Shivani Poddar
Sophomore,
International Institute of Information Technology, Hyderabad
Re: [PATCH] Missing Binding for svn_checksum_t
Posted by Shivani Poddar <sh...@gmail.com>.
On Fri, Dec 7, 2012 at 2:32 PM, Daniel Shahaf <da...@elego.de> wrote:
> > Index: svn_checksum_t.swg
> > ===================================================================
> > --- svn_checksum_t.swg (revision 0)
> > +++ svn_checksum_t.swg (revision 0)
> > @@ -0,0 +1,47 @@
> > +%module checksum/**
> > + * TYPE: svn_checksum_t.; functions typemapped svn_checksum_t ;
> svn_checksum_kind_t, svn_checksum_create and avn_checksum_clear
> > + * The input for the function svn_checksum_clear is of the Type
> svn_check_sum_t so by the typemap rules it maps to the typemap("in")
> defined here.
> > + */
> > +
> > +#ifdef SWIGPYTHON
> > +%typemap(in) svn_checksum_t * {
> > + /* check if the input is not a const unsigned char* digest */
> > + if(!PyBytes_Check($input)){
>
> If the input is a 'bytes' instance, where do you stow the 'kind' member?
>
I simple redirect it to the else statement in that case.
> > + PyErr_SetString(PyExc_TypeError, "Not a valid Bit string");
> > + /* return fail if it isnt*/
> > + SWIG_fail;
> > + }
> > + /*else , pass is as the required argument for the
> svn_checksum_Clear function*/
> > + $1 = svn_checksum_clear(PyBytes_FromString($input));
i beieve having returned the "kind" parameter here would have been a better
thing to do,
the "out" typemap should account for the returning of svn_error * ,
This line:
>
> - Passes a 'bytes' object to a constructor of 'bytes' object;
>
> - Passes a counted-length string to an API that expects a C string;
>
> - Passes a PyObject * to a Subversion API function taking an
> svn_checksum_t *;
>
> - Uses an svn_error_t * as the result of the typemap
>
yes i get your point, prolly covered this lag in subsequent functions ,
will have to retypemap this bit later..
Basically, you managed to condense four type mismatches into two
> function calls and one assignment. And each one of those is a potential
> segfault.
>
> I won't blame you for not noticing that your patch generated new
> compiler warnings --- the bindings build generates mountains of stderr
> spam --- but did you compile and run the code?
>
> About compiling the code, yes as far as my knowledge about building swg
goes (which i am sure is very premature); i did compile it and the
compiling atlease did not give me any errors that i wouldve gotten
concerened with then.
About running in the code i wanted to turn in svn_checksum_create and
svn_checksum_kind_t which i prioritized to this one.
> > +}
> > +#endif
>
> Cheers
>
> Daniel
>
Regards
Shivani
Re: [PATCH] Missing Binding for svn_checksum_t
Posted by Daniel Shahaf <da...@elego.de>.
> Index: svn_checksum_t.swg
> ===================================================================
> --- svn_checksum_t.swg (revision 0)
> +++ svn_checksum_t.swg (revision 0)
> @@ -0,0 +1,47 @@
> +%module checksum/**
> + * TYPE: svn_checksum_t.; functions typemapped svn_checksum_t ; svn_checksum_kind_t, svn_checksum_create and avn_checksum_clear
> + * The input for the function svn_checksum_clear is of the Type svn_check_sum_t so by the typemap rules it maps to the typemap("in") defined here.
> + */
> +
> +#ifdef SWIGPYTHON
> +%typemap(in) svn_checksum_t * {
> + /* check if the input is not a const unsigned char* digest */
> + if(!PyBytes_Check($input)){
If the input is a 'bytes' instance, where do you stow the 'kind' member?
> + PyErr_SetString(PyExc_TypeError, "Not a valid Bit string");
> + /* return fail if it isnt*/
> + SWIG_fail;
> + }
> + /*else , pass is as the required argument for the svn_checksum_Clear function*/
> + $1 = svn_checksum_clear(PyBytes_FromString($input));
This line:
- Passes a 'bytes' object to a constructor of 'bytes' object;
- Passes a counted-length string to an API that expects a C string;
- Passes a PyObject * to a Subversion API function taking an svn_checksum_t *;
- Uses an svn_error_t * as the result of the typemap
Basically, you managed to condense four type mismatches into two
function calls and one assignment. And each one of those is a potential
segfault.
I won't blame you for not noticing that your patch generated new
compiler warnings --- the bindings build generates mountains of stderr
spam --- but did you compile and run the code?
> +}
> +#endif
Cheers
Daniel
Re: [PATCH] Missing Binding for svn_checksum_t
Posted by Ben Reser <be...@reser.org>.
On Sat, Dec 8, 2012 at 5:46 PM, Shivani Poddar
<sh...@gmail.com> wrote:
> Based on the reviews for the earlier PATCH for the svn_checksum.h header's
> python binding i have incorporated the required changes and finally have
> shaped the earlier patch.
> Please find the new patch file attached.
>
>
> log message:
>
> *subversion/bindings/swig/core.i: pulled in header svn_checksum.h
>
> *subversion/bindings/swig/python/tests/:
> checksum.py : New file
> checksum.pyc: New file
> run_all.py: Included a test_suite for checksum.py
>
> Suggested by: breser, danielsh, stsp
> Review by : breser
Thanks for your contribution.
Committed in r1418830.
I made some minor changes. Please consider these changes for future
contributions. One thing I find very helpful is to read through my
diffs while I write my commit message and look for typos, formatting
errors, unnecessary changes, etc...
- Removed some unnecessary whitespace changes in core.i
- Fixed the formatting in run_all.py to preserve spaces after commas
and wrap at 80 columns.
- Made several changes in checksum.py
* Cleaned up the imports. weakref and libsvn.core wasn't used. No
reason to import svn.core twice.
* Use the constant for the kind argument to svn_checksum_create
rather than a hardcoded value.
* Shortened up the variable name for the return from
svn_checksum_crate to make it easier to fit in 80 columns.
* Wrap to 80 columns.
* Fix indentation on comments and put a space after #
* Fix a typo of svn_checksum_to_cstring_display (avn instead of svn)
* Add license header to file.
- Adjusted the log to fit our log message standards:
http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
* Space between * and filename.
* No space between filename and :
* Each file should have a separate * line.
* Mention function touched in run_all.py
* Don't mention generated files (checksum.pyc) that aren't committed.
* Multiple parties should be on separate lines for attribution lines.
* Reviewed by is redundant when I'm committing it (your inclusion
was fine since you didn't know I would be committing).
* Added Patch by line and note that I tweaked it slightly.
Suggestions for moving forward. The digest member of svn_checksum_t
is opaque to Python since unsigned char * isn't typemapped. You can
get at it with the display functions like your test does, but you
shouldn't have to.
I'd also recommend developing tests for all the checksum functions.
You'll find out what does and doesn't work along the way and then you
can start fixing them.
Re: [PATCH] Missing Binding for svn_checksum_t
Posted by Shivani Poddar <sh...@gmail.com>.
Based on the reviews for the earlier PATCH for the svn_checksum.h header's
python binding i have incorporated the required changes and finally have
shaped the earlier patch.
Please find the new patch file attached.
log message:
*subversion/bindings/swig/core.i: pulled in header svn_checksum.h
*subversion/bindings/swig/python/tests/:
checksum.py : New file
checksum.pyc: New file
run_all.py: Included a test_suite for checksum.py
Suggested by: breser, danielsh, stsp
Review by : breser
On Sat, Dec 8, 2012 at 2:13 AM, Shivani Poddar
<sh...@gmail.com>wrote:
>
>
> On Sat, Dec 8, 2012 at 1:51 AM, Ben Reser <be...@reser.org> wrote:
>
>> On Thu, Dec 6, 2012 at 2:40 PM, Shivani Poddar
>> <sh...@gmail.com> wrote:
>> > Log Message:
>> > subversion/bindings/swig/include
>> > (svn_checksum_t.swg) : Generated a typemap.
>> > (svn_checksum_kind_t,svn_checksum_create,svn_checksum_clear): typemapped
>> > these functions
>> >
>> > Suggested by: Stefan Sperling <st...@elego.de>
>> > Ben Reser <br...@fornix.brain.org>
>> > Daniel Shahaf <~d...@apache>
>>
>> In this case I'd say that the three people you're providing
>> attribution to here are all committers. Their attribution can be
>> shortened to just their usernames. Which are: stsp, breser, danielsh.
>>
>> > I hope this PATCH is incorporated and i am able to contribute
>> substantially
>> > to the subversion community.
>>
>> In addition to the things Daniel already brought up, some comments on
>> your patch.
>>
>> 1) The most critical piece of feedback I can give you here is that you
>> need to ensure that your patch is functional and achieves the stated
>> end. Which means at a minimum you need to write some Python code that
>> uses the interfaces you have exposed with the bindings. The Python
>> bindings have a test suite, I'd advise that you add code that
>> exercises the interface. This means that not only have you
>> demonstrated that it works to yourself but we are able to ensure that
>> it stays working.
>>
>> Yes i had a mental note to check this after Daniel mailed in. I will make
> sure i work on that.
>
>
>> 2) You've created a new checksum module. Why? As I tried to explain
>> on IRC we break the modules up by the Subversion libraries. There is
>> a somewhat out of date NOTES document in the subversion/bindings/swig
>> directory that explains this. You can find it online here:
>>
>> http://svn.apache.org/repos/asf/subversion/trunk/subversion/bindings/swig/NOTES
>>
>> In my opinion svn_checksum.h should be added to _core which is used
>> for things in libsvn_subr and that don't fit anywhere else.
>>
>> I tried running it by injecting my typemaps into a .i file which required
> the insertion of a module, i wasnt sure if this were the right way to do
> it, i could not get in any documentations citing the same directly, maybe
> there is a need for me to dig in more regarding this issue.
>
>
>> 3) You have two "%typemap(in) svn_checksum_t *" definitions. I don't
>> understand what you need this for. You should only need one typemap
>> for a given type and SWIG method ('in' is the SWIG method).
>>
>
>
> I should probably have written the later one only.I wanted to turn in
> both the typesmaps i have generated , the svn_checksum_clear being the
> first went in as it is while the svn_checksum_create came in later, i did
> not however anticipate that this would pose any problems since if the
> typemap did not find its required function in the first instance it would
> still keep looking and encounter the later case,This i think now was a
> faulty assumption to make.
>
>
>>
>> 4) The typemaps shouldn't need to call svn_checksum_create() or
>> svn_checksum_clear(). Typemaps just convert types between C and the
>> Binding Language. It's best to think of the typemap as code that is
>> injected into the wrapper that SWIG is building, where in the wrapper
>> it is injected depends on the method of the typemap. Refer to the
>> SWIG documentation I pointed you at on IRC.
>>
>> In this case maintaining the struct and providing accessor functions
>> to it would be the right way to go about this. Simply converting to a
>> string won't be very helpful since you'll be throwing away the kind of
>> checksum (something Daniel has already mentioned).
>>
>
> If we do not take in consideration the svn_checksum_clear i believe i have
> tried prserving the struct, since in create i am returning the integer
> which convers the kind, i havent passed it as $input later since it remains
> fixed throughout the function. Please point if my method of doing this in
> svn_checksum_create is faulty.
>
>>
>> 5) As I'd mentioned on IRC the major things you have to write a
>> typemap for is argouts and callbacks (there are some other minor
>> cases). There are svn_checksum_t's that are used as output arguments
>> so you do need to implement the argout. However, the struct and the
>> enum should just be handled automatically for you by SWIG in the other
>> cases. You may have to do some work to enable accessor functions for
>> the struct members, I'm not sure what's needed on the Python side for
>> that. Which leads me to ...
>>
>> 6) Taking a look at the checksum related functions and types I see
>> that the symbols aren't showing up to SWIG. So someone needs to pull
>> in those symbols into a SWIG module. That should be a simple matter
>> of adding the auto-generated .swg file for the header file to the
>> module.
>>
>> The following comments are nitpicky comments about your patch. Given
>> the above you need to majorly rework the patch in order for it to be
>> functional. However, I still mention these things since we do try to
>> maintain code quality and a functional patch with these sorts of
>> issues will likely receive these sorts of comments.
>>
>> 1) You start a comment on the same line at the end of the checksum
>> module definition. I would put this on a separate line.
>> 2) svn_checksum_clear is mistyped as avn_checksum_clear.
>> 3) Your patch file has just the filename, instead of the full path.
>> We typically run svn diff from the top level of the wc so as to make
>> your patch easier to apply.
>> 4) The log message is not in the proper format per the community
>> guide:
>> http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
>>
>> I'll be around this weekend so if you have any questions, please feel
>> free to ask. I'll do my best to help you get your patch in shape so
>> that it can be committed.
>>
>
>
> Thanks a lot for you feedback.
> Yes i would want to work on it and get it in shape this weekend , that
> would be great :)
>
> Regards,
> Shivani Poddar
>
--
Shivani Poddar,
Bachelors in Computer Sciences and MS in Exact Humanities, Sophomore
International Institute of Information Technology, Hyderabad
Re: [PATCH] Missing Binding for svn_checksum_t
Posted by Shivani Poddar <sh...@gmail.com>.
On Sat, Dec 8, 2012 at 1:51 AM, Ben Reser <be...@reser.org> wrote:
> On Thu, Dec 6, 2012 at 2:40 PM, Shivani Poddar
> <sh...@gmail.com> wrote:
> > Log Message:
> > subversion/bindings/swig/include
> > (svn_checksum_t.swg) : Generated a typemap.
> > (svn_checksum_kind_t,svn_checksum_create,svn_checksum_clear): typemapped
> > these functions
> >
> > Suggested by: Stefan Sperling <st...@elego.de>
> > Ben Reser <br...@fornix.brain.org>
> > Daniel Shahaf <~d...@apache>
>
> In this case I'd say that the three people you're providing
> attribution to here are all committers. Their attribution can be
> shortened to just their usernames. Which are: stsp, breser, danielsh.
>
> > I hope this PATCH is incorporated and i am able to contribute
> substantially
> > to the subversion community.
>
> In addition to the things Daniel already brought up, some comments on
> your patch.
>
> 1) The most critical piece of feedback I can give you here is that you
> need to ensure that your patch is functional and achieves the stated
> end. Which means at a minimum you need to write some Python code that
> uses the interfaces you have exposed with the bindings. The Python
> bindings have a test suite, I'd advise that you add code that
> exercises the interface. This means that not only have you
> demonstrated that it works to yourself but we are able to ensure that
> it stays working.
>
> Yes i had a mental note to check this after Daniel mailed in. I will make
sure i work on that.
> 2) You've created a new checksum module. Why? As I tried to explain
> on IRC we break the modules up by the Subversion libraries. There is
> a somewhat out of date NOTES document in the subversion/bindings/swig
> directory that explains this. You can find it online here:
>
> http://svn.apache.org/repos/asf/subversion/trunk/subversion/bindings/swig/NOTES
>
> In my opinion svn_checksum.h should be added to _core which is used
> for things in libsvn_subr and that don't fit anywhere else.
>
> I tried running it by injecting my typemaps into a .i file which required
the insertion of a module, i wasnt sure if this were the right way to do
it, i could not get in any documentations citing the same directly, maybe
there is a need for me to dig in more regarding this issue.
> 3) You have two "%typemap(in) svn_checksum_t *" definitions. I don't
> understand what you need this for. You should only need one typemap
> for a given type and SWIG method ('in' is the SWIG method).
>
I should probably have written the later one only.I wanted to turn in both
the typesmaps i have generated , the svn_checksum_clear being the first
went in as it is while the svn_checksum_create came in later, i did not
however anticipate that this would pose any problems since if the typemap
did not find its required function in the first instance it would still
keep looking and encounter the later case,This i think now was a faulty
assumption to make.
>
> 4) The typemaps shouldn't need to call svn_checksum_create() or
> svn_checksum_clear(). Typemaps just convert types between C and the
> Binding Language. It's best to think of the typemap as code that is
> injected into the wrapper that SWIG is building, where in the wrapper
> it is injected depends on the method of the typemap. Refer to the
> SWIG documentation I pointed you at on IRC.
>
> In this case maintaining the struct and providing accessor functions
> to it would be the right way to go about this. Simply converting to a
> string won't be very helpful since you'll be throwing away the kind of
> checksum (something Daniel has already mentioned).
>
If we do not take in consideration the svn_checksum_clear i believe i have
tried prserving the struct, since in create i am returning the integer
which convers the kind, i havent passed it as $input later since it remains
fixed throughout the function. Please point if my method of doing this in
svn_checksum_create is faulty.
>
> 5) As I'd mentioned on IRC the major things you have to write a
> typemap for is argouts and callbacks (there are some other minor
> cases). There are svn_checksum_t's that are used as output arguments
> so you do need to implement the argout. However, the struct and the
> enum should just be handled automatically for you by SWIG in the other
> cases. You may have to do some work to enable accessor functions for
> the struct members, I'm not sure what's needed on the Python side for
> that. Which leads me to ...
>
> 6) Taking a look at the checksum related functions and types I see
> that the symbols aren't showing up to SWIG. So someone needs to pull
> in those symbols into a SWIG module. That should be a simple matter
> of adding the auto-generated .swg file for the header file to the
> module.
>
> The following comments are nitpicky comments about your patch. Given
> the above you need to majorly rework the patch in order for it to be
> functional. However, I still mention these things since we do try to
> maintain code quality and a functional patch with these sorts of
> issues will likely receive these sorts of comments.
>
> 1) You start a comment on the same line at the end of the checksum
> module definition. I would put this on a separate line.
> 2) svn_checksum_clear is mistyped as avn_checksum_clear.
> 3) Your patch file has just the filename, instead of the full path.
> We typically run svn diff from the top level of the wc so as to make
> your patch easier to apply.
> 4) The log message is not in the proper format per the community
> guide:
> http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
>
> I'll be around this weekend so if you have any questions, please feel
> free to ask. I'll do my best to help you get your patch in shape so
> that it can be committed.
>
Thanks a lot for you feedback.
Yes i would want to work on it and get it in shape this weekend , that
would be great :)
Regards,
Shivani Poddar
Re: [PATCH] Missing Binding for svn_checksum_t
Posted by Ben Reser <be...@reser.org>.
On Thu, Dec 6, 2012 at 2:40 PM, Shivani Poddar
<sh...@gmail.com> wrote:
> Log Message:
> subversion/bindings/swig/include
> (svn_checksum_t.swg) : Generated a typemap.
> (svn_checksum_kind_t,svn_checksum_create,svn_checksum_clear): typemapped
> these functions
>
> Suggested by: Stefan Sperling <st...@elego.de>
> Ben Reser <br...@fornix.brain.org>
> Daniel Shahaf <~d...@apache>
In this case I'd say that the three people you're providing
attribution to here are all committers. Their attribution can be
shortened to just their usernames. Which are: stsp, breser, danielsh.
> I hope this PATCH is incorporated and i am able to contribute substantially
> to the subversion community.
In addition to the things Daniel already brought up, some comments on
your patch.
1) The most critical piece of feedback I can give you here is that you
need to ensure that your patch is functional and achieves the stated
end. Which means at a minimum you need to write some Python code that
uses the interfaces you have exposed with the bindings. The Python
bindings have a test suite, I'd advise that you add code that
exercises the interface. This means that not only have you
demonstrated that it works to yourself but we are able to ensure that
it stays working.
2) You've created a new checksum module. Why? As I tried to explain
on IRC we break the modules up by the Subversion libraries. There is
a somewhat out of date NOTES document in the subversion/bindings/swig
directory that explains this. You can find it online here:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/bindings/swig/NOTES
In my opinion svn_checksum.h should be added to _core which is used
for things in libsvn_subr and that don't fit anywhere else.
3) You have two "%typemap(in) svn_checksum_t *" definitions. I don't
understand what you need this for. You should only need one typemap
for a given type and SWIG method ('in' is the SWIG method).
4) The typemaps shouldn't need to call svn_checksum_create() or
svn_checksum_clear(). Typemaps just convert types between C and the
Binding Language. It's best to think of the typemap as code that is
injected into the wrapper that SWIG is building, where in the wrapper
it is injected depends on the method of the typemap. Refer to the
SWIG documentation I pointed you at on IRC.
In this case maintaining the struct and providing accessor functions
to it would be the right way to go about this. Simply converting to a
string won't be very helpful since you'll be throwing away the kind of
checksum (something Daniel has already mentioned).
5) As I'd mentioned on IRC the major things you have to write a
typemap for is argouts and callbacks (there are some other minor
cases). There are svn_checksum_t's that are used as output arguments
so you do need to implement the argout. However, the struct and the
enum should just be handled automatically for you by SWIG in the other
cases. You may have to do some work to enable accessor functions for
the struct members, I'm not sure what's needed on the Python side for
that. Which leads me to ...
6) Taking a look at the checksum related functions and types I see
that the symbols aren't showing up to SWIG. So someone needs to pull
in those symbols into a SWIG module. That should be a simple matter
of adding the auto-generated .swg file for the header file to the
module.
The following comments are nitpicky comments about your patch. Given
the above you need to majorly rework the patch in order for it to be
functional. However, I still mention these things since we do try to
maintain code quality and a functional patch with these sorts of
issues will likely receive these sorts of comments.
1) You start a comment on the same line at the end of the checksum
module definition. I would put this on a separate line.
2) svn_checksum_clear is mistyped as avn_checksum_clear.
3) Your patch file has just the filename, instead of the full path.
We typically run svn diff from the top level of the wc so as to make
your patch easier to apply.
4) The log message is not in the proper format per the community
guide: http://subversion.apache.org/docs/community-guide/conventions.html#log-messages
I'll be around this weekend so if you have any questions, please feel
free to ask. I'll do my best to help you get your patch in shape so
that it can be committed.