474732 - Rerunning configure causes the world to be rebuilt

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Used to be that you could rerun 'configure' and then 'make all' would still be pretty good about doing a dep-build. But right now, if I rerun 'configure' then 'make' rebuilds the entire world. This is unfortunate.

Comment 1

[Benjamin Smedberg]

Did anything in autoconf.mk change? The world now depends on autoconf.mk for correctness reasons, so any configure change (even fairly trivial ones) trigger world-rebuilds. This was by design, according to bug 463824.

If a reconfigure without any changes whatsoever causes a rebuild, that's definitely a bug.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I just tested it with "configure; make all; configure; make all".

Comment 3

[Robert Kaiser]

Hmm, does configure always re-write autoconf.mk and make trigger a rebuild if any file has a newer date? If so, that might explain our problem. Not sure how to solve it, though.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have code for other files like mozilla-config.h, that writes to a temporary file and then only overwrites the real file with the temporary if they're different:

  # Only write mozilla-config.h when something changes (or it doesn't exist)
  if cmp -s $_CONFIG_TMP $_CONFIG_DEFS_H; then
    rm $_CONFIG_TMP
  else
    AC_MSG_RESULT("creating $_CONFIG_DEFS_H")
    mv -f $_CONFIG_TMP $_CONFIG_DEFS_H

    echo ==== $_CONFIG_DEFS_H =================================
    cat $_CONFIG_DEFS_H
  fi

Comment 5

[Robert Kaiser]

roc, that sounds interesting, we probably might want to do this with autoconf.mk as well - I had thought about some approach like that but didn't know we're already doing this for other stuff.

Comment 6

[Justin Wood (:Callek)]

(In reply to comment #4)
> that writes to a temporary file and then only overwrites
> the real file with the temporary if they're different:

Sounds like the correct thing to do here, definitely.

Comment 7

[Robert Kaiser]

Attachment: make autoconf.mk not change unnecessarily

So, as we suspected, the problem is that autoconf.mk gets recreated every time we run configure, and because we now have Makefile dependencies on autoconf.mk everywhere which only look at the date/time of the file, we come to rebuild everything on every run of configure.

We're recreating autoconf.mk two times actually:
1) acoutput-fast.pl always spits it out as a Makefile that needs updating, and so we handle that to config.status in $CONFIG_FILES every time. We can avoid that by treating autoconf.mk like other Makefiles in acoutput-fast.pl.
2) After running the NSPR configure, we recreate autoconf.mk with the some new vars from nspr-config every time. We can avoid that by rewriting it to a temp file there and only move that to be the new autoconf.mk if anything changed.

The attached patch includes the mentioned steps for avoiding recreation of autoconf.mk, I could get the build time of SeaMonkey without any code changes down to 1 minute from over 15 minutes before that change.
If the patch is accepted, some or all of those changes also need to be copied to the SpiderMonkey build system, I guess. comm-central doesn't need an extra patch as it's reusing the same acoutput-fast.pl and NSPR is only configured by the Mozilla repo.

Comment 8

[Robert Kaiser]

Attachment: make autoconf.mk recreation not force world rebuilds

Sorry, the analysis of the problem was correct but my first approach omitted that fact that autoconf.mk actually needs to be recreated when its vars change. Because of that, we need to let config.status recreate it but force back the old version (with the old date/time) when nothing has changed. In the main Mozilla configure, we need to do that at the very end to take the NSPR replacing stuff into account, everywhere else we can just do it right after AC_OUTPUT().

Comment 9

[Robert Kaiser]

Attachment: comm-central patch

This is the same thing for comm-central - here we also can catch changes right after AC_OUTPUT().

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 359750 [details] [diff] [review]
comm-central patch

r=me. This looks ok and seems to work fine - I've tried a couple of scenarios and it regenerated (or not) correctly.

Comment 11

[Robert Kaiser]

Comment on attachment 359750 [details] [diff] [review]
comm-central patch

Pushed this comm-central part as http://hg.mozilla.org/comm-central/rev/9b11d41b0844

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 359748 [details] [diff] [review]
make autoconf.mk recreation not force world rebuilds

Looks good, thanks for the patch!

Comment 13

[Robert Kaiser]

Pushed on trunk as http://hg.mozilla.org/mozilla-central/rev/332c6aaee89c - waiting for green cycles with the patch before asking for a1.9.1

Comment 14

[Robert Kaiser]

Comment on attachment 359748 [details] [diff] [review]
make autoconf.mk recreation not force world rebuilds

1) We have builds on all platforms cycling green with this patch,
2) it is quite low-risk as we're doing similar stuff for other files already (cairo-features.h etc.),
3) it is potentially high-reward as it shortens build times when configure runs but autoconf.mk isn't actually changed.

With those reasons, requesting 1.9.1 approval.

Comment 15

[Robert Kaiser]

Pushed to 1.9.1 as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/94629bce8848

Comment 16

[Sylvain Pasche]

MozillaBuild's cmp does not understand the -b option:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234036480.1234047298.26439.gz&fulltext=1

make[3]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32/build/obj-firefox/js/src'
cmp: invalid option -- b
cmp: Try `cmp --help' for more information.


I guess that's a debugging leftover, it should simply use -s (like the other places).

Comment 17

[Robert Kaiser]

Sylvain, you're right in that it's a debugging leftover, it has been filed as bug 477001 and a patch is on there.