[PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[Olaf Hering]

Since commit 338c4375153e39b95ddc82f2ed95f85dd73e0245 ("tools: pass
EXTRA_CFLAGS via environment") it was possible to specify additional
CFLAGS in the environment. That commit gives a good explanation how that
is useful.

Now that we have configure move the knobs from the middle of the source
code to a more prominent place. If the variables are set once via
configure it is possible to rerun make from another shell which may not
have these environment variables. That can happen if one tries to resume
a failed build with either doing chroot into a rpmbuild tree or from
another xterm.

This change adds the configure options --with-extra-cflags-tools=,
--with-extra-cflags-qemu-traditional= and
--with-extra-cflags-qemu-upstream= which will assign their values to the
existing make variables.

It is now required to use configure instead of environment variables to
have a consistent set of CFLAGS across make invocations.

A new knob "skip_extra_cflags" in Rules.mk is recognized to turn off the
assignment to CFLAGS. This is required to build the firmware.

Please rerun autogen.sh after applying this patch.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 config/Tools.mk.in      |  3 +++
 tools/Rules.mk          |  2 ++
 tools/configure.ac      | 21 +++++++++++++++++++++
 tools/firmware/Rules.mk |  2 +-
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 89de5bd..6d0ac1a 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -59,6 +59,9 @@ CONFIG_QEMU_TRAD    := @qemu_traditional@
 CONFIG_QEMU_XEN     := @qemu_xen@
 CONFIG_BLKTAP1      := @blktap1@
 CONFIG_BLKTAP2      := @blktap2@
+EXTRA_CFLAGS_XEN_TOOLS        := @EXTRA_CFLAGS_XEN_TOOLS@
+EXTRA_CFLAGS_QEMU_TRADITIONAL := @EXTRA_CFLAGS_QEMU_TRADITIONAL@
+EXTRA_CFLAGS_QEMU_XEN         := @EXTRA_CFLAGS_QEMU_XEN@
 CONFIG_QEMUU_EXTRA_ARGS:= @EXTRA_QEMUU_CONFIGURE_ARGS@
 CONFIG_REMUS_NETBUF := @remus_netbuf@
 
diff --git a/tools/Rules.mk b/tools/Rules.mk
index 87a56dc..228f9a2 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -88,7 +88,9 @@ endif
 CFLAGS-$(CONFIG_X86_32) += $(call cc-option,$(CC),-mno-tls-direct-seg-refs)
 CFLAGS += $(CFLAGS-y)
 
+ifneq ($(skip_extra_cflags),y)
 CFLAGS += $(EXTRA_CFLAGS_XEN_TOOLS)
+endif
 
 INSTALL_PYTHON_PROG = \
 	$(XEN_ROOT)/tools/python/install-wrap "$(PYTHON_PATH)" $(INSTALL_PROG)
diff --git a/tools/configure.ac b/tools/configure.ac
index 98b2455..0fe9f85 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -223,6 +223,27 @@ AC_ARG_WITH([system-ovmf],
 ],[])
 AC_SUBST(ovmf_path)
 
+AC_ARG_WITH([extra-cflags-tools],
+    AS_HELP_STRING([--with-extra-cflags-tools="EXTRA_CFLAGS"],
+       [Additional CFLAGS to be used to build tools.]),
+       [EXTRA_CFLAGS_XEN_TOOLS=$withval],
+       [EXTRA_CFLAGS_XEN_TOOLS=])
+AC_SUBST(EXTRA_CFLAGS_XEN_TOOLS)
+
+AC_ARG_WITH([extra-cflags-qemu-traditional],
+    AS_HELP_STRING([--with-extra-cflags-qemu-traditional="EXTRA_CFLAGS"],
+       [Additional CFLAGS to be used to build qemu-traditional.]),
+       [EXTRA_CFLAGS_QEMU_TRADITIONAL=$withval],
+       [EXTRA_CFLAGS_QEMU_TRADITIONAL=])
+AC_SUBST(EXTRA_CFLAGS_QEMU_TRADITIONAL)
+
+AC_ARG_WITH([extra-cflags-qemu-upstream],
+    AS_HELP_STRING([--with-extra-cflags-qemu-upstream="EXTRA_CFLAGS"],
+       [Additional CFLAGS to be used to build qemu-upstream.]),
+       [EXTRA_CFLAGS_QEMU_XEN=$withval],
+       [EXTRA_CFLAGS_QEMU_XEN=])
+AC_SUBST(EXTRA_CFLAGS_QEMU_XEN)
+
 AC_ARG_WITH([extra-qemuu-configure-args],
     AS_HELP_STRING([--with-extra-qemuu-configure-args@<:@="--ARG1 ..."@:>@],
        [List of additional configure options for upstream qemu]),[
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index 26bbddc..e3bf480 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -3,7 +3,7 @@ override XEN_TARGET_ARCH = x86_32
 
 # User-supplied CFLAGS are not useful here.
 CFLAGS =
-EXTRA_CFLAGS_XEN_TOOLS =
+skip_extra_cflags := y
 
 include $(XEN_ROOT)/tools/Rules.mk

Re: [PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[Olaf Hering]

On Thu, Oct 09, Olaf Hering wrote:

> +AC_ARG_WITH([extra-cflags-tools],
> +    AS_HELP_STRING([--with-extra-cflags-tools="EXTRA_CFLAGS"],
> +       [Additional CFLAGS to be used to build tools.]),
> +       [EXTRA_CFLAGS_XEN_TOOLS=$withval],
> +       [EXTRA_CFLAGS_XEN_TOOLS=])
> +AC_SUBST(EXTRA_CFLAGS_XEN_TOOLS)

I wonder why all the other --with-foo are not done that way as well. It
looks like they would take the variable from the environment and do
AC_SUBST(). IMO private variables like seabios_path, ovmf_path,
EXTRA_QEMUU_CONFIGURE_ARGS should not be passed via environment. I also
question if --with-foo=no is useful at all. My suggestion is to convert
these three to the style quoted above: a --with-foo=val assigns $val to
the given private variable, otherwise the private variable is
initialized empty.

What do you think?

Olaf

Re: [PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[Ian Campbell]

On Fri, 2014-10-10 at 08:55 +0200, Olaf Hering wrote:
> On Thu, Oct 09, Olaf Hering wrote:
> 
> > +AC_ARG_WITH([extra-cflags-tools],
> > +    AS_HELP_STRING([--with-extra-cflags-tools="EXTRA_CFLAGS"],
> > +       [Additional CFLAGS to be used to build tools.]),
> > +       [EXTRA_CFLAGS_XEN_TOOLS=$withval],
> > +       [EXTRA_CFLAGS_XEN_TOOLS=])
> > +AC_SUBST(EXTRA_CFLAGS_XEN_TOOLS)
> 
> I wonder why all the other --with-foo are not done that way as well. It
> looks like they would take the variable from the environment and do
> AC_SUBST(). IMO private variables like seabios_path, ovmf_path,
> EXTRA_QEMUU_CONFIGURE_ARGS should not be passed via environment.

It looks to me like seabios_path isn't, so I'm confused about what you
are trying to suggest:

AC_ARG_WITH([system-seabios],
    AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@],
       [Use system supplied seabios PATH instead of building and installing
        our own version]),[
    case $withval in
        no) seabios_path= ;;
        *)  seabios_path=$withval ;;
    esac
],[])
AC_SUBST(seabios_path)

Doesn't take anything fro the env, does it?

>  I also
> question if --with-foo=no is useful at all.

Isn't $withval=no the result of passing --without-foo?

>  My suggestion is to convert
> these three to the style quoted above: a --with-foo=val assigns $val to
> the given private variable, otherwise the private variable is
> initialized empty.
> 
> What do you think?

I have no real opinion one way or the other, since I'm not really sure
what the practical implication of what you are suggesting is.

But it certainly doesn't sound like 4.5 material at this point.

Ian.

Re: [PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[Olaf Hering]

On Mon, Oct 20, Ian Campbell wrote:

> It looks to me like seabios_path isn't, so I'm confused about what you
> are trying to suggest:
> 
> AC_ARG_WITH([system-seabios],
>     AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@],
>        [Use system supplied seabios PATH instead of building and installing
>         our own version]),[
>     case $withval in
>         no) seabios_path= ;;
>         *)  seabios_path=$withval ;;
>     esac
> ],[])
> AC_SUBST(seabios_path)
> 
> Doesn't take anything fro the env, does it?

I just tried today:

# git clean -dfx
# env EXTRA_QEMUU_CONFIGURE_ARGS=XXXXXXXXXXXXXXXX ./configure &>/dev/null
#  grep XXXXXXXXXXXXXXXX config/Tools.mk
CONFIG_QEMUU_EXTRA_ARGS:= XXXXXXXXXXXXXXXX

The empty [] in AC_ARG_WITH() is what is supposed to be executed if
--with*-foo was not specified. In this case nothing will be done with
EXTRA_QEMUU_CONFIGURE_ARGS. Later AC_SUBST() takes the environment
variable and does the substitution.  And the same happens if I replace
"EXTRA_QEMUU_CONFIGURE_ARGS" with "seabios_path" in my example.

I think this should not happen. To fix it the empty [] should be filled
with "variable=", like "seabios_path=".

What do you think?

> >  I also
> > question if --with-foo=no is useful at all.
> 
> Isn't $withval=no the result of passing --without-foo?

Yes, looks like it. I will update my patch to handle also the "no" due
to --without-extra-cflags-*.


Olaf

Re: [PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[Ian Campbell]

On Thu, 2014-10-09 at 11:43 +0200, Olaf Hering wrote:

Please do not use the "for-xen-4.5" without making some sort of argument
as to why it should be acceptable for 4.5, usually after the ---
separator.

To me this looks like a feature, but in any case neither Konrad nor I
should be expected to make the argument for you.

Ian.

Re: [PATCH for-xen-4.5] Add configure --with-extra-cflags-*

[Olaf Hering]

On Mon, Oct 20, Ian Campbell wrote:

> On Thu, 2014-10-09 at 11:43 +0200, Olaf Hering wrote:
> 
> Please do not use the "for-xen-4.5" without making some sort of argument
> as to why it should be acceptable for 4.5, usually after the ---
> separator.
> 
> To me this looks like a feature, but in any case neither Konrad nor I
> should be expected to make the argument for you.

The point is that the build system should not take "random" environment
variables. Instead such things should be passed in via configure. Now
that configure does already most of the settings it should do also this.

Olaf