You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexandr Miloslavskiy <al...@syntevo.com> on 2021/07/27 21:38:34 UTC
[PATCH] Fix libsvn_ra_serf build with external serf
It seems that the problem has been there for years, see for example:
https://stackoverflow.com/questions/28663316
https://groups.google.com/g/subversion_users/c/CkbGGwFi6-0
I'm a committer in SVN, but I'm not very experienced with Linux and would
rather have this patch reviewed, thanks!
[[[
Fix libsvn_ra_serf build with external serf
Since `serf` transitioned to `SCons` build system in 2011, it no longer
generates `.la` files. At the same time, `libtool` only adds `-rpath`
when dependency is given as `.la`. As a result, if serf is not installed
in default path, `libsvn_ra_serf-1.so` doesn't know where to find its
`libserf-1` dependency.
When OS's `ld` linker defaults to `RPATH`, build still works, probably
because one of other dependencies introduce path to `serf`. On newer
systems where `ld` defaults to `RUNPATH`, build fails.
Fix this by inserting correct `-rpath` to find `libserf-1`.
* build/ac-macros/serf.m4
Workaround for libtool to append correct -rpath for serf dependency
]]]
Re: [PATCH] Fix libsvn_ra_serf build with external serf
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 17.08.2021 6:45, Daniel Shahaf wrote:
> Could you file an issue?
I decided to study it a bit more and found more problems, see the
newer mail I sent today. For the reasons described there, I decided
to abandon the patch: it only fixes half of the problem. While it's
already something, I'm out of steam to solve both halves.
Re: [PATCH] Fix libsvn_ra_serf build with external serf
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Alexandr Miloslavskiy wrote on Sat, Aug 14, 2021 at 23:20:26 +0300:
> 2) --with-zlib, that has the same problem. But a typical Linux has
> system zlib (on my Ubuntu, it's even needed to run `sudo` !) and
> in absence of RUNPATH, SVN secretly loads system zlib instead of
> the one specified in `--with-zlib`. I discovered this just today.
> I guess I'll leave it for someone else to fix.
Could you file an issue?
Re: [PATCH] Fix libsvn_ra_serf build with external serf
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 14.08.2021 15:13, Daniel Shahaf wrote:
>> Currently, serf has a very specific form of .pc file, hardcoded in
its repo
>> [1].
> That's an implementation detail and may change in a future release.
> Don't rely on it.
>> This only has a single -L.
> You shouldn't rely on this either, if possible, though I don't
> immediately have a*specific* example of when it would break. Perhaps
> when some linker has a -LX flag?
> Don't parse all quotes and escapes. Delegate to the real parser
> (sh(1)?) to do that for you.
> The patch works in one specific case but will not work in other cases.
> For that matter, it won't work if serf.pc starts to emit one leading
> space in front of its -L argument.
> The patch needs to be compatible not only with past and current serf but
> with future ones as well.
I checked serf and the contents of the string hasn't changed in the last
10 years, ever since it was introduced in commit on 2011-08-06 11:52:19.
On top of that, 1.3.8 was released on 2015-08-31 and 1.3.9 was released
on 2020-07-06, so it's not even clear if there will be new releases at all.
Therefore, I find it very unlikely to change. So it's not worthy to
write complex code to deal with unknown future. It sounds more efficient
to deal with it when it happens.
Also, I'm not very experienced with Linux, but my general impression is
that using shell to parse arguments from a string is non trivial and
prone to various escape/quotes issues.
This is one extra reason to not try to over-complicate things in hopes
to predict unlikely future.
So yes, it sounds to me that the current solution is optimal: it fixes
the existing problem, doesn't add new problems, and is simple.
> Could you justify the s/-L/-R/ approach, please? Do the other
> build/*/*.m4 dependency handlers do this?
The problem about serf is that it doesn't have a libtool's `.la` file.
At the same time, libtool only adds runpath for dependencies that has
`.la` files. Dependencies such as --with-apr, --with-apr-util,
--with-sasl do have .la files.
In my SVN build, this leaves two dependencies without `.la` files:
1) --with-serf, which I patch here
2) --with-zlib, that has the same problem. But a typical Linux has
system zlib (on my Ubuntu, it's even needed to run `sudo` !) and
in absence of RUNPATH, SVN secretly loads system zlib instead of
the one specified in `--with-zlib`. I discovered this just today.
I guess I'll leave it for someone else to fix.
Re: [PATCH] Fix libsvn_ra_serf build with external serf
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Alexandr Miloslavskiy wrote on Sat, Jul 31, 2021 at 13:16:37 +0300:
> On 31.07.2021 2:44, Daniel Shahaf wrote:
>
> > > + SVN_SERF_LIBS=["$SVN_SERF_LIBS `$PKG_CONFIG $serf_pc_arg --libs-only-L | $SED -e 's/^-L/-R/g'`"]
> >
> > I'm sceptical about the regex. For all I know, there may be multiple -L
> > options and there may be leading spaces before the first one.
> >
> > More importantly, however, shouldn't we be getting the linker flags from
> > pkg-config(1)? It would be interesting to build serf with a non-default
> > prefix and then grep the resulting .pc file for that prefix.
>
> Thanks for reviewing!
>
Welcome, and sorry for the late reply.
> Currently, serf has a very specific form of .pc file, hardcoded in its repo
> [1].
That's an implementation detail and may change in a future release.
Don't rely on it.
> This only has a single -L.
You shouldn't rely on this either, if possible, though I don't
immediately have a *specific* example of when it would break. Perhaps
when some linker has a -LX flag?
> At the same time, -L could happen to be present in the middle of the path
> (consider '-L~/SERF-LATEST/'), and avoiding to replace that -L in the middle
> is... hard.
configure/make don't usually do fully bulletproof quoting, to the point
that something like "prefix the value by a single space and do «s/ -L//»"
might be par.
> Just think about parsing all the possible quotes and escapes!
Don't parse all quotes and escapes. Delegate to the real parser
(sh(1)?) to do that for you.
You still won't be able to tell what to do if the flags are «-x -Lfoo
-Lbar», though (e.g., consider the roles of -x and -p in «svn diff -x -p»).
> Therefore, I conclude that it's optimal (by patch complexity vs benefit) to
> only replace a single -L in the beginning.
You are seriously arguing that your patch is "optimal" because the
alternative is to reimplement a sh(1) parser.
> At worst, this patch will leave some space for further improvement.
> Currently, to my understanding, patch already fixes the current
> problem without breaking things.
The patch works in one specific case but will not work in other cases.
For that matter, it won't work if serf.pc starts to emit one leading
space in front of its -L argument.
The patch needs to be compatible not only with past and current serf but
with future ones as well.
Could you justify the s/-L/-R/ approach, please? Do the other
build/*/*.m4 dependency handlers do this?
> [1] https://svn.apache.org/repos/asf/serf/trunk/build/serf.pc.in
Cheers,
Daniel
Re: [PATCH] Fix libsvn_ra_serf build with external serf
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 31.07.2021 2:44, Daniel Shahaf wrote:
>> + SVN_SERF_LIBS=["$SVN_SERF_LIBS `$PKG_CONFIG $serf_pc_arg --libs-only-L | $SED -e 's/^-L/-R/g'`"]
>
> I'm sceptical about the regex. For all I know, there may be multiple -L
> options and there may be leading spaces before the first one.
>
> More importantly, however, shouldn't we be getting the linker flags from
> pkg-config(1)? It would be interesting to build serf with a non-default
> prefix and then grep the resulting .pc file for that prefix.
Thanks for reviewing!
Currently, serf has a very specific form of .pc file, hardcoded in its
repo [1]. This only has a single -L. I already build serf with
non-default prefix (hence the struggle and the patch) and it still has
single -L without any -R, that's why libsvn_ra_serf can't find it later
(because in absence of -R it doesn't record -rpath to find serf).
At the same time, -L could happen to be present in the middle of the
path (consider '-L~/SERF-LATEST/'), and avoiding to replace that -L in
the middle is... hard. Just think about parsing all the possible quotes
and escapes!
Therefore, I conclude that it's optimal (by patch complexity vs benefit)
to only replace a single -L in the beginning. At worst, this patch will
leave some space for further improvement. Currently, to my
understanding, patch already fixes the current problem without breaking
things.
[1] https://svn.apache.org/repos/asf/serf/trunk/build/serf.pc.in
Re: [PATCH] Fix libsvn_ra_serf build with external serf
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Alexandr Miloslavskiy wrote on Wed, Jul 28, 2021 at 00:38:34 +0300:
> +++ build/ac-macros/serf.m4 (working copy)
> @@ -170,6 +170,14 @@
> serf_found=yes
> SVN_SERF_INCLUDES=[`$PKG_CONFIG $serf_pc_arg --cflags-only-I`]
> SVN_SERF_LIBS=[`$PKG_CONFIG $serf_pc_arg --libs-only-l`]
> + if test -n "$serf_prefix" ; then
> + dnl User provided a prefix, so the location is non default.
> + dnl serf doesn't create .la since 2011, and it just
> + dnl gives '-lserf-1'. At the same time, libtool doesn't
> + dnl add -rpath for dependencies given as '-l'.
> + dnl The workaround dnl is to hack '-L' into '-R'.
> + SVN_SERF_LIBS=["$SVN_SERF_LIBS `$PKG_CONFIG $serf_pc_arg --libs-only-L | $SED -e 's/^-L/-R/g'`"]
> + fi
I'm sceptical about the regex. For all I know, there may be multiple -L
options and there may be leading spaces before the first one.
More importantly, however, shouldn't we be getting the linker flags from
pkg-config(1)? It would be interesting to build serf with a non-default
prefix and then grep the resulting .pc file for that prefix.
Cheers,
Daniel
> dnl don't use --libs-only-L because then we might miss some options
> LDFLAGS=["$LDFLAGS `$PKG_CONFIG $serf_pc_arg --libs | $SED -e 's/-l[^ ]*//g'`"]
> break