[PATCH] Support APR 1.4 for building on CentOS 7

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Support APR 1.4 for building on CentOS 7

Julian Foad-5
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

support-apr-1.4-v1.patch (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Branko Čibej
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Andrew Marlow
In reply to this post by Julian Foad-5
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 <[hidden email]> 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


--
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Julian Foad-5
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

support-apr-1.4-v3.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Nathan Hartman
On Thu, Sep 17, 2020 at 9:56 AM Julian Foad <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Julian Foad-5
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Julian Foad-5
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Branko Čibej
In reply to this post by Julian Foad-5
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Support APR 1.4 for building on CentOS 7

Daniel Shahaf-2
In reply to this post by Julian Foad-5
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