You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@apache.org> on 2020/09/16 21:14:29 UTC
[PATCH] Support APR 1.4 for building on CentOS 7
Dear devs,
I propose a patch restoring support for APR 1.4 so that svn 1.14 can be
built and packaged on CentOS 7.
The WANdisco folks continue to package svn for a variety of target
systems. Packaging for CentOS 7 which has APR 1.4.8, they ran into the
problem that just before svn 1.14.0 we bumped the dependency requirement
to APR 1.5.0 and removed support for older APR versions.
Here's the change we made:
> r1874094 | jamessan | 2020-02-17 03:49:42
> Require at least version 1.5 of APR
>
> Since r1874057, the apr_pescape_shell() API is being used to escape filenames
> when invoking $SVN_EDITOR. This was added to APR in 1.5.0, released on
> 2013-11-13.
>
> * INSTALL
> (): Document new minimum APR version
>
> * build/generator/gen_win_dependencies.py
> (_find_apr): Bump minimal_apr_version to 1.5.0
>
> * configure.ac
> (APR_VER_REGEXES): Bump minor version in 1.x regex to 1.5
>
> * get-deps.sh:
> (APR_VERSION): Specify 1.5.0 as default version
>
> * subversion/include/private/svn_dep_compat.h
> (apr_time_from_msec): Removed, since it's provided by APR 1.4.0 or later
>
> * subversion/include/svn_types.h
> (apr_hash_this_key, apr_hash_this_key_len, apr_hash_this_val): Remove
> prototypes, since they're provide by APR 1.5.0 or later
>
> * subversion/libsvn_subr/iter.c
> (hash_do_baton): Remove condition requiring APR 1.4.0 to define this type
> (svn_iter_apr_hash): Remove pre-APR 1.4.0 code
> (apr_hash_this_key, apr_hash_this_key_len, apr_hash_this_val): Remove
> definitions, since they're provided by APR 1.5.0 or later
>
> * subversion/libsvn_subr/pool.c
> (apr_pool_create_unmanaged_ex): Remove conditional alias to
> apr_pool_create_core_ex, for APR versions < 1.3.3
The context is we'd recently started using apr_pescape_shell() in
r1874057, shortly afterwards changing to using apr_escape_shell()
instead, and those escaping functions are new in 1.5.0.
Ian at WANdisco told me:
> The dependencies we have issues with are apr and apr-util as 1.14 requires at least APR 1.5 and Red Hat ship 1.4.8 in EL7. I tried ignoring the version and using the old apr but it doesn't compile due to some missing symbols.
>
> Because of this if we want to ship any 1.14 build for EL7 we have to build our own (later) APR and APR-Util packages which in turn means libserf and apache httpd would need to link against these too.
They are looking for a more self-contained solution.
I propose adding support more or less as in the attached patch. It:
- reverts r1874094, thus restoring the support we had for APR 1.4
(and even 1.3 by the look of it), but we never had backward
compatibility support for apr_escape_shell();
- adds a local copy of the 'apr_escape_shell()' function, copied
from APR 1.7.0.
One adjustments is needed before this is ready to commit: I must make
the 'apr_escape_shell' part conditional on APR version. Note also that
the support for 'apr_escape_shell()' adds 4 files, as that's how the
code was laid out in APR. We could combine them but I preferred to copy
it as exactly as possible.
If this is an acceptable approach, I would propose it for backport to a
1.14.x point release.
WANdisco would prefer to package our upstream svn source release exactly
rather than patch it in their packaging, and that is why I am proposing
adding this support.
Of course I understand that APR pre-1.5 is pretty old and the argument
to not complicate the current code with too much backward compatibility
support code. So the question is whether this is too much or whether
supporting building on distros like CentOS 7 out of the box is the
better option.
- Julian
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Branko Čibej <br...@apache.org>.
On 23.09.2020 15:46, Julian Foad wrote:
> Nathan Hartman wrote:
>> Julian Foad wrote:
>>> I have made a smaller version (attached), restoring support just
>>> back to
>>> APR 1.4 (not 1.3), putting all the apr_escape code in one file (128
>>> lines long), omitting EBCDIC support. Does that feel better?
>>
>> In principle I am okay with including a private implementation to
>> give a better experience to our downstream users and packagers.
>>
>>> How's this version for everyone?
>>
>> The patch looks good to me. (I haven't applied or tested it yet but
>> I'll do so if/when it's nominated for backport.)
>>
>> Of course, it will be very helpful to WanDisco if they could help us
>> get a 1.14.1 out the door :-)
>
> I've had confirmation that this is sufficient to fix WANdisco's build
> on CentOS 7.
>
> There seems to be some general consensus for this, so I'll commit it
> and propose for backport, and we can take it to the review stage.
>
> Anyone else want to comment? Brane, you expressed concerns about the
> earlier, bigger version of this. How does this version sit with you?
Looks good, thanks!
-- Brane
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Wed, 23 Sep 2020 16:05 +0100:
> Committed in r1881958.
>
> BTW, how I tested this is by cleaning my build:
>
> (cd obj-dir && make local-clean)
>
> and then rebuilding, adding the following options to configure:
>
> --with-apr=$HOME/.local/apr-1.4.8
> --with-apr-util=$HOME/.local/apr-util-1.5.2
>
> with local builds of apr and apr-util in the stated places.
I know you know this, but it's not well documented so I'll mention it
again:
«make clean» undoes «make».
«make distclean» undoes «configure».
«make realclean» undoes «autogen.sh».
(Modulo bugs, at least. E.g., I think pkg-config files aren't
completely cleaned up when they should be.)
Cheers,
Daniel
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Julian Foad <ju...@apache.org>.
Johan Corveleyn wrote on 2020-12-17:
> I quickly wanted to point out that the backport-proposal of this
> change to 1.14.x is missing one vote, and that your own vote isn't
> "complete", because the proposal was later amended [...]
Thank you for the heads-up, Johan. Reviewed and approved.
--
- Julian
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Sep 23, 2020 at 5:05 PM Julian Foad <ju...@apache.org> wrote:
>
> Committed in r1881958.
>
> BTW, how I tested this is by cleaning my build:
>
> (cd obj-dir && make local-clean)
>
> and then rebuilding, adding the following options to configure:
>
> --with-apr=$HOME/.local/apr-1.4.8
> --with-apr-util=$HOME/.local/apr-util-1.5.2
>
> with local builds of apr and apr-util in the stated places.
>
> - Julian
Hi Julian,
I quickly wanted to point out that the backport-proposal of this
change to 1.14.x is missing one vote, and that your own vote isn't
"complete", because the proposal was later amended with r1882128 (to
silence a warning on OSX) -- so your own vote was annotated with
"(only r1881958)". So if you'd have some time, and could take another
look ... if you can "upgrade" your backport vote this would be enough
for approval I suppose.
(just skimming through STATUS to see what's close to the finish line ...)
--
Johan
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Julian Foad <ju...@apache.org>.
Committed in r1881958.
BTW, how I tested this is by cleaning my build:
(cd obj-dir && make local-clean)
and then rebuilding, adding the following options to configure:
--with-apr=$HOME/.local/apr-1.4.8
--with-apr-util=$HOME/.local/apr-util-1.5.2
with local builds of apr and apr-util in the stated places.
- Julian
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Julian Foad <ju...@apache.org>.
Nathan Hartman wrote:
> Julian Foad wrote:
>> I have made a smaller version (attached), restoring support just back to
>> APR 1.4 (not 1.3), putting all the apr_escape code in one file (128
>> lines long), omitting EBCDIC support. Does that feel better?
>
> In principle I am okay with including a private implementation to give a
> better experience to our downstream users and packagers.
>
>> How's this version for everyone?
>
> The patch looks good to me. (I haven't applied or tested it yet but I'll
> do so if/when it's nominated for backport.)
>
> Of course, it will be very helpful to WanDisco if they could help us get
> a 1.14.1 out the door :-)
I've had confirmation that this is sufficient to fix WANdisco's build on
CentOS 7.
There seems to be some general consensus for this, so I'll commit it and
propose for backport, and we can take it to the review stage.
Anyone else want to comment? Brane, you expressed concerns about the
earlier, bigger version of this. How does this version sit with you?
- Julian
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Sep 17, 2020 at 9:56 AM Julian Foad <ju...@apache.org> wrote:
>
> I have made a smaller version (attached), restoring support just back to
> APR 1.4 (not 1.3), putting all the apr_escape code in one file (128
> lines long), omitting EBCDIC support. Does that feel better?
In principle I am okay with including a private implementation to give a
better experience to our downstream users and packagers.
How's this version for everyone?
The patch looks good to me. (I haven't applied or tested it yet but I'll do
so if/when it's nominated for backport.)
Of course, it will be very helpful to WanDisco if they could help us get a
1.14.1 out the door :-)
Cheers,
Nathan
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Julian Foad <ju...@apache.org>.
Branko Čibej wrote:
> I'm not comfortable with copying whole swathes of APR into our code.
I have made a smaller version (attached), restoring support just back to
APR 1.4 (not 1.3), putting all the apr_escape code in one file (128
lines long), omitting EBCDIC support. Does that feel better?
> Adding escape_path to the editor invocations was a bug fix, and I'd
> argue that if someone wants to use an older APR, they can just live
> with that bug, too. [...]
That's a valid option but I feel it's a last resort. Having decided
this was our bug to fix, rather than one that we just inherited through
the dependency, then it would seem like a bad thing to provide a way to
build Subversion such that the bug that we declared "fixed" is not fixed
in that build configuration.
> [...] For the rest we'd still want to keep the APR >= 1.4 requirement and APR >= 1.5 as the recommended minimum, so for example, leave the get_deps.sh unchanged.
For the rest, this patch does exactly that.
Andrew Marlow wrote:
> sounds good to me. I've built recent versions of subversion on RHEL7 and
> it is a right pain in the neck, chasing those dependencies down to serf,
> especially since the last time I looked serf depended on python2 instead
> of python3. Your proposal gets my vote, FWIW.
Thanks.
How's this version for everyone?
- Julian
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Andrew Marlow <ma...@gmail.com>.
sounds good to me. I've built recent versions of subversion on RHEL7 and it
is a right pain in the neck, chasing those dependencies down to serf,
especially since the last time I looked serf depended on python2 instead of
python3. Your proposal gets my vote, FWIW.
On Wed, 16 Sep 2020 at 22:14, Julian Foad <ju...@apache.org> wrote:
> Dear devs,
>
> I propose a patch restoring support for APR 1.4 so that svn 1.14 can be
> built and packaged on CentOS 7.
>
> The WANdisco folks continue to package svn for a variety of target
> systems. Packaging for CentOS 7 which has APR 1.4.8, they ran into the
> problem that just before svn 1.14.0 we bumped the dependency requirement
> to APR 1.5.0 and removed support for older APR versions.
>
> Here's the change we made:
> > r1874094 | jamessan | 2020-02-17 03:49:42
> > Require at least version 1.5 of APR
> >
> > Since r1874057, the apr_pescape_shell() API is being used to escape
> filenames
> > when invoking $SVN_EDITOR. This was added to APR in 1.5.0, released on
> > 2013-11-13.
> >
> > * INSTALL
> > (): Document new minimum APR version
> >
> > * build/generator/gen_win_dependencies.py
> > (_find_apr): Bump minimal_apr_version to 1.5.0
> >
> > * configure.ac
> > (APR_VER_REGEXES): Bump minor version in 1.x regex to 1.5
> >
> > * get-deps.sh:
> > (APR_VERSION): Specify 1.5.0 as default version
> >
> > * subversion/include/private/svn_dep_compat.h
> > (apr_time_from_msec): Removed, since it's provided by APR 1.4.0 or
> later
> >
> > * subversion/include/svn_types.h
> > (apr_hash_this_key, apr_hash_this_key_len, apr_hash_this_val): Remove
> > prototypes, since they're provide by APR 1.5.0 or later
> >
> > * subversion/libsvn_subr/iter.c
> > (hash_do_baton): Remove condition requiring APR 1.4.0 to define this
> type
> > (svn_iter_apr_hash): Remove pre-APR 1.4.0 code
> > (apr_hash_this_key, apr_hash_this_key_len, apr_hash_this_val): Remove
> > definitions, since they're provided by APR 1.5.0 or later
> >
> > * subversion/libsvn_subr/pool.c
> > (apr_pool_create_unmanaged_ex): Remove conditional alias to
> > apr_pool_create_core_ex, for APR versions < 1.3.3
>
> The context is we'd recently started using apr_pescape_shell() in
> r1874057, shortly afterwards changing to using apr_escape_shell()
> instead, and those escaping functions are new in 1.5.0.
>
> Ian at WANdisco told me:
> > The dependencies we have issues with are apr and apr-util as 1.14
> requires at least APR 1.5 and Red Hat ship 1.4.8 in EL7. I tried ignoring
> the version and using the old apr but it doesn't compile due to some
> missing symbols.
> >
> > Because of this if we want to ship any 1.14 build for EL7 we have to
> build our own (later) APR and APR-Util packages which in turn means libserf
> and apache httpd would need to link against these too.
>
> They are looking for a more self-contained solution.
>
> I propose adding support more or less as in the attached patch. It:
>
> - reverts r1874094, thus restoring the support we had for APR 1.4
> (and even 1.3 by the look of it), but we never had backward
> compatibility support for apr_escape_shell();
>
> - adds a local copy of the 'apr_escape_shell()' function, copied
> from APR 1.7.0.
>
> One adjustments is needed before this is ready to commit: I must make
> the 'apr_escape_shell' part conditional on APR version. Note also that
> the support for 'apr_escape_shell()' adds 4 files, as that's how the
> code was laid out in APR. We could combine them but I preferred to copy
> it as exactly as possible.
>
> If this is an acceptable approach, I would propose it for backport to a
> 1.14.x point release.
>
> WANdisco would prefer to package our upstream svn source release exactly
> rather than patch it in their packaging, and that is why I am proposing
> adding this support.
>
> Of course I understand that APR pre-1.5 is pretty old and the argument
> to not complicate the current code with too much backward compatibility
> support code. So the question is whether this is too much or whether
> supporting building on distros like CentOS 7 out of the box is the
> better option.
>
> - Julian
>
--
Regards,
Andrew Marlow
http://www.andrewpetermarlow.co.uk
Re: [PATCH] Support APR 1.4 for building on CentOS 7
Posted by Branko Čibej <br...@apache.org>.
On 16.09.2020 23:14, Julian Foad wrote:
> Dear devs,
>
> I propose a patch restoring support for APR 1.4 so that svn 1.14 can
> be built and packaged on CentOS 7.
>
> The WANdisco folks continue to package svn for a variety of target
> systems. Packaging for CentOS 7 which has APR 1.4.8, they ran into
> the problem that just before svn 1.14.0 we bumped the dependency
> requirement to APR 1.5.0 and removed support for older APR versions.
>
> Here's the change we made:
>> r1874094 | jamessan | 2020-02-17 03:49:42
>> Require at least version 1.5 of APR
[...]
>>
>
> The context is we'd recently started using apr_pescape_shell() in
> r1874057, shortly afterwards changing to using apr_escape_shell()
> instead, and those escaping functions are new in 1.5.0.
>
> Ian at WANdisco told me:
>> The dependencies we have issues with are apr and apr-util as 1.14
>> requires at least APR 1.5 and Red Hat ship 1.4.8 in EL7. I tried
>> ignoring the version and using the old apr but it doesn't compile due
>> to some missing symbols.
>>
>> Because of this if we want to ship any 1.14 build for EL7 we have to
>> build our own (later) APR and APR-Util packages which in turn means
>> libserf and apache httpd would need to link against these too.
>
> They are looking for a more self-contained solution.
>
> I propose adding support more or less as in the attached patch. It:
>
> - reverts r1874094, thus restoring the support we had for APR 1.4
> (and even 1.3 by the look of it), but we never had backward
> compatibility support for apr_escape_shell();
IIRC we skipped APR 1.4, going from requiring 1.3 to 1.5.
> - adds a local copy of the 'apr_escape_shell()' function, copied
> from APR 1.7.0.
>
> One adjustments is needed before this is ready to commit: I must make
> the 'apr_escape_shell' part conditional on APR version. Note also
> that the support for 'apr_escape_shell()' adds 4 files, as that's how
> the code was laid out in APR. We could combine them but I preferred
> to copy it as exactly as possible.
>
> If this is an acceptable approach, I would propose it for backport to
> a 1.14.x point release.
>
> WANdisco would prefer to package our upstream svn source release
> exactly rather than patch it in their packaging, and that is why I am
> proposing adding this support.
>
> Of course I understand that APR pre-1.5 is pretty old and the argument
> to not complicate the current code with too much backward
> compatibility support code. So the question is whether this is too
> much or whether supporting building on distros like CentOS 7 out of
> the box is the better option.
I'm not comfortable with copying whole swathes of APR into our code.
Adding escape_path to the editor invocations was a bug fix, and I'd
argue that if someone wants to use an older APR, they can just live with
that bug, too.
So instead of copying a bunch of APR source into our code, I'd suggest
to add conditional code that invokes the editor without properly
escaping the paths (or whatever we did before that change). For the rest
we'd still want to keep the APR >= 1.4 requirement and APR >= 1.5 as the
recommended minimum, so for example, leave the get_deps.sh unchanged.
I do worry that would make the resulting patch even more complicated in
the end.
-- Brane