Patchwork [1/6] package infra: remove CPPFLAGS from CFLAGS

login
register
mail settings
Submitter Gustavo Zacarias
Date May 13, 2013, 4:40 p.m.
Message ID <1368463259-18958-1-git-send-email-gustavo@zacarias.com.ar>
Download mbox | patch
Permalink /patch/243442/
State Deferred
Headers show

Comments

Gustavo Zacarias - May 13, 2013, 4:40 p.m.
CPPFLAGS don't belong in CFLAGS, and newer autoconf versions just error
out when it's used that way.
This is required for the libcurl bump to 7.30.0

Signed-off-by: Gustavo Zacarias <gustavo@zacarias.com.ar>
---
 package/Makefile.in | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Thomas Petazzoni - May 13, 2013, 5:10 p.m.
Dear Gustavo Zacarias,

On Mon, 13 May 2013 13:40:54 -0300, Gustavo Zacarias wrote:

> -TARGET_CFLAGS = $(TARGET_CPPFLAGS) $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
> +TARGET_CFLAGS = $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
>  TARGET_CXXFLAGS = $(TARGET_CFLAGS)
>  TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
>  
> @@ -179,7 +179,6 @@ SED:=$(shell which sed || type -p sed) -i -e
>  
>  HOST_CPPFLAGS  = -I$(HOST_DIR)/usr/include
>  HOST_CFLAGS   ?= -O2
> -HOST_CFLAGS   += $(HOST_CPPFLAGS)
>  HOST_CXXFLAGS += $(HOST_CFLAGS)
>  HOST_LDFLAGS  += -L$(HOST_DIR)/lib -L$(HOST_DIR)/usr/lib -Wl,-rpath,$(HOST_DIR)/usr/lib
>  HOST_PATH=$(HOST_DIR)/bin:$(HOST_DIR)/usr/bin:$(PATH)

Do we have packages that pass TARGET_CFLAGS and/or HOST_CFLAGS, and are
not passing TARGET_CPPFLAGS / HOST_CPPFLAGS ?

One example: the bsdiff package:

define BSDIFF_BUILD_CMDS
        $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) \
                $(@D)/bsdiff.c -lbz2 -o $(@D)/bsdiff
        $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) \
                $(@D)/bspatch.c -lbz2 -o $(@D)/bspatch
endef

After your patch, the -D_LARGEFILE_BLABLA stuff is no longer passed
when building this package.

Another example, the feh package. Another one, input-tools. And I'm
sure there are more.

So, I agree with the change, but I think it needs a much thorough
investigation of which packages are using $(TARGET_CFLAGS) without
passing $(TARGET_CPPFLAGS).

Best regards,

Thomas
Gustavo Zacarias - May 13, 2013, 5:22 p.m.
On 05/13/2013 02:10 PM, Thomas Petazzoni wrote:

> Do we have packages that pass TARGET_CFLAGS and/or HOST_CFLAGS, and are
> not passing TARGET_CPPFLAGS / HOST_CPPFLAGS ?
> 
> One example: the bsdiff package:
> 
> define BSDIFF_BUILD_CMDS
>         $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) \
>                 $(@D)/bsdiff.c -lbz2 -o $(@D)/bsdiff
>         $(TARGET_CC) $(TARGET_CFLAGS) $(TARGET_LDFLAGS) \
>                 $(@D)/bspatch.c -lbz2 -o $(@D)/bspatch
> endef
> 
> After your patch, the -D_LARGEFILE_BLABLA stuff is no longer passed
> when building this package.
> 
> Another example, the feh package. Another one, input-tools. And I'm
> sure there are more.
> 
> So, I agree with the change, but I think it needs a much thorough
> investigation of which packages are using $(TARGET_CFLAGS) without
> passing $(TARGET_CPPFLAGS).

On the build-side of things there aren't any unexpected build failures
(allyespackageconfig that lead to my latest fixes because of other issues).
This is meant as a first try and i'm sure there will be packages that'll
need to be modified, but we'd rather do this sooner rather than later,
specially if we want to push new autotools where -D* isn't acceptable in
CFLAGS (be it via auto* bump + autoreconf or just plain shipped
configure in packages based on newer autotools).
We could also filter out for "strict" (new auto* packages) too, but
that's a sloppy way downhill.
I just want to keep this patchset apart from the package-fixing one,
since it's likely this won't change much and i'm not a fan of 1/N with N
being a huge number just to add a few patchfixes here/there (with the
fix patches maybe even getting commited before this patchset - it
shouldn't hurt).
Regards.
Thomas Petazzoni - May 13, 2013, 6:20 p.m.
Dear Gustavo Zacarias,

On Mon, 13 May 2013 14:22:08 -0300, Gustavo Zacarias wrote:

> > After your patch, the -D_LARGEFILE_BLABLA stuff is no longer passed
> > when building this package.
> > 
> > Another example, the feh package. Another one, input-tools. And I'm
> > sure there are more.
> > 
> > So, I agree with the change, but I think it needs a much thorough
> > investigation of which packages are using $(TARGET_CFLAGS) without
> > passing $(TARGET_CPPFLAGS).
> 
> On the build-side of things there aren't any unexpected build failures
> (allyespackageconfig that lead to my latest fixes because of other issues).

Build failures no, but packages not having large file support because
-D_LARGEFILE_BLA is not passed, cerainly.

> This is meant as a first try and i'm sure there will be packages that'll
> need to be modified, but we'd rather do this sooner rather than later,
> specially if we want to push new autotools where -D* isn't acceptable in
> CFLAGS (be it via auto* bump + autoreconf or just plain shipped
> configure in packages based on newer autotools).

Agreed.

> We could also filter out for "strict" (new auto* packages) too, but
> that's a sloppy way downhill.

Yes, we probably don't want to do that.

> I just want to keep this patchset apart from the package-fixing one,
> since it's likely this won't change much and i'm not a fan of 1/N with N
> being a huge number just to add a few patchfixes here/there (with the
> fix patches maybe even getting commited before this patchset - it
> shouldn't hurt).

I believe there should be first an audit of where TARGET_CFLAGS is
used. Then, wherever needed, fixes to ensure both TARGET_CFLAGS and
TARGET_CPPFLAGS are used (or, if possible, usage of
TARGET_CONFIGURE_OPTS). Once those fixes are made, your patch that
separates TARGET_CPPFLAGS from TARGET_CFLAGS can go in.

A quick "grep TARGET_CFLAGS" in package/ gives 110 packages using it.
In most cases, it's probably $(TARGET_CONFIGURE_OPTS) being passed +
CFLAGS="$(TARGET_CFLAGS) -bleh", which is already correct seems
CPPFLAGS is passed. But of course there might be packages that use a
custom Makefile that obeys to CFLAGS but not CPPFLAGS. Not sure what we
can do about these except slowly figuring out that they need fixing.

Best regards,

Thomas
Gustavo Zacarias - May 13, 2013, 10:09 p.m.
On 05/13/2013 03:20 PM, Thomas Petazzoni wrote:
> I believe there should be first an audit of where TARGET_CFLAGS is
> used. Then, wherever needed, fixes to ensure both TARGET_CFLAGS and
> TARGET_CPPFLAGS are used (or, if possible, usage of
> TARGET_CONFIGURE_OPTS). Once those fixes are made, your patch that
> separates TARGET_CPPFLAGS from TARGET_CFLAGS can go in.
> 
> A quick "grep TARGET_CFLAGS" in package/ gives 110 packages using it.
> In most cases, it's probably $(TARGET_CONFIGURE_OPTS) being passed +
> CFLAGS="$(TARGET_CFLAGS) -bleh", which is already correct seems
> CPPFLAGS is passed. But of course there might be packages that use a
> custom Makefile that obeys to CFLAGS but not CPPFLAGS. Not sure what we
> can do about these except slowly figuring out that they need fixing.

Looking into generic packages which are the most likely to have
differences because of custom Makefiles the general majority don't seem
to understand CPPFLAGS, so other than doing
SOMETHING="$(TARGET_CPPFLAGS) $(TARGET_CFLAGS)" there doesn't seem to be
much to do about it.
We could define a TARGET_GENERIC_CFLAGS == TARGET_CPPFLAGS +
TARGET_CFLAGS (like now) or something like that and make those package
use that, after all it's the auto* packages that have issues with that.
On the other hand it's a nice opportunity for some miscellaneous
cleanups like using $(INSTALL) instead of install, removing redundant
*_SOURCE definitions and stuff like that.
Regards.
Arnout Vandecappelle - May 13, 2013, 11:02 p.m.
On 14/05/13 00:09, Gustavo Zacarias wrote:
> On 05/13/2013 03:20 PM, Thomas Petazzoni wrote:
>> I believe there should be first an audit of where TARGET_CFLAGS is
>> used. Then, wherever needed, fixes to ensure both TARGET_CFLAGS and
>> TARGET_CPPFLAGS are used (or, if possible, usage of
>> TARGET_CONFIGURE_OPTS). Once those fixes are made, your patch that
>> separates TARGET_CPPFLAGS from TARGET_CFLAGS can go in.
>>
>> A quick "grep TARGET_CFLAGS" in package/ gives 110 packages using it.
>> In most cases, it's probably $(TARGET_CONFIGURE_OPTS) being passed +
>> CFLAGS="$(TARGET_CFLAGS) -bleh", which is already correct seems
>> CPPFLAGS is passed. But of course there might be packages that use a
>> custom Makefile that obeys to CFLAGS but not CPPFLAGS. Not sure what we
>> can do about these except slowly figuring out that they need fixing.
>
> Looking into generic packages which are the most likely to have
> differences because of custom Makefiles the general majority don't seem
> to understand CPPFLAGS, so other than doing
> SOMETHING="$(TARGET_CPPFLAGS) $(TARGET_CFLAGS)" there doesn't seem to be
> much to do about it.
> We could define a TARGET_GENERIC_CFLAGS == TARGET_CPPFLAGS +
> TARGET_CFLAGS (like now) or something like that and make those package
> use that, after all it's the auto* packages that have issues with that.
> On the other hand it's a nice opportunity for some miscellaneous
> cleanups like using $(INSTALL) instead of install, removing redundant
> *_SOURCE definitions and stuff like that.
> Regards.

  TARGET_CPPFLAGS is _only_ used to set the largefile defines. So I think 
it's much easier to leave it out entirely, and pass those defines in 
CFLAGS. There may be one or two packages that break because they rely on 
correct CPPFLAGS, but for these we can send the largefile defines explicitly.

  So:

TARGET_LARGEFILE_CFLAGS = -D_LARGEFILE_SOURCE ...

TARGET_CFLAGS = $(TARGET_LARGEFILE_CFLAGS) ...

Remove the CPPFLAGS="$(TARGET_CPPFLAGS)" from TARGET_CONFIGURE_OPTS

Fixup the packages that use TARGET_CPPFLAGS


  HOST_CPPFLAGS is another story. It contains -I$(HOST_DIR)/usr/include. 
But that one _will_ trigger build failures if it is not passed correctly, 
so there Gustavo's patch is safe.

  Regards,
  Arnout
Thomas Petazzoni - May 14, 2013, 7:17 a.m.
Dear Arnout Vandecappelle,

On Tue, 14 May 2013 01:02:45 +0200, Arnout Vandecappelle wrote:

>   TARGET_CPPFLAGS is _only_ used to set the largefile defines. So I think 
> it's much easier to leave it out entirely, and pass those defines in 
> CFLAGS. There may be one or two packages that break because they rely on 
> correct CPPFLAGS, but for these we can send the largefile defines explicitly.
> 
>   So:
> 
> TARGET_LARGEFILE_CFLAGS = -D_LARGEFILE_SOURCE ...
> 
> TARGET_CFLAGS = $(TARGET_LARGEFILE_CFLAGS) ...
> 
> Remove the CPPFLAGS="$(TARGET_CPPFLAGS)" from TARGET_CONFIGURE_OPTS

No, that's precisely the problem Gustavo had: newer autotools version
do *NOT* want to have -D_BLABLA in CFLAGS, they only accept it in
CPPFLAGS.

See the original patch from Gustavo:

"""
CPPFLAGS don't belong in CFLAGS, and newer autoconf versions just error
out when it's used that way.
"""

So the whole purpose of the discussion is precisely that we can't any
longer pass the -D_LARGEFILE_SOURCE in TARGET_CFLAGS :)

Best regards,

Thomas
Arnout Vandecappelle - May 14, 2013, 8:54 a.m.
On 14/05/13 09:17, Thomas Petazzoni wrote:
> Dear Arnout Vandecappelle,
>
> On Tue, 14 May 2013 01:02:45 +0200, Arnout Vandecappelle wrote:
>
>>    TARGET_CPPFLAGS is _only_ used to set the largefile defines. So I think
>> it's much easier to leave it out entirely, and pass those defines in
>> CFLAGS. There may be one or two packages that break because they rely on
>> correct CPPFLAGS, but for these we can send the largefile defines explicitly.
>>
>>    So:
>>
>> TARGET_LARGEFILE_CFLAGS = -D_LARGEFILE_SOURCE ...
>>
>> TARGET_CFLAGS = $(TARGET_LARGEFILE_CFLAGS) ...
>>
>> Remove the CPPFLAGS="$(TARGET_CPPFLAGS)" from TARGET_CONFIGURE_OPTS
>
> No, that's precisely the problem Gustavo had: newer autotools version
> do *NOT* want to have -D_BLABLA in CFLAGS, they only accept it in
> CPPFLAGS.
>
> See the original patch from Gustavo:
>
> """
> CPPFLAGS don't belong in CFLAGS, and newer autoconf versions just error
> out when it's used that way.
> """
>
> So the whole purpose of the discussion is precisely that we can't any
> longer pass the -D_LARGEFILE_SOURCE in TARGET_CFLAGS :)

  Okay my bad. I thought the issue was that CPPFLAGS were duplicated in 
CFLAGS.

  Regards,
  Arnout

Patch

diff --git a/package/Makefile.in b/package/Makefile.in
index 9ba6e8c..8ba7d70 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -100,7 +100,7 @@  ifeq ($(BR2_LARGEFILE),y)
 TARGET_CPPFLAGS += -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
 endif
 
-TARGET_CFLAGS = $(TARGET_CPPFLAGS) $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
+TARGET_CFLAGS = $(TARGET_ABI) $(TARGET_OPTIMIZATION) $(TARGET_DEBUGGING)
 TARGET_CXXFLAGS = $(TARGET_CFLAGS)
 TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
 
@@ -179,7 +179,6 @@  SED:=$(shell which sed || type -p sed) -i -e
 
 HOST_CPPFLAGS  = -I$(HOST_DIR)/usr/include
 HOST_CFLAGS   ?= -O2
-HOST_CFLAGS   += $(HOST_CPPFLAGS)
 HOST_CXXFLAGS += $(HOST_CFLAGS)
 HOST_LDFLAGS  += -L$(HOST_DIR)/lib -L$(HOST_DIR)/usr/lib -Wl,-rpath,$(HOST_DIR)/usr/lib
 HOST_PATH=$(HOST_DIR)/bin:$(HOST_DIR)/usr/bin:$(PATH)