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

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

> I will take a closer look later this evening and see if I can find
> anything.

Thank you.

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

Thank you,

Alex.
Received on Wed Mar 09 2011 - 05:14:09 MST