| Message ID | 20190423205302.14382-1-ps.report@gmx.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v6,1/2] meson: add per package optional compiler/linker flags | expand |
Peter, All, On 2019-04-23 22:53 +0200, Peter Seiderer spake thusly: > Add LIBFOO_MESON_CFLAGS, LIBFOO_MESON_LDFLAGS and LIBFOO_MESON_CXXFLAGS > variables to allow per package additional compiler/linker flags. So, this patch (or a variation based on it) is starting to get very much needed to fix the build of libglib2: http://autobuild.buildroot.org/?reason=libglib2-2.60.3 I did not know about this patch, so I sent that one instead: https://patchwork.ozlabs.org/patch/1114211/ Of course, it looks like yours is better and I successfully used it to fix the above issues. Wee! \o/ However, I have a few comments about it, see below... > Signed-off-by: Peter Seiderer <ps.report@gmx.net> > --- [--SNIP--] > diff --git a/package/meson/meson.mk b/package/meson/meson.mk > index 49e27f5527..70128f6bad 100644 > --- a/package/meson/meson.mk > +++ b/package/meson/meson.mk > @@ -60,6 +60,9 @@ define HOST_MESON_INSTALL_CROSS_CONF > -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \ > -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \ > -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \ > + -e "s%@PKG_TARGET_CFLAGS@%%g" \ > + -e "s%@PKG_TARGET_LDFLAGS@%%g" \ > + -e "s%@PKG_TARGET_CXXFLAGS@%%g" \ Actually, if someone wants to use that file outside of Buildroot, and also happend to need to pass special C/LD/CXXFLAGS for their package, how are they expected to do so? Can't we just install it as a template too? (But given that I don't like meson, I am OK that we don't want to offer this to external users and just decide to leave them out in the cold, where they belong. OK, I'm out in the box for the next 5 minutes! ;-] > -e "s%@HOST_DIR@%$(HOST_DIR)%g" \ > $(HOST_MESON_PKGDIR)/cross-compilation.conf.in \ > > $(HOST_DIR)/etc/meson/cross-compilation.conf > diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk > index 886fcf7205..9974b1e513 100644 > --- a/package/pkg-meson.mk > +++ b/package/pkg-meson.mk > @@ -57,6 +57,10 @@ $(2)_NINJA_ENV ?= > ifndef $(2)_CONFIGURE_CMDS > ifeq ($(4),target) > > +$(2)_MESON_SED_CFLAGS = $(if $($(2)_MESON_CFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_CFLAGS)`) > +$(2)_MESON_SED_LDFLAGS = $(if $($(2)_MESON_LDFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_LDFLAGS)`) > +$(2)_MESON_SED_CXXFLAGS = $(if $($(2)_MESON_CXXFLAGS),`printf '"%s"$(comma) ' $$($$(2)_MESON_CXXFLAGS)`) There are two things I am not too fond about those variables: 1. for the other package infras, when a package wants to pass extra such flags, the package is responsible for carrying the original TARGET_CFLAGS (LD, CXX), and this is not automatic. With your path, this would make it automatic for meson, thus diverging from the usual practice. It also precludes packages from actualyl overriding the default flags, like we currently have to do to override -ON for some packages under mucroblaze for example. 2. The naming is not nice to me: why do we need to have "MESON" in the variable name at all? Surely, fir the other infras, we just write things like: FOO_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF not: FOO_AUTOTOOLS_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF FOO_CMAKE_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF etc... Regards, Yann E. MORIN. > # Configure package for target > # > # > @@ -70,6 +74,9 @@ define $(2)_CONFIGURE_CMDS > -e "s%@TARGET_CFLAGS@%$$(HOST_MESON_SED_CFLAGS)%g" \ > -e "s%@TARGET_LDFLAGS@%$$(HOST_MESON_SED_LDFLAGS)%g" \ > -e "s%@TARGET_CXXFLAGS@%$$(HOST_MESON_SED_CXXFLAGS)%g" \ > + -e "s%@PKG_TARGET_CFLAGS@%$$($$(PKG)_MESON_SED_CFLAGS)%g" \ > + -e "s%@PKG_TARGET_LDFLAGS@%$$($$(PKG)_MESON_SED_LDFLAGS)%g" \ > + -e "s%@PKG_TARGET_CXXFLAGS@%$$($$(PKG)_MESON_SED_CXXFLAGS)%g" \ > -e "s%@HOST_DIR@%$$(HOST_DIR)%g" \ > package/meson/cross-compilation.conf.in \ > > $$($$(PKG)_SRCDIR)/build/cross-compilation.conf > -- > 2.21.0 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
All; On Wed, Jun 12, 2019 at 12:37 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Peter, All, > > On 2019-04-23 22:53 +0200, Peter Seiderer spake thusly: > > Add LIBFOO_MESON_CFLAGS, LIBFOO_MESON_LDFLAGS and LIBFOO_MESON_CXXFLAGS > > variables to allow per package additional compiler/linker flags. > > So, this patch (or a variation based on it) is starting to get very much > needed to fix the build of libglib2: > http://autobuild.buildroot.org/?reason=libglib2-2.60.3 > > I did not know about this patch, so I sent that one instead: > https://patchwork.ozlabs.org/patch/1114211/ > > Of course, it looks like yours is better and I successfully used it to > fix the above issues. Wee! \o/ > > However, I have a few comments about it, see below... > > > Signed-off-by: Peter Seiderer <ps.report@gmx.net> > > --- > [--SNIP--] > > diff --git a/package/meson/meson.mk b/package/meson/meson.mk > > index 49e27f5527..70128f6bad 100644 > > --- a/package/meson/meson.mk > > +++ b/package/meson/meson.mk > > @@ -60,6 +60,9 @@ define HOST_MESON_INSTALL_CROSS_CONF > > -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \ > > -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \ > > -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \ > > + -e "s%@PKG_TARGET_CFLAGS@%%g" \ > > + -e "s%@PKG_TARGET_LDFLAGS@%%g" \ > > + -e "s%@PKG_TARGET_CXXFLAGS@%%g" \ > > Actually, if someone wants to use that file outside of Buildroot, and > also happend to need to pass special C/LD/CXXFLAGS for their package, > how are they expected to do so? Can't we just install it as a template > too? > > (But given that I don't like meson, I am OK that we don't want to offer > this to external users and just decide to leave them out in the cold, > where they belong. OK, I'm out in the box for the next 5 minutes! ;-] > > > -e "s%@HOST_DIR@%$(HOST_DIR)%g" \ > > $(HOST_MESON_PKGDIR)/cross-compilation.conf.in \ > > > $(HOST_DIR)/etc/meson/cross-compilation.conf > > diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk > > index 886fcf7205..9974b1e513 100644 > > --- a/package/pkg-meson.mk > > +++ b/package/pkg-meson.mk > > @@ -57,6 +57,10 @@ $(2)_NINJA_ENV ?= > > ifndef $(2)_CONFIGURE_CMDS > > ifeq ($(4),target) > > > > +$(2)_MESON_SED_CFLAGS = $(if $($(2)_MESON_CFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_CFLAGS)`) > > +$(2)_MESON_SED_LDFLAGS = $(if $($(2)_MESON_LDFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_LDFLAGS)`) > > +$(2)_MESON_SED_CXXFLAGS = $(if $($(2)_MESON_CXXFLAGS),`printf '"%s"$(comma) ' $$($$(2)_MESON_CXXFLAGS)`) > > There are two things I am not too fond about those variables: > > 1. for the other package infras, when a package wants to pass extra such > flags, the package is responsible for carrying the original TARGET_CFLAGS > (LD, CXX), and this is not automatic. With your path, this would make it > automatic for meson, thus diverging from the usual practice. > > It also precludes packages from actualyl overriding the default flags, > like we currently have to do to override -ON for some packages under > mucroblaze for example. > > 2. The naming is not nice to me: why do we need to have "MESON" in the > variable name at all? Surely, fir the other infras, we just write things > like: > FOO_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF > not: > FOO_AUTOTOOLS_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF > FOO_CMAKE_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF > etc... > > Regards, > Yann E. MORIN. > I 100% agree with Yann. We should maintain consistency, and there is no real reason that I can find that would make the _MESON_ prefix work better than without. > > # Configure package for target > > # > > # > > @@ -70,6 +74,9 @@ define $(2)_CONFIGURE_CMDS > > -e "s%@TARGET_CFLAGS@%$$(HOST_MESON_SED_CFLAGS)%g" \ > > -e "s%@TARGET_LDFLAGS@%$$(HOST_MESON_SED_LDFLAGS)%g" \ > > -e "s%@TARGET_CXXFLAGS@%$$(HOST_MESON_SED_CXXFLAGS)%g" \ > > + -e "s%@PKG_TARGET_CFLAGS@%$$($$(PKG)_MESON_SED_CFLAGS)%g" \ > > + -e "s%@PKG_TARGET_LDFLAGS@%$$($$(PKG)_MESON_SED_LDFLAGS)%g" \ > > + -e "s%@PKG_TARGET_CXXFLAGS@%$$($$(PKG)_MESON_SED_CXXFLAGS)%g" \ > > -e "s%@HOST_DIR@%$$(HOST_DIR)%g" \ > > package/meson/cross-compilation.conf.in \ > > > $$($$(PKG)_SRCDIR)/build/cross-compilation.conf > > -- > > 2.21.0 > > > > _______________________________________________ > > buildroot mailing list > > buildroot@busybox.net > > http://lists.busybox.net/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On Wed, 12 Jun 2019 18:37:10 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > diff --git a/package/meson/meson.mk b/package/meson/meson.mk > > index 49e27f5527..70128f6bad 100644 > > --- a/package/meson/meson.mk > > +++ b/package/meson/meson.mk > > @@ -60,6 +60,9 @@ define HOST_MESON_INSTALL_CROSS_CONF > > -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \ > > -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \ > > -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \ > > + -e "s%@PKG_TARGET_CFLAGS@%%g" \ > > + -e "s%@PKG_TARGET_LDFLAGS@%%g" \ > > + -e "s%@PKG_TARGET_CXXFLAGS@%%g" \ > > Actually, if someone wants to use that file outside of Buildroot, and > also happend to need to pass special C/LD/CXXFLAGS for their package, > how are they expected to do so? Can't we just install it as a template > too? If we keep @PKG_TARGET_CFLAGS@, @PKG_TARGET_LDFLAGS@, etc. in the cross-compilation.conf, then it is no longer usable as-is: users will _have_ to fixup those @PKG_TARGET_foo@, even if they don't need to pass additional CFLAGS to their package. I think what Peter S. did here is the best we can do. A better solution would be for Meson to support command line provided cflags/ldflags, but that's not supported, and if it was supported, this whole patch wouldn't be necessary. > > +$(2)_MESON_SED_CFLAGS = $(if $($(2)_MESON_CFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_CFLAGS)`) > > +$(2)_MESON_SED_LDFLAGS = $(if $($(2)_MESON_LDFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_LDFLAGS)`) > > +$(2)_MESON_SED_CXXFLAGS = $(if $($(2)_MESON_CXXFLAGS),`printf '"%s"$(comma) ' $$($$(2)_MESON_CXXFLAGS)`) > > There are two things I am not too fond about those variables: > > 1. for the other package infras, when a package wants to pass extra such > flags, the package is responsible for carrying the original TARGET_CFLAGS > (LD, CXX), and this is not automatic. With your path, this would make it > automatic for meson, thus diverging from the usual practice. I agree. > It also precludes packages from actualyl overriding the default flags, > like we currently have to do to override -ON for some packages under > mucroblaze for example. However, this example is not really great: for Microblaze we override with -O0 by passing it after $(TARGET_CFLAGS), i.e we don't need to tweak what's in $(TARGET_CFLAGS) But even if your example is not the best, I agree with you, as it's more consistent with what we do elsewhere, and potentially also dropping/tweaking what's inside TARGET_CFLAGS for a given package, which we do in a few places. > 2. The naming is not nice to me: why do we need to have "MESON" in the > variable name at all? Surely, fir the other infras, we just write things > like: > FOO_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF > not: > FOO_AUTOTOOLS_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF > FOO_CMAKE_CFLAGS = $(TARGET_CFLAGS) -DMY_STUFF > etc... That's true but in fact no other infra has <pkg>_CFLAGS or <pkg>_LDFLAGS variables. These are entirely "internal" to each package, which takes care to propagate them to <pkg>_CONF_ENV for autotools packages, to some -DCMAKE_CFLAGS for cmake packages, etc. So Meson would anyway stand out from the point of view that it will be the only package infra that takes care of passing <pkg>_CFLAGS / <pkg>_LDFLAGS. But like the point above: despite this, I still agree with you that <pkg>_CFLAGS is better. Maybe one day we'll make the autotools/cmake infras also take care of passing <pkg>_CFLAGS/LDFLAGS. So all in all, I agree with your review, except the very first point on keeping the @PKG_TARGET_*@ templates. Thanks! Thomas
Thomas, All, On 2019-06-22 21:13 +0200, Thomas Petazzoni spake thusly: > On Wed, 12 Jun 2019 18:37:10 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > > > diff --git a/package/meson/meson.mk b/package/meson/meson.mk > > > index 49e27f5527..70128f6bad 100644 > > > --- a/package/meson/meson.mk > > > +++ b/package/meson/meson.mk > > > @@ -60,6 +60,9 @@ define HOST_MESON_INSTALL_CROSS_CONF > > > -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \ > > > -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \ > > > -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \ > > > + -e "s%@PKG_TARGET_CFLAGS@%%g" \ > > > + -e "s%@PKG_TARGET_LDFLAGS@%%g" \ > > > + -e "s%@PKG_TARGET_CXXFLAGS@%%g" \ > > > > Actually, if someone wants to use that file outside of Buildroot, and > > also happend to need to pass special C/LD/CXXFLAGS for their package, > > how are they expected to do so? Can't we just install it as a template ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > too? ^^^^ > > If we keep @PKG_TARGET_CFLAGS@, @PKG_TARGET_LDFLAGS@, etc. in the > cross-compilation.conf, then it is no longer usable as-is: users will > _have_ to fixup those @PKG_TARGET_foo@, even if they don't need to pass > additional CFLAGS to their package. > > I think what Peter S. did here is the best we can do. A better solution > would be for Meson to support command line provided cflags/ldflags, but > that's not supported, and if it was supported, this whole patch > wouldn't be necessary. As I explained on IRC: we would install the file like Peter does, but also install the template as well, so that: - packages that don't need extra flags can be built as-is - while for packages that do require extra flag, the user can use the template to generate a custom file and thus we have the best of both worlds. Regards, Yann E. MORIN.
diff --git a/docs/manual/adding-packages-meson.txt b/docs/manual/adding-packages-meson.txt index 30c338f486..dbc22d3a7f 100644 --- a/docs/manual/adding-packages-meson.txt +++ b/docs/manual/adding-packages-meson.txt @@ -97,6 +97,18 @@ will therefore only use a few of them. * +FOO_CONF_OPTS+, to specify additional options to pass to +meson+ for the configuration step. By default, empty. +* +FOO_MESON_CFLAGS+, to specify additional compiler arguments added to + the package specific +cross-compile.conf+ file +c_args+ property. By + default, empty. + +* +FOO_MESON_CXXFLAGS+, to specify additional compiler arguments added to + the package specific +cross-compile.conf+ file +cpp_args+ property. By + default, empty. + +* +FOO_MESON_LDFLAGS+, to specify additional compiler arguments added to + the package specific +cross-compile.conf+ file +c_link_args+ and + +cpp_link_args+ properties. By default, empty. + * +FOO_NINJA_ENV+, to specify additional environment variables to pass to +ninja+, meson companion tool in charge of the build operations. By default, empty. diff --git a/package/meson/cross-compilation.conf.in b/package/meson/cross-compilation.conf.in index fc8e27f7eb..4056c2b2f4 100644 --- a/package/meson/cross-compilation.conf.in +++ b/package/meson/cross-compilation.conf.in @@ -12,10 +12,10 @@ pkgconfig = '@HOST_DIR@/usr/bin/pkg-config' [properties] needs_exe_wrapper = true -c_args = [@TARGET_CFLAGS@] -c_link_args = [@TARGET_LDFLAGS@] -cpp_args = [@TARGET_CXXFLAGS@] -cpp_link_args = [@TARGET_LDFLAGS@] +c_args = [@TARGET_CFLAGS@@PKG_TARGET_CFLAGS@] +c_link_args = [@TARGET_LDFLAGS@@PKG_TARGET_LDFLAGS@] +cpp_args = [@TARGET_CXXFLAGS@@PKG_TARGET_CXXFLAGS@] +cpp_link_args = [@TARGET_LDFLAGS@@PKG_TARGET_LDFLAGS@] [host_machine] system = 'linux' diff --git a/package/meson/meson.mk b/package/meson/meson.mk index 49e27f5527..70128f6bad 100644 --- a/package/meson/meson.mk +++ b/package/meson/meson.mk @@ -60,6 +60,9 @@ define HOST_MESON_INSTALL_CROSS_CONF -e "s%@TARGET_CFLAGS@%$(HOST_MESON_SED_CFLAGS)%g" \ -e "s%@TARGET_LDFLAGS@%$(HOST_MESON_SED_LDFLAGS)%g" \ -e "s%@TARGET_CXXFLAGS@%$(HOST_MESON_SED_CXXFLAGS)%g" \ + -e "s%@PKG_TARGET_CFLAGS@%%g" \ + -e "s%@PKG_TARGET_LDFLAGS@%%g" \ + -e "s%@PKG_TARGET_CXXFLAGS@%%g" \ -e "s%@HOST_DIR@%$(HOST_DIR)%g" \ $(HOST_MESON_PKGDIR)/cross-compilation.conf.in \ > $(HOST_DIR)/etc/meson/cross-compilation.conf diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk index 886fcf7205..9974b1e513 100644 --- a/package/pkg-meson.mk +++ b/package/pkg-meson.mk @@ -57,6 +57,10 @@ $(2)_NINJA_ENV ?= ifndef $(2)_CONFIGURE_CMDS ifeq ($(4),target) +$(2)_MESON_SED_CFLAGS = $(if $($(2)_MESON_CFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_CFLAGS)`) +$(2)_MESON_SED_LDFLAGS = $(if $($(2)_MESON_LDFLAGS),`printf '"%s"$(comma) ' $($(2)_MESON_LDFLAGS)`) +$(2)_MESON_SED_CXXFLAGS = $(if $($(2)_MESON_CXXFLAGS),`printf '"%s"$(comma) ' $$($$(2)_MESON_CXXFLAGS)`) + # Configure package for target # # @@ -70,6 +74,9 @@ define $(2)_CONFIGURE_CMDS -e "s%@TARGET_CFLAGS@%$$(HOST_MESON_SED_CFLAGS)%g" \ -e "s%@TARGET_LDFLAGS@%$$(HOST_MESON_SED_LDFLAGS)%g" \ -e "s%@TARGET_CXXFLAGS@%$$(HOST_MESON_SED_CXXFLAGS)%g" \ + -e "s%@PKG_TARGET_CFLAGS@%$$($$(PKG)_MESON_SED_CFLAGS)%g" \ + -e "s%@PKG_TARGET_LDFLAGS@%$$($$(PKG)_MESON_SED_LDFLAGS)%g" \ + -e "s%@PKG_TARGET_CXXFLAGS@%$$($$(PKG)_MESON_SED_CXXFLAGS)%g" \ -e "s%@HOST_DIR@%$$(HOST_DIR)%g" \ package/meson/cross-compilation.conf.in \ > $$($$(PKG)_SRCDIR)/build/cross-compilation.conf
Add LIBFOO_MESON_CFLAGS, LIBFOO_MESON_LDFLAGS and LIBFOO_MESON_CXXFLAGS variables to allow per package additional compiler/linker flags. Signed-off-by: Peter Seiderer <ps.report@gmx.net> --- Changes v5 -> v6: - add documentation for the new meson flags variables Changes v4 -> v5: - no changes Changes v4: - new patch (base on the original '[RFC v3 2/4] meson: change from global to per package cross-compilation.conf', see [1]) [1] http://lists.busybox.net/pipermail/buildroot/2018-July/226313.html --- docs/manual/adding-packages-meson.txt | 12 ++++++++++++ package/meson/cross-compilation.conf.in | 8 ++++---- package/meson/meson.mk | 3 +++ package/pkg-meson.mk | 7 +++++++ 4 files changed, 26 insertions(+), 4 deletions(-) -- 2.21.0