| Message ID | 20190614210346.121013-12-giulio.benetti@micronovasrl.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/27] package/keyutils: re-enable package on microblaze | expand |
Hello Giulio, On Fri, 14 Jun 2019 23:03:30 +0200 Giulio Benetti <giulio.benetti@micronovasrl.com> wrote: > Package prboom builds using -O2 flag ignoring Buildroot settings, this > is due to the fact that -O2 is appended at the end of CFLAGS. > > Remove -O2 from 'configure' file, this way CFLAGS will contain Buildroot > CFLAGS. > > Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com> > --- > package/prboom/prboom.mk | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/package/prboom/prboom.mk b/package/prboom/prboom.mk > index d370ae3fa5..6d9b55e9f6 100644 > --- a/package/prboom/prboom.mk > +++ b/package/prboom/prboom.mk > @@ -11,6 +11,13 @@ PRBOOM_DEPENDENCIES = sdl sdl_net sdl_mixer > PRBOOM_LICENSE = GPL-2.0+ > PRBOOM_LICENSE_FILES = COPYING > > +# Remove imposed -O2 CFLAG to use TARGET_CFLAGS > +define PRBOOM_FIXUP_CONFIGURE > + sed -i 's:-O2::g' $(@D)/configure > +endef > + > +PRBOOM_PRE_CONFIGURE_HOOKS += PRBOOM_FIXUP_CONFIGURE I am not a big fan of this "blind" sed that replaces every -O2 in the configure script with nothing. Patching configure.ac + autoreconf would be nicer, especially since anyway prboom depends on sdl, and sdl already depends on the autotools machinery. However, ideally the configure.ac script should be fixed to *not* add flags to CFLAGS. CFLAGS is a user variable, the configure script is not supposed to use it. See https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html. But well, fixing the prboom configure.ac is probably too much effort comparing to the benefits. So just do a patch that gets rid of the -O2 in the configure.ac, and that will be good enough. Thanks! Thomas
Hello Thomas, Il 19/06/2019 21:37, Thomas Petazzoni ha scritto: > Hello Giulio, > > On Fri, 14 Jun 2019 23:03:30 +0200 > Giulio Benetti <giulio.benetti@micronovasrl.com> wrote: > >> Package prboom builds using -O2 flag ignoring Buildroot settings, this >> is due to the fact that -O2 is appended at the end of CFLAGS. >> >> Remove -O2 from 'configure' file, this way CFLAGS will contain Buildroot >> CFLAGS. >> >> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com> >> --- >> package/prboom/prboom.mk | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/package/prboom/prboom.mk b/package/prboom/prboom.mk >> index d370ae3fa5..6d9b55e9f6 100644 >> --- a/package/prboom/prboom.mk >> +++ b/package/prboom/prboom.mk >> @@ -11,6 +11,13 @@ PRBOOM_DEPENDENCIES = sdl sdl_net sdl_mixer >> PRBOOM_LICENSE = GPL-2.0+ >> PRBOOM_LICENSE_FILES = COPYING >> >> +# Remove imposed -O2 CFLAG to use TARGET_CFLAGS >> +define PRBOOM_FIXUP_CONFIGURE >> + sed -i 's:-O2::g' $(@D)/configure >> +endef >> + >> +PRBOOM_PRE_CONFIGURE_HOOKS += PRBOOM_FIXUP_CONFIGURE > > I am not a big fan of this "blind" sed that replaces every -O2 in the > configure script with nothing. > > Patching configure.ac + autoreconf would be nicer, especially since > anyway prboom depends on sdl, and sdl already depends on the autotools > machinery. Yes, it's really cleaner. > However, ideally the configure.ac script should be fixed to *not* add > flags to CFLAGS. CFLAGS is a user variable, the configure script is not > supposed to use it. See > https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html. Interesting document. > But well, fixing the prboom configure.ac is probably too much effort > comparing to the benefits. I agree. > So just do a patch that gets rid of the -O2 in the configure.ac, and > that will be good enough. Ok Thank you
diff --git a/package/prboom/prboom.mk b/package/prboom/prboom.mk index d370ae3fa5..6d9b55e9f6 100644 --- a/package/prboom/prboom.mk +++ b/package/prboom/prboom.mk @@ -11,6 +11,13 @@ PRBOOM_DEPENDENCIES = sdl sdl_net sdl_mixer PRBOOM_LICENSE = GPL-2.0+ PRBOOM_LICENSE_FILES = COPYING +# Remove imposed -O2 CFLAG to use TARGET_CFLAGS +define PRBOOM_FIXUP_CONFIGURE + sed -i 's:-O2::g' $(@D)/configure +endef + +PRBOOM_PRE_CONFIGURE_HOOKS += PRBOOM_FIXUP_CONFIGURE + ifeq ($(BR2_PACKAGE_LIBPNG),y) PRBOOM_DEPENDENCIES += libpng endif
Package prboom builds using -O2 flag ignoring Buildroot settings, this is due to the fact that -O2 is appended at the end of CFLAGS. Remove -O2 from 'configure' file, this way CFLAGS will contain Buildroot CFLAGS. Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com> --- package/prboom/prboom.mk | 7 +++++++ 1 file changed, 7 insertions(+)