Auth providers and problems with our packaging

[Ben Reser <be...@reser.org>]

Starting with 1.4 but particularly in 1.6 we started adding platform
specific authorization providers.  This is a good thing because it
allows us to use platform specific authorization stores.

However, there have been some unintended consequences of the way we
implemented these. Specifically we hid these APIs and constants
related to them in our header files behind conditionals.  This means
we don't have a consistent API.

The problem is that we generate SWIG files and ship them in our
tarballs.  Since the header files are conditional based on platform,
SWIG will possibly see interfaces that will not be available on the
users platform.  This is particularly noticeable when the tarballs are
produced on some platform other than the Mac and then someone tries to
build the SWIG bindings on the Mac.  E.G.

[[[
[breser@kong subversion-1.8.0]$ make swig-py
/bin/sh /Users/breser/subversion-1.8.0/libtool --tag=CC --silent
--mode=compile llvm-gcc-4.2 -pipe -g -O2    -DDARWIN
-DSIGPROCMASK_SETS_THREAD_MASK
-I/Users/breser/subversion-1.8.0/subversion
-I/Users/breser/subversion-1.8.0/subversion/include
-I/Users/breser/subversion-1.8.0/subversion/bindings/swig
-I/Users/breser/subversion-1.8.0/subversion/bindings/swig/include
-I/Users/breser/subversion-1.8.0/subversion/bindings/swig/proxy
-I/Users/breser/subversion-1.8.0/subversion/bindings/swig/proxy
-I/usr/include/apr-1  -I/usr/include/apr-1
-I/System/Library/Frameworks/Python.framework/Versions/2.7/include/python2.7
-I/Users/breser/subversion-1.8.0/subversion/bindings/swig/python/libsvn_swig_py
-prefer-pic -c -o subversion/bindings/swig/python/core.lo
subversion/bindings/swig/python/core.c
subversion/bindings/swig/python/core.c:3851: error: expected
declaration specifiers or '...' before
'svn_auth_gnome_keyring_unlock_prompt_func_t'
subversion/bindings/swig/python/core.c: In function
'svn_auth_set_gnome_keyring_unlock_prompt_func':
subversion/bindings/swig/python/core.c:3853: error:
'SVN_AUTH_PARAM_GNOME_KEYRING_UNLOCK_PROMPT_FUNC' undeclared (first
use in this function)
subversion/bindings/swig/python/core.c:3853: error: (Each undeclared
identifier is reported only once
subversion/bindings/swig/python/core.c:3853: error: for each function
it appears in.)
subversion/bindings/swig/python/core.c:3854: error: 'prompt_func'
undeclared (first use in this function)
subversion/bindings/swig/python/core.c:3855: error:
'SVN_AUTH_PARAM_GNOME_KEYRING_UNLOCK_PROMPT_BATON' undeclared (first
use in this function)
subversion/bindings/swig/python/core.c: In function
'_wrap_svn_auth_set_gnome_keyring_unlock_prompt_func':
subversion/bindings/swig/python/core.c:32568: error:
'svn_auth_gnome_keyring_unlock_prompt_func_t' undeclared (first use in
this function)
subversion/bindings/swig/python/core.c:32568: error: expected ';' before 'arg2'
subversion/bindings/swig/python/core.c:32581: error: 'arg2' undeclared
(first use in this function)
subversion/bindings/swig/python/core.c:32587: error: too many
arguments to function 'svn_auth_set_gnome_keyring_unlock_prompt_func'
make: *** [subversion/bindings/swig/python/core.lo] Error 1
]]]

When we first did this in 1.4 by adding the support for the Mac
keychain support this wasn't noticed because we probably never
generate packages from the Mac and if you generate the packages on
some other *nix then the pre-generated SWIG bindings simply lack the
keychain support on the Mac.  Had we produced packages from the Mac
then all the other *nix would have had seen a similar problem.

We can safely exclude APIs based on being on Windows since we do not
ship pre-generated SWIG bindings for Windows and we cannot produce
packages on Windows.  However, we cannot do this with any *nix
platform unless we want to be shipping broken pre-generated SWIG
files.

I propose we stop hiding APIs based on platform.  APIs that return an
svn_error_t should return a new NOT_IMPLEMENTED error code.  APIs that
do not (as is the case with the auth APIs in this case) should provide
some appropriate noop behavior.  In the case of
svn_auth_get_*_provider APIs should return a noop provider (which at
current we don't have) but shouldn't be hard to provide.

This will allow us to provide generated SWIG bindings that work across
the various Unix platforms.

Re: Auth providers and problems with our packaging

[Peter Samuelson <pe...@p12n.org>]

[Ben Reser]
> Actually now that I think about it a bit more I'm not sure that
> helps.  It'll probably just shift it to a link time error because the
> defines will be there so it'll build, SWIG will generate wrappers for
> them that depend on the functions in our libraries, which won't be
> available in the libraries they built because we exclude them on
> their platform.

Oh man.  What a horrible API.  All this time I had assumed that this
was a plugin system, such that the only bit of code that really had to
talk to libsvn_auth_* was a handful of functions in libsvn_subr.
Everyone else would call those with a standard interface.  Neither SWIG
nor anything else outside libsvn_subr would need to know anything about
which plugins had been compiled.

How did we get to a point where the SWIG-based bindings (and, by
implication, third-party scripts) actually have to know how to talk to
specific auth plugins?  Or, to ask it another way, how did
plugin-specific stuff ever end up in a public header?  I suppose it's
far too late to fix that now, but....

Re: Auth providers and problems with our packaging

[Ben Reser <be...@reser.org>]

On 6/14/13 5:20 AM, Ben Reser wrote:
> Actually now that I think about it a bit more I'm not sure that helps.
>  It'll probably just shift it to a link time error because the defines
> will be there so it'll build, SWIG will generate wrappers for them
> that depend on the functions in our libraries, which won't be
> available in the libraries they built because we exclude them on their
> platform.
> 
> We can work around that by adding logic to SWIG to specifically
> exclude these functions based on the same conditions as we have in our
> headers and build systems.  But then we've added yet another place to
> maintain this.
> 
> So it turns into hacks all the way down.
> 
> The alternative is to fix it so the error happens at runtime, which
> shifts that to the library user.  In these specific cases, simply
> providing a noop provider, hides the entire problem from even the
> library user.

So fixing this doesn't require this level of effort.  Daniel's original
suggestion actually would have worked fine for a couple of reasons.

1) The problem is actually caused by r1241554 and the various follow ups trying
to fix it.  This change added support for the callback for retrieving the
gnome-keyring passphrase from the user to unlock the keyring to the bindings.
Having the typedef even when the gnome keyring isn't built is no problem since
even if a client registers the callback it'll never be called and there's
really nothing specific to gnome-keyring about it.  So this typedef and the
supporting constants shouldn't have been excluded based on platform.

2) We already exclude the platform specific provider functions from the
bindings.  So the problem I was concerned about at link time doesn't happen.

They can still use platform specific providers they just need to use
svn_auth_get_platform_specific_provider() or
svn_auth_get_platform_specific_client_providers().

So the fix I'm about to commit simply moves the #if directive down so that the
typedef and macros are available regardless of platform.

We still could go to the effort of making all of the auth apis available
regardless of platform and providing noop implementations of the functions on
platforms without the support.  But we'd have to build the
libsvn_auth_gnome_keyring and libsvn_auth_kwallet libraries on all platforms
(though not linked against gnome or kwallet unless they were found/enabled).
We'd have to do this because we can't move the symbols between libraries
without breaking the ABI.  Anyone uses the
svn_auth_get_{gnome_keyring,kwallet)_*() functions has to be loading the
dynamic library and then doing a symbol search.

To that end I produced a patch (which I've attached for posterity's sake).  But
I don't see a reason to actually apply it.  The only benefit would be allowing
cross platform clients to use these apis without caring about the platform or
to allow libraries on clients to be replaced without making sure to match the
platform specific auth support that their original library had if they're using
it.  So until such a time that someone shows up with such a problem I'm
inclined to punt on this issue.

For that matter I intend to mark these get apis as deprecated by the
svn_auth_get_platform_specific_provider() or
svn_get_get_platform_specific_client_providers() apis.

Re: Auth providers and problems with our packaging

[Ben Reser <be...@reser.org>]

On Fri, Jun 14, 2013 at 2:09 PM, Ben Reser <be...@reser.org> wrote:
> Yes and I guess that's probably the easiest solution for this older
> APIs.  In 1.6. we introduced the platform specific APIs that make it
> easier to deal with runtime detection of platform specific auth
> runtimes.  Given that I guess going back and mucking with the older
> APIs doesn't seem necessary to deal with the runtime issues.

Actually now that I think about it a bit more I'm not sure that helps.
 It'll probably just shift it to a link time error because the defines
will be there so it'll build, SWIG will generate wrappers for them
that depend on the functions in our libraries, which won't be
available in the libraries they built because we exclude them on their
platform.

We can work around that by adding logic to SWIG to specifically
exclude these functions based on the same conditions as we have in our
headers and build systems.  But then we've added yet another place to
maintain this.

So it turns into hacks all the way down.

The alternative is to fix it so the error happens at runtime, which
shifts that to the library user.  In these specific cases, simply
providing a noop provider, hides the entire problem from even the
library user.

Re: Auth providers and problems with our packaging

[Daniel Shahaf <da...@elego.de>]

Ben Reser wrote on Fri, Jun 14, 2013 at 14:09:29 +0200:
> On Fri, Jun 14, 2013 at 1:30 PM, Daniel Shahaf <da...@elego.de> wrote:
> > Would it suffice to change the header file conditions from
> >
> > #if defined(DARWIN) || defined(DOXYGEN)
> >
> > to
> >
> > #if defined(DARWIN) || defined(DOXYGEN) || defined(SWIG)
> 
> Yes and I guess that's probably the easiest solution for this older
> APIs.  In 1.6. we introduced the platform specific APIs that make it
> easier to deal with runtime detection of platform specific auth
> runtimes.  Given that I guess going back and mucking with the older
> APIs doesn't seem necessary to deal with the runtime issues.
> 
> But in general outside of the existing auth cases we should avoid ever
> doing this again.  We should simply design our APIs so that any
> platform specific ones have some sort of not implemented return for
> platforms where they are missing.

+1.

Perhaps we should have an _is_supported() function as well, e.g., an
svn_auth_kwallet_is_supported() would return FALSE on Mac and Windows.

Re: Auth providers and problems with our packaging

[Ben Reser <be...@reser.org>]

On Fri, Jun 14, 2013 at 1:30 PM, Daniel Shahaf <da...@elego.de> wrote:
> Would it suffice to change the header file conditions from
>
> #if defined(DARWIN) || defined(DOXYGEN)
>
> to
>
> #if defined(DARWIN) || defined(DOXYGEN) || defined(SWIG)

Yes and I guess that's probably the easiest solution for this older
APIs.  In 1.6. we introduced the platform specific APIs that make it
easier to deal with runtime detection of platform specific auth
runtimes.  Given that I guess going back and mucking with the older
APIs doesn't seem necessary to deal with the runtime issues.

But in general outside of the existing auth cases we should avoid ever
doing this again.  We should simply design our APIs so that any
platform specific ones have some sort of not implemented return for
platforms where they are missing.

Re: Auth providers and problems with our packaging

[Daniel Shahaf <da...@elego.de>]

Ben Reser wrote on Fri, Jun 14, 2013 at 13:21:13 +0200:
> I propose we stop hiding APIs based on platform.  APIs that return an
> svn_error_t should return a new NOT_IMPLEMENTED error code.  APIs that
> do not (as is the case with the auth APIs in this case) should provide
> some appropriate noop behavior.  In the case of
> svn_auth_get_*_provider APIs should return a noop provider (which at
> current we don't have) but shouldn't be hard to provide.
> 
> This will allow us to provide generated SWIG bindings that work across
> the various Unix platforms.

Would it suffice to change the header file conditions from

#if defined(DARWIN) || defined(DOXYGEN)

to

#if defined(DARWIN) || defined(DOXYGEN) || defined(SWIG)

?