Python bindings build seems broken

[C. Michael Pilato-2]

I don't have the Gnome or KWallet keyring stuff configured in my Subversion
build, but that appears to not be preventing my Python SWIG bindings from
trying to link against such.  Shouldn't our headers files (which are what
SWIG is parsing, IIUC) be conditionally defining those interfaces based on
the presence of #defines like these (from svn_private_config.h):

   /* Is GNOME Keyring support enabled? */
   /* #undef SVN_HAVE_GNOME_KEYRING */

   /* Is Mac OS KeyChain support enabled? */
   /* #undef SVN_HAVE_KEYCHAIN_SERVICES */

   /* Is KWallet support enabled? */
   /* #undef SVN_HAVE_KWALLET */

?

--
C. Michael Pilato <cmpilato@...> | http://cmpilato.blogspot.com/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[David James]

On Wed, Oct 1, 2008 at 8:38 PM, C. Michael Pilato <cmpilato@...> wrote:

> I don't have the Gnome or KWallet keyring stuff configured in my Subversion
> build, but that appears to not be preventing my Python SWIG bindings from
> trying to link against such.  Shouldn't our headers files (which are what
> SWIG is parsing, IIUC) be conditionally defining those interfaces based on
> the presence of #defines like these (from svn_private_config.h):
>
>   /* Is GNOME Keyring support enabled? */
>   /* #undef SVN_HAVE_GNOME_KEYRING */
>
>   /* Is Mac OS KeyChain support enabled? */
>   /* #undef SVN_HAVE_KEYCHAIN_SERVICES */
>
>   /* Is KWallet support enabled? */
>   /* #undef SVN_HAVE_KWALLET */

Hi Mike,

This is a tricky issue. Since we ship the C files generated by SWIG in
the Subversion tarball, our generated files must be
platform-independent. Unfortunately, if we hide functions from SWIG,
our generated C files won't be platform-independent anymore.

I can see three ways to resolve this issue:
  1. Teach SWIG to add the necessary "#if" statements to the generated
C file, so that our generated C files continue to be platform
independent. This is the route that I took with the ctypes python
bindings.
  2. Give up on shipping platform-independent C files, and instead
just add the necessary #if statements to the header files. In this
case, we will also need to update our build scripts so that we don't
include the generated C files in our tarball anymore.
  3. Bypass the whole issue by just teaching SWIG to ignore all of our
platform-specific functions.

If you do add #if statements to the header files, please make sure
that all platform-specific functions are defined when CTYPESGEN macro
is present. This is necessary so that ctypesgen can generate
definitions for the platform-specific functions in a
platform-independent manner. For example:
   #if defined(SVN_HAVE_GNOME_KEYRING) || defined(DOXYGEN) || defined(CTYPESGEN)
  ...

See also http://svn.haxx.se/dev/archive-2008-09/1120.shtml

Cheers,

David

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[C. Michael Pilato]

David James wrote:

> On Wed, Oct 1, 2008 at 8:38 PM, C. Michael Pilato <cmpilato@...> wrote:
>> I don't have the Gnome or KWallet keyring stuff configured in my Subversion
>> build, but that appears to not be preventing my Python SWIG bindings from
>> trying to link against such.  Shouldn't our headers files (which are what
>> SWIG is parsing, IIUC) be conditionally defining those interfaces based on
>> the presence of #defines like these (from svn_private_config.h):
>>
>>   /* Is GNOME Keyring support enabled? */
>>   /* #undef SVN_HAVE_GNOME_KEYRING */
>>
>>   /* Is Mac OS KeyChain support enabled? */
>>   /* #undef SVN_HAVE_KEYCHAIN_SERVICES */
>>
>>   /* Is KWallet support enabled? */
>>   /* #undef SVN_HAVE_KWALLET */
>
> Hi Mike,
>
> This is a tricky issue. Since we ship the C files generated by SWIG in
> the Subversion tarball, our generated files must be
> platform-independent. Unfortunately, if we hide functions from SWIG,
> our generated C files won't be platform-independent anymore.
>
> I can see three ways to resolve this issue:
>   1. Teach SWIG to add the necessary "#if" statements to the generated
> C file, so that our generated C files continue to be platform
> independent. This is the route that I took with the ctypes python
> bindings.
>   2. Give up on shipping platform-independent C files, and instead
> just add the necessary #if statements to the header files. In this
> case, we will also need to update our build scripts so that we don't
> include the generated C files in our tarball anymore.
>   3. Bypass the whole issue by just teaching SWIG to ignore all of our
> platform-specific functions.
>
> If you do add #if statements to the header files, please make sure
> that all platform-specific functions are defined when CTYPESGEN macro
> is present. This is necessary so that ctypesgen can generate
> definitions for the platform-specific functions in a
> platform-independent manner. For example:
>    #if defined(SVN_HAVE_GNOME_KEYRING) || defined(DOXYGEN) || defined(CTYPESGEN)
>   ...

Is this another option:

  4.  Don't have platform-specific visibility for APIs in Subversion at all.
      Instead, move the #defines that toggle the availability down into the
      functions/structures themselves so that on platforms where the
      functionality is not available, a not-implemented error is thrown?

?
--
C. Michael Pilato <cmpilato@...>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

[David James]

On Thu, Oct 2, 2008 at 7:42 AM, C. Michael Pilato <cmpilato@...> wrote:

> David James wrote:
>> On Wed, Oct 1, 2008 at 8:38 PM, C. Michael Pilato <cmpilato@...> wrote:
>>> I don't have the Gnome or KWallet keyring stuff configured in my Subversion
>>> build, but that appears to not be preventing my Python SWIG bindings from
>>> trying to link against such.  Shouldn't our headers files (which are what
>>> SWIG is parsing, IIUC) be conditionally defining those interfaces based on
>>> the presence of #defines like these (from svn_private_config.h):
>>>
>>>   /* Is GNOME Keyring support enabled? */
>>>   /* #undef SVN_HAVE_GNOME_KEYRING */
>>>
>>>   /* Is Mac OS KeyChain support enabled? */
>>>   /* #undef SVN_HAVE_KEYCHAIN_SERVICES */
>>>
>>>   /* Is KWallet support enabled? */
>>>   /* #undef SVN_HAVE_KWALLET */
>>
>> Hi Mike,
>>
>> This is a tricky issue. Since we ship the C files generated by SWIG in
>> the Subversion tarball, our generated files must be
>> platform-independent. Unfortunately, if we hide functions from SWIG,
>> our generated C files won't be platform-independent anymore.
>>
>> I can see three ways to resolve this issue:
>>   1. Teach SWIG to add the necessary "#if" statements to the generated
>> C file, so that our generated C files continue to be platform
>> independent. This is the route that I took with the ctypes python
>> bindings.
>>   2. Give up on shipping platform-independent C files, and instead
>> just add the necessary #if statements to the header files. In this
>> case, we will also need to update our build scripts so that we don't
>> include the generated C files in our tarball anymore.
>>   3. Bypass the whole issue by just teaching SWIG to ignore all of our
>> platform-specific functions.
>>
>> If you do add #if statements to the header files, please make sure
>> that all platform-specific functions are defined when CTYPESGEN macro
>> is present. This is necessary so that ctypesgen can generate
>> definitions for the platform-specific functions in a
>> platform-independent manner. For example:
>>    #if defined(SVN_HAVE_GNOME_KEYRING) || defined(DOXYGEN) || defined(CTYPESGEN)
>>   ...
>
> Is this another option:
>
>  4.  Don't have platform-specific visibility for APIs in Subversion at all.
>      Instead, move the #defines that toggle the availability down into the
>      functions/structures themselves so that on platforms where the
>      functionality is not available, a not-implemented error is thrown?
>

+1. I like that idea! That would make things much easier from a SWIG
point of view.

Cheers,

David

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Jeremy Whitlock]

>> Is this another option:
>>
>>  4.  Don't have platform-specific visibility for APIs in Subversion at all.
>>      Instead, move the #defines that toggle the availability down into the
>>      functions/structures themselves so that on platforms where the
>>      functionality is not available, a not-implemented error is thrown?
>>
> +1. I like that idea! That would make things much easier from a SWIG
> point of view.

While working on making this a reality, I realize that most of the
functions affected by this return void.  One option would be to rev
the related functions, use abort() in the function being revved and
make the new function return an svn_error_t, which would use
SVN_ERR_ASSERT to return the error when being invoked on the wrong
platform.  (This is suggested in another thread:
http://www.nabble.com/-PATCH--svn_dso_initialize-can-*silently*-fail-to-create-lock-td18777052.html)
 But is this necessary?  Since the function signature isn't changing,
nor is the content being returned changing, do we need to go through
the revving process?  In this scenario, either you can call the
function or you can't.  Why should the consumers of these functions
have to care?

--
Take care,

Jeremy Whitlock
http://www.thoughtspark.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[C. Michael Pilato]

Jeremy Whitlock wrote:

>>> Is this another option:
>>>
>>>  4.  Don't have platform-specific visibility for APIs in Subversion at all.
>>>      Instead, move the #defines that toggle the availability down into the
>>>      functions/structures themselves so that on platforms where the
>>>      functionality is not available, a not-implemented error is thrown?
>>>
>> +1. I like that idea! That would make things much easier from a SWIG
>> point of view.
>
> While working on making this a reality, I realize that most of the
> functions affected by this return void.  One option would be to rev
> the related functions, use abort() in the function being revved and
> make the new function return an svn_error_t, which would use
> SVN_ERR_ASSERT to return the error when being invoked on the wrong
> platform.  (This is suggested in another thread:
> http://www.nabble.com/-PATCH--svn_dso_initialize-can-*silently*-fail-to-create-lock-td18777052.html)
>  But is this necessary?  Since the function signature isn't changing,
> nor is the content being returned changing, do we need to go through
> the revving process?  In this scenario, either you can call the
> function or you can't.  Why should the consumers of these functions
> have to care?

I've not thought fully through this, but if the function now returns an
svn_error_t *, and that error goes unhandled (because callers weren't
expecting one), that's a memory leak and, in certain debug modes, an abort()
as a result of such, right?

--
C. Michael Pilato <cmpilato@...>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

[Jeremy Whitlock]

> I've not thought fully through this, but if the function now returns an
> svn_error_t *, and that error goes unhandled (because callers weren't
> expecting one), that's a memory leak and, in certain debug modes, an abort()
> as a result of such, right?

That's what I'd think.  Basically, revving will be more work because
the consuming functions will need to also be updated to do error
handling without any real benefit, unless is protocol/convention.
Instead, I think the easiest way would be to move the #ifdefs into the
functions instead of around the function declarations/definitions and
then abort when being used on the wrong operating system.  Is this
alright or do we need to rev, which will also mean we need to update
the consuming functions?  Since the functions aren't changing their
signature or in some way working differently, revving might not be
necessary.  How would you'd gracefully recover from using a function
not designed for your OS anyways?

--
Take care,

Jeremy Whitlock
http://www.thoughtspark.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Daniel Shahaf-2]

What functions are considered here?  The gnome/kde/kwallet functions are
new in 1.6 so we can still tweak them without revving them.

Daniel

Jeremy Whitlock wrote on Wed, 8 Oct 2008 at 22:41 -0600:

> > I've not thought fully through this, but if the function now returns an
> > svn_error_t *, and that error goes unhandled (because callers weren't
> > expecting one), that's a memory leak and, in certain debug modes, an abort()
> > as a result of such, right?
>
> That's what I'd think.  Basically, revving will be more work because
> the consuming functions will need to also be updated to do error
> handling without any real benefit, unless is protocol/convention.
> Instead, I think the easiest way would be to move the #ifdefs into the
> functions instead of around the function declarations/definitions and
> then abort when being used on the wrong operating system.  Is this
> alright or do we need to rev, which will also mean we need to update
> the consuming functions?  Since the functions aren't changing their
> signature or in some way working differently, revving might not be
> necessary.  How would you'd gracefully recover from using a function
> not designed for your OS anyways?
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Jeremy Whitlock]

On Thu, Oct 9, 2008 at 1:00 AM, Daniel Shahaf <d.s@...> wrote:
> What functions are considered here?  The gnome/kde/kwallet functions are
> new in 1.6 so we can still tweak them without revving them.

Here is a list of specific functions that would need revving:

subversion/include/svn_auth.h:
  svn_auth_get_keychain_simple_provider
  svn_auth_get_keychain_ssl_client_cert_pw_provider
  svn_auth_get_windows_simple_provider
  svn_auth_get_windows_ssl_server_trust_provider

subversion/include/svn_client.h
  svn_client_get_windows_simple_provider (DEPRECATED)

Here is a list of gnome-keyring/kwallet functions that would need revving:

subversion/include/svn_auth.h:
  svn_auth_gnome_keyring_version
  svn_auth_get_gnome_keyring_simple_provider
  svn_auth_get_gnome_keyring_ssl_client_cert_pw_provider
  svn_auth_kwallet_version
  svn_auth_get_kwallet_simple_provider
  svn_auth_get_kwallet_ssl_client_cert_pw_provider

The only other platform specific stuff I found was for Win32 but they
were not part of the public API.  Here are the consumers of the above
functions:

subversion/bindings/javahl/native/SVNClient.cpp:
  get_auth_provider

subversion/libsvn_subr/cmdline.c:
  svn_cmdline_set_up_auth_baton

I think this is complete.  Honestly, the consumers of the functions
above handle the inclusion of the platform specific stuff so that you
should never end up with platform code for another system built in
with your system's code.  But...if we decide that we need to rev the
svn_auth.h and svn_client.h functions to return errors in the revved
version and abort in the deprecated version, we still at least need to
update the code in SVNClient.cpp and cmdline.c to handle the errors
even if there is no way to gracefully handle them so we do not end up
with a memory leak.

--
Take care,

Jeremy Whitlock
http://www.thoughtspark.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Daniel Shahaf-2]

Jeremy Whitlock wrote on Thu, 9 Oct 2008 at 11:07 -0600:

> On Thu, Oct 9, 2008 at 1:00 AM, Daniel Shahaf <d.s@...> wrote:
> > What functions are considered here?  The gnome/kde/kwallet functions are
> > new in 1.6 so we can still tweak them without revving them.
>
> Here is a list of specific functions that would need revving:
>
> subversion/include/svn_auth.h:
>   svn_auth_get_keychain_simple_provider
>   svn_auth_get_keychain_ssl_client_cert_pw_provider
>   svn_auth_get_windows_simple_provider
>   svn_auth_get_windows_ssl_server_trust_provider
>
> subversion/include/svn_client.h
>   svn_client_get_windows_simple_provider (DEPRECATED)
>
> Here is a list of gnome-keyring/kwallet functions that would need revving:
>
> subversion/include/svn_auth.h:
>   svn_auth_gnome_keyring_version
>   svn_auth_get_gnome_keyring_simple_provider
>   svn_auth_get_gnome_keyring_ssl_client_cert_pw_provider
>   svn_auth_kwallet_version
>   svn_auth_get_kwallet_simple_provider
>   svn_auth_get_kwallet_ssl_client_cert_pw_provider
>

Thanks for making the list.  All these functions returns pointers (either
directly or through a pointer-to-pointer parameter).  Have you considered
having them return NULL where they are not implemented?  Given that they
return void, I don't see another way to signal an error (without revving).

Daniel

> The only other platform specific stuff I found was for Win32 but they
> were not part of the public API.  Here are the consumers of the above
> functions:
>
> subversion/bindings/javahl/native/SVNClient.cpp:
>   get_auth_provider
>
> subversion/libsvn_subr/cmdline.c:
>   svn_cmdline_set_up_auth_baton
>
> I think this is complete.  Honestly, the consumers of the functions
> above handle the inclusion of the platform specific stuff so that you
> should never end up with platform code for another system built in
> with your system's code.  But...if we decide that we need to rev the
> svn_auth.h and svn_client.h functions to return errors in the revved
> version and abort in the deprecated version, we still at least need to
> update the code in SVNClient.cpp and cmdline.c to handle the errors
> even if there is no way to gracefully handle them so we do not end up
> with a memory leak.
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Jeremy Whitlock]

> Is this another option:
>
>  4.  Don't have platform-specific visibility for APIs in Subversion at all.
>      Instead, move the #defines that toggle the availability down into the
>      functions/structures themselves so that on platforms where the
>      functionality is not available, a not-implemented error is thrown?

While working on this approach, I have realized that this might not be
a viable option.  Basically, this would work for the Win32 and Darwin
APIs but since the gnome-keyring and kwallet function implementations
reside in their own respective library, on systems where
gnome-keyring/kwallet are not available,  you'd have symbols for the
functions but the library to actually perform the work, even if it is
just setting the auth provider to NULL, will not be there.  I think
the only way to get this working right now is to continue to use SWIG
ignore patterns for the necessary functions.  Thoughts?

--
Take care,

Jeremy Whitlock
http://www.thoughtspark.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Arfrever Frehtes Taifersar Arahesis-2]

2008-10-11 21:46:36 Jeremy Whitlock napisał(a):

> > Is this another option:
> >
> >  4.  Don't have platform-specific visibility for APIs in Subversion at all.
> >      Instead, move the #defines that toggle the availability down into the
> >      functions/structures themselves so that on platforms where the
> >      functionality is not available, a not-implemented error is thrown?
>
> While working on this approach, I have realized that this might not be
> a viable option.  Basically, this would work for the Win32 and Darwin
> APIs but since the gnome-keyring and kwallet function implementations
> reside in their own respective library, on systems where
> gnome-keyring/kwallet are not available,  you'd have symbols for the
> functions but the library to actually perform the work, even if it is
> just setting the auth provider to NULL, will not be there.  I think
> the only way to get this working right now is to continue to use SWIG
> ignore patterns for the necessary functions.  Thoughts?

+1.

--
Arfrever Frehtes Taifersar Arahesis

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Jeremy Whitlock]

As of r33610, this has been fixed.  r33610 makes SWIG conditionally
include platform-specific auth providers.  As I worked on this though,
I found two other problems with SWIG and the auth providers.

1) As the code stands now, you will *never* get keychain or windows
auth providers in the SWIG libraries.  While r33610 has code to
conditionally include OSX/Win32 auth providers, it has no impact on
the SWIG libs just yet.  The reason this happens is because of the way
their respective auth provider functions are declared.  Since each
declaration is within platform-specific "#if defined" blocks, SWIG
cannot see those functions and does not attempt to wrapper them.  My
next task is to make SWIG smart enough to get access to the OSX/Win32
auth provider function declarations.

2) Now that SWIG can conditionally wrapper platform-specific auth
providers, when you try to run the bindings, there is a failure at
runtime.  The reason for this is that there is no linking done to the
SWIG core library to use the functions in the respective library.
This should not be a problem for OSX/Win32 since their functions are
in libsvn_subr but for gnome-keyring/kwallet, this is an issue.  To
fix this, we need to link SWIG core against the gnome-keyring/kwallet
library appropriately.  I have mentioned this to Senthil and he is
looking into this.

Take care,

Jeremy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...