You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2015/08/25 13:55:31 UTC
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
On 25.08.2015 13:49, brane@apache.org wrote:
> Author: brane
> Date: Tue Aug 25 11:49:09 2015
> New Revision: 1697654
>
> URL: http://svn.apache.org/r1697654
> Log:
> * branches/1.9.x/STATUS:
> - Approve r1693886.
> - Temporarily veto r1694481; the change looks broken.
[...]
> @@ -98,5 +84,22 @@ Candidate changes:
> Veto-blocked changes:
> =====================
>
> + * r1694481
> + Fix Unix build on systems without GPG agent.
> + Justification:
> + This is a user-reported issue.
> + Votes:
> + +1: stefan2, philip
> + -1: brane (You can't just remove a public API implementation,
> + even if it is deprecated. And the prototyps is still
> + right there in svn_auth.h)
> +
> Approved changes:
> =================
r1694481 (conditionally) removes the implementation of a public API,
whilst leaving the prototype in svn_auth.h untouched. This is a
violation of our ABI compatibility rules, and also a linking error
waiting to happen.
I suggest the correct fix is for the function to just do nothing if GPG
support is not available. Ideally it would return an error, but the
prototype doesn't let us do that.
For example:
--- ../trunk/subversion/libsvn_subr/deprecated.c (revision 1697651)
+++ ../trunk/subversion/libsvn_subr/deprecated.c (working copy)
@@ -1497,14 +1497,14 @@
#endif /* DARWIN */
#if !defined(WIN32)
-#ifdef SVN_HAVE_GPG_AGENT
void
svn_auth_get_gpg_agent_simple_provider(svn_auth_provider_object_t **provider,
apr_pool_t *pool)
{
+#ifdef SVN_HAVE_GPG_AGENT
svn_auth__get_gpg_agent_simple_provider(provider, pool);
-}
#endif /* SVN_HAVE_GPG_AGENT */
+}
#endif /* !WIN32 */
svn_error_t *
-- Brane
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 26, 2015 at 8:26 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 26.08.2015 07:34, Daniel Shahaf wrote:
> > Branko Čibej wrote on Wed, Aug 26, 2015 at 07:26:47 +0200:
> >> On 25.08.2015 23:12, Stefan Fuhrmann wrote:
> >>> On Tue, Aug 25, 2015 at 4:43 PM, Branko Čibej <br...@wandisco.com>
> wrote:
> >>>> Daniel suggested inserting a dummy handler if we don't have the GPG
> >>>> agent support. I think that may be the only reasonable solution for
> both
> >>>> trunk and 1.9.1 (or .x if we don't thing it's important enough for
> .1).
> >>>>
> >>>> The real effort here is double-checking that a dummy handler won't
> break
> >>>> credentials resolution.
> >>>>
> >>> I think just starting with a full copying the GPG agent handler and
> >>> making each call return "failed" should work. Didn't try it, though.
> >> I'll give this a go and hopefully come up with a test case, too.
> > I think you might be able to get away with:
> >
> > static svn_auth_provider_t dummy = {
> > .cred_kind = "dummy", NULL, NULL, NULL
> > };
>
> I just added a private function to return a dummy simple provider; see
> r1697824.
>
Thanks for taking care of that! Reviewed and approved.
-- Stefan^2.
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Branko Čibej <br...@wandisco.com>.
On 26.08.2015 07:34, Daniel Shahaf wrote:
> Branko Čibej wrote on Wed, Aug 26, 2015 at 07:26:47 +0200:
>> On 25.08.2015 23:12, Stefan Fuhrmann wrote:
>>> On Tue, Aug 25, 2015 at 4:43 PM, Branko Čibej <br...@wandisco.com> wrote:
>>>> Daniel suggested inserting a dummy handler if we don't have the GPG
>>>> agent support. I think that may be the only reasonable solution for both
>>>> trunk and 1.9.1 (or .x if we don't thing it's important enough for .1).
>>>>
>>>> The real effort here is double-checking that a dummy handler won't break
>>>> credentials resolution.
>>>>
>>> I think just starting with a full copying the GPG agent handler and
>>> making each call return "failed" should work. Didn't try it, though.
>> I'll give this a go and hopefully come up with a test case, too.
> I think you might be able to get away with:
>
> static svn_auth_provider_t dummy = {
> .cred_kind = "dummy", NULL, NULL, NULL
> };
I just added a private function to return a dummy simple provider; see
r1697824.
-- Brane
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Wed, Aug 26, 2015 at 07:26:47 +0200:
> On 25.08.2015 23:12, Stefan Fuhrmann wrote:
> > On Tue, Aug 25, 2015 at 4:43 PM, Branko Čibej <br...@wandisco.com> wrote:
> >> Daniel suggested inserting a dummy handler if we don't have the GPG
> >> agent support. I think that may be the only reasonable solution for both
> >> trunk and 1.9.1 (or .x if we don't thing it's important enough for .1).
> >>
> >> The real effort here is double-checking that a dummy handler won't break
> >> credentials resolution.
> >>
> > I think just starting with a full copying the GPG agent handler and
> > making each call return "failed" should work. Didn't try it, though.
>
> I'll give this a go and hopefully come up with a test case, too.
I think you might be able to get away with:
static svn_auth_provider_t dummy = {
.cred_kind = "dummy", NULL, NULL, NULL
};
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Branko Čibej <br...@wandisco.com>.
On 25.08.2015 23:12, Stefan Fuhrmann wrote:
> On Tue, Aug 25, 2015 at 4:43 PM, Branko Čibej <br...@wandisco.com> wrote:
>
>> On 25.08.2015 17:31, Stefan Fuhrmann wrote:
>>> On Tue, Aug 25, 2015 at 12:55 PM, Branko Čibej <br...@wandisco.com>
>> wrote:
>>>> On 25.08.2015 13:49, brane@apache.org wrote:
>>>>> Author: brane
>>>>> Date: Tue Aug 25 11:49:09 2015
>>>>> New Revision: 1697654
>>>>>
>>>>> URL: http://svn.apache.org/r1697654
>>>>> Log:
>>>>> * branches/1.9.x/STATUS:
>>>>> - Approve r1693886.
>>>>> - Temporarily veto r1694481; the change looks broken.
>>>> [...]
>>>>
>>>>> @@ -98,5 +84,22 @@ Candidate changes:
>>>>> Veto-blocked changes:
>>>>> =====================
>>>>>
>>>>> + * r1694481
>>>>> + Fix Unix build on systems without GPG agent.
>>>>> + Justification:
>>>>> + This is a user-reported issue.
>>>>> + Votes:
>>>>> + +1: stefan2, philip
>>>>> + -1: brane (You can't just remove a public API implementation,
>>>>> + even if it is deprecated. And the prototyps is still
>>>>> + right there in svn_auth.h)
>>>>> +
>>>>> Approved changes:
>>>>> =================
>>>> r1694481 (conditionally) removes the implementation of a public API,
>>>> whilst leaving the prototype in svn_auth.h untouched. This is a
>>>> violation of our ABI compatibility rules, and also a linking error
>>>> waiting to happen.
>>>>
>>> Except that the very problem is that
>>> svn_auth__get_gpg_agent_simple_provider
>>> is not implemented either if SVN_HAVE_GPG_AGENT
>>> is not defined. And that linker problem is the one being
>>> already reported and fixed by the patch.
>>>
>>> You are still right that we introduce another linker problem
>>> further down the road for some people that stumbled
>>> across the first one in the past. And not implementing
>>> the public API is a bad thing.
>>>
>>> So, I think we need to do some coding to fix this on /trunk.
>>> Question is whether we want to skip r1694481 as a stop-
>>> gap patch for 1.9.1 and enable people to build SVN again.
>>
>> Daniel suggested inserting a dummy handler if we don't have the GPG
>> agent support. I think that may be the only reasonable solution for both
>> trunk and 1.9.1 (or .x if we don't thing it's important enough for .1).
>>
>> The real effort here is double-checking that a dummy handler won't break
>> credentials resolution.
>>
> I think just starting with a full copying the GPG agent handler and
> making each call return "failed" should work. Didn't try it, though.
I'll give this a go and hopefully come up with a test case, too.
-- Brane
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Aug 25, 2015 at 4:43 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 25.08.2015 17:31, Stefan Fuhrmann wrote:
> > On Tue, Aug 25, 2015 at 12:55 PM, Branko Čibej <br...@wandisco.com>
> wrote:
> >
> >> On 25.08.2015 13:49, brane@apache.org wrote:
> >>> Author: brane
> >>> Date: Tue Aug 25 11:49:09 2015
> >>> New Revision: 1697654
> >>>
> >>> URL: http://svn.apache.org/r1697654
> >>> Log:
> >>> * branches/1.9.x/STATUS:
> >>> - Approve r1693886.
> >>> - Temporarily veto r1694481; the change looks broken.
> >> [...]
> >>
> >>> @@ -98,5 +84,22 @@ Candidate changes:
> >>> Veto-blocked changes:
> >>> =====================
> >>>
> >>> + * r1694481
> >>> + Fix Unix build on systems without GPG agent.
> >>> + Justification:
> >>> + This is a user-reported issue.
> >>> + Votes:
> >>> + +1: stefan2, philip
> >>> + -1: brane (You can't just remove a public API implementation,
> >>> + even if it is deprecated. And the prototyps is still
> >>> + right there in svn_auth.h)
> >>> +
> >>> Approved changes:
> >>> =================
> >> r1694481 (conditionally) removes the implementation of a public API,
> >> whilst leaving the prototype in svn_auth.h untouched. This is a
> >> violation of our ABI compatibility rules, and also a linking error
> >> waiting to happen.
> >>
> > Except that the very problem is that
> > svn_auth__get_gpg_agent_simple_provider
> > is not implemented either if SVN_HAVE_GPG_AGENT
> > is not defined. And that linker problem is the one being
> > already reported and fixed by the patch.
> >
> > You are still right that we introduce another linker problem
> > further down the road for some people that stumbled
> > across the first one in the past. And not implementing
> > the public API is a bad thing.
> >
> > So, I think we need to do some coding to fix this on /trunk.
> > Question is whether we want to skip r1694481 as a stop-
> > gap patch for 1.9.1 and enable people to build SVN again.
>
>
> Daniel suggested inserting a dummy handler if we don't have the GPG
> agent support. I think that may be the only reasonable solution for both
> trunk and 1.9.1 (or .x if we don't thing it's important enough for .1).
>
> The real effort here is double-checking that a dummy handler won't break
> credentials resolution.
>
I think just starting with a full copying the GPG agent handler and
making each call return "failed" should work. Didn't try it, though.
-- Stefan^2.
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Branko Čibej <br...@wandisco.com>.
On 25.08.2015 17:31, Stefan Fuhrmann wrote:
> On Tue, Aug 25, 2015 at 12:55 PM, Branko Čibej <br...@wandisco.com> wrote:
>
>> On 25.08.2015 13:49, brane@apache.org wrote:
>>> Author: brane
>>> Date: Tue Aug 25 11:49:09 2015
>>> New Revision: 1697654
>>>
>>> URL: http://svn.apache.org/r1697654
>>> Log:
>>> * branches/1.9.x/STATUS:
>>> - Approve r1693886.
>>> - Temporarily veto r1694481; the change looks broken.
>> [...]
>>
>>> @@ -98,5 +84,22 @@ Candidate changes:
>>> Veto-blocked changes:
>>> =====================
>>>
>>> + * r1694481
>>> + Fix Unix build on systems without GPG agent.
>>> + Justification:
>>> + This is a user-reported issue.
>>> + Votes:
>>> + +1: stefan2, philip
>>> + -1: brane (You can't just remove a public API implementation,
>>> + even if it is deprecated. And the prototyps is still
>>> + right there in svn_auth.h)
>>> +
>>> Approved changes:
>>> =================
>> r1694481 (conditionally) removes the implementation of a public API,
>> whilst leaving the prototype in svn_auth.h untouched. This is a
>> violation of our ABI compatibility rules, and also a linking error
>> waiting to happen.
>>
> Except that the very problem is that
> svn_auth__get_gpg_agent_simple_provider
> is not implemented either if SVN_HAVE_GPG_AGENT
> is not defined. And that linker problem is the one being
> already reported and fixed by the patch.
>
> You are still right that we introduce another linker problem
> further down the road for some people that stumbled
> across the first one in the past. And not implementing
> the public API is a bad thing.
>
> So, I think we need to do some coding to fix this on /trunk.
> Question is whether we want to skip r1694481 as a stop-
> gap patch for 1.9.1 and enable people to build SVN again.
Daniel suggested inserting a dummy handler if we don't have the GPG
agent support. I think that may be the only reasonable solution for both
trunk and 1.9.1 (or .x if we don't thing it's important enough for .1).
The real effort here is double-checking that a dummy handler won't break
credentials resolution.
-- Brane
Re: svn commit: r1697654 - /subversion/branches/1.9.x/STATUS
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Aug 25, 2015 at 12:55 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 25.08.2015 13:49, brane@apache.org wrote:
> > Author: brane
> > Date: Tue Aug 25 11:49:09 2015
> > New Revision: 1697654
> >
> > URL: http://svn.apache.org/r1697654
> > Log:
> > * branches/1.9.x/STATUS:
> > - Approve r1693886.
> > - Temporarily veto r1694481; the change looks broken.
>
> [...]
>
> > @@ -98,5 +84,22 @@ Candidate changes:
> > Veto-blocked changes:
> > =====================
> >
> > + * r1694481
> > + Fix Unix build on systems without GPG agent.
> > + Justification:
> > + This is a user-reported issue.
> > + Votes:
> > + +1: stefan2, philip
> > + -1: brane (You can't just remove a public API implementation,
> > + even if it is deprecated. And the prototyps is still
> > + right there in svn_auth.h)
> > +
> > Approved changes:
> > =================
>
> r1694481 (conditionally) removes the implementation of a public API,
> whilst leaving the prototype in svn_auth.h untouched. This is a
> violation of our ABI compatibility rules, and also a linking error
> waiting to happen.
>
Except that the very problem is that
svn_auth__get_gpg_agent_simple_provider
is not implemented either if SVN_HAVE_GPG_AGENT
is not defined. And that linker problem is the one being
already reported and fixed by the patch.
You are still right that we introduce another linker problem
further down the road for some people that stumbled
across the first one in the past. And not implementing
the public API is a bad thing.
So, I think we need to do some coding to fix this on /trunk.
Question is whether we want to skip r1694481 as a stop-
gap patch for 1.9.1 and enable people to build SVN again.
-- Stefan^2.