Re: [PATCH] Support libecap v0.2.0; fixed eCAP body handling and logging

On 09/03/11 18:13, Alex Rousskov wrote:
> On 03/08/2011 06:47 PM, Amos Jeffries wrote:
>>>> src/adaptation/Makefile.am:
>>>
>>> Changed based on your comments below, but I am not sure I got all of
>>> them correctly. Since most "natural" macro combination (IMHO) does not
>>> work with libtool, I cannot improve it further on my own. If something
>>> else needs to be done, please let me know.
>>>
>>>
>>>> * Why is an automake macro called *_LIBS being added to LDFLAGS?
>>>
>>> Because the macro (EXTLIBECAP_LIBS) contains ld flags (-L and -l). The
>>> macro is created by pkg-config, and Squid has no control over its name.
>>> Furthermore, I suspect the _LIBS suffix is standard in this pkg-config
>>> context.
>>
>> Ah, -lecap a flag?
>> That is a MUST for adding to LDADD instead of FLAGS according to the
>> autotools docs. I found that while investigating kerberos helper build
>> failures on 3.1. Don't want to add those same failures again for ecap.
>>
>> -L is apparently optionally usable in both FLAGS and LDADD. We in Squid
>> have a tendency to place it in LDADD immediately before the library
>> which uses it. But we take what pkg-config gives us on that.
>>

Meh. Ignore me. LIBADD is not LDADD :(

>>>
>>> I started with _LIBADD, but libtool fails when I add EXTLIBECAP_LIBS to
>>> libxecap_la_LIBADD:
>>>
>>> libtool: link: rm -fr .libs/libxecap.a .libs/libxecap.la
>>> libtool: link: (cd .libs/libxecap.lax/libecap.a&& /usr/bin/ar x
>>>
>>> "/home/rousskov/Edit/squid3/3p2-ecap/src/adaptation/ecap/.libs/libecap.a")
>>>
>>> /usr/bin/ar:
>>>
>>> /home/rousskov/Edit/squid3/3p2-ecap/src/adaptation/ecap/.libs/libecap.a:
>>> No such file or directory
>>> make: *** [libxecap.la] Error 9
>>>
>>> The above is for adding EXTLIBECAP_LIBS to libxecap_la_LIBADD in
>>> adaptation/ecap/Makefile.am, not in adaptation/Makefile.am. However,
>>> adding EXTLIBECAP_LIBS to adaptation_la_LIBADD (in
>>> adaptation/Makefile.am) seems to work. So that is what I did to address
>>> your comment. Please let me know if that was wrong.
>>>
>>> If you can make it work using a more natural arrangement, please let me
>>> know how.
>>>
>>
>> Looks like a dirty Makefile or build area to start with.
>> ar is trying to play with adaptation/ecap/libecap.la instead of the
>> renamed new adaptation/ecap/libxecap.la
>
> That was my thought as well, after I renamed our library to understand
> which libecap libtool is talking about. I did "make clean all" and
> watched it succeed. After doing the celebratory dance, I did "touch
> ServerRep.cc; make" which failed with the above error, despite all that
> cleaning. I believe I tried "make distclean" as well. This would not be
> the first time I am doing this libtool magic wrong, of course.
>

So, I have now built trunk, patched, then rebuilt in the same sandbox
builddir. NO problems.

The tests: make clean all + rebuild, touch a file and rebuild.
   NO problems for either.

Altering the ecap/Makefile.am to uncomment the LIBADD set. And removing
EXT* from adaptation/Makefile.am. Rebuild, NO problems.

Repeat the clean and touch tests. NO problems.

This is on Ubuntu 10.9 (Maverick) with latest toolchains.

I think with this brief check the problem was an anomaly, possibly of
the shared "libecap" naming (resolved by your use of x) or of the
incorrect use of LDFLAGS.
I agree the linking into libxecap is probably best. I also feel the
alternative you used was suitable workaround. Whichever you want to
commit is fine.

Would you be happy using libecap-squid.la instead of libxecap.la on the
final commit? to make it clear which was the squid internal one.

>>>> src/cf.data.pre:
>>>> * while you are updating the documentation for
>>>> icap_client_username_header please correct the typo which mentions a
>>>> non-existent "send_username" to be "adaptation_send_username"
>>>
>>> Out of scope. Will fix later.
>>>
>>
>> Very much in scope IMO.
>> Alter the option name means altering all its public references to match,
>> even if they started off as typo versions of it.
>
> We disagree because I try to limit ICAP changes in this project to
> renaming/moving things that must be renamed or moved, even if they are
> buggy (primarily because it makes it harder to introduce bugs and makes
> it easier to review the changes) while you want to fix related ICAP bugs
> (they are bugs after all!).
>
> Since I promise to fix those bugs and polish that code anyway, I think
> it is a minor disagreement.
>

Maybe. On the others yes.

On this config typo one I am considering what you would have done had
the typo not been there:

   You would have seen it as icap_send_client_username and updated it to
adaptation_send_username.

Therefore; it is entirely relevant to rename the typo directly to
adaptation_send_username in this patch.

I said on IRC, but again in case you missed that.
  When testing I found the HttpMsg.h hunk from line 220 altering Msg()
const-ness. Exists in the second complete bundle but is already part of
trunk.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.11
   Beta testers wanted for 3.2.0.5