271929 - (Rest_the_camel!) Remove config/build-list.pl, config/purge-old-headers.pl, and .headerlist from the build process

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Hans-Andreas Engel]

Remove the perl scripts config/build-list.pl and config/purge-old-headers.pl and
the temporary .headerlist from the build process.

These scripts originate from  Bug 59454, Comment 17.  They have the following
purpose.  If the header "header.h" is moved from module_1 to module_2, the build
system should ensure that the copy in "dist/include/module_1/header.h" is removed.

The current build cycle works as follows.
  1) Initially, there is no .headerlist in dist/include/module_1/, effectively
     marking all files in dist/include/module_1/ for deletion.
  2) During the build, all encountered headers in module_1 (including those
     resulting from .idl files) are nsinstalled to dist.  Then, "build-list.pl" 
     adds the corresponding filenames to dist/include/module_1/.headerlist.
  3) Finally, "purge-old-headers.pl" removes all files in dist/include/module_1/
     which are not marked in .headerlist.  It also removes .headerlist itself.
In summary, all headers which are not nsinstalled will be removed.

Thus, the build procedure can be substantially simplified by just
  1) Initially, run "rm -rf dist/include".
  2) As before, nsinstall all encountered headers to dist/include/module_1.

Comment 1

[Hans-Andreas Engel]

Attachment: dist/include: do not use config/build-list.pl, config/purge-old-headers.pl, and .headerlist

implying to also "cvs remove config/purge-old-headers.pl"
(note: config/build-list.pl is still needed to build the list of libraries)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

does nsinstall preserve the timestamp of the .h files? because otherwise, I
suspect this will triggere a full rebuild on each make, which would be undesirable.

Comment 3

[Hans-Andreas Engel]

Thank you for commenting so quickly.  I completely agree, a full rebuild must
not be tolerated.  However, I do not think that full rebuilds will be triggered
with the proposed patch. 

Since the current build system avoids full rebuilds, we can just compare the
cases where there is already a header in dist/include (current code) with the
case where the header has been deleted (proposed code).  As far as I have
understood, nsinstall basically runs in two different modes.

i)  When nsinstall is run in copy-mode and if there is already a file in the dist 
    directory, it will be removed before copying,
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/config/nsinstall.c&rev=3.24&root=/cvsroot#178
    So both the current and the proposed code create a new file and there is no 
    difference in mtime handling or in performance.

ii) In symlink-mode, nsinstall keeps a symlink if it already exists (current 
    code), otherwise a new link is created (proposed code).
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/config/nsinstall.c&rev=3.24&root=/cvsroot#435
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/config/nsinstall.c&rev=3.24&root=/cvsroot#442
    Since for |make|, the mtime of the link target is relevant, there is no 
    difference between current and proposed code.  Regarding performance, the 
    cost is |readlink()| (current) vs. |symlink()| (new).

Thus, I think that there are no additional rebuilds.  Performance of nsinstall
should not be significantly decreased (leading to an overall faster compilation,
since the perl scripts are not run).

Comment 4

[cls]

IIRC, under the old nmake build system, win32 used makecopy.exe which did not
preserve timestamps so removing $(DIST)/include would cause a full rebuild. 
That's no longer the case. 

The only problem I see with the 'rm -rf' solution is that it will erase
everything under $(DIST)/include rather than just the headers in the directories
with .headerlist.   It's not a big deal for our in-tree modules but other
applications that build on top of our tree, it may be a problem since
$(DIST)/include will no longer be preserved between build cycles.

Comment 5

[Hans-Andreas Engel]

I see.  Building an application on top of mozilla with `make application; make
mozilla' would be ok, however, the dist/include would be incomplete.  So one
must use `make mozilla; make application' for a successful build and a complete
dist/include.

Similarly, a lone `make mozilla' would be bad while a lone `make application' is
fine.

PS: cls, thank you for the historical background!

Comment 6

[Benjamin Smedberg]

I am inclined to WONTFIX this bug. I don't particularly like the .headerlist
files, but I think that rm -rf dist/include is worse, and it does affect
build-on-top applications.

Is there any reason to do this other than build simplification and perhaps some
small time savings?

Comment 7

[Hans-Andreas Engel]

bsmedberg:  Could you please explain how |rm -rf dist/include| "affects
build-on-top applications" and give the shell build commands (e.g,
|./configure-app; make this; make that;|) which would be broken by the current
patch?  This would be very helpful regarding this and future efforts on dealing
with .headerlists.

The main reason for this bug is indeed build simplification.  (Since the current
build system uses several somewhat obscure and non-standard mechanisms, I think
that simplifications are good.)

However, if there is a realistic scenario which would be badly broken by this
patch, I would strongly support WONTFIXing this bug.

Comment 8

[Hans-Andreas Engel]

Currently, there seems little interest in this issue.  BSD written me that he
doesn't "want to change the build system at this point unless there is a
definite benefit being provided."

I think that a great benefit would result from reducing the complexity of
mozilla's build system, since this would significantly reduce the entry barrier
for tinkering with mozilla.  However, the proposed change is only a small step
into this direction and so the benefit of the proposed solutions is small.  I
agree that this issue has a low priority.

Consequently, I follow Comment 6 and mark this bug as WONTFIX.  Please reopen if
resources for reviewing become available.

Comment 9

[Benjamin Smedberg]

OK, I've decided to go ahead and do this... I'll be watching the tinderbox cycle
times to see whether it significantly hurts build time.

Comment 10

[Benjamin Smedberg]

Landed on trunk.

Comment 11

[Darin Fisher]

For whatever reason, this patch is not correct.  My Win32 builds are constantly
ending up in horked states because of this patch.  For some reason, dist/include
is not getting rebuilt correctly, so I have to manually 'make export' in various
subdirectories in order to get my build working again.  It has come to the point
that I am afraid to type 'make -f client.mk build' anymore.  Am I the only one
seeing this problem?  I'm pulling my hair out over this change :-(

Comment 12

[Darin Fisher]

BTW, I'm experiencing this problem while building XULRunner/LibXUL.  Maybe the
problem only impacts that build, because others have said that they aren't
seeing this.  I certainly haven't seen a problem while building firefox on
linux.  And, biesi says that seamonkey on linux is fine too.

Comment 13

[Darin Fisher]

Attachment: zipped build log showing error

It appears to me as though 'make export' never happens in
toolkit/components/startup before we start compiling in toolkit/xre :-(

Comment 14

[Darin Fisher]

Ok, nevermind.  I think the problem has nothing to do with this patch actually.
 The LIBXUL build code was doing |echo "make export" make export|, which is a
bit of an oops ;-)