648980 - Add build system hooks for allowing applications to provide their own configure options and autoconf variables

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

As discussed in bug 648979 we want to move the comm-central build system underneath mozilla-central.

We'd therefore like to be able to add hooks so that any application specified via --enable-application can supply configure script/options for additional arguments and have these picked up in autoconf.mk and/or the mozilla-config.h file.

Comment 1

[Mike Hommey [:glandium]]

The problem I see is that adding configure script/options for additional arguments would require running autoconf on them. We could aggregate these snippets and the main configure.in before running autoconf, but that would mean a different toplevel configure in m-c for each application, which seems backwards. So, why not "simply" have a (minimalist) sub-configure under the application directory, and similarly have sub-autoconf.mk/mozilla-config.h, that would be included from the main ones when they exist (since ff and xulrunner wouldn't have those, at least for now)?

Comment 2

[(not currently active) Ted Mielczarek]

That's a potential solution for this bug. I was hand-wavy on the details, we can implement it in whatever way gives the desired end result, but doesn't require comm-central to maintain copies of the logic in m-c.

Comment 3

[Benjamin Smedberg]

What I would like to do here is have the generic compiler-selection and library-finding logic currently in configure.in be put into an autoconf module (an .m4 file), which we can then include in /configure.in js/src/configure.in and c-c/configure.in

It would also make lots of stuff significantly easier to read and understand.

Comment 4

[Justin Wood (:Callek)]

(In reply to comment #1)
> The problem I see is that adding configure script/options for additional
> arguments would require running autoconf on them.

A way to do that could also be giving an app a specific m4 macro to expando into configure, but we could also theoretically let configure "pass down" to a subconfigure any options not handled by itself, and then let any leftover (completely unhandled) options warn/error at the end of the main confugure.

Comment 5

[Mark Banner (:standard8)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> What I would like to do here is have the generic compiler-selection and
> library-finding logic currently in configure.in be put into an autoconf
> module (an .m4 file), which we can then include in /configure.in
> js/src/configure.in and c-c/configure.in
> 
> It would also make lots of stuff significantly easier to read and understand.

Whilst that would be good, it doesn't actually handle this bug, where we'd like to allow apps to add their own options to the mozilla-central configure.in.

Comment 6

[Mark Banner (:standard8)]

Ok, I think comments 1 and 4 are probably indicating the way to go here. I'm going to move this back into Mailnews Core land - I think we can prototype this in the comm-central build system, and then port the implementation across to mozilla-central, otherwise we'll be trying to implement something in mozilla-central that we can't test.

Comment 7

[Joshua Cranmer [:jcranmer]]

After some discussion on IRC, here are some more thoughts about implementation details:

By the time we get to the level of what an application's subconfigure would like to do in its configure, we boil down to needing the following features in the subconfigure:
* AC_SUBST and AC_DEFINE
* Run some shell scripts
* Run a subconfigure (AC_OUTPUT_SUBDIRS with different arguments, effectively)
* Use argument handling options (MOZ_ARG_*)

The problem with full subconfigure scripts is that they would have their own AC_SUBST and AC_DEFINE lists (and hence autoconf.mk/*-config.h). But comm-central still needs some of the regular mozilla-central AC_SUBST/AC_DEFINE for things like platform detection or packaging requirements. Without any other support, this would cascade into duplication of the build system.

Recalling that configure.in is merely a bash script that gets processed by m4. If we provide definitions for a limited set of autoconf functionality, we can turn the subconfigure into a script that gets sourced by the main configure that would add the sub AC_SUBST/AC_DEFINE options to the main ones as well.

Comment 8

[Mark Banner (:standard8)]

Joshua, would you mind prototyping what you said in comment 7, then we can see what it looks like and it'd be easier to expand it out?

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: Rough prototype of subconfigure via m4

This doesn't have a hookup to the actual main configure yet, since actually splicing in the AC_SUBST is difficult without overriding AC_OUTPUT at the end. Also, getting help to work properly would be a royal pain in the behind, since ac_help is evaluated way too early.

The main idea I have for this approach is to have main configure provide
add_sub_subst and add_sub_def functions for the subconfigure script (I really hate writing m4, if you couldn't guess). Then, the actual script would be created by running:
m4 build/autoconf/subconfigure.m4 [mozilla/]build/autoconf/altoptions.m4 configure.in > $tmpscript
. $tmpscript

I haven't decided where to do this yet, but towards the end of the script (before the massive AC_SUBST list?) sounds about right, which would let the subconfigure override some variables if necessary.

The subconfigure should be most of c-c's additional arguments (I'm missing one, IIRC), so it should reflect most of the necessary complexity...

Comment 10

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #9)
> m4 build/autoconf/subconfigure.m4 [mozilla/]build/autoconf/altoptions.m4
> configure.in > $tmpscript
> . $tmpscript

Why would you need to generate the script at build time? Why can't it be built at landing time like other configures?

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #10)
> (In reply to Joshua Cranmer [:jcranmer] from comment #9)
> > m4 build/autoconf/subconfigure.m4 [mozilla/]build/autoconf/altoptions.m4
> > configure.in > $tmpscript
> > . $tmpscript
> 
> Why would you need to generate the script at build time? Why can't it be
> built at landing time like other configures?

Given the long-term goals of this bug, mozilla-central would have to be fine with hard-coding client.mk/mach to generate the subconfigure scripts for comm-central if they exist...

Comment 12

[Joshua Cranmer [:jcranmer]]

Attachment: End-to-end prototype

I can compile and run Thunderbird with this patch, and it looks like it works well enough: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=cb055d60308b

This is probably as much playing as I can do with the patch for a while; if someone else wants to pick this up, please feel free to.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 667081 [details] [diff] [review]
End-to-end prototype

Review of attachment 667081 [details] [diff] [review]:
-----------------------------------------------------------------

::: aclocal.m4
@@ +4,5 @@
>  dnl
>  
>  builtin(include, mozilla/build/autoconf/toolchain.m4)dnl
>  builtin(include, mozilla/build/autoconf/ccache.m4)dnl
> +builtin(include, build/autoconf/config.status.m4)dnl

Why not include mozilla/build/autoconf/config.status.m4?

::: build/autoconf/subconfigure.m4
@@ +1,2 @@
> +dnl We are not running in a real autoconf environment. So we're using real m4
> +dnl here, not the crazier environment that autoconf provides.

Might be better to put this file somewhere else, if it's not meant to be included in aclocal.m4.

::: configure.in
@@ +4187,5 @@
>  fi
>  
> +if test -f "${srcdir}/${MOZ_BUILD_APP}/configure.in" ; then
> +add_sub_subst() {
> +  _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"

I don't see where this variable is used. Also, why does this function have to be defined in configure?

@@ +4189,5 @@
> +if test -f "${srcdir}/${MOZ_BUILD_APP}/configure.in" ; then
> +add_sub_subst() {
> +  _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"
> +}
> +tmpscript=/tmp/a.sh

Using a hardcoded temporary file name is not a good idea.

@@ +4193,5 @@
> +tmpscript=/tmp/a.sh
> +m4 "${srcdir}/build/autoconf/subconfigure.m4" \
> +   "${srcdir}/mozilla/build/autoconf/altoptions.m4" \
> +   "${srcdir}/${MOZ_BUILD_APP}/configure.in" > $tmpscript
> +. $tmpscript

(In reply to Joshua Cranmer [:jcranmer] from comment #11)
> Given the long-term goals of this bug, mozilla-central would have to be fine
> with hard-coding client.mk/mach to generate the subconfigure scripts for
> comm-central if they exist...

It's not clear to me why this would have to be generated in m-c.

Comment 14

[Mike Hommey [:glandium]]

> (In reply to Joshua Cranmer [:jcranmer] from comment #11)
> > Given the long-term goals of this bug, mozilla-central would have to be fine
> > with hard-coding client.mk/mach to generate the subconfigure scripts for
> > comm-central if they exist...
> 
> It's not clear to me why this would have to be generated in m-c.

Forget this part.

Comment 15

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #13)
> ::: aclocal.m4
> @@ +4,5 @@
> >  dnl
> >  
> >  builtin(include, mozilla/build/autoconf/toolchain.m4)dnl
> >  builtin(include, mozilla/build/autoconf/ccache.m4)dnl
> > +builtin(include, build/autoconf/config.status.m4)dnl
> 
> Why not include mozilla/build/autoconf/config.status.m4?

I didn't want to have two patches for the experiment (one for m-c and one for c-c), so I cloned m-c's copy of config.status.m4 and made the changes I needed to in c-c. I believe there were only a few changes in c-c's version (adding 115-120).

> ::: build/autoconf/subconfigure.m4
> @@ +1,2 @@
> > +dnl We are not running in a real autoconf environment. So we're using real m4
> > +dnl here, not the crazier environment that autoconf provides.
> 
> Might be better to put this file somewhere else, if it's not meant to be
> included in aclocal.m4.

I do agree that placing somewhere else might be better, but it is autoconf related after all...

> ::: configure.in
> @@ +4187,5 @@
> >  fi
> >  
> > +if test -f "${srcdir}/${MOZ_BUILD_APP}/configure.in" ; then
> > +add_sub_subst() {
> > +  _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"
> 
> I don't see where this variable is used. Also, why does this function have
> to be defined in configure?

This gets used in the modified config.status.m4 file:
dnl Add in the output from the subconfigure script
for ac_subst_arg in $_subconfigure_ac_subst_args; do
  variable='$'$ac_subst_arg
  echo "    (''' $ac_subst_arg ''', r''' `eval echo $variable` ''')," >> $CONFIG_STATUS
done

> 
> @@ +4189,5 @@
> > +if test -f "${srcdir}/${MOZ_BUILD_APP}/configure.in" ; then
> > +add_sub_subst() {
> > +  _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"
> > +}
> > +tmpscript=/tmp/a.sh
> 
> Using a hardcoded temporary file name is not a good idea.

Oops, that wasn't meant to be hardcoded.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 667081 [details] [diff] [review]
End-to-end prototype

Review of attachment 667081 [details] [diff] [review]:
-----------------------------------------------------------------

This feels like a good approach and should be a lot easier to maintain than our current system.

::: mail/configure.in
@@ +1,1 @@
> +dnl -*- Mode: Autoconf; tab-width: 4; indent-tabs-mode: nil; -*-

I've a feeling we've moved to 2-space indentation in configure now.

Comment 17

[Joshua Cranmer [:jcranmer]]

Attachment: m4-based subconfigure

This is a patch for mozilla-central which adds the hook. According to try, all platforms appear to keep building with this patch, and I can verify that it does what we want when I apply comm-central-specific subconfigures.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 704620 [details] [diff] [review]
m4-based subconfigure

Review of attachment 704620 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take another look with fresh eyes, but here are some nits already:

::: configure.in
@@ +4392,5 @@
> +  add_sub_subst() {
> +    _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"
> +  }
> +  : ${TMPDIR=/tmp}
> +  tmpscript=`mktemp ${TMPDIR}/subscript.XXXXXX` || exit 1

mktemp already uses TMPDIR (and falls back to /tmp) when you give it a file template (without a directory)

::: js/src/build/autoconf/subconfigure.m4
@@ +3,5 @@
> +
> +dnl Autoconf expects [] for quotes; give it to them
> +changequote([, ])
> +
> +dnl AC_DEFUN is provided to use instead of define in autoconf. Provide it to.

too

@@ +31,5 @@
> +dnl Override for AC_DEFINE. This turns out to be the simple one. :-)
> +AC_DEFUN([AC_DEFINE],
> +[
> +cat >>confdefs.h <<\EOF
> +[#define] $1 ifelse($#, 2, [$2], $#, 3, [$2], 1) 

trailing whitespace

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 704620 [details] [diff] [review]
m4-based subconfigure

Review of attachment 704620 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

::: build/autoconf/subconfigure.m4
@@ +39,5 @@
> +EOF
> +])
> +
> +dnl AC_OUTPUT_SUBDIRS(subdirectory)
> +define([AC_OUTPUT_SUBDIRS],

I would be more comfortable if we didn't have such a huge AC_OUTPUT_SUBDIRS. Assuming you copied from autodonf, I see that essentially, it's going over the m4 arguments in ashell for loop, which means that if you do AC_OUTPUT_SUBDIR($variable) in the main configure.in, and make the subconfigure just set $variable, it should just work. I'd be more comfortable with that.

::: configure.in
@@ +4392,5 @@
> +  add_sub_subst() {
> +    _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"
> +  }
> +  : ${TMPDIR=/tmp}
> +  tmpscript=`mktemp ${TMPDIR}/subscript.XXXXXX` || exit 1

mktemp already uses TMPDIR (and falls back to /tmp) when you give it a file template (without a directory)

::: js/src/build/autoconf/subconfigure.m4
@@ +3,5 @@
> +
> +dnl Autoconf expects [] for quotes; give it to them
> +changequote([, ])
> +
> +dnl AC_DEFUN is provided to use instead of define in autoconf. Provide it to.

too

@@ +31,5 @@
> +dnl Override for AC_DEFINE. This turns out to be the simple one. :-)
> +AC_DEFUN([AC_DEFINE],
> +[
> +cat >>confdefs.h <<\EOF
> +[#define] $1 ifelse($#, 2, [$2], $#, 3, [$2], 1) 

trailing whitespace

Comment 20

[Joshua Cranmer [:jcranmer]]

Attachment: m4-based subconfigure

The automatically looking up tmpdir only works if you use -t, so I've switched to that flag instead.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 709418 [details] [diff] [review]
m4-based subconfigure

Review of attachment 709418 [details] [diff] [review]:
-----------------------------------------------------------------

Almost r+ :)

::: configure.in
@@ +4347,5 @@
>  
> +# Allow the application to provide a subconfigure script
> +if test -f "${srcdir}/${MOZ_BUILD_APP}/configure.in" ; then
> +  add_sub_subst() {
> +    _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"

Any particular reason not to do that in subconfigure.m4?

@@ +4350,5 @@
> +  add_sub_subst() {
> +    _subconfigure_ac_subst_args="$_subconfigure_ac_subst_args $1"
> +  }
> +  do_output_subdirs() {
> +    AC_OUTPUT_SUBDIRS([$1])

I'm not sure it's good to run subconfigure from here, considering it involves config.cache. I'd be more comfortable with doing AC_OUTPUT_SUBDIRS around the end of configure.in, in which case do_output_subdirs would just add the given subdir to a variable, and we'd add AC_OUTPUT_SUBDIRS([$that_variable]) closer to the end of configure.in.

Comment 22

[Joshua Cranmer [:jcranmer]]

Attachment: m4-based subconfigure

I've moved the subconfigure much later, but I'm not sure where the best placement of it is really. The problem with delaying it so late is that you lose control over the environment of the subconfigure. In the end, the main reason for this is so that we can configure the LDAP c-sdks (which only needs custom arguments, not environment munging), so this should be sufficient.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 718076 [details] [diff] [review]
m4-based subconfigure

Review of attachment 718076 [details] [diff] [review]:
-----------------------------------------------------------------

If that's enough for your purpose, let's go for it.

Comment 24

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13ccc41033ec

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13ccc41033ec

Comment 26

[Joshua Cranmer [:jcranmer]]

Attachment: Work around mktemp not existing on windows

Apparently, mozillabuild doesn't contain an equivalent to GNU coreutils.

Comment 27

[Joshua Cranmer [:jcranmer]]

Followup pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4504520f7f24

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4504520f7f24

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 726705 [details] [diff] [review]
Work around mktemp not existing on windows

Review of attachment 726705 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +4381,5 @@
>       fi
>      _subconfigure_subdir="$1"
>      _subconfigure_config_args="$ac_configure_args"
>    }
> +  tmpscript=`python -c 'import os, tempfile; print tempfile.mktemp(prefix="subscript.").replace(os.sep, "/")'` || exit 1

Will this work if python is python3?

Comment 30

[Mike Hommey [:glandium]]

Ah good point, it might be better to use $PYTHON

Comment 31

[Landry Breuil (:gaston)]

A bit late at the party, but i'm confirming it breaks as-is on systems where there's no python in the path, i can't run configure within c-c on OpenBSD. Whoever beats me to fix it to use $PYTHON gets a pony.

Comment 32

[Landry Breuil (:gaston)]

Attachment: use $PYTHON instead of python

Followup to the followup..

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 735916 [details] [diff] [review]
use $PYTHON instead of python

Review of attachment 735916 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the configure.in part.
You should however probably file it as a followup of this bug. Patches on fixed bugs are confusing.

::: security/nss/lib/ssl/ssl3con.c
@@ +27,5 @@
>  
>  #include "pk11func.h"
>  #include "secmod.h"
>  #ifndef NO_PKCS11_BYPASS
> +#error "baaaaadtothebone"

I don't think erroring here is something the NSS team would like.