Closed Bug 846540 Opened 11 years ago Closed 11 years ago

Emasculate comm-central/configure.in

Categories

(MailNews Core :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 23.0

People

(Reporter: jcranmer, Assigned: jcranmer)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

Bug 648980 allows us to move all of our comm-central-specific logic into app-specific configures, so it's time to use it to eliminate all but the necessary stuff from configure.in.

So what's necessary? Configure effectively emits three files: config.status, comm-config.h, and autoconf.mk.

comm-config.h is the easiest to dispense with, containing just every AC_DEFINE, and there's only one thing that has a path in there (MOZ_BUILD_APP). It looks like no one in comm-central would notice if this define were changed (substitution is different).

It's the AC_SUBST that is more annoying to delete. We use COMM_BUILD to indicate the build system we're in, and diffing output autoconf.mks indicates that the following variables have directory changes (excluding expansions of variables like $(DEPTH)):
MOZ_BRANDING_DIRECTORY
MOZ_BUILD_APP
MOZ_BUILD_ROOT
MOZ_BZ2_LIBS
MOZ_CAIRO_LIBS
MOZ_JPEG_LIBS
MOZ_ZLIB_LIBS
top_srcdir

The MOZ_*_LIBS is already wrong for comm-central anyways, so we can probably ignore those changes. The others are important changes, from what I can tell by MXR searches.

I propose the following patches as an intermediate step to cc-rework:
1. Port the subconfigure changes (minus the logic to call LDAP's configure) to comm-central's configure.in and shovel out the actual new configure logic to mail/, calendar/, and suite/ configure.in packages.
2. Eliminate comm-config.h from our build system and use mozilla-config.h in our build system.
3. Introduce some magic to our configure to pull mozilla's config.status output and touch it up as necessary for our build system and use that for config.status and autoconf.mk.
4. Delete all of the configure logic except for that introduced in step 3 and as necessary to call mozilla's configure with the appropriate arguments.

At the end of this process, we would lose the need to maintain a copy of all configuration logic, and we would have less work to do to execute the ccrework pivot. I'm currently testing a build that does the first two steps by themselves...
Attached patch WIP (obsolete) — Splinter Review
This is a combined patch that illustrates what things should look like by the end. It appears to work pretty well on Linux, and testing OS X ran into current bustage; Windows-on-try is busted because I need an m-c patch to get checked in to get this stuff to work in the first place.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
I have no idea who should review this patch. This class was forked to ldap/xpcom in comm-central long ago and now serves mainly to break anybody who enables MOZ_LDAP_XPCOM in mozilla-central, as comm-central would be doing shortly. The last non-automated change made to this file was done in 2007, when mozilla-central was still tracking the CVS changes (!).
Attachment #723273 - Flags: review?(mbanner)
Comment on attachment 723273 [details] [diff] [review]
Kill dead code in m-c that breaks if MOZ_LDAP_XPCOM is enabled

Ok, I think its the right thing to do, as this is dead code. Keeping the query alongside the ldap code is probably the better place for it anyway, so r=me.

I think I'm fine reviewing here as it is NPOTB anyway for FF & other m-c apps.
Attachment #723273 - Flags: review?(mbanner) → review+
m-c LDAP_XPCOM changes have been pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef912e53996
Whiteboard: [leave open]
This patch ports the subconfigure work to comm-central, and moves the necessary parts we need for configures to mail/configure.in and suite/configure.in. I'm not also making the changes for Sunbird, since that's a dead project and I don't think it really needs a subconfigure like mail and suite do.

I've tested this extensively on Try, where it works on all platforms (once the corresponding fix to work around mktemp not existing on Windows lands). Mark has reported that the config builds for SeaMonkey, and I'm doing one last check to make sure --enable-calendar still works locally.
Attachment #723271 - Attachment is obsolete: true
Attachment #727216 - Flags: review?(mbanner)
Attachment #727216 - Flags: review?(bugspam.Callek)
This renders all AC_DEFINE's obsolete. Since there are no paths in mozilla-config.h itself, there's no need to play the same tricks I need to do for autoconf.mk.

Also tested on try as working on all platforms.
Attachment #727220 - Flags: review?(mbanner)
Well, I want to name this kill autoconf.mk, but we can't quite do that since we need to change a few things. Basically, we add COMM_BUILD = 1 to config.status, and remaps paths to work properly for the rest of the variables. This is done by making config.status an interesting little python file.

I'll freely admit that this is a fairly gross hack, but it should be obsolete once cc-rework is finished.
Attachment #727224 - Flags: review?(mbanner)
Attachment #727224 - Flags: review?(gps)
With parts 2 and 3, there is no longer any need for most of the code in the configure.in. Basically, all we need are enough arguments to be able to run mozilla-central/configure with the right stuff, and a few other things to make the configuration file not blow up in our face (it turns out, incidentally, that autoconf really doesn't like it if you AC_OUTPUT nothing).

Not requesting review yet since Try indicates that this blows up on universal OS X builds and Windows, probably because I was still too aggressive in removing everything.
As requested, I'm going to describe my methodology for making these patches. I originally made the mail/configure.in by running vimdiff configure.in mozilla/configure.in and picking out every configure option that was in comm-central and moving that block of code to mail/configure.in. I omitted a few of the configure options that were different because they appeared to be dead code. These are the ones that were not ported, along with the variables they control:
--disable-libnotify -> MOZ_ENABLE_LIBNOTIFY
--disable-plugins -> MOZ_PLUGINS
--enable-plaintext-editor-only -> MOZ_PLAINTEXT_EDITOR_ONLY
--disable-composer -> MOZ_COMPOSER [note that MOZ_COMPOSER is still AC_SUBST'd]
--disable-xpcom-fastload -> MOZ_NO_FAST_LOAD
--disable-xtf -> MOZ_XTF
--enable-storage -> MOZ_STORAGE
--enable-timeline -> MOZ_TIMELINE
--disable-rdf -> MOZ_RDF

I made the next batch of changes by comparing the autoconf.mk of comm-central (for Thunderbird) and and noting which variables were present in the comm-central variant but not the mozilla-central. I grepped through the code to see which ones were actually used, and moved those into AC_SUBST variables in mail/configure.in.

The code for suite/configure.in was then generated by copying the mail/configure.in and tweaking it slightly as appropriate. The one major difference is the use of PBBUILD, which is used only for one of the Thunderbird-specific OS X integration pieces.

As for regression potential: I'll admit that I haven't tried this code under a large set of possible configure options, so it's possible that I'm breaking some obscure or not-so-obscure configuration. I'm confident in platform coverage as a result of try coverage appearing to be as good as it normally gets. Fixing most regressions should be possible by amending mail/configure.in and suite/configure.in as appropriate.
Comment on attachment 727216 [details] [diff] [review]
Part 1: Split off subconfigure [check-in: comment 16]

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

Ok, this looks fine to me, its passed through try, and I know I've done at least one run with SM build. I've also just done a manual l10n repack with part 1 and part 2 patches applied.
Attachment #727216 - Flags: review?(mbanner) → review+
Attachment #727220 - Flags: review?(mbanner) → review+
Comment on attachment 727224 [details] [diff] [review]
Part 3: Use the AC_SUBST from mozilla-central

Looks good.
Attachment #727224 - Flags: review?(mbanner) → review+
Blocks: 805023
Comment on attachment 727216 [details] [diff] [review]
Part 1: Split off subconfigure [check-in: comment 16]

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

I did not locally test either TB or SeaMonkey, but this logic all looks sound!

::: configure.in
@@ +3883,5 @@
> +  m4 "${srcdir}/mozilla/build/autoconf/subconfigure.m4" \
> +     "${srcdir}/mozilla/build/autoconf/altoptions.m4" \
> +     "${srcdir}/${MOZ_BUILD_APP}/configure.in" > $tmpscript
> +  . $tmpscript
> +  rm -f $tmpscript

nit: is there a solution we can do here without creating an executed subscript that we then delete? I can forsee issues with this approach if we then have issues in what was generated. 

(not blocking review)

Also would *love* a way we can "import" extra scripts here (so that suite and TB can share some configure code, as an example) -- which is ok as a followup, imho.

::: mail/configure.in
@@ +19,5 @@
> +fi
> +if test "$OS_ARCH" != "WINNT" -a "$OS_ARCH" != "OS2"; then
> +  MOZ_MOVEMAIL=1
> +fi
> +AC_SUBST(MOZ_MOVEMAIL)

I'm confused why MOVEMAIL isn't removed from top-level

@@ +26,5 @@
> +if test "$COMM_BUILD" = "1"; then
> +  commdir=$topsrcdir
> +else
> +  commdir=$topsrcdir/..
> +fi

followup-worthy, make a m4 helper for _dir_of_comm_ or something, that does this magic anywhere we need it (makes future switching easier, and less potential for mistakes in any new place)

@@ +33,5 @@
> +AC_SUBST(SUNBIRD_VERSION)
> +AC_SUBST(SEAMONKEY_VERSION)
> +
> +dnl =========================================================
> +dnl = Calendar client

nit: don't carry-forward "client" in comment here, suggest "Lightning Extension"

@@ +92,5 @@
> +AC_SUBST(LDAP_CFLAGS)
> +AC_SUBST(LDAP_LIBS)
> +
> +# Rerun the confvars, since we need to add to our components list.
> +. "${srcdir}/${MOZ_BUILD_APP}/confvars.sh"

I'd rather move to here, the stuff we *need* to do after these magic lines run from confvars.sh. Now that we have the app-specific configure.in so that we can avoid the re-run of confvars.sh (re-entrancy makes for poor bed fellows there)

This too can be a followup (but this, I'll block landing if you don't at least file a bug for it)

@@ +103,5 @@
> +    test -n "$PBBUILD_BIN" && break
> +  done
> +  AC_SUBST(PBBUILD_BIN)
> +  ;;
> +esac

curious - do you know why/if we really need this for our stuff, yet Firefox doesn't.

@@ +105,5 @@
> +  AC_SUBST(PBBUILD_BIN)
> +  ;;
> +esac
> +
> +_save_ac_configure_args="$ac_configure_args"

Nit: can you plop a comment here saying why we're saving the config args, (something as simple as "Save args configure knows about to restore after we call LDAP's configure". I'm not picky on the wording you choose)

Addtl-Nit: Can you pull the save/restore conf args to inside the if .. MOZ_LDAP_XPCOM rather than outside it (no sense saving/restoring if we never actually change it)

@@ +109,5 @@
> +_save_ac_configure_args="$ac_configure_args"
> +ac_configure_args="$_SUBDIR_CONFIG_ARGS"
> +
> +# if we're building the LDAP XPCOM component, we need to build
> +# the c-sdk first.

we (will) need to do some sort of subconfigure magic to generate the LDAP configure, since m-c doesn't use it. But thats not needed as of this patch, just a thing to keep in mind if you were not already

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

pretty much exact same comments/nits here as the mail/configure.in
Attachment #727216 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 727220 [details] [diff] [review]
Part 2: Kill comm-config.h [check-in: comment 16]

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

Assuming you tested this, great. I just worry if you didn't because I am suspecting that these defines get passed down to mozilla's configure before it has had a chance to generate the .h we point to
(In reply to Joshua Cranmer [:jcranmer] from comment #10)
> --disable-libnotify -> MOZ_ENABLE_LIBNOTIFY
> --disable-plugins -> MOZ_PLUGINS
> --enable-plaintext-editor-only -> MOZ_PLAINTEXT_EDITOR_ONLY
> --disable-composer -> MOZ_COMPOSER [note that MOZ_COMPOSER is still
> AC_SUBST'd]
> --disable-xpcom-fastload -> MOZ_NO_FAST_LOAD
> --disable-xtf -> MOZ_XTF
> --enable-storage -> MOZ_STORAGE
> --enable-timeline -> MOZ_TIMELINE
> --disable-rdf -> MOZ_RDF
> 
> As for regression potential: I'll admit that I haven't tried this code under
> a large set of possible configure options, so it's possible that I'm
> breaking some obscure or not-so-obscure configuration. I'm confident in
> platform coverage as a result of try coverage appearing to be as good as it
> normally gets. Fixing most regressions should be possible by amending
> mail/configure.in and suite/configure.in as appropriate.

I'm using many of the --disable-* options so I'll notify you if any of them breaks :)
Blocks: 859125
Parts 1 and 2 have been pushed onto the closed tree, since they help fix the bustage:
https://hg.mozilla.org/comm-central/rev/c845afc74ced
https://hg.mozilla.org/comm-central/rev/6fc0ac415187
Attachment #727216 - Attachment description: Part 1: Split off subconfigure → Part 1: Split off subconfigure [check-in: comment 16]
Attachment #727220 - Attachment description: Part 2: Kill comm-config.h → Part 2: Kill comm-config.h [check-in: comment 16]
Comment on attachment 727224 [details] [diff] [review]
Part 3: Use the AC_SUBST from mozilla-central

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

I'd feel better if we moved all the logic in config.status to a shared .py file which we could parameterize and unit test. But, this is probably fine for a first run (especially considering this is very similar to how m-c does config.status today). So, meh.

This is good enough.

::: configure.in
@@ +6409,5 @@
> +
> +dnl Fix up substitutitons
> +def remap_subst(pair):
> +    name, value = pair
> +    value = value.replace('\$(DEPTH)', '\$(DEPTH)/mozilla')

I assume the \$ is for m4? Python definitely doesn't need this escaping.

@@ +6414,5 @@
> +    value = value.replace('\$(topsrcdir)', '\$(topsrcdir)/mozilla')
> +    if name == 'ac_configure_args':
> +        value = r''' $ac_configure_args '''[1:-1]
> +    elif name in ['MOZ_BRANDING_DIRECTORY', 'MOZ_BUILD_APP']:
> +        value = value[3:] # Strip off ../ from the beginning

value = value.lstrip('../')

@@ +6416,5 @@
> +        value = r''' $ac_configure_args '''[1:-1]
> +    elif name in ['MOZ_BRANDING_DIRECTORY', 'MOZ_BUILD_APP']:
> +        value = value[3:] # Strip off ../ from the beginning
> +    elif name in ['top_srcdir', 'MOZ_BUILD_ROOT']:
> +        value = value[:-8] # Strip off /mozilla from the end

value = value.rstrip('/mozilla')
Attachment #727224 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #17)
> I'd feel better if we moved all the logic in config.status to a shared .py
> file which we could parameterize and unit test. But, this is probably fine
> for a first run (especially considering this is very similar to how m-c does
> config.status today). So, meh.

I'm hoping this is very temporary, but there is nothing quite so permanent as a temporary fix.

> ::: configure.in
> @@ +6409,5 @@
> > +
> > +dnl Fix up substitutitons
> > +def remap_subst(pair):
> > +    name, value = pair
> > +    value = value.replace('\$(DEPTH)', '\$(DEPTH)/mozilla')
> 
> I assume the \$ is for m4? Python definitely doesn't need this escaping.

Bash, actually. Otherwise it tries to execute DEPTH. :-)
Sorry about more changes after this has already been reviewed, but I realized I dropped the code in the wrong spot. Since the config.status variant is the same as before, I'll carry forward gps's review.
Attachment #727224 - Attachment is obsolete: true
Attachment #735003 - Flags: review?(mbanner)
Depends on: 861207
Comment on attachment 735003 [details] [diff] [review]
Part 3: Use the AC_SUBST from mozilla-central [check-in: comment 21]

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

Looks fine, lets try it.
Attachment #735003 - Flags: review?(mbanner) → review+
A slightly modified version of this, to pass on try, was pushed:
https://hg.mozilla.org/comm-central/rev/46356c9afbfd

The changes were rereviewed by gps and Standard8 over IRC.
Whiteboard: [leave open]
Attachment #735003 - Attachment description: Part 3: Use the AC_SUBST from mozilla-central → Part 3: Use the AC_SUBST from mozilla-central [check-in: comment 21]
Here's an updated version of part 4 that brings configure.in to its barebones. Unlike the older variant of this patch, I'm not trying to also cut out most of aclocal.m4, having been burned on getting this working on Windows so many times.

As a summary, this is basically the things that we need in configure still:
1. mozconfig reading
2. virtualenv setup (for config.status)
3. --with-l10n-base, --with-unify-dist, branding (path correctness)
4. --enable-application, confvars.sh (for official branding paths)
5. custom configure options for mozilla's configure.

I could probably eliminate the branding stuff if I'm really clever about things, but I'd rather spend more energy on getting rid of this build system black magic altogether than making this file the absolute minimum size (I'm already getting rid of 95.7% of the file!).
Attachment #727227 - Attachment is obsolete: true
Attachment #737788 - Flags: review?(mbanner)
(In reply to Joshua Cranmer [:jcranmer] from comment #21)
> A slightly modified version of this, to pass on try, was pushed:
> https://hg.mozilla.org/comm-central/rev/46356c9afbfd

I pushed a follow-up bustage fix to re-disable the accessibility module on Mac for Thunderbird:

https://hg.mozilla.org/comm-central/rev/7182ff7ca21e

The comm-central configure.in had the old disabling on Mac which Firefox removed a while ago. It would probably have been fine, but for the test failures that indicate it isn't quite working for us, so I re-disabled the accessibility module on Mac and we can look at the work for enabling it in bug 862238.
Comment on attachment 737788 [details] [diff] [review]
Part 4: Kill dead code in configure.in

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

Looks good, lets do it.
Attachment #737788 - Flags: review?(mbanner) → review+
And the final part:
https://hg.mozilla.org/comm-central/rev/49dde1b4cfab

Now let's keep an eye out for other regressions.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
I'm using a ton of --disable-* options and I updated my tree right now with these patches. Everything seems to work so far. Thanks.
Depends on: 863300
Depends on: 864067
Part 4 seems to have broken the fix in Bug 708799 (WSEnable.exe.manifest: manifest authoring error c1010001)
See Bug 707021 Comment 9:

> Removing the obsolete "-MANIFESTUAC:NO" workaround actually broke the VC 9 builds - see
> bug 708799.

> We need to work out what we're missing as we've obviously not got something right. I
> don't mind if we want to move this manifest section to a different bug.
Flags: needinfo?(Pidgeot18)
Also see Mozilla-Central Bug 570365 (Remove -MANIFESTUAC:NO linker flag from configure)
(In reply to Philip Chee from comment #27)
> Part 4 seems to have broken the fix in Bug 708799 (WSEnable.exe.manifest:
> manifest authoring error c1010001)
> See Bug 707021 Comment 9:
> 
> > Removing the obsolete "-MANIFESTUAC:NO" workaround actually broke the VC 9 builds - see
> > bug 708799.
> 
> > We need to work out what we're missing as we've obviously not got something right. I
> > don't mind if we want to move this manifest section to a different bug.

In the mid-term, the plan is to completely get rid of configure.in in comm-central and use mozilla-central's configure.in augmented by mail/configure.in and suite/configure.in. Reading the other bugs, it looks like it should be unnecessary, but we're missing some packaging fix that made it unnecessary in mozilla-central.
Flags: needinfo?(Pidgeot18)
Please can we file new bugs for these follow-up issues. It'll make it simpler for tracking.
Depends on: 864379
Depends on: 869445
Depends on: 901326
You need to log in before you can comment on or make changes to this bug.