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
