diff mbox series

[v3] configure: preserve various environment variables in config.status

Message ID 20180904123603.10016-1-berrange@redhat.com
State New
Headers show
Series [v3] configure: preserve various environment variables in config.status | expand

Commit Message

Daniel P. Berrangé Sept. 4, 2018, 12:36 p.m. UTC 
The config.status script is auto-generated by configure upon
completion. The intention is that config.status can be later invoked by
the developer directly, or by make indirectly, to re-detect the same
environment that configure originally used.

The current config.status script, however, only contains a record of the
command line arguments to configure. Various environment variables have
an effect on what configure will find. In particular PKG_CONFIG_LIBDIR &
PKG_CONFIG_PATH vars will affect what libraries pkg-config finds. The
PATH var will affect what toolchain binaries and XXXX-config scripts are
found. The LD_LIBRARY_PATH var will affect what libraries are
found. Most commands have env variables that will override the name/path
of the default version configure finds.

All these key env variables should be recorded in the config.status script.

Autoconf would also preserve CFLAGS, LDFLAGS, LIBS, CPPFLAGS, but QEMU
deals with those differently, expecting extra flags to be set using
configure args, rather than env variables. At the end of the script we
also don't have the original values of those env vars, as we modify them
during configure.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 configure | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Previously sent 3 (!) years ago:

  v1: http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg03953.html
  v2: http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04074.html

Changed in v3:

 - Added 'unset' code (Stefan)
 - Clarify CFLAGS handling (Eric)

Comments

Eric Blake Sept. 4, 2018, 4:21 p.m. UTC | #1 
On 09/04/2018 07:36 AM, Daniel P. Berrangé wrote:
> The config.status script is auto-generated by configure upon
> completion. The intention is that config.status can be later invoked by
> the developer directly, or by make indirectly, to re-detect the same
> environment that configure originally used.
> 
> The current config.status script, however, only contains a record of the
> command line arguments to configure. Various environment variables have
> an effect on what configure will find. In particular PKG_CONFIG_LIBDIR &
> PKG_CONFIG_PATH vars will affect what libraries pkg-config finds. The
> PATH var will affect what toolchain binaries and XXXX-config scripts are
> found. The LD_LIBRARY_PATH var will affect what libraries are
> found. Most commands have env variables that will override the name/path
> of the default version configure finds.
> 
> All these key env variables should be recorded in the config.status script.
> 
> Autoconf would also preserve CFLAGS, LDFLAGS, LIBS, CPPFLAGS, but QEMU
> deals with those differently, expecting extra flags to be set using
> configure args, rather than env variables. At the end of the script we
> also don't have the original values of those env vars, as we modify them
> during configure.

The latter half could be overcome by creating envvars named 'FOO_saved' 
near the beginning of configure, if they prove important. I still find 
it odd that we aren't precisely mirroring the (nice!) semantics that 
autoconf users have come to rely on, but this is definitely a step 
closer, and solves the immediate problem documented in the commit 
message, so I see no reason to wait for even more complexity to be added 
without a definite user of such complexity.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>   configure | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> Previously sent 3 (!) years ago:
> 
>    v1: http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg03953.html
>    v2: http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04074.html
> 
> Changed in v3:
> 
>   - Added 'unset' code (Stefan)
>   - Clarify CFLAGS handling (Eric)
> 
> 
> diff --git a/configure b/configure
> index e7bddc04b0..718e89724a 100755
> --- a/configure
> +++ b/configure
> @@ -7518,6 +7518,46 @@ cat <<EOD >config.status
>   # Compiler output produced by configure, useful for debugging
>   # configure, is in config.log if it exists.
>   EOD
> +
> +preserve_env() {
> +    envname=$1
> +
> +    eval envval=\$$envname
> +
> +    if test -n "$envval"

We are not catering to the case where $envname is set but explicitly 
empty (autoconf is able to track that case).  However...

> +    then
> +	echo "$envname='$envval'" >> config.status
> +	echo "export $envname" >> config.status
> +    else
> +	echo "unset $envname" >> config.status
> +    fi
> +}
> +
> +# Preserve various env variables that influence what
> +# features/build target configure will detect
> +preserve_env AR
> +preserve_env AS
> +preserve_env CC
> +preserve_env CPP
> +preserve_env CXX
> +preserve_env INSTALL
> +preserve_env LD
> +preserve_env LD_LIBRARY_PATH
> +preserve_env LIBTOOL
> +preserve_env MAKE
> +preserve_env NM
> +preserve_env OBJCOPY
> +preserve_env PATH
> +preserve_env PKG_CONFIG
> +preserve_env PKG_CONFIG_LIBDIR
> +preserve_env PKG_CONFIG_PATH
> +preserve_env PYTHON
> +preserve_env SDL_CONFIG
> +preserve_env SDL2_CONFIG
> +preserve_env SMBD
> +preserve_env STRIP
> +preserve_env WINDRES

...of all of the variables we are marking precious (at least, that's the 
terminology autoconf uses for this behavior), I seriously doubt anyone 
would ever purposefully set the variable to empty with an expectation of 
things working.  So, we are safe in assuming that if a variable matters, 
it is either not set or else set to a non-empty value.

Thus my R-b from years ago still holds.
Thomas Huth Sept. 4, 2018, 6:37 p.m. UTC | #2 
On 2018-09-04 14:36, Daniel P. Berrangé wrote:
> The config.status script is auto-generated by configure upon
> completion. The intention is that config.status can be later invoked by
> the developer directly, or by make indirectly, to re-detect the same
> environment that configure originally used.
> 
> The current config.status script, however, only contains a record of the
> command line arguments to configure. Various environment variables have
> an effect on what configure will find. In particular PKG_CONFIG_LIBDIR &
> PKG_CONFIG_PATH vars will affect what libraries pkg-config finds. The
> PATH var will affect what toolchain binaries and XXXX-config scripts are
> found. The LD_LIBRARY_PATH var will affect what libraries are
> found. Most commands have env variables that will override the name/path
> of the default version configure finds.
> 
> All these key env variables should be recorded in the config.status script.
> 
> Autoconf would also preserve CFLAGS, LDFLAGS, LIBS, CPPFLAGS, but QEMU
> deals with those differently, expecting extra flags to be set using
> configure args, rather than env variables. At the end of the script we
> also don't have the original values of those env vars, as we modify them
> during configure.

It's a little bit inconsequent to ignore CFLAGS etc., but to save the CC
and CXX variable, since we've got a configure option for setting $CC and
$CXX, too ("--cc" and "--cxx"). Maybe we should clean that up one day,
but that's for another patch...

For this one here:

Reviewed-by: Thomas Huth <thuth@redhat.com>
Peter Maydell Sept. 4, 2018, 9:16 p.m. UTC | #3 
On 4 September 2018 at 17:21, Eric Blake <eblake@redhat.com> wrote:
> I still find it odd
> that we aren't precisely mirroring the (nice!) semantics that autoconf users
> have come to rely on

I think that's more the result of (a) most people don't know the
details of the autoconf semantics and (b) configure works well-enough
that nobody spends more time messing with it than they need to. I think
as an aspiration "behave like an autoconf configure script" is
probably a sensible goal.

thanks
-- PMM
Daniel P. Berrangé Sept. 5, 2018, 8:39 a.m. UTC | #4 
On Tue, Sep 04, 2018 at 10:16:26PM +0100, Peter Maydell wrote:
> On 4 September 2018 at 17:21, Eric Blake <eblake@redhat.com> wrote:
> > I still find it odd
> > that we aren't precisely mirroring the (nice!) semantics that autoconf users
> > have come to rely on
> 
> I think that's more the result of (a) most people don't know the
> details of the autoconf semantics and (b) configure works well-enough
> that nobody spends more time messing with it than they need to. I think
> as an aspiration "behave like an autoconf configure script" is
> probably a sensible goal.

autoconf is so complex that we'll never have behavioural parity with it,
unless we actually rewrite QEMU to use autoconf, but I'm not really
going to advocate that. If there are easy changes we can do to make it
look a little more like autoconf, that's fine, but I wouldn't spend any
non-trivial amount of time on it as there's better uses of our time.

Regards,
Daniel
Paolo Bonzini Sept. 11, 2018, 1:11 p.m. UTC | #5 
On 04/09/2018 14:36, Daniel P. Berrangé wrote:
> The config.status script is auto-generated by configure upon
> completion. The intention is that config.status can be later invoked by
> the developer directly, or by make indirectly, to re-detect the same
> environment that configure originally used.
> 
> The current config.status script, however, only contains a record of the
> command line arguments to configure. Various environment variables have
> an effect on what configure will find. In particular PKG_CONFIG_LIBDIR &
> PKG_CONFIG_PATH vars will affect what libraries pkg-config finds. The
> PATH var will affect what toolchain binaries and XXXX-config scripts are
> found. The LD_LIBRARY_PATH var will affect what libraries are
> found. Most commands have env variables that will override the name/path
> of the default version configure finds.
> 
> All these key env variables should be recorded in the config.status script.
> 
> Autoconf would also preserve CFLAGS, LDFLAGS, LIBS, CPPFLAGS, but QEMU
> deals with those differently, expecting extra flags to be set using
> configure args, rather than env variables. At the end of the script we
> also don't have the original values of those env vars, as we modify them
> during configure.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  configure | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> Previously sent 3 (!) years ago:
> 
>   v1: http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg03953.html
>   v2: http://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04074.html
> 
> Changed in v3:
> 
>  - Added 'unset' code (Stefan)
>  - Clarify CFLAGS handling (Eric)
> 
> 
> diff --git a/configure b/configure
> index e7bddc04b0..718e89724a 100755
> --- a/configure
> +++ b/configure
> @@ -7518,6 +7518,46 @@ cat <<EOD >config.status
>  # Compiler output produced by configure, useful for debugging
>  # configure, is in config.log if it exists.
>  EOD
> +
> +preserve_env() {
> +    envname=$1
> +
> +    eval envval=\$$envname
> +
> +    if test -n "$envval"
> +    then
> +	echo "$envname='$envval'" >> config.status
> +	echo "export $envname" >> config.status
> +    else
> +	echo "unset $envname" >> config.status
> +    fi
> +}
> +
> +# Preserve various env variables that influence what
> +# features/build target configure will detect
> +preserve_env AR
> +preserve_env AS
> +preserve_env CC
> +preserve_env CPP
> +preserve_env CXX
> +preserve_env INSTALL
> +preserve_env LD
> +preserve_env LD_LIBRARY_PATH
> +preserve_env LIBTOOL
> +preserve_env MAKE
> +preserve_env NM
> +preserve_env OBJCOPY
> +preserve_env PATH
> +preserve_env PKG_CONFIG
> +preserve_env PKG_CONFIG_LIBDIR
> +preserve_env PKG_CONFIG_PATH
> +preserve_env PYTHON
> +preserve_env SDL_CONFIG
> +preserve_env SDL2_CONFIG
> +preserve_env SMBD
> +preserve_env STRIP
> +preserve_env WINDRES
> +
>  printf "exec" >>config.status
>  printf " '%s'" "$0" "$@" >>config.status
>  echo ' "$@"' >>config.status
> 

Queued, thanks.

Paolo
diff mbox series

Patch

diff --git a/configure b/configure
index e7bddc04b0..718e89724a 100755
--- a/configure
+++ b/configure
@@ -7518,6 +7518,46 @@  cat <<EOD >config.status
 # Compiler output produced by configure, useful for debugging
 # configure, is in config.log if it exists.
 EOD
+
+preserve_env() {
+    envname=$1
+
+    eval envval=\$$envname
+
+    if test -n "$envval"
+    then
+	echo "$envname='$envval'" >> config.status
+	echo "export $envname" >> config.status
+    else
+	echo "unset $envname" >> config.status
+    fi
+}
+
+# Preserve various env variables that influence what
+# features/build target configure will detect
+preserve_env AR
+preserve_env AS
+preserve_env CC
+preserve_env CPP
+preserve_env CXX
+preserve_env INSTALL
+preserve_env LD
+preserve_env LD_LIBRARY_PATH
+preserve_env LIBTOOL
+preserve_env MAKE
+preserve_env NM
+preserve_env OBJCOPY
+preserve_env PATH
+preserve_env PKG_CONFIG
+preserve_env PKG_CONFIG_LIBDIR
+preserve_env PKG_CONFIG_PATH
+preserve_env PYTHON
+preserve_env SDL_CONFIG
+preserve_env SDL2_CONFIG
+preserve_env SMBD
+preserve_env STRIP
+preserve_env WINDRES
+
 printf "exec" >>config.status
 printf " '%s'" "$0" "$@" >>config.status
 echo ' "$@"' >>config.status