Re: [Xen-devel] [PATCH v5 11/14] autoconf: xen: move standard variables to a generic place

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-21 at 23:54 +0200, Luis R. Rodriguez wrote:
> > > new file mode 100644
> > > index 0000000..717fcd1
> > > --- /dev/null
> > > +++ b/m4/expand_config.m4
> > > @@ -0,0 +1,61 @@
> > > +AC_DEFUN([AX_XEN_EXPAND_CONFIG], [
> > > +dnl expand these early so we can use this for substitutions
> > > +test "x$prefix" = "xNONE" && prefix=$ac_default_prefix
> > > +test "x$exec_prefix" = "xNONE" && exec_prefix=$ac_default_prefix
> > > +
> > > +BINDIR=$prefix/bin
> > > +AC_SUBST(BINDIR)
> > > +
> > > +SBINDIR=$prefix/sbin
> > > +AC_SUBST(SBINDIR)
> > 
> > TBH I thought that after going through all this fuss --bindir and
> > --sbindir might worl.
> 
> We can surely do that but that is an atomic change and I'd much prefer
> this was treated separately as a separate patch given that this would
> change existing behavior. If you want me to roll this in I can do so
> but do not think that's a good idea for regression analysis.

Fair enough, if you feel like doing a followup that would be fine, or
else we can leave it for now.

> > > diff --git a/stubdom/configure.ac b/stubdom/configure.ac
> > > index 6468203..d6b0fbf 100644
> > > --- a/stubdom/configure.ac
> > > +++ b/stubdom/configure.ac
> > > @@ -16,6 +16,9 @@ m4_include([../m4/features.m4])
> > >  m4_include([../m4/path_or_fail.m4])
> > >  m4_include([../m4/depends.m4])
> > >  m4_include([../m4/fetcher.m4])
> > > +m4_include([../m4/expand_config.m4])
> > > +
> > > +AX_XEN_EXPAND_CONFIG()
> > 
> > Doesn't only the toplevel configure.ac need this, because you've put the
> > things into config/Toplevel.mk (soon to be config/Paths.mk).
> 
> I had similar expectations and was equally surprised to see this didn't fly.

Possibly this will go away with moving things to config/Path.mk instead
of Toplevel.mk?

> 
> Here's the latest version of the patch let me know what you folks think,

It's much better, thanks. Still some comments though.
 

> diff --git a/Config.mk b/Config.mk
> index 6a93533..6580e47 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -175,10 +175,17 @@ define buildmakevars2file-closure

You should probably rename the function, both to make it match the new
behaviour and to root out all the existing uses.
 
>                 SBINDIR BINDIR LIBEXEC LIBDIR SHAREDIR PRIVATE_BINDIR     \
>                 XENFIRMWAREDIR XEN_CONFIG_DIR XEN_SCRIPT_DIR XEN_LOCK_DIR \
>                 XEN_RUN_DIR XEN_PAGING_DIR,                               \
> -               echo "$(var)=\"$($(var))\"" >>$(1).tmp;)        \
> +               echo "#define $(var) \"$($(var))\"" >>$(1).tmp;)        \
>       $(call move-if-changed,$(1).tmp,$(1))
>  endef
>  
> +empty_genpath = $(eval $(call empty_genpath-closure))
> +define empty_genpath-closure
> +    .PHONY: genpath-skip
> +    genpath-skip:
> +     @# do nothing
> +endef

What is this for?

> +
>  ifeq ($(debug_symbols),y)
>  CFLAGS += -g
>  endif
> diff --git a/Makefile b/Makefile
> index 41dabbf..fa3427e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -39,10 +39,26 @@ endif
>  test:
>       $(MAKE) -C tools/python test
>  
> +XEN_ENV_HEADER="$(XEN_ROOT)/config/xen-environment-header"
> +CONFIG_RUN="$(XEN_ROOT)/config.status"
> +
> +genpath-target = $(call buildmakevars2file,$(XEN_ENV_HEADER))
> +$(eval $(genpath-target))

I intended for you to use this in each of the submakefiles which needs
it rather than at the toplevel, to avoid interactions with the xen and
mini-os subtrees. Is there some particular reason to put them here?


> diff --git a/config/xen-environment-scripts.in 
> b/config/xen-environment-scripts.in
> new file mode 100644
> index 0000000..06913f8
> --- /dev/null
> +++ b/config/xen-environment-scripts.in

This appears to be completely duplicating Paths.mk.


> +m4_include([../m4/paths.m4])
> +
> +AX_XEN_EXPAND_CONFIG()

I think with your latest changes you should try again removing these
from all the sub-configures. If it doesn't work please show the error
and we'll see if we can figure something out.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel