Last Comment Bug 696498 - Clean up *, add omitted makefiles & use more conditionals
: Clean up *, add omitted makefiles & use more conditionals
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Ed Morley [:emorley]
: Gregory Szorc [:gps]
Depends on: 707469 714394
  Show dependency treegraph
Reported: 2011-10-21 14:57 PDT by Ed Morley [:emorley]
Modified: 2011-12-30 17:12 PST (History)
5 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Analysis script (4.11 KB, text/plain)
2011-10-23 17:46 PDT, Ed Morley [:emorley]
no flags Details
Initial results (10.71 KB, text/plain)
2011-10-23 17:51 PDT, Ed Morley [:emorley]
no flags Details changes (3.04 KB, patch)
2011-10-23 17:55 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review
browser/ changes (4.98 KB, patch)
2011-10-23 17:56 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review
mobile/ changes (1.65 KB, patch)
2011-10-23 17:57 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review - A: Make conditionals consistent (2.04 KB, patch)
2011-10-24 07:02 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review - B: Remove dupes + invalid (2.73 KB, patch)
2011-10-24 07:09 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review - C: Remove NPOTB + clean up variables (5.39 KB, patch)
2011-11-01 15:48 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review - D: Add more conditionals (41.40 KB, patch)
2011-11-01 16:06 PDT, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review - E: Add omitted (23.08 KB, patch)
2011-11-17 17:07 PST, Ed Morley [:emorley]
khuey: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-10-21 14:57:35 PDT
Bug 689043, bug 689040 & bug 689036 cleaned up some of the smaller makefile generation scripts in the tree; this bug covers most of the rest (, browser/, mobile/, toolkit/

I've created a script that scans the in-tree makefiles and compares the result with those already included in the generation scripts, producing a list of those that:
a) have been omitted
b) are no longer in the tree and can be removed (eg those under a conditional and so don't appear in the standard configure warning for makefile not found)
c) appear more than once

This bug will deal with cases A+B+C as well as add further conditionals (OS-specific, enable tests, feature based; within reason) to reduce the number of unnecessarily generated makefiles on each platform.

For some context, a cursory examination using a draft version of the script, found ~250 makefiles missing from the generation scripts (which as a proportion of the 1143 makefiles in the tree, is quite high). I also believe there to be 150-200 unnecessarily created makefiles in my local obj-dir, due to missing conditionals. 

I don't know how much difference each of the above makes to build times (until this patch series is finished at least), but I guess every little helps.
Comment 1 Ed Morley [:emorley] 2011-10-23 17:46:21 PDT
Created attachment 568986 [details]
Analysis script

Script combines the Makefiles listed in * + NSPR configure, and compares to those present in the tree after applying a whitelist. A list of invalid (ie since been removed from the tree), omitted (from the generation scripts, so need to be added) and duplicate Makefiles is saved to makefiles-generated-*.txt in topsrcdir. See inline comments for more info.
Comment 2 Ed Morley [:emorley] 2011-10-23 17:51:07 PDT
Created attachment 568987 [details]
Initial results

The output from the script, combined into one file for easier attachment.

* 26 invalid
* 6 duplicate
* 235 omitted
Comment 3 Ed Morley [:emorley] 2011-10-23 17:55:18 PDT
Created attachment 568988 [details] [diff] [review] changes

Clean up
* Adds 11 omitted makefiles, under appropriate conditionals
* Moves some of the existing makefile entries to platform/compiler conditionals
Comment 4 Ed Morley [:emorley] 2011-10-23 17:56:28 PDT
Created attachment 568989 [details] [diff] [review]
browser/ changes

Clean up browser/
* Adds 21 omitted makefiles, under appropriate conditionals
* Moves some of the existing makefile entries to platform/feature conditionals
Comment 5 Ed Morley [:emorley] 2011-10-23 17:57:22 PDT
Created attachment 568990 [details] [diff] [review]
mobile/ changes

Clean up mobile/
* Removes various non-mobile Makefile entries, since they already exist in (which is always run unless --with-libxul-sdk used)
* Adds missing $MOZ_BRANDING_DIRECTORY/content/Makefile
* Moves mobile/chrome/tests/Makefile to ENABLE_TESTS conditional
Comment 6 Ed Morley [:emorley] 2011-10-24 07:02:47 PDT
Created attachment 569054 [details] [diff] [review] - A: Make conditionals consistent
Comment 7 Ed Morley [:emorley] 2011-10-24 07:09:03 PDT
Created attachment 569058 [details] [diff] [review] - B: Remove dupes + invalid

* Remove duplicate |extensions/pref/Makefile| entry, since already present higher up in the file
* Remove l10n/* makefile entries since they no longer exist in the tree
Comment 8 Ed Morley [:emorley] 2011-11-01 15:48:05 PDT
Created attachment 571170 [details] [diff] [review] - C: Remove NPOTB + clean up variables

* Remove MAKEFILES_plugin since not assigned anything
* Rename MAKEFILES_libpr0n to MAKEFILES_imagelib, to match bug 66984
* Remove the following NPOTB makefiles:
* Remove the following commented out makefiles:
* Remove embedding/tests/winEmbed/Makefile since it belongs in xulrunner/ (will sort this out in a followup bug, since the logic seems squiffy [], so will sort out at the same time)

(Broken these changes out from the part D patch, to make it's diff a little bit more readable).
Comment 9 Ed Morley [:emorley] 2011-11-01 16:06:04 PDT
Created attachment 571173 [details] [diff] [review] - D: Add more conditionals

* Adds missing platform, feature and/or ENABLE_TESTS based conditionals to ~170
  entries that were missing them and so sometimes being generated unnecessarily.
* Pre-emptively assumes RDF and SMIL are always built, since bug 597789 and
  bug 698630 will land soon removing the option to disable them.
* Moves all conditionally included makefiles to the end of the file. 

Please let me know your/ted's preference as to what order the tests vs platform vs feature sections should be in, as well as the ordering of each block inside the feature specific section. I didn't want to move things around too much in the initial pass, to try and keep the diff as readable as possible - so left the existing feature conditionals where they were, and added the new ones alphabetically above them.

(One final patch after this to come, which will be adding the ~200 omitted makefiles.)
Comment 10 Ed Morley [:emorley] 2011-11-17 17:07:30 PST
Created attachment 575338 [details] [diff] [review] - E: Add omitted

Adds ~160 makefiles omitted from, under conditionals where required.
Comment 11 Ed Morley [:emorley] 2011-11-19 17:29:01 PST
Comment 12 Ed Morley [:emorley] 2011-11-30 17:38:08 PST
Landed after rebasing plus:
- Adjusting android conditionals for recent gonk landings (OS_ARCH vs MOZ_WIDGET_TOOLKIT etc)
- Removed browser/themes/<theme>/browser/Makefile post bug 701212
- s_mobile/_mobile/xul_g

Still a few things to clean up (eg new Makefiles landed in the tree since this patch series was reviewed, generated makefiles in js/src/configure and nsprpub/configure), but will deal with them elsewhere.
Comment 13 Nicholas Nethercote [:njn] 2011-11-30 18:00:02 PST

This is causing the following compile error and a bunch of similar ones for me on 64-bit debug Linux builds:

  In file included from /home/njn/moz/mi0/gfx/cairo/cairo/src/cairoint.h:70:0,
                   from /home/njn/moz/mi0/gfx/cairo/cairo/src/cairo-bentley-ottmann-rectilinear.c:39:
  /home/njn/moz/mi0/gfx/cairo/cairo/src/cairo.h:42:28: fatal error: cairo-features.h: No such file or directory
  compilation terminated.

I bisected to determine this.  The same error also occurs in later revisions after all the other build system clean-ups landed.

I don't understand the patch, but it would be nice if I could build the browser again!
Comment 14 Ed Morley [:emorley] 2011-11-30 19:38:32 PST
Ok, thanks to Nick emailing me the log, the above was tracked down to:
sed: can't read /home/njn/moz/mi3/gfx/cairo/cairo/src/cairo-features.h/ No such file or directory
rm: cannot remove `gfx/cairo/cairo/src/cairo-features.h': Is a directory
./config.status: 2800: cannot create gfx/cairo/cairo/src/cairo-features.h: Is a directory
creating }
sed: can't read /home/njn/moz/mi3/}.in: No such file or directory

Which implied that the braces in the |browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/Makefile| entry added in 79f0181b02f8 causes (certain versions of) sed to choke, breaking make-makefiles thus meaning cairo-features.h isn't present later in the build.

For reference:
Local win32 build using MozillaBuild (sed 3.02): works
Builds on multiple platforms on try/mozilla-inbound (sed-4.1.5-5.fc6): works
njn's local build on linux64 (sed 4.2.1): doesn't work

Typically, only after all this, I spotted this changeset from 2009, having to revert exactly the same thing:

Anyway, I've removed the entry for the default theme's makefile again, and added a comment to ensure it doesn't happen a third time:
Comment 16 Ed Morley [:emorley] 2011-12-03 13:06:23 PST
Added a handful of makefiles that have been created in the tree since the patches in this bug were reviewed. Not attached/reviewed, since trivial additions have a standing rs=build.
Comment 17 Ed Morley [:emorley] 2011-12-04 07:19:34 PST
(In reply to Ed Morley [:edmorley] from comment #16)
> Added a handful of makefiles that have been created in the tree since the
> patches in this bug were reviewed.

Note You need to log in before you can comment on or make changes to this bug.