Re: [PATCH] Fix libsvn_ra_serf build with external serf

[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

[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

[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

[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.