[Mesa-dev] [PATCH 1/7] i965: use AM_CPPFLAGS instead of AM_CFLAGS/CXXFLAGS

[Gaetan Nadon]

On 12-01-17 08:28 PM, Matt Turner wrote:
> On Tue, Jan 17, 2012 at 7:21 PM, Gaetan Nadon <memsize at videotron.ca> wrote:
>> On 12-01-17 03:58 PM, Matt Turner wrote:
>>> ---
>>>  src/mesa/drivers/dri/i965/Makefile.am |    4 +---
>>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/Makefile.am b/src/mesa/drivers/dri/i965/Makefile.am
>>> index 5512381..93937b1 100644
>>> --- a/src/mesa/drivers/dri/i965/Makefile.am
>>> +++ b/src/mesa/drivers/dri/i965/Makefile.am
>>> @@ -26,7 +26,7 @@ include Makefile.sources
>>>  # Hack to make some of the non-automake variables work.
>>>  TOP=$(top_builddir)
>>>
>>> -AM_CFLAGS = \
>>> +AM_CPPFLAGS = \
>>>       -I$(top_srcdir)/include \
>>>       -I$(top_srcdir)/src/ \
>>>       -I$(top_srcdir)/src/mapi \
>> This looks suspicious and/or error prone. Are you sure that none of the
>> variables assigned to AM_CPPFLAGS will ever contain any C compiler
>> flags? While AM_CFLAGS can contain pre-processor flags, AM_CPPFLAGS
>> cannot contain C compiler flags. I was not able to find where
>> INTEL_CFLAGS is assigned, but as its name implies, it may contain C
>> flags. If it is not used, it should be deleted. Automake has all the
>> features to allow a user to append C flags on the make command line.
> It's a little unclear, but I think
>
> PKG_CHECK_MODULES([INTEL], [libdrm_intel >= $LIBDRM_INTEL_REQUIRED])
>
> is defining INTEL_{CFLAGS,LIBS}:
>
> INTEL_CFLAGS = -I/usr/include/libdrm
> INTEL_LIBS = -ldrm_intel -ldrm

I thought it was a private variable.

Anyway, now I am sure there is issue. You cannot be sure that libdrm or
any package for that matter will not put a compiler flag in there. In
fact, that is what it was designed for.

        --cflags
              This prints pre-processor and compile flags required to 
compile
              the  packages on the command line, including flags for all
their
              dependencies. Flags are "compressed" so that each
identical flag
              appears  only  once.  pkg-config exits with a nonzero code
if it
              can't find metadata for one or more of the packages on
the  com-
              mand line.

For example, the xserver put the visibility flag in the xorg-server.pc file:

    Cflags: -I${sdkdir} -fvisibility=hidden

There is this option which has never been used (which would have to be
in pkg-config >= 0.22):

           --cflags-only-I
                  This  prints  the -I part of "--cflags". That is, it
    defines the
                  header search path but doesn't specify anything else.

I guess it all depends on the level of risk one is willing to take. If
you control libdrm and can make sure of what goes in there...
>
> and for radeon and nouveau:
>
> RADEON_CFLAGS = -I/usr/include/libdrm
> RADEON_LIBS = -ldrm_radeon
>
> NOUVEAU_CFLAGS = -I/usr/include/libdrm -I/usr/include/nouveau
> NOUVEAU_LIBS = -ldrm_nouveau
>
> so those look appropriate for AM_CPPFLAGS, right?
>
> Thanks,
> Matt
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120118/4b5d9cdc/attachment.htm>