You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vlad Georgescu <vg...@gmail.com> on 2006/06/03 19:45:02 UTC
Re: [PATCH] Update the build system on *nix to support Cyrus SASL
On 5/31/06, David James <dj...@collab.net> wrote:
> Nice patch, Vlad! I have a few suggestions on how we can improve this patch:
> - If the user does not supply a --with-sasl option, we should look
> in the standard locations for the SASL library (e.g. /usr,
> /usr/local). If SASL is found, we should use it.
> - If the user supplies a --with-sasl option, but does not specify a
> directory, we should look in the standard locations (e.g. /usr,
> /usr/local).
> - If the user supplies a --with-sasl option, and SASL is not found,
> configure should abort with an error message.
>
> Also, I found a few tab characters in your patch. Please replace these
> characters with spaces.
>
> Other than these minor issues, your patch looks excellent. Keep up the
> good work -- I'm looking forward to the next version of your patch!
>
> Cheers,
>
> David
I attached a revised version of the M4 file. Let me know what you think.
I'd also like some feedback on the following idea:
I'm thinking of extracting the authentication-related functions from
libsvn_ra_svn/client.c, putting them in another file, and then
creating a new file with the same interface that does authentication
through SASL. I would then conditionally compile & link only one of
those files depending on whether or not SASL is present on the system.
Does something like this make sense?
Re: [PATCH] Update the build system on *nix to support Cyrus SASL
Posted by David James <dj...@collab.net>.
On 6/5/06, Julian Foad <ju...@btopenworld.com> wrote:
> Julian Foad wrote:
> > Why do this now, rather than when we add the code that uses it? I'm
> > probably missing something, but we don't normally link extra libraries
> > in just because we expect to use them in the future.
>
> I've now read the rest of the weekend's mail and seen that this has already
> been asked and responded to. Let it be, as a courtesy and encouragement to
> Vlad, but let's not make a habit of it.
Thanks for the courtesy. I won't make a habit of this. For now, we're
just planning on leaving this patch in trunk, but if it turns out that
SASL support doesn't show up as planned we'll yank the build system
support. See http://svn.haxx.se/dev/archive-2006-06/0165.shtml
In future it'd make more sense to wait until the functionality is
immediately useful before committing, because in that case you won't
have to worry about the possibility of yanking it.
For Serf, we said it was OK to commit incremental (and not-yet-useful)
patches to trunk, but I now understand that this is a special
exception and does not apply as a general principle.
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Update the build system on *nix to support Cyrus SASL
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> Why do this now, rather than when we add the code that uses it? I'm
> probably missing something, but we don't normally link extra libraries
> in just because we expect to use them in the future.
I've now read the rest of the weekend's mail and seen that this has already
been asked and responded to. Let it be, as a courtesy and encouragement to
Vlad, but let's not make a habit of it.
Thanks,
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Update the build system on *nix to support Cyrus SASL
Posted by Julian Foad <ju...@btopenworld.com>.
David James wrote:
> Looks good! I've committed your patch to svn trunk in r19923. I made a
> few tweaks to the log message before committing (see below).
>
> [[[
> Update the Unix build system to link Cyrus SASL into svnserve, if
> Cyrus SASL is available.
>
> NOTE: We don't actually use the Cyrus SASL library yet, but we are
> planning to do so.
Why do this now, rather than when we add the code that uses it? I'm probably
missing something, but we don't normally link extra libraries in just because
we expect to use them in the future.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Update the build system on *nix to support Cyrus SASL
Posted by David James <dj...@collab.net>.
On 6/3/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 5/31/06, David James <dj...@collab.net> wrote:
> > Nice patch, Vlad! I have a few suggestions on how we can improve this patch:
> > - If the user does not supply a --with-sasl option, we should look
> > in the standard locations for the SASL library (e.g. /usr,
> > /usr/local). If SASL is found, we should use it.
> > - If the user supplies a --with-sasl option, but does not specify a
> > directory, we should look in the standard locations (e.g. /usr,
> > /usr/local).
> > - If the user supplies a --with-sasl option, and SASL is not found,
> > configure should abort with an error message.
> > [...]
> I attached a revised version of the M4 file. Let me know what you think.
Looks good! I've committed your patch to svn trunk in r19923. I made a
few tweaks to the log message before committing (see below).
[[[
Update the Unix build system to link Cyrus SASL into svnserve, if
Cyrus SASL is available.
NOTE: We don't actually use the Cyrus SASL library yet, but we are
planning to do so.
Patch by: Vlad Georgescu <vg...@gmail.com>
Review by: me
* build/ac-macros/sasl.m4: New file.
* configure.in: Call SVN_LIB_SASL.
* Makefile.in
(SVN_SASL_LIBS): New AC_SUBSTed variables.
(INCLUDES): Add SVN_SASL_INCLUDES.
* aclocal.m4
Add build/ac-macros/sasl.m4 to the list of supplementary macros.
* build.conf
(sasl): New entry.
(libsvn_ra_svn, svnserve): Add sasl to the list of dependencies.
]]]
> I'd also like some feedback on the following idea:
> I'm thinking of extracting the authentication-related functions from
> libsvn_ra_svn/client.c, putting them in another file, and then
> creating a new file with the same interface that does authentication
> through SASL. I would then conditionally compile & link only one of
> those files depending on whether or not SASL is present on the system.
> Does something like this make sense?
That sounds reasonable to me, but I'm not particularly familiar with
svnserve at this point. Could you post your svnserve questions in a
separate thread so that they get more attention?
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org