| Message ID | 1368463259-18958-1-git-send-email-gustavo@zacarias.com.ar |
|---|---|
| State | Deferred |
| Headers | show |
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
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.
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
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.
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
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
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
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)
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(-)