You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2026/01/06 17:48:10 UTC

Re: Guarding against SERF_VERSION_AT_LEAST(unreleased version) logic in libsvn_ra_serf

Den fre 12 dec. 2025 kl 19:43 skrev Evgeny Kotkov via dev <
dev@subversion.apache.org>:

> Hi all,
>
> I noticed that libsvn_ra_serf on trunk currently contains several blocks
> of code conditional on SERF_VERSION_AT_LEAST(1, 4, 0).  Since Serf 1.4.0
> is unreleased, this code relies on development snapshots rather than a
> stable API:
>
>   subversion/libsvn_ra_serf/eagain_bucket.c(69):
>       #if !SERF_VERSION_AT_LEAST(1, 4, 0)
>   subversion/libsvn_ra_serf/eagain_bucket.c(102):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0)
>   subversion/libsvn_ra_serf/sb_bucket.c(120):
>       #if !SERF_VERSION_AT_LEAST(1, 4, 0)
>   subversion/libsvn_ra_serf/sb_bucket.c(163):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0)
>   subversion/libsvn_ra_serf/serf.c(168):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>   subversion/libsvn_ra_serf/serf.c(255):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>   subversion/libsvn_ra_serf/serf.c(322):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>   subversion/libsvn_ra_serf/serf.c(334):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>   subversion/libsvn_ra_serf/serf.c(594):
>       #if defined(SVN_DEBUG) && !SERF_VERSION_AT_LEAST(1,4,0)
>   subversion/libsvn_ra_serf/update.c(619):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0)
>   subversion/libsvn_ra_serf/util.c(483):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && defined(SVN__SERF_TEST_HTTP2)
>   subversion/libsvn_ra_serf/util.c(560):
>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && defined(SVN__SERF_TEST_HTTP2)
>
> It seems that this is essentially a ticking timebomb.  The code encodes
> assumptions about a development version of Serf but will automatically
> activate if a user builds against an officially released Serf 1.4.0+.
>
> More specifically:
>
> - The code may not compile, depending on the final API state in Serf
> 1.4.0+.
>
> - Even if it compiles, assumptions about Serf's behavior may no longer
> hold.
>
> - Even if the assumptions hold, these code paths may still not work as
> intended,
>   because they are effectively hidden from our standard release testing and
>   buildbots.
>
> I think we should treat this as a compatibility issue.  I can think of the
> following ad-hoc fix:
>
> 1) Place such code under an additional guard (#ifdef
> SVN__SERF_EXPERIMENTAL)
>    that is never defined in production builds.
>
> 2) Backport this change to 1.14.x as well.
>
>
> Thanks,
> Evgeny Kotkov
>

This might be tangential to Evgeny's original suggestion (which was
resolved and committed in r1930808) but I'll steal the thread anyway.

The new APIs in Serf are now marked as @since 1.5 in the docstring[1].

Would it make sense to update the above to SERF_VERSION_AT_LEAST(1, 5, 0)?

Not that there will be a Serf 1.4 release at all [2] but it might cause
less confusion in the future.

I don't suggest any change to the SVN__SERF_EXPERIMENTAL flag - Evgeny's
arguments about the stability of the SERF 1.5 APIs still hold (although I
think Serf is nearing a 1.5 release).

Cheers,
Daniel

[1] https://lists.apache.org/thread/cnxhq0920st3ny6dt9c58og7oh7c1zsc
[2] https://lists.apache.org/thread/8d3b77gj35jfd646byhscgkb15886rfm

Re: Guarding against SERF_VERSION_AT_LEAST(unreleased version) logic in libsvn_ra_serf

Posted by Nathan Hartman <ha...@gmail.com>.

On Tue, Jan 6, 2026 at 12:48 PM Daniel Sahlberg <da...@gmail.com>
wrote:

> Den fre 12 dec. 2025 kl 19:43 skrev Evgeny Kotkov via dev <
> dev@subversion.apache.org>:
>
>> Hi all,
>>
>> I noticed that libsvn_ra_serf on trunk currently contains several blocks
>> of code conditional on SERF_VERSION_AT_LEAST(1, 4, 0).  Since Serf 1.4.0
>> is unreleased, this code relies on development snapshots rather than a
>> stable API:
>>
>>   subversion/libsvn_ra_serf/eagain_bucket.c(69):
>>       #if !SERF_VERSION_AT_LEAST(1, 4, 0)
>>   subversion/libsvn_ra_serf/eagain_bucket.c(102):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0)
>>   subversion/libsvn_ra_serf/sb_bucket.c(120):
>>       #if !SERF_VERSION_AT_LEAST(1, 4, 0)
>>   subversion/libsvn_ra_serf/sb_bucket.c(163):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0)
>>   subversion/libsvn_ra_serf/serf.c(168):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>>   subversion/libsvn_ra_serf/serf.c(255):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>>   subversion/libsvn_ra_serf/serf.c(322):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>>   subversion/libsvn_ra_serf/serf.c(334):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && !defined(SVN_SERF_NO_LOGGING)
>>   subversion/libsvn_ra_serf/serf.c(594):
>>       #if defined(SVN_DEBUG) && !SERF_VERSION_AT_LEAST(1,4,0)
>>   subversion/libsvn_ra_serf/update.c(619):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0)
>>   subversion/libsvn_ra_serf/util.c(483):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && defined(SVN__SERF_TEST_HTTP2)
>>   subversion/libsvn_ra_serf/util.c(560):
>>       #if SERF_VERSION_AT_LEAST(1, 4, 0) && defined(SVN__SERF_TEST_HTTP2)
>>
>> It seems that this is essentially a ticking timebomb.  The code encodes
>> assumptions about a development version of Serf but will automatically
>> activate if a user builds against an officially released Serf 1.4.0+.
>>
>> More specifically:
>>
>> - The code may not compile, depending on the final API state in Serf
>> 1.4.0+.
>>
>> - Even if it compiles, assumptions about Serf's behavior may no longer
>> hold.
>>
>> - Even if the assumptions hold, these code paths may still not work as
>> intended,
>>   because they are effectively hidden from our standard release testing
>> and
>>   buildbots.
>>
>> I think we should treat this as a compatibility issue.  I can think of the
>> following ad-hoc fix:
>>
>> 1) Place such code under an additional guard (#ifdef
>> SVN__SERF_EXPERIMENTAL)
>>    that is never defined in production builds.
>>
>> 2) Backport this change to 1.14.x as well.
>>
>>
>> Thanks,
>> Evgeny Kotkov
>>
>
> This might be tangential to Evgeny's original suggestion (which was
> resolved and committed in r1930808) but I'll steal the thread anyway.
>
> The new APIs in Serf are now marked as @since 1.5 in the docstring[1].
>
> Would it make sense to update the above to SERF_VERSION_AT_LEAST(1, 5, 0)?
>


It makes sense to me.


Not that there will be a Serf 1.4 release at all [2] but it might cause
> less confusion in the future.
>


Agreed.


I don't suggest any change to the SVN__SERF_EXPERIMENTAL flag - Evgeny's
> arguments about the stability of the SERF 1.5 APIs still hold (although I
> think Serf is nearing a 1.5 release).
>
> Cheers,
> Daniel
>
> [1] https://lists.apache.org/thread/cnxhq0920st3ny6dt9c58og7oh7c1zsc
> [2] https://lists.apache.org/thread/8d3b77gj35jfd646byhscgkb15886rfm
>
>
Thanks,
Nathan