Last Comment Bug 784841 - Move DIRS logic out of Makefiles; eliminate allmakefiles
: Move DIRS logic out of Makefiles; eliminate allmakefiles
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Gregory Szorc [:gps]
:
Mentors:
: 389393 416982 579494 666439 800635 836693 (view as bug list)
Depends on: 849668 827308 846523 846609 847369 847382
Blocks: 774049 827343 844509 846425 nomakefiles 774572 818246 831236 844635 844654 844655 845089 845247 846463 846524 851975 860920
  Show dependency treegraph
 
Reported: 2012-08-22 15:21 PDT by Gregory Szorc [:gps]
Modified: 2013-05-16 14:12 PDT (History)
49 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Conversion strawman, v1 (908.50 KB, patch)
2012-09-01 22:02 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Just the Python bits (29.50 KB, patch)
2012-09-02 10:37 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 1: Generic container classes, v1 (5.53 KB, patch)
2012-09-07 18:06 PDT, Gregory Szorc [:gps]
k0scist: review+
Details | Diff | Splinter Review
Part 2: Implement Python sandboxing and tree reading, v1 (89.14 KB, patch)
2012-09-08 21:15 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 2: Implement Python sandboxing and tree reading, v2 (87.07 KB, patch)
2012-09-09 16:36 PDT, Gregory Szorc [:gps]
ted: review+
k0scist: feedback+
Details | Diff | Splinter Review
Part 2: Implement Python sandboxing and tree reading, v3 (97.40 KB, patch)
2012-10-12 01:48 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 3: Convert executed sandboxes into data structures, v1 (14.79 KB, patch)
2012-10-12 17:38 PDT, Gregory Szorc [:gps]
ted: review+
mh+mozilla: review+
Details | Diff | Splinter Review
Part 2: Implement Python sandboxing and tree reading, v4 (99.68 KB, patch)
2012-10-13 16:54 PDT, Gregory Szorc [:gps]
ted: review+
mh+mozilla: review+
Details | Diff | Splinter Review
Makefile conversion (860.04 KB, patch)
2012-12-20 19:06 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 2b: Add parameter to control whether to descend into referenced children, v1 (4.86 KB, patch)
2012-12-21 20:09 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 4a: Convert /b2g, v1 (8.91 KB, patch)
2012-12-21 23:49 PST, Gregory Szorc [:gps]
ted: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4b: Convert /, /build, /config, v1 (15.91 KB, patch)
2012-12-21 23:50 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4c: Convert /memory, /mfbt, /mozglue, v1 (9.19 KB, patch)
2012-12-21 23:55 PST, Gregory Szorc [:gps]
ted: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4d: Convert /toolkit, v1 (137.70 KB, patch)
2012-12-21 23:57 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4e: Convert /browser, v1 (85.00 KB, patch)
2012-12-22 00:03 PST, Gregory Szorc [:gps]
gavin.sharp: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4f: Convert /content, v1 (51.79 KB, patch)
2012-12-22 00:06 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4g: Convert /docshell, v1 (5.84 KB, patch)
2012-12-22 00:09 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4h: Convert /dom, v1 (138.31 KB, patch)
2012-12-22 00:16 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4i: Convert /editor, v1 (12.71 KB, patch)
2012-12-22 00:17 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4j: Convert /embedding, v1 (17.98 KB, patch)
2012-12-22 00:23 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4k: Convert /extensions, v1 (16.84 KB, patch)
2012-12-22 00:24 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4l: Convert /gfx, v1 (9.60 KB, patch)
2012-12-22 00:26 PST, Gregory Szorc [:gps]
jmuizelaar: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4m: Convert /image, v1 (11.69 KB, patch)
2012-12-22 00:29 PST, Gregory Szorc [:gps]
jmuizelaar: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4n: Convert /intl, v1 (24.52 KB, patch)
2012-12-22 00:30 PST, Gregory Szorc [:gps]
ted: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4o: Convert /ipc, v1 (9.25 KB, patch)
2012-12-22 00:34 PST, Gregory Szorc [:gps]
ted: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4p: Convert /js, v1 (15.31 KB, patch)
2012-12-22 00:35 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4q: Convert /layout, v1 (22.34 KB, patch)
2012-12-22 00:36 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4r: Convert /media, v1 (33.49 KB, patch)
2012-12-22 00:39 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4s: Convert /accessible, v1 (23.49 KB, patch)
2012-12-22 00:40 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4t: Convert /caps, v1 (2.36 KB, patch)
2012-12-22 00:41 PST, Gregory Szorc [:gps]
khuey: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4u: Convert /mobile/android, v1 (17.92 KB, patch)
2012-12-22 00:44 PST, Gregory Szorc [:gps]
ted: review-
Details | Diff | Splinter Review
Part 4v: Convert /mobile/xul, v1 (16.96 KB, patch)
2012-12-22 00:45 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4w: Convert /modules, v1 (12.41 KB, patch)
2012-12-22 00:45 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4x: Convert /netwerk, v1 (22.20 KB, patch)
2012-12-22 00:47 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4y: Convert /parser, v1 (10.34 KB, patch)
2012-12-22 00:48 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4z: Convert /profile, v1 (3.38 KB, patch)
2012-12-22 00:49 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4α: Convert /rdf, v1 (9.35 KB, patch)
2012-12-22 00:59 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4β: Convert /security, v1 (8.50 KB, patch)
2012-12-22 01:00 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4γ: Convert /services, v1 (12.33 KB, patch)
2012-12-22 01:01 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4δ: Convert /storage, v1 (2.55 KB, patch)
2012-12-22 01:01 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4ε: Convert /testing, v1 (13.42 KB, patch)
2012-12-22 01:02 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 4ζ: Convert /tools, v1 (5.87 KB, patch)
2012-12-22 01:03 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 4η: Convert /uriloader, v1 (4.16 KB, patch)
2012-12-22 01:04 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4θ: Convert /webapprt, v1 (4.48 KB, patch)
2012-12-22 01:04 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4ι: Convert /widget, v1 (9.68 KB, patch)
2012-12-22 01:05 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4κ: Convert /xpcom, v1 (27.97 KB, patch)
2012-12-22 01:06 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4λ: Convert /xpfe, v1 (5.78 KB, patch)
2012-12-22 01:06 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4μ: Convert /xulrunner, v1 (10.21 KB, patch)
2012-12-22 01:07 PST, Gregory Szorc [:gps]
ted: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4ν: Convert misc remaining, v1 (9.65 KB, patch)
2012-12-22 01:08 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4d: Convert /toolkit, v2 (137.68 KB, patch)
2012-12-22 01:14 PST, Gregory Szorc [:gps]
dtownsend: feedback+
Details | Diff | Splinter Review
Part 5: Use relpath in ConfigStatus.py (10.85 KB, patch)
2012-12-22 18:11 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 6: Require virtualenv to build SpiderMonkey, v1 (10.64 KB, patch)
2012-12-22 21:41 PST, Gregory Szorc [:gps]
mh+mozilla: feedback-
Details | Diff | Splinter Review
Part 7: Move parts of ConfigStatus into mozbuild, v1 (33.04 KB, patch)
2012-12-22 22:06 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Some fixes (23.61 KB, patch)
2012-12-23 09:02 PST, :Ms2ger (⌚ UTC+1/+2)
gps: feedback+
Details | Diff | Splinter Review
Part 4β: Convert /security, v2 (12.24 KB, patch)
2012-12-23 09:54 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 6: Require virtualenv to build SpiderMonkey, v2 (11.48 KB, patch)
2012-12-23 17:15 PST, Gregory Szorc [:gps]
mh+mozilla: review-
Details | Diff | Splinter Review
Part 4b: Convert /, /build, /config, v1 (10.11 KB, patch)
2012-12-23 17:34 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4b: Convert /, /build, /config, v2 (10.60 KB, patch)
2012-12-24 13:21 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 7: Move parts of ConfigStatus into mozbuild, v2 (40.25 KB, patch)
2012-12-24 13:22 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 8: Add external make variables to moz.build files, v1 (4.00 KB, patch)
2012-12-24 13:24 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 9: Add warning and error functions to moz.build files, v1 (3.29 KB, patch)
2012-12-24 13:25 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 10: Ability to declare tier directories as external, v1 (7.79 KB, patch)
2012-12-24 13:27 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v1 (5.57 KB, patch)
2012-12-24 13:31 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 12: Integrate with config.status, v1 (8.70 KB, patch)
2012-12-24 13:33 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 13: rules.mk integration, v1 (5.39 KB, patch)
2012-12-24 13:35 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 14: Remove allmakefiles.sh, v1 (68.76 KB, patch)
2012-12-24 13:41 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4λ: Convert /xpfe, v2 (6.60 KB, patch)
2013-01-05 03:54 PST, :Ms2ger (⌚ UTC+1/+2)
ted: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 4b: Convert /, /build, /config, v3 (19.33 KB, patch)
2013-01-07 09:02 PST, :Ms2ger (⌚ UTC+1/+2)
ted: review+
Details | Diff | Splinter Review
Part 5: Require virtualenv to build SpiderMonkey, v3 (11.48 KB, patch)
2013-01-15 21:12 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 6: Move parts of ConfigStatus into mozbuild, v3 (41.54 KB, patch)
2013-01-15 22:54 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 7: Recursive make backend, v1 (16.93 KB, patch)
2013-01-16 23:19 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 8: Add external make variables to moz.build files, v2 (10.80 KB, patch)
2013-01-17 21:06 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 10: Add warning and error functions to moz.build files, v2 (10.63 KB, patch)
2013-01-17 22:00 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 10: Ability to declare tier directories as external, v2 (8.69 KB, patch)
2013-01-17 23:14 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v2 (11.33 KB, patch)
2013-01-18 00:02 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 7: Recursive make backend, v2 (17.76 KB, patch)
2013-01-18 19:49 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 12: Capture moz.build state when feeding into backend, v1 (8.85 KB, text/plain)
2013-01-18 19:59 PST, Gregory Szorc [:gps]
no flags Details
Part 7: Recursive make backend, v3 (17.94 KB, patch)
2013-01-22 12:09 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 8: Capture moz.build state when feeding into backend, v2 (11.12 KB, patch)
2013-01-22 12:33 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 9: Add external make variables to moz.build files, v3 (10.77 KB, patch)
2013-01-22 12:34 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 11: Ability to declare tier directories as external, v3 (10.49 KB, patch)
2013-01-22 12:39 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3 (11.57 KB, patch)
2013-01-22 12:41 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 14: rules.mk integration, v2 (9.98 KB, patch)
2013-01-22 12:43 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4u: Convert /mobile/android, v2 (18.90 KB, patch)
2013-01-28 07:00 PST, :Ms2ger (⌚ UTC+1/+2)
ted: review+
Details | Diff | Splinter Review
Part 4v: Convert /mobile/xul, v2 (17.44 KB, patch)
2013-01-28 07:02 PST, :Ms2ger (⌚ UTC+1/+2)
ted: review+
Details | Diff | Splinter Review
Use filesystem encoding to decode environment variables (1.39 KB, patch)
2013-01-30 06:37 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Use filesystem encoding to decode environment variables (1.72 KB, patch)
2013-01-30 06:56 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 12: Don't read moz.build files in static tier directories, v1 (3.80 KB, patch)
2013-01-31 11:27 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part 13: Use moz.build files to build the tree, v1 (27.09 KB, patch)
2013-01-31 14:07 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v2 (26.64 KB, patch)
2013-02-04 20:11 PST, Gregory Szorc [:gps]
ted: review+
mh+mozilla: review-
Details | Diff | Splinter Review
Part 13: Use absolute paths in config.status, v1 (2.01 KB, patch)
2013-02-04 20:50 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Ms2ger: checkin+
Details | Diff | Splinter Review
Part 4h: Convert /dom, v2 (150.90 KB, patch)
2013-02-19 12:21 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 4e: Convert /browser, v2 (91.66 KB, patch)
2013-02-19 12:22 PST, Gregory Szorc [:gps]
gavin.sharp: review+
Details | Diff | Splinter Review
Part 14: 4-space indent for dom/imptests/*.py, v1 (14.40 KB, patch)
2013-02-19 14:32 PST, Gregory Szorc [:gps]
Ms2ger: review+
Ms2ger: checkin+
Details | Diff | Splinter Review
Part 14: Produce moz.build files for DOM implementation tests, v1 (10.43 KB, patch)
2013-02-19 15:02 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 15: Produce moz.build files for DOM implementation tests, v2 (11.36 KB, patch)
2013-02-21 10:48 PST, Gregory Szorc [:gps]
Ms2ger: review+
Details | Diff | Splinter Review
Part 4h (i): dom/imptests (importTestsuite.py) (20.01 KB, patch)
2013-02-21 12:29 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Part 4h (ii): dom/imptests (parseFailures.py) (11.38 KB, patch)
2013-02-21 12:30 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Part 4h: Convert /dom, v3 (123.53 KB, patch)
2013-02-21 14:57 PST, Gregory Szorc [:gps]
Ms2ger: feedback+
Details | Diff | Splinter Review
Part 17: Handle unicode environment variables in packager, v1 (1.13 KB, patch)
2013-02-22 18:02 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v3 (26.17 KB, patch)
2013-02-22 18:26 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Remove |from __future__ import unicode_literals| from config.status (1.60 KB, patch)
2013-02-22 23:49 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v4 (26.17 KB, patch)
2013-02-23 13:31 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v5 (26.25 KB, patch)
2013-02-23 13:42 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v6 (28.54 KB, patch)
2013-02-23 16:20 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v7 (28.55 KB, patch)
2013-02-23 16:41 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 16: Use moz.build files to build the tree, v8 (29.39 KB, patch)
2013-02-23 19:19 PST, Gregory Szorc [:gps]
ted: review+
mh+mozilla: review+
Details | Diff | Splinter Review
Part 4e: Convert /browser, v3 (91.23 KB, patch)
2013-02-23 19:20 PST, Gregory Szorc [:gps]
jaws: review+
Details | Diff | Splinter Review
Part ∞: Remove allmakefiles.sh and friends, v1 (69.93 KB, patch)
2013-02-25 12:03 PST, Gregory Szorc [:gps]
ted: review+
Details | Diff | Splinter Review
Part ∞+1: Support for external build systems, v1 (9.84 KB, patch)
2013-02-25 23:14 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part ∞+1: Support for external build systems, v2 (20.93 KB, patch)
2013-02-27 20:46 PST, Gregory Szorc [:gps]
mh+mozilla: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-08-22 15:21:35 PDT
As described in https://groups.google.com/d/topic/mozilla.dev.platform/SACOnl-avMs/discussion, we want to move some logic out of Makefile.in's and into some kind of new collection of files.

Per IRC discussion with Mike Hommey, we think the initial land will involve replacing allmakefiles and the DIRS* variables. Whatever we land here will be used to control the directory traversal of the existing recursive make backend. 

Steps:

1) Figure out the new file format (ongoing conversation in dev-platform thread)
2) Write Python to read in these files and convert into a data structure
3) Hook the output of #2 into config.status. This involves writing out .mk files containing the directory traversal definitions and rules.mk magic to include these files as part of Makefile evaluation
4) Convert the tree to the new world.
Comment 1 Gregory Szorc [:gps] 2012-09-01 22:02:05 PDT
Created attachment 657629 [details] [diff] [review]
Conversion strawman, v1

Here is a strawman. It contains Python code in python/mozbuild/frontend for reading Python frontend files (which I've called "mozbuild files"). I've also moved all (or nearly all) directory traversal variables from Makefile.in's into "build.mozbuild" files. Low-level details are documented in the Python modules in python/mozbuild/frontend/.

The current code merely traverses the tree of linked .mozbuild files by examining the DIRS* variables. It stops short of integrating with the existing build system. I want to be sure I'm on the right track before I put effort into writing that.

There are no shortage of things to discuss:

* Is "build.mozbuild" the right name for the frontend files?
* Do people like the behavior of the CONFIG variable? Currently, defines and substitutions get stuffed into the same dict. Should we keep them apart?
* What's the behavior of defines? Do we magically returned not defined on unknown variable (like current behavior) or should we require that all referenced variables be defined?
* Is "if CONFIG.FOO" for defined a proper syntax? What about "if defined('FOO')"?
* We certainly could merge defines and substitutions into the global namespace (like make's behavior). I kind of like keeping them separate *and* read-only. I know some globals like "CXXFLAGS" will need to be adjusted. I was thinking we could keep the original variables read-only and do things like "STRIP_CXXFLAGS += ['-WError']" instead. This ensures that the output of any mozbuild file is what's actually in the file, not a union of the config. IMO a problem with make is everything is normalized into one unwieldy namespace. This leads to issues like pollution from environment variables.
* There is a function for registering tier directories. This is because of the regular vs static directories non-sense. I welcome alternative ideas. Keep in mind order needs to be preserved.

Other known issues:

* Not Python 2.5 compatible (required for current Linux builders). I don't intend on backporting until I need to. Hopefully releng can come through and deliver 2.7 before this lands. If not, I'll endure the pain.
* Test coverage far from complete.

Other comments:

* This is pretty fast. On my machine, it is crawling ~680 mozbuild files in ~200ms. That's without any optimization. That's 1 thread. That's without Python bytecode caching (.pyc files) (compile() is below the bytecode caching layer). At this juncture, I'm not too worried about performance.
* It's probably not worth your time to look at every mozbuild file just yet. If you want to, go right ahead! I'm sure there are minor mistakes.
* The cargo culting in our build system is ridiculous. There are constructs like "DIRS = $(NULL)" and empty Makefiles that make me gag.

As far as the end state, I'm thinking things will look something like:

1) Configure writes out config.status (like today)
2) Configure invokes "build backend files" command (integrated with config.status/ConfigStatus.py) (like today except semantics are different - see below)
3) mozbuild files are crawled. Data structures of build configuration is obtained.
4) For each build.mozbuild file with data, a mozbuild.mk file (or similar) is written alongside the substituted Makefile in the object directory. These mozbuild.mk files translate the extracted data to make syntax. Initially, we'll just be writing "DIRS :=" "PARALLEL_DIRS :=" etc.
5) At Makefile execution time, rules.mk does a make "-include" of mozbuild.mk at the top of the file. Makefile execution proceeds like today. mozbuild defined variables "just work"

Let the bikeshedding begin.
Comment 2 Mike Hommey [:glandium] 2012-09-02 00:31:31 PDT
It would be easier if the interesting code was separated from the Makefile.in/mozbuild dance.
Comment 3 Mike Hommey [:glandium] 2012-09-02 01:33:47 PDT
(In reply to Gregory Szorc [:gps] from comment #1)
> * Is "build.mozbuild" the right name for the frontend files?

build.manifest ?

> * Do people like the behavior of the CONFIG variable? Currently, defines and
> substitutions get stuffed into the same dict. Should we keep them apart?
> * What's the behavior of defines? Do we magically returned not defined on
> unknown variable (like current behavior) or should we require that all
> referenced variables be defined?

AC_SUBSTs are always defined, whatever their value is. AC_DEFINEs aren't. Note that we also do AC_SUBST a lot of AC_DEFINEd stuff just so that they can also be accessed from Makefiles. Having access to both substs and defines from build manifests would definitely help. OTOH, this might confuse people.

> * Is "if CONFIG.FOO" for defined a proper syntax? What about "if
> defined('FOO')"?
> * We certainly could merge defines and substitutions into the global
> namespace (like make's behavior). I kind of like keeping them separate *and*
> read-only. I know some globals like "CXXFLAGS" will need to be adjusted. I
> was thinking we could keep the original variables read-only and do things
> like "STRIP_CXXFLAGS += ['-WError']" instead. This ensures that the output
> of any mozbuild file is what's actually in the file, not a union of the
> config. IMO a problem with make is everything is normalized into one
> unwieldy namespace. This leads to issues like pollution from environment
> variables.
> * There is a function for registering tier directories. This is because of
> the regular vs static directories non-sense. I welcome alternative ideas.
> Keep in mind order needs to be preserved.

tier_dirs['app'] += ['foo'] ?

> * This is pretty fast. On my machine, it is crawling ~680 mozbuild files in
> ~200ms. That's without any optimization. That's 1 thread. That's without
> Python bytecode caching (.pyc files) (compile() is below the bytecode
> caching layer). At this juncture, I'm not too worried about performance.

I am worried about source directory pollution with .pyc files

> 2) Configure invokes "build backend files" command (integrated with
> config.status/ConfigStatus.py) (like today except semantics are different -
> see below)

So, configure invokes config.status, like today, and config.status invokes build backend files, right?
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-09-02 02:59:47 PDT
Comment on attachment 657629 [details] [diff] [review]
Conversion strawman, v1

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

Looks pretty!

::: accessible/build/build.mozbuild
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

I heard someone say something about "empty Makefiles that make me gag"... ;)

::: accessible/public/build.mozbuild
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG.MOZ_WIDGET_TOOLKIT == 'windows':

Hot pink... I mean, do we want the "MOZ_" prefix?

::: accessible/src/build.mozbuild
@@ +8,5 @@
> +    'windows': ['msaa', 'windows'],
> +    'cocoa': ['mac']
> +}
> +
> +DIRS += toolkit_directories.get(CONFIG.MOZ_WIDGET_TOOLKIT, ['other'])

Honestly not convinced this is better than an if/elif chain

::: b2g/build.mozbuild
@@ +5,5 @@
> +
> +DIRS += ['chrome', 'components', 'locales']
> +
> +if CONFIG.OS_ARCH == 'WINNT':
> +    DIRS += [TOPDIR + '/xulrunner/tools/redit']

Should TOPDIR be a global variable?

::: browser/app.mozbuild
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if not CONFIG.LIBXUL_SDK:
> +    include('%s/toolkit/toolkit.mozbuild' % TOPSRCDIR)

TOPDIR or TOPSRCDIR, % or +?

::: build.mb
@@ +1,1 @@
> +TIERS['base'] = ['config', 'build', 'probes', 'mfbt']

.mb?

@@ +1,5 @@
> +TIERS['base'] = ['config', 'build', 'probes', 'mfbt']
> +
> +if not LIBXUL_SDK:
> +    if MOZ_WIDGET_TOOLKIT in ('android', 'gonk'):
> +        TIERS['base'].append('other-licenses/android')

Append instead of +=?

::: build.mozbuild
@@ +5,5 @@
> +
> +# Bring in the configuration for the configured application.
> +include('%s/app.mozbuild' % CONFIG.MOZ_BUILD_APP)
> +
> +add_tier_dir('base', ['config', 'build', 'probes', 'mfbt'])

Seen this all before, is build.mb something that shouldn't be in this patch?

::: build/mozconfig.py
@@ +1,1 @@
> +import imp, os, sys

MPL header

::: build/win32/build.mozbuild
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG._MSC_VER and CONFIG.OS_TEST != 'x86_64':

Meh, CONFIG._MSC_VER. Can we have something more readable?

::: dom/imptests/Makefile.in
@@ +11,3 @@
>  include $(srcdir)/editing.mk
>  include $(srcdir)/html.mk
>  include $(srcdir)/webapps.mk

Note that those .mk's also change DIRS

::: dom/imptests/editing/Makefile.in
@@ -10,5 @@
> -DIRS = \
> -  css \
> -  conformancetest \
> -  selecttest \
> -  $(NULL)

Those are autogenerated; can you update dom/imptests/writeMakefile.py (after bug 782651 lands).

::: embedding/build.mozbuild
@@ +7,5 @@
> +TEST_DIRS += ['test']
> +
> +if CONFIG.MOZ_WIDGET_TOOLKIT == 'android':
> +    if CONFIG.MOZ_BUILD_APP in ('mobile/xul', 'b2g'):
> +        DIRS += ['android']

if CONFIG.MOZ_WIDGET_TOOLKIT == 'android' and CONFIG.MOZ_BUILD_APP in ('mobile/xul', 'b2g'):

?

::: embedding/components/build.mozbuild
@@ +12,5 @@
> +    'webbrowserpersist',
> +    'commandhandler',
> +]
> +
> +if CONFIG.MOZ_XUL and CONFIG.NS_PRINTING:

MOZ_* and NS_* in one condition, yay! :)

::: extensions/spellcheck/tests/chrome/build.mozbuild
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +
> +DIRS += ['base', 'map']

Drop a newline

::: gfx/build.mozbuild
@@ +25,5 @@
> +
> +if CONFIG.MOZ_ENABLE_SKIA:
> +    DIRS += ['skia']
> +
> +TEST_TOOL_DIRS += ['tests']

<3

::: image/decoders/build.mozbuild
@@ +5,5 @@
> +
> +toolkit = CONFIG.MOZ_WIDGET_TOOLKIT
> +
> +# The Icon Channel stuff really shouldn't live in decoders/icon, but we'll
> +# fix that another time.

(Hmm, this comment is only 2 years old)

::: ipc/ipdl/Makefile.in
@@ +70,5 @@
>    $(PROTOCOLS:%.ipdl=%.cpp)			\
>    $(PROTOCOLS:%.ipdlh=%.cpp)			\
>    $(NULL)
>  
> +GARBAGE_DIRS += _ipdlheaders

You don't seem to be consistent about putting GARBAGE_DIRS in mozbuild files or makefiles?

::: ipc/ipdl/test/build.mozbuild
@@ +7,5 @@
> +# quick and painless
> +TEST_DIRS += ['ipdl']
> +
> +if CONFIG.MOZ_IPDL_TESTS:
> +    TEST_DIRS += ['css']

I think 'cxx' was what you're looking for.

::: js/src/build.mozbuild
@@ +15,5 @@
> +if not CONFIG.JS_DISABLE_SHELL:
> +    DIRS += ['shell']
> +
> +if CONFIG.OS_ARCH != 'ANDROID':
> +    TEST_DIRS += ['jsapi-tests']

You lost the FIXME

::: layout/base/tests/build.mozbuild
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +TEST_DIRS += ['chrome']
> +TOOL_DIRS ++ ['cpp-tests']

Ahem.

::: mobile/android/build.mozbuild
@@ +14,5 @@
> +    'app',
> +]
> +
> +if CONFIG.LIBXUL_SDK:
> +    PARALLEL_DIRS += ['../../xulrunner/tools/redit']

Mm, B2G used TOPDIR

::: mobile/locales/build.mozbuild
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +GENERATED_DIRS += .deps

Nah

::: mobile/xul/build.mozbuild
@@ +12,5 @@
> +    'themes/core',
> +    'app',
> +]
> +
> +if CONFIG.LUBXUL_SDK:

Yay for lubxul

::: nsprpub/build.mozbuild
@@ +1,2 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public

I don't think we'll want to do nspr just yet

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +6,5 @@
> +# data structures.
> +
> +r"""Read build frontend files into data structures.
> +
> +The build system consists of frontend files call "mozbuild" files. mozbuild

called

@@ +47,5 @@
> +
> +from .variables import FRONTEND_VARIABLES
> +
> +# We start with some ultra-generic data structures. These should ideally be
> +# elsewhere. Where?

mozmoz?

@@ +134,5 @@
> +    the instance inside a with statement. e.g.
> +
> +        ns = GlobalNamespace()
> +        with ns:
> +            ns['foo'] = True

Huh.

@@ +289,5 @@
> +
> +        with self._globals as d:
> +            # Register additional global variables.
> +            d['TOPSRCDIR'] = config.topsrcdir
> +            d['TOPOBJDIR'] = config.topobjdir

Hmm, so TOPDIR isn't actually defined?

@@ +335,5 @@
> +            return self._result
> +
> +        variables = {}
> +        for k, v in self._globals.iteritems():
> +            if k in ('CONFIG', 'TOPSRCDIR', 'TOPOBJDIR'):

Don't think I like that this list is basically in two places

@@ +355,5 @@
> +
> +    def _add_tier_directory(self, tier, reldir, static=False):
> +        """Register a tier directory with the build."""
> +        if isinstance(reldir, basestring):
> +            reldir = [reldir]

Do we need this?

@@ +365,5 @@
> +            }
> +
> +        key = 'regular'
> +        if static:
> +            key = 'static'

Why not

if static:
    key = 'static'
else:
    key = 'regular'

or python's sorry excuse for a ternary expression?

@@ +453,5 @@
> +            dir_vars.extend(['TEST_DIRS', 'TEST_TOOL_DIRS'])
> +
> +        # It's very tempting to use a set here. Unfortunately, the recursive
> +        # make backend needs order preserved. Once we autogenerate all backend
> +        # files, we should be able to conver this to a set.

convert

::: python/mozbuild/mozbuild/frontend/variables.py
@@ +3,5 @@
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +######################################################################
> +# DO NOT UPDATE THIS FILE WITHOUT SIGN-OFF FROM A BUILD MODULE PEER. #
> +######################################################################

Really, this should be obvious, but I guess you're right that it isn't...

::: python/mozbuild/mozbuild/test/frontend/test_containers.py
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import unittest
> +

Tests \o/

Are those run automatically?

::: toolkit/components/urlformatter/build.mozbuild
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +
> +TEST_DIRS += ['tests']

One less empty line
Comment 5 Gregory Szorc [:gps] 2012-09-02 10:37:31 PDT
Created attachment 657695 [details] [diff] [review]
Just the Python bits

There is also a rules.mk change in there. It's horribly broken but shows what I was thinking of.
Comment 6 Gregory Szorc [:gps] 2012-09-02 11:00:03 PDT
(In reply to Mike Hommey [:glandium] from comment #3)
> > * Do people like the behavior of the CONFIG variable? Currently, defines and
> > substitutions get stuffed into the same dict. Should we keep them apart?
> > * What's the behavior of defines? Do we magically returned not defined on
> > unknown variable (like current behavior) or should we require that all
> > referenced variables be defined?
> 
> AC_SUBSTs are always defined, whatever their value is. AC_DEFINEs aren't.
> Note that we also do AC_SUBST a lot of AC_DEFINEd stuff just so that they
> can also be accessed from Makefiles. Having access to both substs and
> defines from build manifests would definitely help. OTOH, this might confuse
> people.

Ahhh - I didn't realize this was actually how it's done. Since my implementation sets AC_DEFINEs first then merges AC_SUBSTs on top of it, I think I've made a huge mistake.

> > * There is a function for registering tier directories. This is because of
> > the regular vs static directories non-sense. I welcome alternative ideas.
> > Keep in mind order needs to be preserved.
> 
> tier_dirs['app'] += ['foo'] ?

How do you handle the static dirs bit? In the end, you either have the ugly:

  TIERS['app']['static'] += ['foo']
  TIERS['app']['regular'] += ['bar']

or

  STATIC_TIERS['app'] += ['foo']
  TIERS['app'] += ['bar']

We can certainly make these variables OrderedDict instances (we need to capture the tier order because that is how tier execution works). When you have multiple variables, now you need to capture writes to both and figure out the orders in which the keys were created across multiple variables. Yuck. Or, perhaps we could just hardcode the set of known tiers. If it's empty, we skip it. That seems like the simpler solution.
 
> > * This is pretty fast. On my machine, it is crawling ~680 mozbuild files in
> > ~200ms. That's without any optimization. That's 1 thread. That's without
> > Python bytecode caching (.pyc files) (compile() is below the bytecode
> > caching layer). At this juncture, I'm not too worried about performance.
> 
> I am worried about source directory pollution with .pyc files

Not a worry! .pyc file generation occurs inside the module importing library. compile() operates below this. We won't have bytecode files written unless we explicitly hook up the marshall module. If we do this, we also have total control over where these files are written/read!
 
> > 2) Configure invokes "build backend files" command (integrated with
> > config.status/ConfigStatus.py) (like today except semantics are different -
> > see below)
> 
> So, configure invokes config.status, like today, and config.status invokes
> build backend files, right?

Maybe? config.status definitely generates the backend files. What invokes/runs them (make today) is anyone's guess. This isn't a problem until we have multiple build backends, I think. I think this is where a tool like mach comes into play. From the topsrcdir, you can just type |./mach build| (uses the default) or |./mach build --backend tup|.

I think we should marginalize the need to run things inside the objdir. It's there for advanced users (all of us, admittedly). But, I'd like to move to a model where the objdir can be treated as a black box. IMO this makes Firefox easier to hack on since it reduces the amount of knowledge people need to actually do stuff. I would count "run config.status to build" as one such step better served by an intelligent proxy in the topsrcdir.
Comment 7 Gregory Szorc [:gps] 2012-09-02 11:23:58 PDT
(In reply to :Ms2ger from comment #4)
> ::: accessible/build/build.mozbuild
> @@ +1,5 @@
> > +# vim: set filetype=python:
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> 
> I heard someone say something about "empty Makefiles that make me gag"... ;)

Yes, it hurts me too. Admittedly, I ran a script to create the boilerplate files in every directory containing a Makefile.in so I wouldn't be burdened with this as part of the porting process. Empty ones could potentially be deleted.

But, empty mozbuild files do serve a purpose: they exist. As long as a directory is referenced by a DIR* entry, it needs to have a mozbuild file, just like Makefile.in files are required today. I think missing files should be an error. I think bugs can come up if you ignore them. Even if you print a warning, there's no chance it will be seen. There are some weird configurations out there that very few people see...

Sometime (I'm not sure when), we'll want to go through the tree and trim some of this fat. This is a potential source for bugs. I don't feel it is appropriate to do this as part of the initial migration to mozbuild files.

> ::: accessible/public/build.mozbuild
> @@ +2,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +if CONFIG.MOZ_WIDGET_TOOLKIT == 'windows':
> 
> Hot pink... I mean, do we want the "MOZ_" prefix?

Removing is tempting, but we have a number of variables without that prefix. See objdir/config.status for the full list.
 
> ::: accessible/src/build.mozbuild
> @@ +8,5 @@
> > +    'windows': ['msaa', 'windows'],
> > +    'cocoa': ['mac']
> > +}
> > +
> > +DIRS += toolkit_directories.get(CONFIG.MOZ_WIDGET_TOOLKIT, ['other'])
> 
> Honestly not convinced this is better than an if/elif chain

I did this as a one-off to see what people would think. I'm not convinced either :)

> ::: b2g/build.mozbuild
> @@ +5,5 @@
> > +
> > +DIRS += ['chrome', 'components', 'locales']
> > +
> > +if CONFIG.OS_ARCH == 'WINNT':
> > +    DIRS += [TOPDIR + '/xulrunner/tools/redit']
> 
> Should TOPDIR be a global variable?

Yikes. That's a legitimate bug. If I tried building B2G, this would fail. It's supposed to be TOPSRCDIR.
 
> ::: browser/app.mozbuild
> @@ +3,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +if not CONFIG.LIBXUL_SDK:
> > +    include('%s/toolkit/toolkit.mozbuild' % TOPSRCDIR)
> 
> TOPDIR or TOPSRCDIR, % or +?

We could use either %s or + here. The end result is the same.

> ::: build.mb
> Seen this all before, is build.mb something that shouldn't be in this patch?

Yes. Left over from an earlier version. I'll delete it.

> ::: dom/imptests/Makefile.in
> @@ +11,3 @@
> >  include $(srcdir)/editing.mk
> >  include $(srcdir)/html.mk
> >  include $(srcdir)/webapps.mk
> 
> Note that those .mk's also change DIRS

Good catch. I haven't looked at all the .mk files yet...
 
> ::: dom/imptests/editing/Makefile.in
> @@ -10,5 @@
> > -DIRS = \
> > -  css \
> > -  conformancetest \
> > -  selecttest \
> > -  $(NULL)
> 
> Those are autogenerated; can you update dom/imptests/writeMakefile.py (after
> bug 782651 lands).

Another great catch!

> ::: embedding/components/build.mozbuild
> @@ +12,5 @@
> > +    'webbrowserpersist',
> > +    'commandhandler',
> > +]
> > +
> > +if CONFIG.MOZ_XUL and CONFIG.NS_PRINTING:
> 
> MOZ_* and NS_* in one condition, yay! :)

There are very few NS* defines/substs. I have no clue what they are for. I'm just preserving existing semantics here :)

> ::: ipc/ipdl/Makefile.in
> @@ +70,5 @@
> >    $(PROTOCOLS:%.ipdl=%.cpp)			\
> >    $(PROTOCOLS:%.ipdlh=%.cpp)			\
> >    $(NULL)
> >  
> > +GARBAGE_DIRS += _ipdlheaders
> 
> You don't seem to be consistent about putting GARBAGE_DIRS in mozbuild files
> or makefiles?

Doh. I had initially moved GARBAGE_DIRS as well then decided against it. I thought I had restored every file to its original state. I guess I messed up with one. There should be no diff touching a GARBAGE_DIRS line.

> ::: mobile/android/build.mozbuild
> @@ +14,5 @@
> > +    'app',
> > +]
> > +
> > +if CONFIG.LIBXUL_SDK:
> > +    PARALLEL_DIRS += ['../../xulrunner/tools/redit']
> 
> Mm, B2G used TOPDIR

This touches on an important point: the distinction between the srcdir and the objdir. In theory, there should be no mozbuild files in the objdir. This was using that. Currently, all DIRS* variables in mozbuild files are relative. I suppose we could allow absolute paths within topsrcdir to be used...

> 
> ::: mobile/xul/build.mozbuild
> @@ +12,5 @@
> > +    'themes/core',
> > +    'app',
> > +]
> > +
> > +if CONFIG.LUBXUL_SDK:
> 
> Yay for lubxul

A perfect example of why I would like to see access to defines and substs be strict (e.g. throw an error if you attempt to use a variable that configure doesn't export). Currently, all unknown variables in CONFIG evaluate to None.

> @@ +134,5 @@
> > +    the instance inside a with statement. e.g.
> > +
> > +        ns = GlobalNamespace()
> > +        with ns:
> > +            ns['foo'] = True
> 
> Huh.

GlobalNamespace has a context manager that when active allows *any* variable to be written to it. I thought this was easier than exposing a function to register fields (I thought calling a function to set a field would look ugly). I will probably rewrite the context manager to be:

  with ns.unlock():
     ns['foo'] = True

Less magic == win.

> @@ +355,5 @@
> > +
> > +    def _add_tier_directory(self, tier, reldir, static=False):
> > +        """Register a tier directory with the build."""
> > +        if isinstance(reldir, basestring):
> > +            reldir = [reldir]
> 
> Do we need this?

I thought it was helpful. I was actually thinking of making a custom class that inherits from list and overrides __iadd__ and friends so we could do "DIRS += 'foo'" instead of "DIRS += ['foo']". I know Python people will frown on this, so I held off.
 
> Tests \o/
> 
> Are those run automatically?

Not yet. They will by the time this lands.
Comment 8 Mike Hommey [:glandium] 2012-09-02 12:51:22 PDT
(In reply to Gregory Szorc [:gps] from comment #6)
> > So, configure invokes config.status, like today, and config.status invokes
> > build backend files, right?
> 
> Maybe? config.status definitely generates the backend files. What
> invokes/runs them (make today) is anyone's guess. This isn't a problem until
> we have multiple build backends, I think. I think this is where a tool like
> mach comes into play. From the topsrcdir, you can just type |./mach build|
> (uses the default) or |./mach build --backend tup|.
> 
> I think we should marginalize the need to run things inside the objdir. It's
> there for advanced users (all of us, admittedly). But, I'd like to move to a
> model where the objdir can be treated as a black box. IMO this makes Firefox
> easier to hack on since it reduces the amount of knowledge people need to
> actually do stuff. I would count "run config.status to build" as one such
> step better served by an intelligent proxy in the topsrcdir.

Independently of how it's being run by developers, I think it's clearer to have a workflow that only goes one way, so configure -> config.status -> build. Not going back and forth between configure, config.status, and other things. It also allows not actually re-doing the configure step when you change build manifests.
Comment 9 Gregory Szorc [:gps] 2012-09-02 13:45:30 PDT
(In reply to Mike Hommey [:glandium] from comment #8)
> Independently of how it's being run by developers, I think it's clearer to
> have a workflow that only goes one way, so configure -> config.status ->
> build. Not going back and forth between configure, config.status, and other
> things. It also allows not actually re-doing the configure step when you
> change build manifests.

I agree with this. We conceptually have the 3 phases of:

1) configure
2) backend config of build system (config.status)
3) build

There are certainly some reverse arrows from the build into backend config (e.g. ensuring the Makefile's are up to date). Until we break the habit of people manually invoking Makefile's from the objdir, we will still need those backwards arrows, unfortunately.

I'm optimistic #2 can be fast enough to be a transparent step at the top of *any* build. i.e. any time you do a build it ensures that all frontend files haven't changed and the backend config is up to date. This may sound silly now. But, when we start doing things like generate non-recursive make files, we will need to ensure this.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-09-03 11:48:45 PDT
On another note: I noticed "eliminate allmakefiles" in the summary of this bug...
Comment 11 Gregory Szorc [:gps] 2012-09-03 12:33:39 PDT
(In reply to :Ms2ger from comment #10)
> On another note: I noticed "eliminate allmakefiles" in the summary of this
> bug...

"allmakefiles" refers to allmakefiles.sh and the various related .sh scripts scattered across the tree. They try to duplicate DIRS functionality. Their existence is purely an optimization to make Makefile.in -> Makefile generation more efficient (they allow us to do it all in one batch instead of 1 file at a time by parsing Makefile.in). Once DIRS lives in the Python files, we'll switch Makefile generation to key off of this, rendering allmakefiles.sh and friends useless.
Comment 12 Gregory Szorc [:gps] 2012-09-03 22:50:04 PDT
And integration with the existing build system is working. Kinda.

First, read the docs at:

https://github.com/indygreg/mozilla-central/tree/python-build-files/python/mozbuild#overview
https://github.com/indygreg/mozilla-central/blob/python-build-files/python/mozbuild/mozbuild/frontend/README.rst

Please, read the docs. They explain a lot about how things work and what I think the architecture should be. Note that some pieces (like builders) aren't implemented yet. This is more for the future when we have something besides recursive make.

Once you've read the docs, see the new code at:

https://github.com/indygreg/mozilla-central/blob/python-build-files/python/mozbuild/mozbuild/frontend/emitter.py
https://github.com/indygreg/mozilla-central/blob/python-build-files/python/mozbuild/mozbuild/frontend/data.py
https://github.com/indygreg/mozilla-central/blob/python-build-files/python/mozbuild/mozbuild/backend/recursivemake.py

You can see quite hacky and incomplete integration with config.status at:

https://github.com/indygreg/mozilla-central/commit/bdd61ff103856647bf19f21d01847713c6033f04#build/ConfigStatus.py

So, what this now does is write out dirs.mk files alongside Makefile in objdir when config.status is executed. Surprisingly, the build actually gets through base, nspr, and partially through js before it blows up! It's a start.

Regarding config.status, we currently have two modes today: full and partial. In full mode, we iterate over all known build frontend/Makefile.in files and do the generation of the build backend files (Makefile's) in the object directory. In partial mode (which is invoked automagically during make if the Makefile needs to be regenerated because Makefile.in changed), we only generate 1 Makefile at a time. This is much cheaper than crawling the entire tree.

Anyway, the second we add non-recursive make files (I'm going to make a case that the next big step after DIRS lands is porting IDLSRCS and EXPORTS and we should port them to non-recursive make files because we can), partial regeneration of the backend files is going to be challenging, if not impossible. Today, we have a nice clean 1 to 1 mapping between frontend and backend files. In recursive make, we lose this. We may still write out per-directory .mk files and #include them in a top-level .mk file (this is what I do in build splendid today). But, if you add or remove a directory from that list, you'll need a full tree scan to pick up that change (maybe not for deletes, but certainly for adds).

So, I think we should start moving away from incremental backend generation and just do it all or nothing. Fortunately, the new Python files seem to be fast. On my i7-2600K, it's loading ~1000 files in ~350ms. When you add in the I/O to write dirs.mk, it does it in ~500ms. Granted, the Python files don't have everything defined in them yet. But, I'm optimistic that with everything converted we're still only talking about 1-1.5s to do the loading and about the same to perform I/O. I'm inclined to ignore the I/O overhead because a side-effect of this is it pages the files into memory. The build will do this anyway, so as long as there is no page cache eviction. Not to mention no-op builds still take >60s, so 2-3s is negligible in the grand scheme of things. And, backend generation could only be invoked in a top-level build.

Anyway, if we move away from incremental config.status invocations to generate the backend files, this changes how people use the build system. Today, people rely on |make -C path/in/objdir| automatically regenerating the Makefile. People will complain if this changes.

I'll say it again: we need to start discouraging people from hanging out in the objdir. We should provide a clean frontend to perform partial tree builds which also ensures build files are up to date, etc. client.mk is where we'd do this today. mach (bug 751795) is where we would do it tomorrow.

I recommend we disable the per-directory automatic Makefile generation rule as part of landing this bug. Instead, |make -C topobjdir| and |make -f client.mk| will always do regeneration for the entire tree. Maybe we also expose a new target |make backendconfig| or something. Anyway, if people change a Makefile.in or build.mozbuild in the srcdir, they need to run a separate command before doing a |make -C objdir/subtree| or else those changes won't be reflected. From an architecture perspective, this is also cleaner because it removes a gross backwards arrow from build backend to build configuration. IMO those arrows should just be one way: configure -> build configuration -> builder and we have an intelligent proxy (client.mk/mach) that's smart enough to toggle each only when necessary.
Comment 13 Mike Hommey [:glandium] 2012-09-04 00:28:38 PDT
> Anyway, the second we add non-recursive make files (I'm going to make a case
> that the next big step after DIRS lands is porting IDLSRCS and EXPORTS and
> we should port them to non-recursive make files because we can),

Better yet, we could have a separate program that just installs them all in a smart manner, which could entirely avoid rm -rf'ing dist/idl and dist/include. The FileCopier class in the PoC patch from bug 780561 provides a helper to do just that.
Comment 14 Gregory Szorc [:gps] 2012-09-07 18:06:27 PDT
Created attachment 659417 [details] [diff] [review]
Part 1: Generic container classes, v1

The sandboxing code will use these. AFAICT they don't exist in the standard library. http://docs.python.org/library/collections.html#collections.defaultdict is the closest they have and I don't think that exactly fits the bill. It's simple enough to implement, so I'm not too worried about maybe a little code duplication.

This code has nothing build specific, so I'm giving to jhammel for review. If anyone with Python knowledge and review power wants to take this, I'm sure jhammel won't mind less work :)
Comment 15 Gregory Szorc [:gps] 2012-09-08 21:15:07 PDT
Created attachment 659547 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v1

Here we have the new core of the build system.

As with the previous patch, please look at the README.rst's for overall system design. The one in frontend/ references things that haven't landed yet. This is by design. I think it's best if all of what I'm planning is in the mind of the reviewers up front.

The important code files are python/mozbuild/mozbuild/frontend/{reader,sandbox_symbols}.py.

The rest of the patch is tests. And, there's a lot of them. I'm pretty satisfied with the test coverage. I've refactored things a few times and have found the tests breaking when they should, etc. There some advanced cases not covered by them, but I think they are certainly good enough for an initial land. We can certainly fill in gaps later.

An absurd amount of effort was put into error handling in reader.py. This is all in the name of user-friendly error messages. Basically, if an error is encountered, you should get a very clear message saying exactly what failed, where it failed (including the actual source from the processed file), and a helpful message saying what needs to be changed.

I'm pretty confident the Python is doing what it's supposed to be doing (courtesy of the tests). So, the reviewers really just need to focus on whether it's doing what we want it to be doing. See previous discussion for points of focus. Mainly:

* Treatment of defines and substitutions
* Variable and function conventions
* Overall planned architecture

I don't see this changing very much before we actually plug it in. The one part we may need to add is support for including separate instances of a BuildReader environment from within one. e.g. we want a way for separate independent projects under the same tree to all be hooked up together. We'll (likely) need this to build spidermonkey. I'm content with this being handled as a follow-up patch.

I would like to get r+ from at least 2 build module members, with Ted (owner) being one of them.

I've added jhammel for Python-specific feedback.

If anyone else wants to add feedback, please do. I like eyeballs.
Comment 16 Gregory Szorc [:gps] 2012-09-08 21:23:11 PDT
Current code requires Python 2.7, so blocking on having 2.7 deployed to all the build slaves. Note this code would run on just the builders, not the tests hosts.

Things /could/ be backported. But, I would prefer not to do that unless the timelines for this project and 2.7 deployment are really out of whack.
Comment 17 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-09-08 23:45:18 PDT
Our current RHEL comes with python 2.6, as does MacOS 10.6 and a fair number of community machines still. Unless python 2.7 is fundamentally important for some feature, I think we should try sticking with 2.6 as a baseline.
Comment 18 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-09-08 23:46:30 PDT
FWIW, I'm fine with requiring python 2.7 on *Windows* if we update mozillabuild... it's the *nix-based systems that are often much more difficult to upgrade.
Comment 19 Gregory Szorc [:gps] 2012-09-09 01:26:45 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Our current RHEL comes with python 2.6, as does MacOS 10.6 and a fair number
> of community machines still. Unless python 2.7 is fundamentally important
> for some feature, I think we should try sticking with 2.6 as a baseline.

I thought the 2.7-at-a-minimum decision had been made? If it hasn't, then perhaps this should be taken to a mailing list.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> FWIW, I'm fine with requiring python 2.7 on *Windows* if we update
> mozillabuild... it's the *nix-based systems that are often much more
> difficult to upgrade.

MozillaBuild ships with 2.7.

I think you are over-estimating the difficulty to upgrade *nix-based systems. Many distros have 2.7 packages (python27 in yum) if they don't ship 2.7 out of the box. It's easy to get via homebrew on OS X. As a last resort, the manual build instructions are literally curl | tar && configure && make install.

Of course, once this goes to the mailing list someone will say "y u no Python 3" and we'll have a nice little can of worms on our hands because they'll have a point.
Comment 20 Gregory Szorc [:gps] 2012-09-09 16:36:39 PDT
Created attachment 659604 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v2

Now with Python 3.2 compatibility. Might as well cross this bridge now since Python 3 is inevitable.

I also removed some TypeError foo on the namespaces, as I think it is unnecessary.

Also, dev-platform thread started regarding Python 2.6 vs 2.7.
Comment 21 Jeff Hammel 2012-09-09 19:54:13 PDT
Comment on attachment 659417 [details] [diff] [review]
Part 1: Generic container classes, v1

--- a/python/mozbuild/mozbuild/util.py

I'd be more inclined to put all of the dict classes in a new file vs utils, but this is fine for the time being.

+        if defaults is None:
+            defaults = {}
+
+        self._defaults = defaults

I usually use the pattern: `self._defaults = defaults or {}`

Though I'm not really sure what the point of the defaults argument as used is, though (I haven't poked the other patches for context, my laziness).

...global_default=(None,)

This is used as a magic singleton ABICT.  I would much prefer making e.g. an `undefined` singleton:

class undefined(object):
    """represents an undefined key blah blah blah"""
undefined = undefined()

+    def __getattr__(self, k):
+        return self.__getitem__(k)
+

Not sure I get why you're doing this?  Maybe a docstring/comment would help.

Too bad http://www.python.org/dev/peps/pep-0416/ didn't happen :(  I recall there being something like this in some version of the stdlib, but I can't recall OTTOMH.

Can we use http://docs.python.org/library/collections.html#collections.defaultdict versus rolling our own?

r+, with the undefined singleton change or similar (or explanation why (None,) is a good magical value, as you could conceivably want this as a global default)
Comment 22 Jeff Hammel 2012-09-09 19:55:32 PDT
Also, in the general sense, we might want to start moving out these generic items to packages at the /python level (or mozbase, etc)
Comment 23 Jeff Hammel 2012-09-09 20:14:03 PDT
Comment on attachment 659604 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v2

+if sys.version < '3':

I tend to use sys.version_info instead

+    def allow_all_writes(self):
a docstring would help

+        if name.isupper():
+            return self._globals[name]

I find this a bit magical.  If we are doing this -- i.e. enforcing upper-case names == globals -- I'd like to see this documented better.

+class SandboxError(Exception):
+    pass

docstring, plz :)

+class Sandbox(object):

Again, I'm wondering in the medium-long term if we should make this pattern its own package consumed by mozbuild but not tied to it.  But less important right now.

+        topobjdir = os.path.abspath(config.topobjdir)
+
+        # This may not always hold true. If we ever have autogenerated mozbuild
+        # files in topobjdir, we'll need to change this.
+        assert path.startswith(config.topsrcdir)
+        assert not path.startswith(topobjdir)

Maybe better to use os.path.realpath then?

+        key = 'regular'
+        if static:
+            key = 'static'

key = 'static' if static else 'regular'
Comment 24 Gregory Szorc [:gps] 2012-09-09 21:48:32 PDT
(In reply to Jeff Hammel [:jhammel] from comment #21)
> ...global_default=(None,)
> 
> This is used as a magic singleton ABICT.  I would much prefer making e.g. an
> `undefined` singleton:
> 
> class undefined(object):
>     """represents an undefined key blah blah blah"""
> undefined = undefined()

Changed locally.
 
> +    def __getattr__(self, k):
> +        return self.__getitem__(k)
> +
> 
> Not sure I get why you're doing this?  Maybe a docstring/comment would help.

This snuck in. It's for the CONFIG global variable to make things look prettier, IMO. I'll move it to reader.py. There's a chance we may not keep it.

> Can we use
> http://docs.python.org/library/collections.html#collections.defaultdict
> versus rolling our own?

I thought about it. But, I would need to override __missing__. At that point, I don't think there is any advantage to using this class.
Comment 25 :Ms2ger (⌚ UTC+1/+2) 2012-09-16 05:27:30 PDT
s/interpretter/interpreter/g, please.
Comment 26 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-09-17 10:15:23 PDT
Comment on attachment 659604 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v2

As much as I'd love to help and understand this, my other projects are eating all my cycles so I'm going to bow out of detailed feedback. If you have specific design or code decisions that you'd like input on, feel free to ask.
Comment 27 Ted Mielczarek [:ted.mielczarek] 2012-09-19 08:40:39 PDT
Comment on attachment 659604 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v2

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

This patch was a lot scarier looking until I realized that most of the files included are just test files.

r=me with mostly nitpicky comments along with a few naming/etc suggestions.

Thanks for taking this on, this is huge!

::: python/mozbuild/mozbuild/frontend/README.rst
@@ +28,5 @@
> +=========================
> +
> +As stated above, *mozbuild* files are actually Python scripts. However,
> +their behavior is very different from what you would expect if you executed
> +the file using the standard Python interpretter from the command line.

"interpreter"

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +41,5 @@
> +from contextlib import contextmanager
> +from io import StringIO
> +
> +from mozbuild.frontend.sandbox_symbols import FUNCTIONS
> +from mozbuild.frontend.sandbox_symbols import VARIABLES

I know we have some style rule that suggests one import per line, but I think importing N symbols from the same module should just go on the same line.

@@ +51,5 @@
> +    text_type = unicode
> +    type_type = types.TypeType
> +else:
> +    text_type = str
> +    type_type = type

This is a little ugly, but given that you're not using these all over the place it's not as bad as I thought.

@@ +125,5 @@
> +            dict.__setitem__(self, name, value)
> +            return
> +
> +        # We don't need to check for name.isupper() here because we should
> +        # never be in a position to be writing non-uppercase variables here.

Why?

@@ +199,5 @@
> +    def __init__(self, file_stack, exc_type, exc_value, trace):
> +        self.file_stack = file_stack
> +        self.exc_type = exc_type
> +        self.exc_value = exc_value
> +        self.trace = trace

Do you want to move file_stack and trace up to SandboxError, since both subclasses implement them?

@@ +229,5 @@
> +    executing Python code like normal is that the executed code is very limited
> +    in what it can do: the sandbox only exposes a very limited set of Python
> +    functionality. Only specific types and functions are available. This
> +    prevents executed code from doing things like import modules, open files,
> +    etc.

It would be nice to mention here that the set of globals provided to a Sandbox is in the variables in the sandbox_symbols module.

@@ +268,5 @@
> +        with self._globals.allow_all_writes() as d:
> +            # Register additional global variables.
> +            d['TOPSRCDIR'] = config.topsrcdir
> +            d['TOPOBJDIR'] = topobjdir
> +            d['RELDIR'] = reldir

We call this "relativesrcdir" in Makefiles currently, FWIW. "RELDIR" is a bit obtuse for me.

@@ +270,5 @@
> +            d['TOPSRCDIR'] = config.topsrcdir
> +            d['TOPOBJDIR'] = topobjdir
> +            d['RELDIR'] = reldir
> +            d['SRCDIR'] = os.path.join(config.topsrcdir, reldir).rstrip('/')
> +            d['OBJDIR'] = os.path.join(topobjdir, reldir).rstrip('/')

This is a little confusing, since when we refer to "objdir" we usually mean the root of the objdir. I guess we don't actually use that as a variable name (we use $(DEPTH) in makefiles), but it seems potentially confusing. I don't have a great alternate suggestion. gmake uses $(CURDIR) to mean "the current directory", but overloading that might be weird.

@@ +273,5 @@
> +            d['SRCDIR'] = os.path.join(config.topsrcdir, reldir).rstrip('/')
> +            d['OBJDIR'] = os.path.join(topobjdir, reldir).rstrip('/')
> +
> +            # DEPTH intentionally skipped because it is silly. Use TOPOBJDIR
> +            # instead.

I don't think this documentation is particularly useful here. Perhaps in a user guide, or a migration from makefiles guide.

@@ +298,5 @@
> +        Executed files must reside within topsrcdir. This is an arbitrary
> +        restriction coded mostly for sanity.
> +        """
> +        if not os.path.isabs(path):
> +            if len(self._execution_stack):

if self._execution_stack:

@@ +349,5 @@
> +        except SandboxError as e:
> +            raise e
> +        except NameError as e:
> +            # A NameError is raised when a local or global could not be found.
> +            # The original KeyError has been dropped by the interpretter.

"interpreter"

@@ +354,5 @@
> +            # However, we should have it cached in our namespace instances!
> +
> +            # Unless a script is doing something wonky like catching NameError
> +            # itself (that would be silly), if there is an exception on the
> +            # global namespace, that's our error.

Do sandboxed scripts have the ability to try/except like this?

@@ +430,5 @@
> +    messages. Execution errors should say:
> +
> +      - Why they failed
> +      - Where they failed
> +      - What can be done to prevent the error

I really, really like the lengths you went through here to provide useful error messages. Great stuff!

@@ +471,5 @@
> +
> +        s.write('=' * 30 + '\n')
> +        s.write('ERROR PROCESSING MOZBUILD FILE\n')
> +        s.write('=' * 30 + '\n')
> +        s.write('\n')

This feels like a really weird way to build up a string. Why not just use a """ string with interpolation? If there's some benefit to doing this this way, at least use triple-quoted multiline strings for each block between the conditionals.

@@ +498,5 @@
> +        return s.getvalue()
> +
> +    def _print_sandbox_error(self, s):
> +        # Try to find the frame of the executed code. There is probably a
> +        # better way to do this employing low-level Python interpretter

"interpreter", dammit. :)

@@ +499,5 @@
> +
> +    def _print_sandbox_error(self, s):
> +        # Try to find the frame of the executed code. There is probably a
> +        # better way to do this employing low-level Python interpretter
> +        # magic. And, it should probably be done in Sandbox.

If this should be done in Sandbox, then why not put this code in sandbox?

@@ +576,5 @@
> +            s.write('\n')
> +            s.write('    %s\n' % inner.text)
> +            s.write((' ' * (inner.offset + 4)) + '^\n')
> +            s.write('\n')
> +            s.write('Change the file to use valid Python and try again.\n')

I'd probably just say "Fix the syntax error and try again."

@@ +708,5 @@
> +        Traversal is performed depth first (for no particular reason).
> +        """
> +        self._execution_stack.append(path)
> +        try:
> +            for s in self._read_mozbuild(path, read_tiers=read_tiers):

What's the reasoning for splitting this method into public and private halves?

@@ +760,5 @@
> +                continue
> +
> +            for d in sandbox[var]:
> +                if d not in dirs:
> +                    dirs.append(d)

This should error if you try to list the same directory twice.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +22,5 @@
> +# This defines the set of mutable global variables.
> +#
> +# Each variable is a tuple of:
> +#
> +#   (type, default_value, docs)

Is the explicit type here just for future sanity? Looks like all the instances you have here could just use type(default). Do you have some use case in mind that would need this?

@@ +68,5 @@
> +        complete.
> +        """),
> +
> +    'TEST_TOOL_DIRS': (list, [],
> +        """TOOL_DIRS that is only executed if tests are enabled."""),

Feels like overkill, we don't use this currently.

@@ +87,5 @@
> +        the build backend. They will likely be replaced by DIRS.
> +
> +        This variable is typically not populated directly. Instead, it is
> +        populated by calling add_tier_dir().
> +        """),

Given that this is purely transitional, is there much value in propagating it to the new world order? Ostensibly we could replace tiers with DIRS entirely and get almost the same result, right? (Except that we'd export everything before building anything, but that doesn't seem terribly important.)

@@ +121,5 @@
> +        include('sibling.mozbuild')
> +
> +        # Include "foo.mozbuild" from a path within the top source directory.
> +        include(TOPSRCDIR + '/elsewhere/foo.mozbuild')
> +        """),

Might be prematurely optimizing, but I wonder if it's too clever to think about making the syntax for topsrcdir-relative-includes simpler?

@@ +141,5 @@
> +        add_tier_dir('app', ['foo', 'bar'])
> +
> +        # Register a directory as having static content (no dependencies).
> +        add_tier_dir('base', 'foo', static=True)
> +        """),

As before, I wonder if we shouldn't just jettison tiers as part of the transition.

@@ +192,5 @@
> +        through this object. e.g. ENABLE_TESTS, CFLAGS, etc.
> +
> +        Values can be accessed through dictionary keys or by attributes. e.g.
> +        CONFIG['CFLAGS'] == CONFIG.CFLAGS. By convention, attribute access is
> +        preferred as it is less typing.

Clever, but if we prefer attribute access maybe we should just make that the only way?

::: python/mozbuild/mozbuild/test/frontend/data/include-basic/build.mozbuild
@@ +1,2 @@
> +# Any copyright is dedicated to the Public Domain.
> +# http://creativecommons.org/publicdomain/zero/1.0/

Totally bikesheding, but "build.mozbuild" is really redundant. I wouldn't block this work on a perfect name, but ugh.
Comment 28 Mike Hommey [:glandium] 2012-09-19 08:52:40 PDT
> > +        # Include "foo.mozbuild" from a path within the top source directory.
> > +        include(TOPSRCDIR + '/elsewhere/foo.mozbuild')
> > +        """),

> Might be prematurely optimizing, but I wonder if it's too clever to think about
> making the syntax for topsrcdir-relative-includes simpler?

include('/elsewhere/foo.mozbuild') would seem a simple solution, here. There's no point trying to include something with a pure absolute directory, so that can be relative to the top source directory.
Comment 29 Ted Mielczarek [:ted.mielczarek] 2012-09-19 09:01:07 PDT
That was what I had in mind as well.
Comment 30 :Ms2ger (⌚ UTC+1/+2) 2012-09-19 09:45:51 PDT
(In reply to Ted Mielczarek [:ted] from comment #27)
> @@ +68,5 @@
> > +        complete.
> > +        """),
> > +
> > +    'TEST_TOOL_DIRS': (list, [],
> > +        """TOOL_DIRS that is only executed if tests are enabled."""),
> 
> Feels like overkill, we don't use this currently.

Looking at <http://mxr.mozilla.org/mozilla-central/search?string=TOOL_DIRS&find=&findi=&filter=test&hitlimit=&tree=mozilla-central>, I think this would be good to have.
Comment 31 Ted Mielczarek [:ted.mielczarek] 2012-09-19 09:55:15 PDT
I suspect we could probably put all TEST_DIRS into TOOL_DIRS and not break anything.
Comment 32 Gregory Szorc [:gps] 2012-10-07 16:12:53 PDT
(In reply to Ted Mielczarek [:ted] from comment #27)
> @@ +51,5 @@
> > +    text_type = unicode
> > +    type_type = types.TypeType
> > +else:
> > +    text_type = str
> > +    type_type = type
> 
> This is a little ugly, but given that you're not using these all over the
> place it's not as bad as I thought.

If you want Python 2 + 3 compat, this is how it needs to be done. I anticipate that over time we'll have to create a pycompat.py file holding all this stuff. Until we have enough to warrant splitting it out, it will just have to live in separate modules. If you want me to be proactive, I can be...

FWIW, I've made the personal decision that all new Python I write will be 2.7 + 3.2 compatible. Python 3 is coming whether we like it or not. Ubuntu 12.10 is still AFAIK planning to ship 3.2 as the default Python. This may just be the tipping point for Python 3 adoption. (FWIW, Ubuntu will make 2.7 available as a package. But, 3.2 will be the default.) We have a long way to go before existing Python in m-c is Python 3 compatible. But, at least I won't be contributing to future engineering debt :)

> 
> @@ +199,5 @@
> > +    def __init__(self, file_stack, exc_type, exc_value, trace):
> > +        self.file_stack = file_stack
> > +        self.exc_type = exc_type
> > +        self.exc_value = exc_value
> > +        self.trace = trace
> 
> Do you want to move file_stack and trace up to SandboxError, since both
> subclasses implement them?

I should probably do that, yes.

> 
> @@ +268,5 @@
> > +        with self._globals.allow_all_writes() as d:
> > +            # Register additional global variables.
> > +            d['TOPSRCDIR'] = config.topsrcdir
> > +            d['TOPOBJDIR'] = topobjdir
> > +            d['RELDIR'] = reldir
> 
> We call this "relativesrcdir" in Makefiles currently, FWIW. "RELDIR" is a
> bit obtuse for me.

I was trying to sneak in less typing in the new world. You caught me :)

> @@ +270,5 @@
> > +            d['TOPSRCDIR'] = config.topsrcdir
> > +            d['TOPOBJDIR'] = topobjdir
> > +            d['RELDIR'] = reldir
> > +            d['SRCDIR'] = os.path.join(config.topsrcdir, reldir).rstrip('/')
> > +            d['OBJDIR'] = os.path.join(topobjdir, reldir).rstrip('/')
> 
> This is a little confusing, since when we refer to "objdir" we usually mean
> the root of the objdir. I guess we don't actually use that as a variable
> name (we use $(DEPTH) in makefiles), but it seems potentially confusing. I
> don't have a great alternate suggestion. gmake uses $(CURDIR) to mean "the
> current directory", but overloading that might be weird.

I agree that "objdir" is overloaded. However, I'm not sure what we should do. How much nomenclature should we carry forward [for convenience]? If we want to make a clean break, should we try to avoid previous nomenclature?

We definitely need a distinction between source/input directories and object/output directories. How about "input" and "output?" {topinputdir, topoutputdir, inputdir, outputdir}? Maybe we just leave "source" and substitute "output" for "object?" {topsrcdir, topoutdir, srcdir, outdir}? I dunno. Naming is hard.

 
> @@ +354,5 @@
> > +            # However, we should have it cached in our namespace instances!
> > +
> > +            # Unless a script is doing something wonky like catching NameError
> > +            # itself (that would be silly), if there is an exception on the
> > +            # global namespace, that's our error.
> 
> Do sandboxed scripts have the ability to try/except like this?

try/except are built-in language keywords, so scripts could use them. However, the Exception classes/symbols aren't exported to the sandbox. So, scripts would effectively be limited to an empty except clause.


> @@ +499,5 @@
> > +
> > +    def _print_sandbox_error(self, s):
> > +        # Try to find the frame of the executed code. There is probably a
> > +        # better way to do this employing low-level Python interpretter
> > +        # magic. And, it should probably be done in Sandbox.
> 
> If this should be done in Sandbox, then why not put this code in sandbox?

Will do.

> 
> @@ +708,5 @@
> > +        Traversal is performed depth first (for no particular reason).
> > +        """
> > +        self._execution_stack.append(path)
> > +        try:
> > +            for s in self._read_mozbuild(path, read_tiers=read_tiers):
> 
> What's the reasoning for splitting this method into public and private
> halves?

It makes it easier to catch exceptions and handle the logic around execution stacks.

> @@ +760,5 @@
> > +                continue
> > +
> > +            for d in sandbox[var]:
> > +                if d not in dirs:
> > +                    dirs.append(d)
> 
> This should error if you try to list the same directory twice.

Agreed. Will change.

> 
> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> @@ +22,5 @@
> > +# This defines the set of mutable global variables.
> > +#
> > +# Each variable is a tuple of:
> > +#
> > +#   (type, default_value, docs)
> 
> Is the explicit type here just for future sanity? Looks like all the
> instances you have here could just use type(default). Do you have some use
> case in mind that would need this?

There are some scenarios where you may want to allow multiple types through. e.g. str and unicode or float and int.

> 
> @@ +68,5 @@
> > +        complete.
> > +        """),
> > +
> > +    'TEST_TOOL_DIRS': (list, [],
> > +        """TOOL_DIRS that is only executed if tests are enabled."""),
> 
> Feels like overkill, we don't use this currently.

As I was converting the Makefile.in, I noticed a common pattern of:

  ifdef ENABLE_TESTS
  TOOL_DIRS += XXX
  endif

We have TEST_DIRS, which is similar. I figured I'd factor it out.

> @@ +87,5 @@
> > +        the build backend. They will likely be replaced by DIRS.
> > +
> > +        This variable is typically not populated directly. Instead, it is
> > +        populated by calling add_tier_dir().
> > +        """),
> 
> Given that this is purely transitional, is there much value in propagating
> it to the new world order? Ostensibly we could replace tiers with DIRS
> entirely and get almost the same result, right? (Except that we'd export
> everything before building anything, but that doesn't seem terribly
> important.)

Eventually tiers will go away, yes. However, I'm a fan of not biting off too much as part of a transition. I think we should keep the structure of the build system as-is or close to as-is during the initial transition. Once we've proven everything works out, we can start changing the behavior.
 
> @@ +141,5 @@
> > +        add_tier_dir('app', ['foo', 'bar'])
> > +
> > +        # Register a directory as having static content (no dependencies).
> > +        add_tier_dir('base', 'foo', static=True)
> > +        """),
> 
> As before, I wonder if we shouldn't just jettison tiers as part of the
> transition.

See above. I don't want to risk backout because jettisoning tiers broke something. We can jettison tiers later.

> 
> @@ +192,5 @@
> > +        through this object. e.g. ENABLE_TESTS, CFLAGS, etc.
> > +
> > +        Values can be accessed through dictionary keys or by attributes. e.g.
> > +        CONFIG['CFLAGS'] == CONFIG.CFLAGS. By convention, attribute access is
> > +        preferred as it is less typing.
> 
> Clever, but if we prefer attribute access maybe we should just make that the
> only way?

Make sense to me.

> 
> ::: python/mozbuild/mozbuild/test/frontend/data/include-basic/build.mozbuild
> @@ +1,2 @@
> > +# Any copyright is dedicated to the Public Domain.
> > +# http://creativecommons.org/publicdomain/zero/1.0/
> 
> Totally bikesheding, but "build.mozbuild" is really redundant. I wouldn't
> block this work on a perfect name, but ugh.

Yes, it sucks. I'm open to suggestions!
Comment 33 Ted Mielczarek [:ted.mielczarek] 2012-10-08 06:06:30 PDT
(In reply to Gregory Szorc [:gps] from comment #32)
> FWIW, I've made the personal decision that all new Python I write will be
> 2.7 + 3.2 compatible. Python 3 is coming whether we like it or not. Ubuntu
> 12.10 is still AFAIK planning to ship 3.2 as the default Python. This may
> just be the tipping point for Python 3 adoption. (FWIW, Ubuntu will make 2.7
> available as a package. But, 3.2 will be the default.) We have a long way to
> go before existing Python in m-c is Python 3 compatible. But, at least I
> won't be contributing to future engineering debt :)

This is absolutely the right plan. We've been hamstrung by not having Python 2.7 on our build infrastructure, but hopefully once we've got that settled this will be easier to do. It'd be great to have all of our Python be 3.0-compat.

> I agree that "objdir" is overloaded. However, I'm not sure what we should
> do. How much nomenclature should we carry forward [for convenience]? If we
> want to make a clean break, should we try to avoid previous nomenclature?
> 
> We definitely need a distinction between source/input directories and
> object/output directories. How about "input" and "output?" {topinputdir,
> topoutputdir, inputdir, outputdir}? Maybe we just leave "source" and
> substitute "output" for "object?" {topsrcdir, topoutdir, srcdir, outdir}? I
> dunno. Naming is hard.

I think srcdir and objdir are pretty inoffensive names and people understand what they mean. I don't think we should change those, even if you want to change other things.

> Eventually tiers will go away, yes. However, I'm a fan of not biting off too
> much as part of a transition. I think we should keep the structure of the
> build system as-is or close to as-is during the initial transition. Once
> we've proven everything works out, we can start changing the behavior.

OK, let's file a followup on ditching tiers then.

> > Totally bikesheding, but "build.mozbuild" is really redundant. I wouldn't
> > block this work on a perfect name, but ugh.
> 
> Yes, it sucks. I'm open to suggestions!

We could just call them .build files, and use "moz.build".
Comment 34 :Ehsan Akhgari (away Aug 1-5) 2012-10-11 16:33:54 PDT
*** Bug 800635 has been marked as a duplicate of this bug. ***
Comment 35 Gregory Szorc [:gps] 2012-10-12 01:48:37 PDT
Created attachment 670718 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v3

Incorporated a lot of feedback. I tried to get most of it. I'm sure I'm missing some. Eyes, please.

I moved the core sandbox code into sandbox.py. It is now rather generic and could be used to serve other needs. (One interesting idea would be to leverage it for a mozconfig replacement. Just a thought - not saying we should do that.)

While I moved the generic sandboxing code into its own file/module, the testing code is still Mozilla centric. If we ever come up with another use for it, we can untangle the test coupling.

Since in the new world we have Python data structures defining what's allowed in the build system, I hooked up a mach command to view help on them:

---

$ ./mach help mozbuild-reference
usage: mach [global arguments] mozbuild-reference
       [-h] [--name-only] [symbol [symbol ...]]

positional arguments:
  symbol           Symbol to view help on. If not specified, all will be
                   shown.

optional arguments:
  -h, --help       show this help message and exit
  --name-only, -n  Print symbol names only.


$ ./mach mozbuild-reference TIERS
TIERS
=====

Defines directories constituting the tier traversal mechanism.

Type: OrderedDict
Default Value: OrderedDict()

The recursive make backend iteration is organized into tiers. There are ...

---

The output is reST, so you can easily convert to HTML or whatever markup language all the cool kids are using.

I should probably add mach commands to feed individual files into a sandbox or into the build reader (mainly as a debugging tool). But, I'll need the config.status integration in place before that can be done.

There were some things I said I would do but didn't. The big one is refactoring all the error formatting. I tried. I tried moving the error formatting into sandbox.py but it was asking too much. The execution model of the build reader and the generic nature of sandboxes was too big a wall to overcome. I'd have to incorporate lots of extra APIs on sandboxes to capture state and that seemed overkill.

I made some minor changes to exception handling. Again, all in the name of helpful error messages.

I'm pretty happy with the state of this patch and barring any radical feedback from Mike Hommey (Ted already gave r+), I think I'll start giving parts 3+ some love and ready them for review.
Comment 36 Gregory Szorc [:gps] 2012-10-12 01:56:19 PDT
Also, I'm still soliciting ideas for a better file name than "moz.build." One possible future has these files serving more than just the build system.

For example, we could capture general tree metadata in them as well. We could, say, capture the Bugzilla component for a directory tree. That way, an automated tool could come along and figure out which Bugzilla component to create a bug in from a file in the repository. We could also capture owners or peers of directories to help find points of contact or suitable reviewers for a patch. Perhaps we could record mailing lists for different directories. There's no shortage of ideas here.

With that in mind "moz.build" files implies build system and that seems a little constraining. I'd like something more "metadata-y" but I'm not sure what. I'll happily sign off on something that makes no or little practical sense but has a silly acronym (.mfbt files, anyone?).
Comment 37 Ed Morley [:emorley] 2012-10-12 02:01:27 PDT
(In reply to Gregory Szorc [:gps] from comment #36)
> an automated tool could come along and figure out which Bugzilla component
> to create a bug in from a file in the repository. 

That would be sweet for eg bug 779529.
Comment 38 Ted Mielczarek [:ted.mielczarek] 2012-10-12 06:36:26 PDT
You could just use source.info or something suitably generic as the filename. In general I don't think calling them .build files and then sticking non-build-related info in them is as bad as you think.

(In reply to Gregory Szorc [:gps] from comment #35)
> I moved the core sandbox code into sandbox.py. It is now rather generic and
> could be used to serve other needs. (One interesting idea would be to
> leverage it for a mozconfig replacement. Just a thought - not saying we
> should do that.)

Couldn't be much worse than sourcing mozconfig as a shell script!

> The output is reST, so you can easily convert to HTML or whatever markup
> language all the cool kids are using.

Clever, we could totally export this to MDN. (We have a build glossary there, but it's probably fairly out-of-date.)
Comment 39 Jeff Hammel 2012-10-12 10:34:14 PDT
> 
> > The output is reST, so you can easily convert to HTML or whatever markup
> > language all the cool kids are using.
> 
> Clever, we could totally export this to MDN. 

A big +1 to that
Comment 40 Mike Hommey [:glandium] 2012-10-12 11:49:11 PDT
(In reply to Gregory Szorc [:gps] from comment #36)
> Also, I'm still soliciting ideas for a better file name than "moz.build."

build.manifest?
Comment 41 Gregory Szorc [:gps] 2012-10-12 17:38:02 PDT
Created attachment 671000 [details] [diff] [review]
Part 3: Convert executed sandboxes into data structures, v1

This patch takes the stream of executed MozbuildSandbox instances emitted from BuildReader and converts them into static data structures.

The code is relatively straightforward since all we are currently doing is slurping off DIRS* and tiers variables.

_emit_directory_traversal_from_sandbox is its own function because emit_from_sandbox will grow over time and we might as well factor it out now (I want to avoid monolithic functions wherever possible).

The end state of this patch effectively constitute the frontend portion of our new build system. We have a stream of objects represents the parsed state of all the moz.build files. The next set of patches will focus on doing something useful with this data.
Comment 42 :Ms2ger (⌚ UTC+1/+2) 2012-10-13 13:20:02 PDT
Comment on attachment 670718 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v3

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

::: python/mozbuild/README.rst
@@ +9,5 @@
>  ================
>  
>  * mozbuild.compilation -- Functionality related to compiling. This
>    includes managing compiler warnings.
> +  includes managing compiler warnings.

Double line

@@ +11,5 @@
>  * mozbuild.compilation -- Functionality related to compiling. This
>    includes managing compiler warnings.
> +  includes managing compiler warnings.
> +* mozbuild.frontend -- Functionality for reading build frontend files
> +  (what defines the build system) and converting it to data structures

...converting them

::: python/mozbuild/mozbuild/frontend/mach_commands.py
@@ +20,5 @@
> +    VARIABLES,
> +)
> +
> +
> +def get_doc(doc):

A little docstring, please.

@@ +101,5 @@
> +        print('=================')
> +        print('')
> +
> +        for v in sorted(SPECIAL_VARIABLES.keys()):
> +            self.special_reference(v)

return 0

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +1,2 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: "file,"
Comment 43 Gregory Szorc [:gps] 2012-10-13 16:54:51 PDT
Created attachment 671150 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v4

Incorporated Ms2ger's feedback. Minor code cleanup.
Comment 44 Ted Mielczarek [:ted.mielczarek] 2012-12-04 12:35:58 PST
Comment on attachment 671150 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v4

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

::: python/mozbuild/mozbuild/frontend/mach_commands.py
@@ +1,3 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

This is clever, but how well does it work in practice? Is it unwieldy to look at this documentation in a console?
Comment 45 Gregory Szorc [:gps] 2012-12-04 12:41:11 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #44)
> Comment on attachment 671150 [details] [diff] [review]
> Part 2: Implement Python sandboxing and tree reading, v4
> 
> Review of attachment 671150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/frontend/mach_commands.py
> @@ +1,3 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> This is clever, but how well does it work in practice? Is it unwieldy to
> look at this documentation in a console?

The full output is probably of marginal value to the console. But, single symbol reference could be useful.

I would like to glue in an HTML renderer at some point. As soon as we add an RST -> HTML module in the tree, the mach command could emit HTML and open up your web browser automatically (http://docs.python.org/2/library/webbrowser.html). We could even post HTML to MDN via their new publish API.
Comment 46 Mike Hommey [:glandium] 2012-12-10 02:37:25 PST
Comment on attachment 671150 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v4

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

rs=me

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +67,5 @@
> +    """
> +    def __init__(self, config, path):
> +        """Create an empty mozbuild Sandbox.
> +
> +        The passed in config is a ConfigStatus instance (the output of

Missing word before passed?

@@ +107,5 @@
> +    def exec_file(self, path, filesystem_absolute=False):
> +        """Override exec_file to normalize paths and restrict file loading.
> +
> +        If the path is absolute, behavior is governed by filesystem_absolute.
> +        If filesystem_absolute is True, the path is interpretted as absolute on

interpreted
Comment 47 Mike Hommey [:glandium] 2012-12-10 02:39:52 PST
Comment on attachment 671000 [details] [diff] [review]
Part 3: Convert executed sandboxes into data structures, v1

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

rs=me

::: python/mozbuild/mozbuild/frontend/README.rst
@@ +24,5 @@
>  Once a *mozbuild* file has executed, it is converted into a set of static
>  data structures.
>  
>  The set of all data structures from all relevant *mozbuild* files
> +constitute all of the metadata from the tree.

The set (...) constitute*s*
Comment 48 Mike Hommey [:glandium] 2012-12-10 02:42:42 PST
Question: how will we handle "third-party" sub directories? Currently, we're just putting them in DIRS like anything else, but that won't work for e.g. nsprpub, js/src, media/webrtc...
Comment 49 Gregory Szorc [:gps] 2012-12-10 09:58:18 PST
(In reply to Mike Hommey [:glandium] from comment #48)
> Question: how will we handle "third-party" sub directories? Currently, we're
> just putting them in DIRS like anything else, but that won't work for e.g.
> nsprpub, js/src, media/webrtc...

I foresee two possibilities:

1) PROJECT_DIRS (or similar) which define directories that have their own independent "tree" of configs. We'll need to change the traversal logic to be able to find and load different config.status, etc. We'll also need to change the emit API to allow for multiple projects.

2) MAKE_DIRS (or similar) defines directories in which we should invoke |make| to build.

Since some of these sub-projects use rules.mk, we'll need some kind of back door to allow variables like DIRS to exist in Makefile (the plan is to introduce checks at the top of rules.mk to disallow variables that have been moved to moz.build). I was thinking we could add a variable to except Makefile.ins e.g. ALLOW_LEGACY_VARIABLES := 1. We could even have the moz.build traversal logic create .mk files in directories with Makefile's.

Lots of choices here...
Comment 50 Gregory Szorc [:gps] 2012-12-20 19:06:46 PST
Created attachment 694664 [details] [diff] [review]
Makefile conversion

I removed bitrot from the Makefile.in -> moz.build DIRS conversion.

I'm submitting this so people have an idea of what things will look like. I'm going to eventually split this patch roughly on module borders. Then, I will want r? from at least 1 build peer and f+ from a module owner/peer. For this review, I'm looking for high-level feedback.

I'll see about submitting the rest of the Python patches so people can start applying things locally and experimenting (I know Mike Shal is eager to start converting IDLs).
Comment 51 Ted Mielczarek [:ted.mielczarek] 2012-12-21 06:59:16 PST
I don't think we need to get module owner signoff on this. We've declared it as the plan of record and I haven't heard any real dissent. If you need to make invasive changes (like for the DOM test suites that get imported) then we should get review on those bits, but just for "hacking the crap out of your Makefile.ins" build peer review is fine.
Comment 52 :Ms2ger (⌚ UTC+1/+2) 2012-12-21 12:00:18 PST
Comment on attachment 694664 [details] [diff] [review]
Makefile conversion

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

Did accessible/—js/, and toolkit-tiers.mk / toolkit.mozbuild. You missed a few things already :)

Are we going with moz.build and foo.mozbuild?

::: accessible/src/xforms/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

This folder doesn't exist anymore

::: b2g/app.mozbuild
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

So this is the new build.mk? Do we also want to make configure check that it exists for --enable-application?

@@ +11,5 @@
> +if CONFIG['MOZ_EXTENSIONS']:
> +    add_tier_dir('app', 'extensions')
> +
> +if CONFIG['MOZ_SERVICES_SYNC']:
> +    add_tier_dir('app', 'services')

Unconditional

::: b2g/components/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

TEST_DIRS = \
	test \
	$(NULL)

didn't get moved over

::: b2g/moz.build
@@ +5,5 @@
> +
> +DIRS += ['chrome', 'components', 'locales']
> +
> +if CONFIG['OS_ARCH'] == 'WINNT':
> +    DIRS += ['../xulrunner/tools/redit']

I'm a little unhappy about the hard-coded '../'

@@ +7,5 @@
> +
> +if CONFIG['OS_ARCH'] == 'WINNT':
> +    DIRS += ['../xulrunner/tools/redit']
> +
> +if CONFIG['GAIDIR']:

GAIADIR

@@ +10,5 @@
> +
> +if CONFIG['GAIDIR']:
> +    DIRS += ['gaia']
> +
> +DIRS += 'app'

Not a list?

::: browser/branding/nightly/Makefile.in
@@ +4,5 @@
>  
> +DEPTH := @DEPTH@
> +topsrcdir := @top_srcdir@
> +srcdir := @srcdir@
> +VPATH := @srcdir@

No objection, but, why?

::: browser/components/privatebrowsing/test/browser/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Got lost:

ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
DIRS += perwindow
else
DIRS += global obsolete
endif

::: browser/devtools/framework/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

TEST_DIRS += test

::: browser/devtools/inspector/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Same here

::: browser/devtools/profiler/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Ditto

::: browser/modules/test/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

DIRS = \
	chrome \
	$(NULL)

::: content/canvas/Makefile.in
@@ +12,3 @@
>  ifdef ENABLE_TESTS
>  TOOL_DIRS += compiledtest
>  endif

And this one?

::: content/xtf/public/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

XTF is dead, long live XTF... Actually, scrap that last half. Three files can go.

::: dbm/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Is dbm something we can touch?

::: docshell/moz.build
@@ +8,5 @@
> +    'shistory',
> +    'build',
> +    'resources/content',
> +    'test',
> +]

test should be in TEST_DIRS

::: dom/alarm/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['test']

TEST_DIRS

::: dom/apps/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +PARALLEL_DIRS += ['src']
> +DIRS += ['tests']

TEST_DIRS

::: dom/base/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['test']

I know it wasn't before, but TEST_DIRS, and remove the line from the Makefile.in

::: dom/battery/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['test']

Test...

::: dom/cellbroadcast/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

PARALLEL_DIRS = interfaces src

::: dom/contacts/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['tests']

...

::: dom/devicestorage/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['test', 'ipc']

...

::: dom/encoding/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

TEST_DIRS += ['test']

::: dom/file/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['test']

...

::: dom/icc/Makefile.in
@@ +11,1 @@
>  TEST_DIRS += tests

and this

::: dom/imptests/editing/Makefile.in
@@ -13,5 @@
> -  css \
> -  conformancetest \
> -  selecttest \
> -  $(NULL)
> -

You missed the

# THIS FILE IS AUTOGENERATED BY importTestsuite.py - DO NOT EDIT

part.

::: dom/ipc/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':

This was "ifneq"

::: dom/media/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

ifdef MOZ_WEBRTC
DIRS += bridge
endif

TEST_DIRS += \
  tests/mochitest \
  $(NULL)

::: dom/moz.build
@@ +32,5 @@
> +    'apps',
> +]
> +
> +for i in interfaces:
> +    PARALLEL_DIRS.append('interfaces/' + i)

IMO,

PARALLEL_DIRS += ['interfaces/' + i for i in interfaces]

But maybe I'm just addicted to list comprehensions.

::: dom/permission/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

TEST_DIRS += tests

::: dom/phonenumberutils/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

TEST_DIRS += tests

::: embedding/components/printingui/src/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_PDF_PRINTING']:
> +    DIRS += ['unixshared']

This is a change. Previously, PLATFORM_DIR would be overridden if MOZ_WIDGET_TOOLKIT was one of os2,windows,cocoa.

::: embedding/moz.build
@@ +7,5 @@
> +TEST_DIRS += ['test']
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':
> +    if CONFIG['MOZ_BUILD_APP'] in ('mobile/xul', 'b2g'):
> +        DIRS += ['android']

Use the magical 'and' keyword here to avoid two ifs.

::: ipc/chromium/src/third_party/libevent/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Hrm, these seem to use their own mess. We'll need to figure out something for those, but after this has landed is soon enough.

::: ipc/ipdl/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_IPDL_TESTS']:
> +    TEST_DIRS += ['test']

Might want to keep this DIRS

::: ipc/ipdl/test/moz.build
@@ +7,5 @@
> +# quick and painless
> +TEST_DIRS += ['ipdl']
> +
> +if CONFIG['MOZ_IPDL_TESTS']:
> +    TEST_DIRS += ['cxx']

(This is silly, we don't end up recursing into this dir unless CONFIG['MOZ_IPDL_TESTS'] is true)

::: toolkit/toolkit.mozbuild
@@ +19,5 @@
> +if CONFIG['MOZ_TREE_FREETYPE']:
> +    add_tier_dir('platform', 'modules/freetype2', static=True)
> +
> +# This must precede xpcom
> +if CONFIG['MOZ_DMD']:

This was

ifdef MOZ_DMDV

@@ +93,5 @@
> +    add_tier_dir('platform', [
> +        'media/webrtc',
> +        'media/mtransport/third_party',
> +        'media/mtransport/standalone',
> +    ])

This was:

tier_platform_dirs += \
  media/webrtc \
  media/mtransport/third_party \
  media/mtransport/build \
  media/mtransport/standalone \
  $(NULL)

@@ +123,5 @@
> +        'media/omx-plugin/sony',
> +    ])
> +
> +if CONFIG['MOZ_NATIVE_PNG']:
> +    add_tier_dir('platform', 'media/libpng')

if *not* CONFIG['MOZ_NATIVE_PNG']:

@@ +126,5 @@
> +if CONFIG['MOZ_NATIVE_PNG']:
> +    add_tier_dir('platform', 'media/libpng')
> +
> +if CONFIG['ENABLE_TESTS']:
> +    add_tier_dir('platform', 'testing/specialpowers')

We don't have a test-only version of add_tier_dir, I suppose?

@@ +151,5 @@
> +# have been built
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':
> +    add_tier_dir('platform', 'other-licenses/skia-npapi')
> +
> +if CONFIG['MOZ_UNIVERAL_CHARDET']:

This was:

ifdef MOZ_UNIVERSALCHARDET

@@ +180,5 @@
> +    add_tier_dir('platform', 'security/manager')
> +else:
> +    add_tier_dir('platform', [
> +        'security/manager/boot/public',
> +        'security/manager/sll/public',

ssl, not sll

@@ +207,5 @@
> +
> +# Applications can cheat and ask for code to be
> +# built before libxul so it can be linked into libxul.
> +if CONFIG['APP_LIBXUL_DIRS']:
> +    # TODO the value is probably a whitespace delimited string.

Yes, it is. See <http://mxr.mozilla.org/comm-central/source/bridge/bridge.mk#5> for example. We probably need add_tier_dir/assignment to *DIRS* to throw on spaces.
Comment 53 Gregory Szorc [:gps] 2012-12-21 20:09:37 PST
Created attachment 695090 [details] [diff] [review]
Part 2b: Add parameter to control whether to descend into referenced children, v1

As part of porting Makefile.in to moz.build, I wanted a way to process individual moz.build files in isolation using as similar logic as possible to the actual reader to aid in validation. So, I added a parameter to control whether reading should descend into children.

I plan to fold this into part 2 on commit.
Comment 54 Gregory Szorc [:gps] 2012-12-21 23:49:07 PST
Created attachment 695107 [details] [diff] [review]
Part 4a: Convert /b2g, v1

So it begins.

I've only partially tested all the patches I'm about to submit. I figure I can get eyes on the bulk of the work. Then, as issues are found, we can upload new revisions of just the subset that needs fixed.
Comment 55 Gregory Szorc [:gps] 2012-12-21 23:50:54 PST
Created attachment 695108 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v1
Comment 56 Gregory Szorc [:gps] 2012-12-21 23:55:06 PST
Created attachment 695111 [details] [diff] [review]
Part 4c: Convert /memory, /mfbt, /mozglue, v1
Comment 57 Gregory Szorc [:gps] 2012-12-21 23:57:00 PST
Created attachment 695112 [details] [diff] [review]
Part 4d: Convert /toolkit, v1
Comment 58 Gregory Szorc [:gps] 2012-12-22 00:03:44 PST
Created attachment 695113 [details] [diff] [review]
Part 4e: Convert /browser, v1

Incorporated feedback from Ms2ger. Requesting Gavin to look at things because extra eyes is helpful. Actually, extra eyes on all these patches is a good thing...
Comment 59 Gregory Szorc [:gps] 2012-12-22 00:06:54 PST
Created attachment 695114 [details] [diff] [review]
Part 4f: Convert /content, v1
Comment 60 Gregory Szorc [:gps] 2012-12-22 00:09:20 PST
Created attachment 695115 [details] [diff] [review]
Part 4g: Convert /docshell, v1
Comment 61 Gregory Szorc [:gps] 2012-12-22 00:16:17 PST
Created attachment 695116 [details] [diff] [review]
Part 4h: Convert /dom, v1

This will definitely need a v2 to address the autogenerated Makefile.in files. The bulk of the review should be fine to perform.
Comment 62 Gregory Szorc [:gps] 2012-12-22 00:17:52 PST
Created attachment 695117 [details] [diff] [review]
Part 4i: Convert /editor, v1
Comment 63 Gregory Szorc [:gps] 2012-12-22 00:23:11 PST
Created attachment 695118 [details] [diff] [review]
Part 4j: Convert /embedding, v1
Comment 64 Gregory Szorc [:gps] 2012-12-22 00:24:57 PST
Created attachment 695119 [details] [diff] [review]
Part 4k: Convert /extensions, v1
Comment 65 Gregory Szorc [:gps] 2012-12-22 00:26:46 PST
Created attachment 695120 [details] [diff] [review]
Part 4l: Convert /gfx, v1
Comment 66 Gregory Szorc [:gps] 2012-12-22 00:29:44 PST
Created attachment 695122 [details] [diff] [review]
Part 4m: Convert /image, v1
Comment 67 Gregory Szorc [:gps] 2012-12-22 00:30:21 PST
Created attachment 695123 [details] [diff] [review]
Part 4n: Convert /intl, v1
Comment 68 Gregory Szorc [:gps] 2012-12-22 00:34:16 PST
Created attachment 695126 [details] [diff] [review]
Part 4o: Convert /ipc, v1
Comment 69 Gregory Szorc [:gps] 2012-12-22 00:35:52 PST
Created attachment 695127 [details] [diff] [review]
Part 4p: Convert /js, v1
Comment 70 Gregory Szorc [:gps] 2012-12-22 00:36:32 PST
Created attachment 695128 [details] [diff] [review]
Part 4q: Convert /layout, v1
Comment 71 Gregory Szorc [:gps] 2012-12-22 00:39:29 PST
Created attachment 695129 [details] [diff] [review]
Part 4r: Convert /media, v1
Comment 72 Gregory Szorc [:gps] 2012-12-22 00:40:10 PST
Created attachment 695130 [details] [diff] [review]
Part 4s: Convert /accessible, v1
Comment 73 Gregory Szorc [:gps] 2012-12-22 00:41:09 PST
Created attachment 695133 [details] [diff] [review]
Part 4t: Convert /caps, v1

Averaging out khuey's patch sizes :)
Comment 74 Gregory Szorc [:gps] 2012-12-22 00:44:24 PST
Created attachment 695134 [details] [diff] [review]
Part 4u: Convert /mobile/android, v1
Comment 75 Gregory Szorc [:gps] 2012-12-22 00:45:02 PST
Created attachment 695135 [details] [diff] [review]
Part 4v: Convert /mobile/xul, v1
Comment 76 Gregory Szorc [:gps] 2012-12-22 00:45:39 PST
Created attachment 695136 [details] [diff] [review]
Part 4w: Convert /modules, v1
Comment 77 Gregory Szorc [:gps] 2012-12-22 00:47:59 PST
Created attachment 695137 [details] [diff] [review]
Part 4x: Convert /netwerk, v1
Comment 78 Gregory Szorc [:gps] 2012-12-22 00:48:36 PST
Created attachment 695138 [details] [diff] [review]
Part 4y: Convert /parser, v1
Comment 79 Gregory Szorc [:gps] 2012-12-22 00:49:59 PST
Created attachment 695140 [details] [diff] [review]
Part 4z: Convert /profile, v1
Comment 80 Gregory Szorc [:gps] 2012-12-22 00:59:49 PST
Created attachment 695142 [details] [diff] [review]
Part 4α: Convert /rdf, v1
Comment 81 Gregory Szorc [:gps] 2012-12-22 01:00:27 PST
Created attachment 695143 [details] [diff] [review]
Part 4β: Convert /security, v1
Comment 82 Gregory Szorc [:gps] 2012-12-22 01:01:18 PST
Created attachment 695144 [details] [diff] [review]
Part 4γ: Convert /services, v1
Comment 83 Gregory Szorc [:gps] 2012-12-22 01:01:53 PST
Created attachment 695145 [details] [diff] [review]
Part 4δ: Convert /storage, v1
Comment 84 Gregory Szorc [:gps] 2012-12-22 01:02:42 PST
Created attachment 695146 [details] [diff] [review]
Part 4ε: Convert /testing, v1
Comment 85 Gregory Szorc [:gps] 2012-12-22 01:03:25 PST
Created attachment 695147 [details] [diff] [review]
Part 4ζ: Convert /tools, v1
Comment 86 Gregory Szorc [:gps] 2012-12-22 01:04:08 PST
Created attachment 695148 [details] [diff] [review]
Part 4η: Convert /uriloader, v1
Comment 87 Gregory Szorc [:gps] 2012-12-22 01:04:46 PST
Created attachment 695149 [details] [diff] [review]
Part 4θ: Convert /webapprt, v1
Comment 88 Gregory Szorc [:gps] 2012-12-22 01:05:25 PST
Created attachment 695150 [details] [diff] [review]
Part 4ι: Convert /widget, v1
Comment 89 Gregory Szorc [:gps] 2012-12-22 01:06:05 PST
Created attachment 695151 [details] [diff] [review]
Part 4κ: Convert /xpcom, v1
Comment 90 Gregory Szorc [:gps] 2012-12-22 01:06:39 PST
Created attachment 695152 [details] [diff] [review]
Part 4λ: Convert /xpfe, v1
Comment 91 Gregory Szorc [:gps] 2012-12-22 01:07:17 PST
Created attachment 695153 [details] [diff] [review]
Part 4μ: Convert /xulrunner, v1
Comment 92 Gregory Szorc [:gps] 2012-12-22 01:08:22 PST
Created attachment 695154 [details] [diff] [review]
Part 4ν: Convert misc remaining, v1

And we're done! Poor ξ.
Comment 93 Gregory Szorc [:gps] 2012-12-22 01:14:49 PST
Created attachment 695155 [details] [diff] [review]
Part 4d: Convert /toolkit, v2

Incorporated feedback from Ms2ger. Also, requesting an extra pair of eyes from Toolkit owner.

Dave: Feel free to decline and/or redirect. Your participation is not required for landing. But, Toolkit is a large and complex module so extra eyeballs would be appreciated.
Comment 94 :Ms2ger (⌚ UTC+1/+2) 2012-12-22 07:56:45 PST
Comment on attachment 695108 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v1

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

::: Makefile.in
@@ +27,5 @@
>    other-licenses/android \
>    $(NULL)
>  endif
>  ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
>  tier_base_dirs += \

You'll want to remove all this too.
Comment 95 :Ms2ger (⌚ UTC+1/+2) 2012-12-22 08:04:49 PST
Comment on attachment 695111 [details] [diff] [review]
Part 4c: Convert /memory, /mfbt, /mozglue, v1

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

::: memory/jemalloc/src/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Is this one necessary?

::: memory/moz.build
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['mozjemalloc']
> +
> +if CONFIG['MOZ_JEMALOC3'] or CONFIG['MOZ_REPLACE_MALLOC']:

MALLOC, not MALOC, and you lost the ifndef MOZ_NATIVE_JEMALLOC

::: memory/replace/moz.build
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Build jemalloc3 as a replace-malloc lib when building with mozjemalloc
> +if not CONFIG['MOZ_JEMALLOC']:
> +    DIRS += ['jemalloc']

We'll need to decide if we're going to do 2-space or 4-space indentation
Comment 96 Gregory Szorc [:gps] 2012-12-22 18:11:03 PST
Created attachment 695271 [details] [diff] [review]
Part 5: Use relpath in ConfigStatus.py

Since we require Python 2.7, we no longer need to implement os.path.relpath inside ConfigStatus.py.
Comment 97 Gregory Szorc [:gps] 2012-12-22 19:40:09 PST
Can somebody remind me what the requirements are for standalone builds of SpiderMonkey? Is it just that js/src builds can be performed independent of the rest of the build system? Is it that js/src can be built without a full copy of mozilla-central (just js/src)?
Comment 98 Gregory Szorc [:gps] 2012-12-22 21:41:45 PST
Created attachment 695282 [details] [diff] [review]
Part 6: Require virtualenv to build SpiderMonkey, v1

This patch will break SpiderMonkey standalone builds workflow. In the new world, the SpiderMonkey configure requires the virtualenv created as part of mozilla-central's configure. If the virtualenv is not present, the user will see a long (hopefully helpful) error message explaining how to create it (read the patch).

Of course, creating the virtualenv requires mozilla-central because the build depends on Python code in mozilla-central. If this is unacceptable, we could continue to copy large parts of the tree into js/src and keep things in sync with the rest of the tree. I would prefer not to do this because it is hacky. If shipping a standalone SpiderMonkey source archive is a requirement, we can modify the archiving code to copy files from mozilla-central.

Anyway, I think copying files from mozilla-central into js/src is not a long term solution. I'd like to stop the practice entirely. This patch paves the way for no shared Python. Follow-up bugs could kill the remaining copied files (build/autoconf/*.m4, rules.mk, etc).

If this patch is declined, it will make work in subsequent parts of this bug more difficult. It won't be impossible. But, code will be a little hackier than I'd like. e.g. we'd have to copy python/mozbuild and possibly python/mach into js/src. So, I hope the small number of people inconvenienced by this understand the larger needs of the overall build system. Of course, if anyone has suggestions for making the developer workflow for standalone SpiderMonkey builds easier, I'm all ears.
Comment 99 Gregory Szorc [:gps] 2012-12-22 22:06:31 PST
Created attachment 695283 [details] [diff] [review]
Part 7: Move parts of ConfigStatus into mozbuild, v1

The plan is to move core build system foo into the mozbuild Python package so mozbuild is self-contained and doesn't have to call out into other systems. Subsequent patches should make the overall direction clearer.
Comment 100 :Ms2ger (⌚ UTC+1/+2) 2012-12-22 23:48:51 PST
(In reply to Gregory Szorc [:gps] from comment #98)
> Created attachment 695282 [details] [diff] [review]
> Part 6: Require virtualenv to build SpiderMonkey, v1
> 
> Of course, creating the virtualenv requires mozilla-central because the
> build depends on Python code in mozilla-central.

Note that building SM already requires m-c/mfbt.
Comment 101 Gregory Szorc [:gps] 2012-12-22 23:54:16 PST
(In reply to :Ms2ger from comment #100)
> Note that building SM already requires m-c/mfbt.

Oh good. Then that patch shouldn't be too controversial.

In other news, https://tbpl.mozilla.org/?tree=Try&rev=7789acdde5a3. I expect that to go red everywhere. But, it should get past config.status hopefully everywhere.

Yes, that means I'm nearly feature complete on my end. At this point, I'm just chasing down bugs and bringing code quality up to decent standards. I should hopefully have all the feature patches up for review within a day or two. That should give us plenty of time to refine and land so we can merge into m-c as soon as 21 is cut.
Comment 102 Gary Kwong [:gkw] [:nth10sd] 2012-12-23 00:01:57 PST
(In reply to Gregory Szorc [:gps] from comment #97)
> Can somebody remind me what the requirements are for standalone builds of
> SpiderMonkey? Is it just that js/src builds can be performed independent of
> the rest of the build system? Is it that js/src can be built without a full
> copy of mozilla-central (just js/src)?

Yes, and yes. I copy out js/src, js/public and js/mfbt from the m-c tree and a js shell can be compiled.

Within js/src, ['jit-test', 'tests', 'trace-test', 'xpconnect'] are all not really needed for compilation too.
Comment 103 Gary Kwong [:gkw] [:nth10sd] 2012-12-23 00:03:27 PST
> Yes, and yes.

Yes, and no, is perhaps what I meant. Spidermonkey can be built without a full copy of m-c, but besides js/src, js/public and js/mfbt are also needed.
Comment 104 Gary Kwong [:gkw] [:nth10sd] 2012-12-23 00:07:04 PST
> Yes, and no, is perhaps what I meant. Spidermonkey can be built without a
> full copy of m-c, but besides js/src, js/public and js/mfbt are also needed.

3rd correction:

js/src, js/public and mfbt/ are needed to compile a js shell.
Comment 105 Mike Hommey [:glandium] 2012-12-23 01:38:37 PST
Comment on attachment 695271 [details] [diff] [review]
Part 5: Use relpath in ConfigStatus.py

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

::: build/ConfigStatus.py
@@ -41,5 @@
> -    if not rel_list:
> -        return os.curdir
> -    return os.path.join(*rel_list)
> -
> -relpath = getattr(os.path, "relpath", my_relpath)

from os.path import relpath
would avoid modifying the rest of the code.
Comment 106 Mike Hommey [:glandium] 2012-12-23 01:43:58 PST
Comment on attachment 695282 [details] [diff] [review]
Part 6: Require virtualenv to build SpiderMonkey, v1

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

::: js/src/build/autoconf/python-virtualenv.m4
@@ +13,5 @@
> +    if test -z "$PYTHON"; then
> +        AC_MSG_ERROR([python was not found in \$PATH])
> +    fi
> +
> +    virtualenv_populator=$_topsrcdir/build/virtualenv/populate_virtualenv.py

In practice, unless you copy build/virtualenv under js/src/build, this is going to always be a pita for spidermonkey standalone builds.
Comment 107 Mike Hommey [:glandium] 2012-12-23 02:07:19 PST
Another concern: I don't know how exactly you plan to generate Makefiles, replacing allmakefiles.sh, but unless you treat js/src differently, the main config.status is going to create js/src Makefiles, which will screw up their DEPTH, and then js/src/config.status is going to overwrite them with the right DEPTH. In practice, this means it won't be much of the problem for the Makefile contents, but it will force a rebuild of everything under js/src each time configure is run, which, on buildbots, is every build, because the Makefiles will have been modified.
Comment 108 Cameron Kaiser [:spectre] 2012-12-23 08:50:22 PST
Since this looks like it has a lot of side effects of varying impact, could I request it land in Fx21, not Fx20? I have to figure out how this affects the TenFourFox internal build system (probably not much, but I don't want any surprises).
Comment 109 :Ms2ger (⌚ UTC+1/+2) 2012-12-23 09:02:18 PST
Created attachment 695314 [details] [diff] [review]
Some fixes

I had some fun with your patches today; we now get through the compile step and die in make buildsymbols (except for Android 2.2 X86 opt, which dies in /tools/android-ndk-r7b/sources/cxx-stl/stlport/src/messages.cpp (?!)).

Could you merge these changes in the appropriate patches? :)

https://tbpl.mozilla.org/?tree=Try&rev=1074e0f2d865
Comment 110 Gregory Szorc [:gps] 2012-12-23 09:11:42 PST
(In reply to Cameron Kaiser from comment #108)
> Since this looks like it has a lot of side effects of varying impact, could
> I request it land in Fx21, not Fx20?

We are planning to land this in 21.
Comment 111 Gregory Szorc [:gps] 2012-12-23 09:54:37 PST
Created attachment 695324 [details] [diff] [review]
Part 4β: Convert /security, v2

Incorporated Ms2ger's patches (mainly adding security/manager to the mix).
Comment 112 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-23 10:00:51 PST
Comment on attachment 695113 [details] [diff] [review]
Part 4e: Convert /browser, v1

>diff --git a/browser/moz.build b/browser/moz.build

>+DIRS += ['devtools', 'app']

This wasn't mentioned in the old Makefile for some reason, but "app" needs to be last here for Mac packaging to work right. Could you add a comment?

>diff --git a/browser/themes/Makefile.in b/browser/themes/Makefile.in

>-ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
>-DIRS = pinstripe
>-else
>-DIRS = winstripe
>-endif
>-ifneq (,$(filter gtk2 qt,$(MOZ_WIDGET_TOOLKIT)))
>-DIRS = gnomestripe
>-endif

>diff --git a/browser/themes/moz.build b/browser/themes/moz.build

>+if toolkit == 'cocoa':
>+    DIRS += ['pinstripe']
>+else:
>+    DIRS += ['winstripe']
>+
>+if toolkit in ('gtk2', 'qt'):
>+    DIRS += ['gnomestripe']

Bug: the previous code clobbered a previously-set DIRS for the "gnomestripe" case, so the new code needs to avoid setting winstripe+gnomestripe. Basically, DIRS should only ever be set to one of "winstripe"/"pinstripe"/"gnomestripe" (for browser/ - different story in toolkit/).
Comment 113 :Ms2ger (⌚ UTC+1/+2) 2012-12-23 12:20:42 PST
Comment on attachment 695153 [details] [diff] [review]
Part 4μ: Convert /xulrunner, v1

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

::: xulrunner/app.mozbuild
@@ +9,5 @@
> +    add_tier_dir('app', 'extensions')
> +
> +if CONFIG['OS_ARCH'] == 'WINNT' and (CONFIG['ENABLE_TESTS'] or
> +        CONFIG['MOZILLA_OFFICIAL']):
> +    add_tier_dir('app', 'embedding/tests/wimEmbed')

s/wim/win/ and you'll win. (Excuse the pun.)

::: xulrunner/build.mk
@@ -1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -include $(topsrcdir)/toolkit/toolkit-tiers.mk

You're losing the 

ifdef LIBXUL_SDK
$(error toolkit-tiers.mk is not compatible with --enable-libxul-sdk=)
endif

that's still left in toolkit/toolkit-tiers.mk.
Comment 114 Gregory Szorc [:gps] 2012-12-23 17:15:27 PST
Created attachment 695376 [details] [diff] [review]
Part 6: Require virtualenv to build SpiderMonkey, v2

I refactored the approach a bit. In my opinion, the approach is a bit hacky. But, if someone runs a standalone SpiderMonkey configure from mozilla-central, it will find and use the virtualenv automatically. If there is a source distribution, we'll just need to add /build/virtualenv, /python, and other locations referenced by the virtualenv and things will "just work." I'm not aware of a source distribution. If it exists and the packaging code exists in m-c, I'll happily code up a patch and add it to this bug.
Comment 115 Gregory Szorc [:gps] 2012-12-23 17:34:15 PST
Created attachment 695379 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v1

Incorporated feedback from Ms2ger.
Comment 116 Gregory Szorc [:gps] 2012-12-24 09:36:05 PST
Comment on attachment 695314 [details] [diff] [review]
Some fixes

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

I have incorporated most of your changes into my local patch queue (https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc if you want to try things yourself).

The biggest change from my work is that I introduced a few new features to moz.build files to define "external" projects and make-built directories. Essentially, directories are defined as "build with make" but moz.build traversal will not occur in them. This saves us from having to define empty moz.build files for 3rd party code. There are probably some empty moz.build files alongside 3rd party Makefile's in the patches on this bug. If you see some, please mark during review.

Latest try is at https://tbpl.mozilla.org/?tree=Try&rev=3fc31c7cdf9f. I'm optimistic some builds may go green.
Comment 117 Gregory Szorc [:gps] 2012-12-24 09:37:03 PST
Comment on attachment 695314 [details] [diff] [review]
Some fixes

Patch has been incorporated into other patches (not all of which have been uploaded to this bug yet).
Comment 118 Gregory Szorc [:gps] 2012-12-24 13:21:30 PST
Created attachment 695523 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v2

Fixes to |make check|.
Comment 119 Gregory Szorc [:gps] 2012-12-24 13:22:48 PST
Created attachment 695524 [details] [diff] [review]
Part 7: Move parts of ConfigStatus into mozbuild, v2

Ported over tests to mozbuild.
Comment 120 Gregory Szorc [:gps] 2012-12-24 13:24:22 PST
Created attachment 695525 [details] [diff] [review]
Part 8: Add external make variables to moz.build files, v1

Not requesting review yet because this needs test coverage. Posting it so everyone can see how external projects are handled.
Comment 121 Gregory Szorc [:gps] 2012-12-24 13:25:36 PST
Created attachment 695526 [details] [diff] [review]
Part 9: Add warning and error functions to moz.build files, v1

error() is needed to abort termination. I figured I'd provide warning() as well. Not requesting review because this needs tests.
Comment 122 Gregory Szorc [:gps] 2012-12-24 13:27:00 PST
Created attachment 695527 [details] [diff] [review]
Part 10: Ability to declare tier directories as external, v1

Not requesting review because this needs tests. This /might/ be redundant with "static" tier directories. I'm not sure what exactly static tier directories are. Someone please enlighten me.
Comment 123 Gregory Szorc [:gps] 2012-12-24 13:31:38 PST
Created attachment 695528 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v1

I added a variable to moz.build files that effectively does the same thing as AC_OUTPUT. This is needed to generate some Makefiles (like things in /config/test) that aren't part of directory recursion, don't have the {export, platform, tools} targets to make them part of recursion. I also use it later on to handle things formerly in allmakefiles.sh.

This patch needs tests. We also probably need to write out the collected paths in make files so they can be regenerated if out of date.

We /may/ want a better way to hook this up with config.status. I'll probably be poking glandium about things as soon as I see him around IRC.
Comment 124 Gregory Szorc [:gps] 2012-12-24 13:33:23 PST
Created attachment 695530 [details] [diff] [review]
Part 12: Integrate with config.status, v1

This hooks up moz.build files with config.status \o/. Not ready for review yet, mainly due to missing tests.
Comment 125 Gregory Szorc [:gps] 2012-12-24 13:35:07 PST
Created attachment 695531 [details] [diff] [review]
Part 13: rules.mk integration, v1

This hooks up our auto-generated .mk files with rules.mk. I still need some rules to regenerate the moz.build-derived .mk files if the moz.build files have changed.
Comment 126 Gregory Szorc [:gps] 2012-12-24 13:41:03 PST
Created attachment 695533 [details] [diff] [review]
Part 14: Remove allmakefiles.sh, v1

Good riddance. I probably also need to clean up some lingering references in the tree.

I believe this bug now contains all of the patches in my tree. The final patch set will likely look very similar to what we have now (I hope). There will definitely be some reordering (the conversion will probably come after a lot of the Python patches). See my Mercurial patch queue at https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc for the definitive ordering.

Ted, Mike, etc should now have a full idea of what we're looking at.

I'll try to get all patches to r? status by 2013. But, I also need to work on bug 718066, so I may be quiet on this bug for a little while. Ping me if you get nervous about not having enough turnaround time to finish reviews before 21 (Jan 7 I believe). We don't have to land on Jan 7. But, I'd sure like to board the train as soon as it arrives.
Comment 127 Gregory Szorc [:gps] 2012-12-24 15:39:11 PST
And we are green on Linux and OS X! Windows is green except for a single Python unit test failure. Android and B2G are failing for an unknown reason. I suspect a bug in the moz.build conversion. Android x86 is failing for a separate reason from the others. I'm scratching my head over all the failures.

https://tbpl.mozilla.org/?tree=Try&rev=844a092e8556
Comment 128 :Ms2ger (⌚ UTC+1/+2) 2012-12-25 06:59:42 PST
(In reply to Gregory Szorc [:gps] from comment #127)
> And we are green on Linux and OS X! Windows is green except for a single
> Python unit test failure. Android and B2G are failing for an unknown reason.
> I suspect a bug in the moz.build conversion. Android x86 is failing for a
> separate reason from the others. I'm scratching my head over all the
> failures.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=844a092e8556

I'm pretty sure that

+if not CONFIG['LIBXUL_SDK']:
+    include('/toolkit/toolkit.mozbuild')
+
+    add_tier_dir('platform', 'mobile/android/components/build')

makes components build *after* libxul, while

-# Needed for building our components as part of libxul
-APP_LIBXUL_DIRS += mobile/android/components/build
-
-include $(topsrcdir)/toolkit/toolkit-tiers.mk

makes it build *before*.

(Similar code in mobile/xul.)
Comment 129 Gregory Szorc [:gps] 2012-12-25 13:19:32 PST
(In reply to :Ms2ger from comment #128)
> I'm pretty sure that
> 
> +if not CONFIG['LIBXUL_SDK']:
> +    include('/toolkit/toolkit.mozbuild')
> +
> +    add_tier_dir('platform', 'mobile/android/components/build')
> 
> makes components build *after* libxul, while
> 
> -# Needed for building our components as part of libxul
> -APP_LIBXUL_DIRS += mobile/android/components/build
> -
> -include $(topsrcdir)/toolkit/toolkit-tiers.mk
> 
> makes it build *before*.
> 
> (Similar code in mobile/xul.)

Ahh - good catch! I got my Android development environment set up on my local machine. Hopefully I can repro and come up with a fix.

I also tracked down the compilation failure in stlport. Turns out we weren't converting build/stlport/stl/config/_android.h.in. One would think the compiler would choke on a missing header file. Who knows what's going on there...
Comment 130 Gregory Szorc [:gps] 2012-12-25 17:55:40 PST
And we have green builds everywhere except for Windows (2 minor Python unit test failures) \o/

https://tbpl.mozilla.org/?tree=Try&rev=128b2b1ea62a
Comment 131 Mike Hommey [:glandium] 2012-12-26 00:21:17 PST
(In reply to Gregory Szorc [:gps] from comment #129)
> (In reply to :Ms2ger from comment #128)
> > I'm pretty sure that
> > 
> > +if not CONFIG['LIBXUL_SDK']:
> > +    include('/toolkit/toolkit.mozbuild')
> > +
> > +    add_tier_dir('platform', 'mobile/android/components/build')
> > 
> > makes components build *after* libxul, while
> > 
> > -# Needed for building our components as part of libxul
> > -APP_LIBXUL_DIRS += mobile/android/components/build
> > -
> > -include $(topsrcdir)/toolkit/toolkit-tiers.mk
> > 
> > makes it build *before*.
> > 
> > (Similar code in mobile/xul.)
> 
> Ahh - good catch! I got my Android development environment set up on my
> local machine. Hopefully I can repro and come up with a fix.
> 
> I also tracked down the compilation failure in stlport. Turns out we weren't
> converting build/stlport/stl/config/_android.h.in. One would think the
> compiler would choke on a missing header file. Who knows what's going on
> there...

It doesn't choke because build/stlport/stl/config/_android.h is a wrapper around @STLPORT_SOURCES@/stlport/stl/config/_android.h.
Comment 132 :Ms2ger (⌚ UTC+1/+2) 2012-12-31 04:21:58 PST
Comment on attachment 695154 [details] [diff] [review]
Part 4ν: Convert misc remaining, v1

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

::: mobile/locales/Makefile.in
@@ -32,5 @@
>      $(wildcard $(MOZILLA_DIR)/config/JarMaker.py) \
>      $(topsrcdir)/config/JarMaker.py \
>    )
>  
> -GENERATED_DIRS += .deps

Seems unrelated?
Comment 133 :Ms2ger (⌚ UTC+1/+2) 2012-12-31 05:35:13 PST
Comment on attachment 695155 [details] [diff] [review]
Part 4d: Convert /toolkit, v2

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

::: toolkit/mozapps/update/test/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':
> +    DIRS += ['chrome']

This should be '!=': https://hg.mozilla.org/users/Ms2ger_gmail.com/gps-patches/rev/81f8000a335a
Comment 134 :Ms2ger (⌚ UTC+1/+2) 2012-12-31 07:30:56 PST
(In reply to :Ms2ger from comment #133)
> Comment on attachment 695155 [details] [diff] [review]
> Part 4d: Convert /toolkit, v2
> 
> Review of attachment 695155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/test/moz.build
> @@ +3,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':
> > +    DIRS += ['chrome']
> 
> This should be '!=':
> https://hg.mozilla.org/users/Ms2ger_gmail.com/gps-patches/rev/81f8000a335a

And much to my surprise, this change seems to have fixed the ~10MB leak in OSX mochitest-chrome we hit on <https://tbpl.mozilla.org/?tree=Try&rev=959c75d370a1>.
Comment 135 Mike Hommey [:glandium] 2013-01-02 00:55:43 PST
Comment on attachment 695114 [details] [diff] [review]
Part 4f: Convert /content, v1

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

::: content/moz.build
@@ +19,5 @@
> +
> +if CONFIG['MOZ_MEDIA']:
> +    PARALLEL_DIRS += ['media']
> +
> +TEST_TOOL_DIRS += ['test']

Not sure TEST_TOOL_DIRS is a good name. Note that we could probably add autodiscovery of the test directories in the future (or just now, if you feel like it).
Comment 136 Mike Hommey [:glandium] 2013-01-02 01:09:41 PST
Comment on attachment 695118 [details] [diff] [review]
Part 4j: Convert /embedding, v1

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

::: embedding/components/printingui/src/moz.build
@@ +14,5 @@
> +    platform_dir = 'os2'
> +elif toolkit == 'windows':
> +    platform_dir = 'win'
> +elif toolkit == 'cocoa':
> +    platform_dir = 'mac'

if toolkit == 'os2':
    DIRS += ['os2']
elif toolkit == 'windows':
(...)
elif CONFIG[MOZ_PDF_PRINTING']:
    DIRS += ['unixshared']

would match the current definition and would be less confusion.

::: embedding/moz.build
@@ +6,5 @@
> +DIRS += ['base', 'components', 'browser']
> +TEST_DIRS += ['test']
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' \
> +    and CONFIG['MOZ_BUILD_APP'] in ('mobile/xul', 'b2g'):

I know it's what is in embedding/Makefile.in but it makes no sense: you can't have CONFIG['MOZ_BUILD_APP'] == 'b2g' with CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android'. (well, configure may allow it, but it will break in plenty of ways ; arguably, configure not explicitely rejecting it is a bug ; we never run such builds, though)

So, if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android' and CONFIG['MOZ_BUILD_APP'] == 'mobile/xul'
Comment 137 Mike Hommey [:glandium] 2013-01-02 01:44:15 PST
Comment on attachment 695129 [details] [diff] [review]
Part 4r: Convert /media, v1

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

::: media/omx-plugin/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> +    DIRS += ['lib/ics/libutils', 'lib/ics/libstagefright']

This doesn't have a Makefile.in counterpart in the patch, and it appears the Makefile.in counterpart in the tree has gone in bug 787228.

::: media/webrtc/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# TODO should be defined as GYP directories.

I guess this will need to be done before landing.
Comment 138 Mike Hommey [:glandium] 2013-01-02 01:57:54 PST
Comment on attachment 695376 [details] [diff] [review]
Part 6: Require virtualenv to build SpiderMonkey, v2

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

::: build/autoconf/python-virtualenv.m4
@@ +13,5 @@
> +  if test -z "$PYTHON"; then
> +      AC_MSG_ERROR([python was not found in \$PATH])
> +  fi
> +else
> +  AC_MSG_RESULT([Using Python from environment variable $PYTHON])

Is the intent to print "$PYTHON" or "/path/to/python", here? if the former, you need to escape the $, or remove it.

@@ +22,5 @@
> +
> +dnl If this is a mozilla-central, we'll find the virtualenv in the top
> +dnl source directory. If this is a SpiderMonkey build, we assume we're at
> +dnl js/src and try to find the virtualenv from the mozilla-central root.
> +for base in $MOZILLA_CENTRAL_PATH $_topsrcdir $_topsrcdir/../..; do

Why do you need $MOZILLA_CENTRAL_PATH if you try $_topsrcdir/../..?

@@ +69,5 @@
> +
> +AC_MSG_CHECKING([Python environment is Mozilla virtualenv])
> +$PYTHON -c "import mozbuild.base"
> +if test "$?" != 0; then
> +    AC_MSG_ERROR([Python environment does not appear to be sane.])

I think it would be better to make this test what triggers to populate a virtualenv or not, instead of DONT_POPULATE_VIRTUALENV. It also might be better to test something like from buildconfig import topsrcdir, because it's typically something that'll always be in a build virtualenv (in case mozbuild is ever used outside of m-c). BTW, the buildconfig module will likely be a problem under js/src :-/
Comment 139 Ted Mielczarek [:ted.mielczarek] 2013-01-02 06:41:39 PST
(In reply to :Ms2ger from comment #95)
> We'll need to decide if we're going to do 2-space or 4-space indentation

Since these are just a subset of Python I think we ought to follow PEP-8 style and use 4-space.

(In reply to Gregory Szorc [:gps] from comment #97)
> Can somebody remind me what the requirements are for standalone builds of
> SpiderMonkey? Is it just that js/src builds can be performed independent of
> the rest of the build system? Is it that js/src can be built without a full
> copy of mozilla-central (just js/src)?

Currently (AIUI) Spidermonkey won't actually build without files from mozilla-central, so I don't think this is an issue. There is work afoot to produce standalone Spidermonkey source tarballs, so it'd be nice if we had a solution for them to generate tarballs which could be built without requiring all of m-c, but I don't think that needs to be a blocker on this work. AxS was doing some work on JS releases in bug 812265, he might be able to provide more info.
Comment 140 :Ms2ger (⌚ UTC+1/+2) 2013-01-02 08:50:37 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #139)
> (In reply to :Ms2ger from comment #95)
> > We'll need to decide if we're going to do 2-space or 4-space indentation
> 
> Since these are just a subset of Python I think we ought to follow PEP-8
> style and use 4-space.

WFM. I'm wondering if we can require that somehow... (Though I don't want to scope-creep this bug even more :))
Comment 141 Gary Kwong [:gkw] [:nth10sd] 2013-01-02 08:55:29 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #139)
> Currently (AIUI) Spidermonkey won't actually build without files from
> mozilla-central

js/src, js/public and mfbt/ are needed to compile a js shell. Other files are not really needed. (ref comment 104)
Comment 142 Ian Stakenvicius 2013-01-02 09:24:14 PST
(In reply to Gary Kwong [:gkw] from comment #141)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #139)
> > Currently (AIUI) Spidermonkey won't actually build without files from
> > mozilla-central
> 
> js/src, js/public and mfbt/ are needed to compile a js shell. Other files
> are not really needed. (ref comment 104)

Gary beat me to the posting (and has apparently posted this multiple times)

However, to elaborate, the above paths (actually subtrees) are all that is needed to build a standalone libmozjs as well.

RE comment # 97 -- it is (imo) imperative that a standalone source tarball can be rolled to allow separate distribution of spidermonkey.  That said, whatever changes are being proposed (as long as they don't require some sort of configuration be run in mc-root, ie, could be copied into the 'config' or similar dir within js/src so the build system is independent), this shouldn't be hard to do or hold anything back
Comment 143 :Ms2ger (⌚ UTC+1/+2) 2013-01-04 00:53:24 PST
Comment on attachment 695123 [details] [diff] [review]
Part 4n: Convert /intl, v1

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

::: intl/uconv/tools/moz.build
@@ +1,4 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

FWIW, this is NPOTB

::: intl/unicharutil/tools/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

As is this, AFAICT.
Comment 144 :Ms2ger (⌚ UTC+1/+2) 2013-01-04 00:59:57 PST
Comment on attachment 695126 [details] [diff] [review]
Part 4o: Convert /ipc, v1

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

::: ipc/chromium/src/third_party/libevent/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

I don't think you need the ones under libevent.

::: ipc/ipdl/test/moz.build
@@ +7,5 @@
> +# quick and painless
> +DIRS += ['ipdl']
> +
> +if CONFIG['MOZ_IPDL_TESTS']:
> +    TEST_DIRS += ['cxx']

DIRS
Comment 145 :Ms2ger (⌚ UTC+1/+2) 2013-01-04 07:31:37 PST
Comment on attachment 695128 [details] [diff] [review]
Part 4q: Convert /layout, v1

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

::: layout/base/tests/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +TEST_DIRS += ['chrome']
> +TOOL_DIRS += ['cpp-tests']

TEST_DIRS/TOOL_TEST_DIRS or DIRS/TOOL_DIRS, but don't mix, please.

::: layout/moz.build
@@ +30,5 @@
> +        'xul/base/test',
> +    ]
> +
> +    if CONFIG['MOZ_DEBUG']:
> +        DIRS += ['tools/layout-debug']

This used to be built after build and media; maybe keep it that way.
Comment 146 Ted Mielczarek [:ted.mielczarek] 2013-01-04 11:26:40 PST
Comment on attachment 695107 [details] [diff] [review]
Part 4a: Convert /b2g, v1

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

Seems a bummer to have to put empty moz.build files in leaf dirs, but I guess it's a temporary measure anyway.
Comment 147 Ted Mielczarek [:ted.mielczarek] 2013-01-04 11:32:25 PST
Comment on attachment 695111 [details] [diff] [review]
Part 4c: Convert /memory, /mfbt, /mozglue, v1

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

::: memory/jemalloc/src/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Looks like a false-positive from jemalloc's Makefile.in.

::: memory/moz.build
@@ +5,5 @@
> +
> +DIRS += ['mozjemalloc']
> +
> +if CONFIG['MOZ_JEMALOC3'] or CONFIG['MOZ_REPLACE_MALLOC']:
> +  DIRS += ['jemalloc']

I mentioned this elsewhere, but we should use 4-space indent in these files to follow PEP-8.
Comment 148 Ted Mielczarek [:ted.mielczarek] 2013-01-04 11:39:42 PST
Comment on attachment 695126 [details] [diff] [review]
Part 4o: Convert /ipc, v1

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

::: ipc/moz.build
@@ +7,5 @@
> +    'chromium',
> +    'glue',
> +    'ipdl',
> +    'testshell',
> +]

thinking out loud: should we use this formatting for all multiple-DIRS assignments?
Comment 149 Ted Mielczarek [:ted.mielczarek] 2013-01-04 12:04:07 PST
Comment on attachment 695147 [details] [diff] [review]
Part 4ζ: Convert /tools, v1

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

::: tools/codesighs/moz.build
@@ +1,4 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

note bug 823915

::: tools/testy/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['sample']

This code looks dead. We should just "hg rm tools/testy" instead.
Comment 150 Ted Mielczarek [:ted.mielczarek] 2013-01-04 12:16:00 PST
Comment on attachment 695146 [details] [diff] [review]
Part 4ε: Convert /testing, v1

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

It strikes me that testing/mochitest has a lot of extraneous directory structure.
Comment 151 Ted Mielczarek [:ted.mielczarek] 2013-01-04 12:23:30 PST
Comment on attachment 695153 [details] [diff] [review]
Part 4μ: Convert /xulrunner, v1

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

::: xulrunner/app.mozbuild
@@ +9,5 @@
> +    add_tier_dir('app', 'extensions')
> +
> +if CONFIG['OS_ARCH'] == 'WINNT' and (CONFIG['ENABLE_TESTS'] or
> +        CONFIG['MOZILLA_OFFICIAL']):
> +    add_tier_dir('app', 'embedding/tests/wimEmbed')

We should probably just `hg rm winEmbed` while we're at it.
Comment 152 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 03:26:16 PST
Comment on attachment 695324 [details] [diff] [review]
Part 4β: Convert /security, v2

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

::: security/nss/tests/pkcs11/netscape/trivial/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

I think this can be dropped.
Comment 153 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 03:44:30 PST
Comment on attachment 695154 [details] [diff] [review]
Part 4ν: Convert misc remaining, v1

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

::: db/sqlite3/src/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

4δ (storage) would be a better place for this change
Comment 154 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 03:54:03 PST
Created attachment 698268 [details] [diff] [review]
Part 4λ: Convert /xpfe, v2
Comment 155 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 08:24:32 PST
Comment on attachment 695151 [details] [diff] [review]
Part 4κ: Convert /xpcom, v1

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

::: xpcom/reflect/xptcall/src/md/test/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

This seems like dead code too.

::: xpcom/reflect/xptcall/src/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +
> +DIRS += ['md']

Nit: one empty line is enough
Comment 156 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 08:29:22 PST
Comment on attachment 695150 [details] [diff] [review]
Part 4ι: Convert /widget, v1

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

::: widget/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS == ['shared', 'xpwidgets']

Fixed locally

@@ +12,5 @@
> +
> +if toolkit == 'windows':
> +    DIRS += ['windows']
> +
> +TEST_DIRS += ['tests']

Made this TEST_TOOL_DIRS

@@ +24,5 @@
> +if CONFIG['MOZ_ENABLE_GTK2']:
> +    DIRS += ['gtk2']
> +
> +    if CONFIG['MOZ_X11']:
> +        DIRS == ['gtkxtbin']

And this

::: widget/windows/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_METRO']:
> +    DIRS += ['winrt']

You forgot to add widget/windows/winrt/moz.build

@@ +5,5 @@
> +
> +if CONFIG['MOZ_METRO']:
> +    DIRS += ['winrt']
> +
> +TEST_DIRS += ['tests']

This was TEST_TOOL_DIRS
Comment 157 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 11:15:19 PST
Comment on attachment 695119 [details] [diff] [review]
Part 4k: Convert /extensions, v1

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

::: extensions/pref/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +
> +DIRS += ['autoconfig']

One empty line

::: extensions/spellcheck/hunspell/tests/unit/data/moz.build
@@ +1,5 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Probably don't need this and extensions/spellcheck/hunspell/tests/unit/data/suggestiontest/moz.build
Comment 158 :Ms2ger (⌚ UTC+1/+2) 2013-01-05 11:25:49 PST
Comment on attachment 695122 [details] [diff] [review]
Part 4m: Convert /image, v1

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

::: image/test/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +TEST_DIRS += ['mochitest', 'browser']
> +

Drop the trailing empty line
Comment 159 :Ms2ger (⌚ UTC+1/+2) 2013-01-07 00:25:23 PST
Comment on attachment 695137 [details] [diff] [review]
Part 4x: Convert /netwerk, v1

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

::: netwerk/system/moz.build
@@ +8,5 @@
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa':
> +    DIRS += ['mac']
> +
> +if CONFIG['MOZ_ENABLE_LICENSING']:

LIBCONIC: https://hg.mozilla.org/users/Ms2ger_gmail.com/gps-patches/rev/7efc2b32072a#l3.1
Comment 160 Ted Mielczarek [:ted.mielczarek] 2013-01-07 04:57:03 PST
In the interest of reviewer sanity, I'm happy to treat Ms2ger's feedback+ as build config peer review+ on these patches.
Comment 161 :Ms2ger (⌚ UTC+1/+2) 2013-01-07 09:02:14 PST
Created attachment 698731 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v3
Comment 162 :Ms2ger (⌚ UTC+1/+2) 2013-01-07 11:01:25 PST
Comment on attachment 698268 [details] [diff] [review]
Part 4λ: Convert /xpfe, v2

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

::: xpfe/components/autocomplete/test/moz.build
@@ +1,4 @@
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Appears to be dead
Comment 163 Ed Morley [:emorley] 2013-01-07 11:04:51 PST
(In reply to :Ms2ger from comment #162)
> Comment on attachment 698268 [details] [diff] [review]
> Part 4λ: Convert /xpfe, v2
> 
> Review of attachment 698268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpfe/components/autocomplete/test/moz.build
> Appears to be dead

Used by c-c if my memory serves me correctly.
Comment 164 Gregory Szorc [:gps] 2013-01-08 11:17:15 PST
Per discussion in weekly platform meeting, we're going to hold off landing until at least January 15 out of concerns for other landings.
Comment 165 neil@parkwaycc.co.uk 2013-01-10 12:29:26 PST
Comment on attachment 698268 [details] [diff] [review]
Part 4λ: Convert /xpfe, v2

>diff --git a/xpfe/components/autocomplete/Makefile.in b/xpfe/components/autocomplete/Makefile.in
This part is only invoked from c-c, not directly by anywhere in m-c, does this needs any c-c build system changes first?

>diff --git a/xpfe/components/autocomplete/test/moz.build b/xpfe/components/autocomplete/test/moz.build
Agreed, this is dead. Some autocomplete functionality is tested through existing mochitests (yes, xpfe autocomplete passes toolkit tests).
Comment 166 Gregory Szorc [:gps] 2013-01-15 21:11:25 PST
(In reply to Mike Hommey [:glandium] from comment #138)
> Comment on attachment 695376 [details] [diff] [review]
> Part 6: Require virtualenv to build SpiderMonkey, v2

> @@ +22,5 @@
> > +
> > +dnl If this is a mozilla-central, we'll find the virtualenv in the top
> > +dnl source directory. If this is a SpiderMonkey build, we assume we're at
> > +dnl js/src and try to find the virtualenv from the mozilla-central root.
> > +for base in $MOZILLA_CENTRAL_PATH $_topsrcdir $_topsrcdir/../..; do
> 
> Why do you need $MOZILLA_CENTRAL_PATH if you try $_topsrcdir/../..?

This is a back door to allow anything to integrate with the virtualenv. I put it here in case anyone wanted to copy the m4 file.

> @@ +69,5 @@
> > +
> > +AC_MSG_CHECKING([Python environment is Mozilla virtualenv])
> > +$PYTHON -c "import mozbuild.base"
> > +if test "$?" != 0; then
> > +    AC_MSG_ERROR([Python environment does not appear to be sane.])
> 
> I think it would be better to make this test what triggers to populate a
> virtualenv or not, instead of DONT_POPULATE_VIRTUALENV. It also might be
> better to test something like from buildconfig import topsrcdir, because
> it's typically something that'll always be in a build virtualenv (in case
> mozbuild is ever used outside of m-c). BTW, the buildconfig module will
> likely be a problem under js/src :-/

DONT_POPULATE_VIRTUALENV is so configures launched from the main configure (and inherit the m4 files) don't perform redundant work. I'm sure there is a better way to do this with autoconf. I'm no wizard.

We don't have a good way to detect if a virtualenv is up to date. We certainly could have a virtualenv with mozbuild.base or buildconfig or whatnot and not be up to date because packages.txt is newer, a setup.py changed, etc. Rather than solve this problem, I decided to always execute populate_virtualenv.py.

I avoided buildconfig because of the issue you mentioned. If we ever distribute mozbuild as a standalone package, we should revisit the heuristic used to test the virtualenv for sanity.
Comment 167 Gregory Szorc [:gps] 2013-01-15 21:12:41 PST
Created attachment 702668 [details] [diff] [review]
Part 5: Require virtualenv to build SpiderMonkey, v3

Escaped "$PYTHON" and that's it. See previous comment.
Comment 168 Gregory Szorc [:gps] 2013-01-15 21:59:27 PST
(In reply to :Ms2ger from comment #140)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #139)
> > (In reply to :Ms2ger from comment #95)
> > > We'll need to decide if we're going to do 2-space or 4-space indentation
> > 
> > Since these are just a subset of Python I think we ought to follow PEP-8
> > style and use 4-space.
> 
> WFM. I'm wondering if we can require that somehow... (Though I don't want to
> scope-creep this bug even more :))

We could if we wanted. See http://docs.python.org/2/library/language.html if you want to dive into the Python dark arts. But, that's beyond initial landing.
Comment 169 Gregory Szorc [:gps] 2013-01-15 22:36:48 PST
The time has come.

https://hg.mozilla.org/projects/build-system/rev/a9021c50ccf9
https://hg.mozilla.org/projects/build-system/rev/ec072cee0502
https://hg.mozilla.org/projects/build-system/rev/d8ea5b8be44d
https://hg.mozilla.org/projects/build-system/rev/ee04a251a5ef

These are parts 1, 2, 3, and 5 from the patches in this bug.

I /may/ merge things to m-c if b-s is green and if the patches don't impact the existing build system. We still need to coordinate the merge of the moz.build transition with dev-planning, etc.
Comment 170 Gregory Szorc [:gps] 2013-01-15 22:54:11 PST
Created attachment 702677 [details] [diff] [review]
Part 6: Move parts of ConfigStatus into mozbuild, v3

Moved unit tests as well. Added missing __init__.py file. This builds the tree fine locally! And, previous try builds seemed to work as well. Should be a pretty straightforward rubber stamp.
Comment 171 Mike Hommey [:glandium] 2013-01-16 00:09:55 PST
Comment on attachment 702668 [details] [diff] [review]
Part 5: Require virtualenv to build SpiderMonkey, v3

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

Fair enough
Comment 172 Gregory Szorc [:gps] 2013-01-16 00:24:16 PST
https://hg.mozilla.org/projects/build-system/rev/d2cce982a7c8

Part 6 (Virtualenv changes) landed as part 5.

If this goes green, I'm very tempted to merge into m-c so we can hash out any trouble separately from the moz.build transition. Will check with Ted on IRC in the morning.
Comment 174 Gregory Szorc [:gps] 2013-01-16 23:19:49 PST
Created attachment 703178 [details] [diff] [review]
Part 7: Recursive make backend, v1

This is the real part 7 as it exists in my queue. Don't bother trying to match the part numbers in the bug to the order they land in.

Anyway, this patch has 2 major goals:

1) Defines a base class for build backends
2) Implements our basic recursive make backend

A build backend is simply a consumer of the objects derived from moz.build files. It currently has a single API, consume(), which takes the generator of derived objects and does "something" with them.

For the recursive make backend, that "something" is:

a) Perform Makefile.in -> Makefile conversion (using existing ConfigEnvironment class - formerly in ConfigStatus - see previous patch)
b) Generate dirs.mk file

"a" should be familiar to you. "b" is new. For each moz.build file, we generate a corresponding dirs.mk file next to the Makefile in the objdir. The dirs.mk file contains all the "DIRS =" variable assignments as derived from the contents of moz.build files. In a future patch (currently part 13 in this bug), there will be rules.mk integration to include these dirs.mk files. Thus, the semantics of the build don't change. The only difference is *DIRS variables are coming from moz.build files.

As new variables are added to moz.build files, functionality will be added to the recursive make backend to consume the derived data. e.g. when we add IDL files, the recursive make backend will write out "IDLSRCS" or inline make rules.

I'm not sure if I submitted this patch before. If I haven't, the major change since last time is we now have test coverage. It's pretty basic. But, it's better than nothing.
Comment 175 Mike Hommey [:glandium] 2013-01-17 03:40:34 PST
Comment on attachment 702668 [details] [diff] [review]
Part 5: Require virtualenv to build SpiderMonkey, v3

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

Fair enough

::: build/autoconf/python-virtualenv.m4
@@ +67,5 @@
> +
> +AC_SUBST(PYTHON)
> +
> +AC_MSG_CHECKING([Python environment is Mozilla virtualenv])
> +$PYTHON -c "import mozbuild.base"

mozbuild.base ends up importing the world, including multiprocessing, which breaks openbsd builds:

checking Python environment is Mozilla virtualenv... Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/var/buildslave-mozilla/mozilla-central-amd64/build/python/mozbuild/mozbuild/base.py", line 19, in <module>
    from .mozconfig import (
  File "/var/buildslave-mozilla/mozilla-central-amd64/build/python/mozbuild/mozbuild/mozconfig.py", line 11, in <module>
    import pymake.parser
  File "/var/buildslave-mozilla/mozilla-central-amd64/build/build/pymake/pymake/parser.py", line 37, in <module>
    import data, functions, util, parserdata
  File "/var/buildslave-mozilla/mozilla-central-amd64/build/build/pymake/pymake/data.py", line 6, in <module>
    import parserdata, parser, functions, process, util, implicit
  File "/var/buildslave-mozilla/mozilla-central-amd64/build/build/pymake/pymake/process.py", line 429, in <module>
    class ParallelContext(object):
  File "/var/buildslave-mozilla/mozilla-central-amd64/build/build/pymake/pymake/process.py", line 435, in ParallelContext
    _condition = multiprocessing.Condition()
  File "/usr/local/lib/python2.7/multiprocessing/__init__.py", line 189, in Condition
    from multiprocessing.synchronize import Condition
  File "/usr/local/lib/python2.7/multiprocessing/synchronize.py", line 59, in <module>
    " function, see issue 3770.")
ImportError: This platform lacks a functioning sem_open implementation, therefore, the required synchronization primitives needed will not function, see issue 3770.
configure: error: Python environment does not appear to be sane.
Comment 177 Gregory Szorc [:gps] 2013-01-17 10:31:29 PST
Hopeful patch for the BSD multiprocessing issue in bug 808280.
Comment 178 Landry Breuil (:gaston) 2013-01-17 12:17:20 PST
Thanks, that particular issue is now fixed for me
Comment 179 Gregory Szorc [:gps] 2013-01-17 21:06:16 PST
Created attachment 703750 [details] [diff] [review]
Part 8: Add external make variables to moz.build files, v2

This patch adds some new variables to moz.build files:

  EXTERNAL_MAKE_DIRS
  PARALLEL_EXTERNAL_MAKE_DIRS

The purpose of these variables is to declare directories that are built with |make| but are external to our build system.

For the short term, these variables will be used to build all external projects, even those derived from GYP and in the case of js/src, even those derived from moz.build files. In the long term, we'll introduce new variables to cover GYP projects and moz.build-built trees.

The current implementation ignores the problem of build dependencies, assuming external projects exist as a black box. I /think/ this is sufficient for now. If we need a mechanism to capture dependencies, we can add it later. I'm not too concerned about changing it later because the number of places we use these variables should be low.

It's worth noting explicitly that we need these variables separate from DIRS and PARALLEL_DIRS because the moz.build reader uses DIRS and PARALLEL_DIRS to descend into linked files. This will error if there is no moz.build file present. We don't want to create moz.build files in external projects, so we introduce these "external" variables. Besides, a little extra metadata capture isn't too bad, is it?
Comment 180 Gregory Szorc [:gps] 2013-01-17 22:00:46 PST
Created attachment 703770 [details] [diff] [review]
Part 10: Add warning and error functions to moz.build files, v2

Now with more proper error messages and some test coverage.

warning() could use some more love. meh. Follow-up.
Comment 181 Gregory Szorc [:gps] 2013-01-17 23:14:11 PST
Created attachment 703780 [details] [diff] [review]
Part 10: Ability to declare tier directories as external, v2

Now with tests for RecursiveMakeBackend.

We add a boolean to add_tier_dir() to define whether the directory is "external." This is similar to EXTERNAL_MAKE_DIRS except it is part of tier traversal.

We need the functionality to identify directories that are part of tier traversal but don't have moz.build files.

As an aside, all the extra *DIRS variables and ordering stuff is all hopefully temporary until we can wean ourselves off of recursive make. It's easier to copy the semantics of the build system as-is and change later than to attempt it all in one go.

I /think/ behavior of external=True is very similar (possibly identical!) to "static" tier directories. If it is, this patch should be to change "static" so it doesn't incur moz.build processing. Or, we could just rubber stamp this and sort it all out later :)
Comment 182 Gregory Szorc [:gps] 2013-01-18 00:02:37 PST
Created attachment 703793 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v2

Now with tests!

This patch adds SUBSTITUTE_CONFIG_FILES to moz.build files. This variable holds a list of files that are to be generated as part of configuring the build backend. As the usage docs say, this is a near equivalent of AC_OUTPUT. For each path listed, we find {path}.in from the srcdir and perform variable substitution (@FOO@) on the file and write out the result in the objdir.

This variable is a supplement for AC_OUTPUT. AC_OUTPUT isn't going away. Although, we may have lesser use for it in the future.

The reason we need this is because allmakefiles contained not only Makefiles but other .in-based files as well. I figured it would be better for the definition of the set of substituted files to live in the directory in which they are present rather than in configure (isolated in the top directory).

The implementation is simple and the test coverage isn't excellent (there is a whole slew of path normalization we should probably do). We don't use this variable very often. I figure we can staple on additional checks later if it becomes a footgun.
Comment 183 Mike Hommey [:glandium] 2013-01-18 00:14:04 PST
(In reply to Gregory Szorc [:gps] from comment #182)
> This variable is a supplement for AC_OUTPUT. AC_OUTPUT isn't going away.
> Although, we may have lesser use for it in the future.

Well, AC_OUTPUT is still what outputs config.status, so it's not going to go away until we stop using autoconf ;)
Comment 184 :Ms2ger (⌚ UTC+1/+2) 2013-01-18 13:31:06 PST
Comment on attachment 695116 [details] [diff] [review]
Part 4h: Convert /dom, v1

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

Alright, fine. I hope you're happy for wasting well over an hour of my life. Found one issue: https://hg.mozilla.org/users/Ms2ger_gmail.com/gps-patches/rev/d2b0fb70f67b
Comment 185 Gregory Szorc [:gps] 2013-01-18 19:49:41 PST
Created attachment 704172 [details] [diff] [review]
Part 7: Recursive make backend, v2

Changed the API slightly to facilitate future magic in the base class.
Comment 186 Gregory Szorc [:gps] 2013-01-18 19:59:11 PST
Created attachment 704173 [details]
Part 12: Capture moz.build state when feeding into backend, v1

There are essentially two parts to this patch:

1) Capture which files influenced the execution of a moz.build sandbox
2) Creating a file in the object directory whose mtime is that of the newest file that influenced the build.

#2 will be used in the make rules to influence when we need to incur a build backend refresh. This is forthcoming in a subsequent patch.
Comment 187 Gregory Szorc [:gps] 2013-01-18 23:46:36 PST
I was attempting to code up the rules.mk changes to support the final integration and came to the stark realization that make voodoo has been nearly fully purged from my brain. Maybe it's the beer?

Anyway, I'm soliciting help for the rules.mk integration because a) it's going to take me a while to figure it out on my own and b) I know others with more make knowledge should be able to crank it out without much effort.

Anyway, steps to contributing:

1) Grab my Mercurial patch queue from https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc
2) Edit the series file and ensure there are no patches on the stack above pybuild-*
3) hg qpush pybuild-rules-mk-integration
4) Start hacking

Per IRC discussions with Ted and Ms2ger today, the proposed solution has the following qualities:

* Generation of backend files is all or nothing. There is no mapping of single input files to output files. If a single input file is newer than the time the build was last generated, we perform a full backend rebuild.
* Backend rebuilds are performed by invoking |$(topobjdir)/config.status -n| (probably via $(DEPTH)/config.status)
* The test for "backend generation is needed" is smart enough to not have to stat() every single input file. We figured we'd create a single file with the mtime of the newest input file as part of the backend generation phase.
* Backend generation should never need to occur more than once for any single Makefile and never more than once for any recursive make traversal.
* The make rules need to be aware of make-based projects that aren't using moz.build files. Thus, we need to retain the existing Makefile.in -> Makefile, etc rules where appropriate.

Essentially no matter where you are in the directory hierarchy a |make| will cause backend generation for the *entire* tree to occur. Yes, this does sacrifice some performance in the short term. However, backend generation on my MBP only takes ~2s. More annoying than a few ms to recreate a single Makefile, sure. However, we're looking to the future here. The existing model assumes a 1:1 relationship between input and output files. As we start doing things like coalescing data from multiple moz.build files into single .mk files, this relationship will no longer hold true. We may still install make targets in individual directories. However, those make tarkets may depends on data from many moz.build files. Therefore, we need to incur a full scan and backend generation. Please note the newly added $(topobjdir)/backend.RecursiveMakeBackend.built file which has an mtime of the newest file that influenced the backend file generation.

Most of the work should be in rules.mk. You may need to modify python to capture dependencies (e.g. substituted files currently aren't outputted anywhere readable by make). If you don't want to create a "clean" patch, hack something up and I'll divide it into parts, create tests, etc.

I'll be tagging everyone I know with make + Python experience with the exception of Ms2ger. Ms2ger has already done enough and I won't ask for more. Although, if you want to do more...

FWIW, this is essentially the last patch that needs implemented. Once it's done, we'll just be waiting for reviews! I'm optimistic for landing between Jan 22 and 24.
Comment 188 Gregory Szorc [:gps] 2013-01-18 23:47:10 PST
See above comment.
Comment 189 Gregory Szorc [:gps] 2013-01-18 23:47:54 PST
This is a long shot... (see comment #187).
Comment 190 Mike Hommey [:glandium] 2013-01-19 01:28:41 PST
> Essentially no matter where you are in the directory hierarchy a |make|
> will cause backend generation for the *entire* tree to occur.

I *really* don't like that. And I'm pretty sure it's possible not to have to. Yes, even with multiple moz.build retrofitting in one Makefile.
Comment 191 Gregory Szorc [:gps] 2013-01-22 12:09:48 PST
Created attachment 705045 [details] [diff] [review]
Part 7: Recursive make backend, v3

Decided to change up .mk generation a bit to facilitate better integration with rules.mk. Instead of dirs.mk, we now write out a single backend.mk file. This means buffering everything in memory until the entire tree metadata is consumed. I don't think it will be an issue.
Comment 192 Gregory Szorc [:gps] 2013-01-22 12:33:37 PST
Created attachment 705060 [details] [diff] [review]
Part 8: Capture moz.build state when feeding into backend, v2

More capture of input and output files for rules.mk.
Comment 193 Gregory Szorc [:gps] 2013-01-22 12:34:27 PST
Created attachment 705063 [details] [diff] [review]
Part 9: Add external make variables to moz.build files, v3

Rebased.
Comment 194 Gregory Szorc [:gps] 2013-01-22 12:39:36 PST
Created attachment 705065 [details] [diff] [review]
Part 11: Ability to declare tier directories as external, v3

Rebased.
Comment 195 Gregory Szorc [:gps] 2013-01-22 12:41:57 PST
Created attachment 705067 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3

Rebased.
Comment 196 Gregory Szorc [:gps] 2013-01-22 12:43:55 PST
Created attachment 705068 [details] [diff] [review]
Part 14: rules.mk integration, v2

Newer iteration of rules.mk integration. Not yet complete. Need to fix some tests. Hopefully this gives you an idea of where I think we're going.
Comment 197 Ted Mielczarek [:ted.mielczarek] 2013-01-25 11:45:58 PST
Comment on attachment 705045 [details] [diff] [review]
Part 7: Recursive make backend, v3

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +65,5 @@
> +        backend_file = self._backend_files.get(obj.srcdir,
> +            BackendMakeFile(obj.srcdir, obj.objdir))
> +
> +        if isinstance(obj, DirectoryTraversal):
> +            self._process_directory_traversal(obj, backend_file)

So in theory this will grow as we add more data to the moz.build files? Does it make more sense to try to push this up into the base class, or are you okay with having multiple backends repeat this logic (when we have >1 backend)?

@@ +70,5 @@
> +
> +        self._backend_files[obj.srcdir] = backend_file
> +
> +    def consume_finished(self):
> +        for srcdir in sorted(self._backend_files.keys()):

Any particular reason you sort these?

@@ +77,5 @@
> +            if not os.path.exists(bf.objdir):
> +                os.makedirs(bf.objdir)
> +
> +            makefile_in = os.path.join(srcdir, 'Makefile.in')
> +            if os.path.exists(makefile_in):

Feels like this should have an else branch that logs an error or a warning, until we get to the point where it's feasible to have moz.build files without Makefile.ins next to them.

@@ +86,5 @@
> +                bf.outputs.add(out_path)
> +
> +            bf.close()
> +
> +    def _process_directory_traversal(self, o, bf):

The one and two-character variable names here don't really help readability.

@@ +93,5 @@
> +
> +        for tier, dirs in o.tier_dirs.iteritems():
> +            fh.write('TIERS += %s\n' % tier)
> +
> +            if len(dirs):

I've always thought it was more Pythonic to just write "if dirs:".

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ +50,5 @@
> +            'VPATH = %s' % env.topsrcdir,
> +            '',
> +            'include $(DEPTH)/config/autoconf.mk',
> +            '',
> +            'include $(topsrcdir)/config/rules.mk'

Some of these tests seem awfully fiddly, but I suppose it's better to have them than not.
Comment 198 Ted Mielczarek [:ted.mielczarek] 2013-01-25 11:52:38 PST
Comment on attachment 705060 [details] [diff] [review]
Part 8: Capture moz.build state when feeding into backend, v2

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

::: python/mozbuild/mozbuild/backend/base.py
@@ +60,5 @@
> +        # is newer than the build system source of truth. i.e. if a file known
> +        # to be part of the build system is newer than the newest file the last
> +        # time we executed the build backend, we should perform a new backend
> +        # run to pull in new data.
> +        class_path = sys.modules[self.__class__.__module__].__file__

You should probably comment that this is to include the mtime of the mozbuild backend as a prereq.

@@ +72,5 @@
> +
> +        age_file = os.path.join(self.environment.topobjdir,
> +            'backend.%s.built' % self.__class__.__name__)
> +        with open(age_file, 'a'):
> +            os.utime(age_file, (os.path.getatime(age_file), newest_mtime))

This feels like a weird way to update the mtime. Did you get this from somewhere else?

Also, it feels like it'd be better to just update this file to the current time at the end of consume_finished(), such that it's newer than all the output files as well, rather than just utime'ing it to be the same age as the newest input...

::: python/mozbuild/mozbuild/frontend/data.py
@@ +41,5 @@
>      )
>  
>      def __init__(self, sandbox):
> +        self.sandbox_main_path = sandbox.main_path
> +        self.sandbox_all_paths = sandbox.all_paths

Can you comment as to what these are somewhere? I figured it out but only after I read all the code.
Comment 199 Ted Mielczarek [:ted.mielczarek] 2013-01-25 11:55:25 PST
Comment on attachment 705063 [details] [diff] [review]
Part 9: Add external make variables to moz.build files, v3

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

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +128,5 @@
> +        """),
> +
> +    'PARALLEL_EXTERNAL_MAKE_DIRS': (list, [],
> +        """Parallel version of EXTERNAL_MAKE_DIRS.
> +        """),

Do we actually have need of both of these? I would sort of assume that anything that's not being built with our build system could be built in parallel.
Comment 200 Ted Mielczarek [:ted.mielczarek] 2013-01-25 11:58:40 PST
Comment on attachment 703770 [details] [diff] [review]
Part 10: Add warning and error functions to moz.build files, v2

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

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +281,5 @@
>          script_frame = None
> +
> +        # We don't currently capture the trace for SandboxCalledError.
> +        # Therefore, we don't get line numbers.
> +        # FUTURE capture this.

Line numbers from what, the moz.build file? That seems like it'd be pretty unfortunate to not have.
Comment 201 Mike Hommey [:glandium] 2013-01-25 12:00:22 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #199)
> Do we actually have need of both of these? I would sort of assume that
> anything that's not being built with our build system could be built in
> parallel.

There are interdependencies, for example nss depends on nspr, and some things under media/ depend on others.
Comment 202 Ted Mielczarek [:ted.mielczarek] 2013-01-25 12:04:17 PST
Comment on attachment 705065 [details] [diff] [review]
Part 11: Ability to declare tier directories as external, v3

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

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +196,5 @@
>          add_tier_dir('base', 'foo', static=True)
> +
> +        # Register a directory to be recursed into but is not part of this
> +        # build config tree.
> +        add_tier_dir('js', 'js/src', external=True)

What's the relationship between static and external? Historically, 'static' in the Mozilla build system has meant almost exactly what you're defining 'external' to mean: a third-party directory. I'm not sure I understand the value in making this distinction.
Comment 203 Ted Mielczarek [:ted.mielczarek] 2013-01-25 12:10:48 PST
Comment on attachment 705067 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +78,5 @@
>              self._process_directory_traversal(obj, backend_file)
> +        elif isinstance(obj, ConfigFileSubstitution):
> +            backend_file.inputs.add(obj.input_path)
> +            backend_file.outputs.add(obj.output_path)
> +            self.environment.create_config_file(obj.output_path)

It seems weird to do this in the backend when this is really sort of configure's job. Won't we just have to copy-paste this logic to each new backend?

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +130,5 @@
>      'PARALLEL_EXTERNAL_MAKE_DIRS': (list, [],
>          """Parallel version of EXTERNAL_MAKE_DIRS.
>          """),
> +
> +    'SUBSTITUTE_CONFIG_FILES': (list, [],

Not to bikeshed you, but this name isn't very self-descriptive. Maybe if you flip it around a little, like CONFIGURE_SUBST_FILES? To me that reads a little more like "things that get configure subst's".
Comment 204 Ted Mielczarek [:ted.mielczarek] 2013-01-25 12:39:44 PST
Comment on attachment 698731 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v3

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

::: build/moz.build
@@ +10,5 @@
> +
> +if CONFIG['STLPORT_SOURCES']:
> +    DIRS += ['stlport']
> +
> +DIRS += ['pgo']

I bet these could probably all be PARALLEL_DIRS, but that could be done later.
Comment 205 Ted Mielczarek [:ted.mielczarek] 2013-01-28 06:57:12 PST
Comment on attachment 695134 [details] [diff] [review]
Part 4u: Convert /mobile/android, v1

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

This mostly looks fine, but there's a showstopper in here.

::: mobile/android/app.mozbuild
@@ +5,5 @@
> +
> +if not CONFIG['LIBXUL_SDK']:
> +    include('/toolkit/toolkit.mozbuild')
> +
> +    add_tier_dir('platform', 'mobile/android/components/build')

This is not actually a reasonable substitution for APP_LIBXUL_DIRS. APP_LIBXUL_DIRS gets built *before* libxul:
http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit-tiers.mk#284

::: mobile/android/build.mk
@@ -3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -ifndef LIBXUL_SDK
> -# Needed for building our components as part of libxul
> -APP_LIBXUL_DIRS += mobile/android/components/build

This seems to have been lost. We need to keep this.
Comment 206 :Ms2ger (⌚ UTC+1/+2) 2013-01-28 07:00:54 PST
Created attachment 707052 [details] [diff] [review]
Part 4u: Convert /mobile/android, v2
Comment 207 :Ms2ger (⌚ UTC+1/+2) 2013-01-28 07:02:05 PST
Created attachment 707053 [details] [diff] [review]
Part 4v: Convert /mobile/xul, v2
Comment 208 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-28 18:22:45 PST
(In reply to Gregory Szorc [:gps] from comment #12)
> So, I think we should start moving away from incremental backend generation
> and just do it all or nothing. Fortunately, the new Python files seem to be
> fast. On my i7-2600K, it's loading ~1000 files in ~350ms. When you add in
> the I/O to write dirs.mk, it does it in ~500ms. Granted, the Python files
> don't have everything defined in them yet. But, I'm optimistic that with
> everything converted we're still only talking about 1-1.5s to do the loading
> and about the same to perform I/O.

Are these numbers from Windows? Please make sure you measure on Windows before drawing conclusions from these kinds of measurements.
Comment 209 Gregory Szorc [:gps] 2013-01-29 06:28:00 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #197)
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +65,5 @@
> > +        backend_file = self._backend_files.get(obj.srcdir,
> > +            BackendMakeFile(obj.srcdir, obj.objdir))
> > +
> > +        if isinstance(obj, DirectoryTraversal):
> > +            self._process_directory_traversal(obj, backend_file)
> 
> So in theory this will grow as we add more data to the moz.build files? Does
> it make more sense to try to push this up into the base class, or are you
> okay with having multiple backends repeat this logic (when we have >1
> backend)?

This will need to be pushed up or factored out at some point. I was lazy for the first iteration.

> @@ +70,5 @@
> > +
> > +        self._backend_files[obj.srcdir] = backend_file
> > +
> > +    def consume_finished(self):
> > +        for srcdir in sorted(self._backend_files.keys()):
> 
> Any particular reason you sort these?

It isn't strictly required. I added it so behavior is consistent. If nothing else, verbose logging will look sane.

> @@ +77,5 @@
> > +            if not os.path.exists(bf.objdir):
> > +                os.makedirs(bf.objdir)
> > +
> > +            makefile_in = os.path.join(srcdir, 'Makefile.in')
> > +            if os.path.exists(makefile_in):
> 
> Feels like this should have an else branch that logs an error or a warning,
> until we get to the point where it's feasible to have moz.build files
> without Makefile.ins next to them.

Implemented locally.

 
> @@ +93,5 @@
> > +
> > +        for tier, dirs in o.tier_dirs.iteritems():
> > +            fh.write('TIERS += %s\n' % tier)
> > +
> > +            if len(dirs):
> 
> I've always thought it was more Pythonic to just write "if dirs:".

Yes it is. Changed locally.

> ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
> @@ +50,5 @@
> > +            'VPATH = %s' % env.topsrcdir,
> > +            '',
> > +            'include $(DEPTH)/config/autoconf.mk',
> > +            '',
> > +            'include $(topsrcdir)/config/rules.mk'
> 
> Some of these tests seem awfully fiddly, but I suppose it's better to have
> them than not.

Indeed. I'm noticing this as I add new features. We'll probably end up writing test helpers to strip out boilerplate foo, empty lines, etc.
Comment 210 Gregory Szorc [:gps] 2013-01-29 06:29:40 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #198)
> @@ +72,5 @@
> > +
> > +        age_file = os.path.join(self.environment.topobjdir,
> > +            'backend.%s.built' % self.__class__.__name__)
> > +        with open(age_file, 'a'):
> > +            os.utime(age_file, (os.path.getatime(age_file), newest_mtime))
> 
> This feels like a weird way to update the mtime. Did you get this from
> somewhere else?
> 
> Also, it feels like it'd be better to just update this file to the current
> time at the end of consume_finished(), such that it's newer than all the
> output files as well, rather than just utime'ing it to be the same age as
> the newest input...

Changed locally. All this code has been replaced by a single line. Yay simplicity!
Comment 211 Gregory Szorc [:gps] 2013-01-29 06:32:58 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #199)
> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> @@ +128,5 @@
> > +        """),
> > +
> > +    'PARALLEL_EXTERNAL_MAKE_DIRS': (list, [],
> > +        """Parallel version of EXTERNAL_MAKE_DIRS.
> > +        """),
> 
> Do we actually have need of both of these? I would sort of assume that
> anything that's not being built with our build system could be built in
> parallel.

Medium term we could probably merge these. I introduced serial vs parallel modes because there are "external" dirs in the existing build system that differentiate between DIRS and PARALLEL_DIRS and I didn't want to risk change too much during porting. Long term, most of these DIRS variables are going away since we'll have non-recursive make or similar.
Comment 212 Gregory Szorc [:gps] 2013-01-29 06:34:56 PST
Comment on attachment 705067 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +78,5 @@
>              self._process_directory_traversal(obj, backend_file)
> +        elif isinstance(obj, ConfigFileSubstitution):
> +            backend_file.inputs.add(obj.input_path)
> +            backend_file.outputs.add(obj.output_path)
> +            self.environment.create_config_file(obj.output_path)

This will get pulled into the base class or factored out at some point.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +130,5 @@
>      'PARALLEL_EXTERNAL_MAKE_DIRS': (list, [],
>          """Parallel version of EXTERNAL_MAKE_DIRS.
>          """),
> +
> +    'SUBSTITUTE_CONFIG_FILES': (list, [],

Changed locally.
Comment 214 Gregory Szorc [:gps] 2013-01-29 08:56:50 PST
https://hg.mozilla.org/projects/build-system/rev/11658ed5bc17

Fixed Windows test failure in part 11. Self-reviewed because trivial.
Comment 216 Masatoshi Kimura [:emk] 2013-01-30 06:13:16 PST
I'm unable to build the tree because of infinite error messages:

UnicodeDecodeError: 'ascii' codec can't decode byte 0x83 in position 0: ordinal
not in range(128)
h:\m\mozilla-central\config\rules.mk:1141:0: command 'h:/m/mozilla-central/obj-i
686-pc-mingw32/_virtualenv/Scripts/python.exe ../../config.status -n --file=Make
file' failed, return code 1
Error remaking makefiles (ignored)
Traceback (most recent call last):
  File "../../config.status", line 1887, in <module>
    config_status(**args)
  File "h:\m\mozilla-central\build\ConfigStatus.py", line 84, in config_status
    non_global_defines=non_global_defines, substs=substs)
  File "h:\m\mozilla-central\python\mozbuild\mozbuild\backend\configenvironment.
py", line 81, in __init__
    self.substs[name]) for name in self.substs]))
Comment 217 Masatoshi Kimura [:emk] 2013-01-30 06:37:38 PST
Created attachment 708109 [details] [diff] [review]
Use filesystem encoding to decode environment variables

It looks like localized msvc messages bit me once again (following bug 587372 and bug 780430).
Comment 218 Masatoshi Kimura [:emk] 2013-01-30 06:56:01 PST
Created attachment 708119 [details] [diff] [review]
Use filesystem encoding to decode environment variables

Forgot to import sys
Comment 219 Gregory Szorc [:gps] 2013-01-30 06:57:26 PST
Comment on attachment 708109 [details] [diff] [review]
Use filesystem encoding to decode environment variables

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

I have an alternative idea: remove the "from __future__ import unicode_literals" from configenvironment.py. If that works for you, let's r+ and land in inbound.
Comment 220 Masatoshi Kimura [:emk] 2013-01-30 07:23:30 PST
(In reply to Gregory Szorc [:gps] from comment #219)
> I have an alternative idea: remove the "from __future__ import
> unicode_literals" from configenvironment.py. If that works for you, let's r+
> and land in inbound.

It appears that it works. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/7faec6303aa9
Comment 221 :Ms2ger (⌚ UTC+1/+2) 2013-01-31 04:00:25 PST
With my tree on

https://hg.mozilla.org/mozilla-central/file/20bbf73921f4
https://hg.mozilla.org/users/Ms2ger_gmail.com/gps-patches/file/b2a5ab5f6bca

I get

 0:14.50 creating config files and headers...
 0:14.66 Traceback (most recent call last):
 0:14.66   File "./config.status", line 931, in <module>
 0:14.66     config_status(**args)
 0:14.66   File "~/Mozilla/build-system/build/ConfigStatus.py", line 130, in config_status
 0:14.66     backend.consume(definitions)
 0:14.66   File "~/Mozilla/build-system/python/mozbuild/mozbuild/backend/base.py", line 57, in consume
 0:14.66     for obj in objs:
 0:14.66   File "~/Mozilla/build-system/python/mozbuild/mozbuild/frontend/emitter.py", line 34, in emit
 0:14.66     for out in output:
 0:14.66   File "~/Mozilla/build-system/python/mozbuild/mozbuild/frontend/reader.py", line 522, in read_mozbuild
 0:14.66     raise bre
 0:14.66 mozbuild.frontend.reader.BuildReaderError: ==============================
 0:14.66 ERROR PROCESSING MOZBUILD FILE
 0:14.66 ==============================
 0:14.66 
 0:14.66 The error occurred while processing the following file:
 0:14.66 
 0:14.66     ~/Mozilla/build-system/moz.build
 0:14.66 
 0:14.66 The underlying problem is we referenced a path that does not exist. That path is:
 0:14.66 
 0:14.66     ~/Mozilla/build-system/nsprpub/moz.build
 0:14.66 
 0:14.66 Either create the file if it needs to exist or do not reference it.
 0:14.66 
 0:14.67 *** Fix above errors and then restart with               "/usr/bin/make -f client.mk build"
 0:14.67 make[2]: *** [configure] Error 1
 0:14.67 make[1]: *** [obj-x86_64-unknown-linux-gnu/config.status] Error 2
 0:14.67 make: *** [build] Error 2
 0:14.70 1844 compiler warnings present.

though config/nspr/nspr.mozbuild has

    add_tier_dir('nspr', 'nsprpub', static=True)

Can you please fix that, and add a reference to config/nspr/nspr.mozbuild in the error message?
Comment 222 Gregory Szorc [:gps] 2013-01-31 11:25:49 PST
Comment on attachment 705065 [details] [diff] [review]
Part 11: Ability to declare tier directories as external, v3

This patch was removed because it is redundant with static tier dirs.
Comment 223 Gregory Szorc [:gps] 2013-01-31 11:27:58 PST
Created attachment 708700 [details] [diff] [review]
Part 12: Don't read moz.build files in static tier directories, v1

static tier directories == external make-based build systems. This patch makes it so we no longer attempt to read moz.build files in static tier directories.
Comment 224 Ted Mielczarek [:ted.mielczarek] 2013-01-31 11:39:56 PST
Comment on attachment 708700 [details] [diff] [review]
Part 12: Don't read moz.build files in static tier directories, v1

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

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +594,5 @@
> +                    if d in dirs:
> +                        raise SandboxValidationError(
> +                            'Tier directory (%s) registered multiple '
> +                            'times in %s' % (d, tier))
> +                    dirs.append(d)

Might be worthwhile to still test the multiply-set thing for static dirs, but if it really complicates things then probably not.
Comment 225 Ryan VanderMeulen [:RyanVM] 2013-01-31 13:39:46 PST
https://hg.mozilla.org/mozilla-central/rev/7faec6303aa9
Comment 226 Gregory Szorc [:gps] 2013-01-31 13:50:52 PST
*** Bug 836693 has been marked as a duplicate of this bug. ***
Comment 227 Gregory Szorc [:gps] 2013-01-31 14:07:49 PST
Created attachment 708793 [details] [diff] [review]
Part 13: Use moz.build files to build the tree, v1

This is the last critical patch!

I've tested locally and it seems to work. Not sure if I hit all the edge cases for Makefile regeneration, especially wrt external build systems.

I'd /really/ like multiple review on this.

Try at https://tbpl.mozilla.org/?tree=Try&rev=e2f4ad71bbc9
Comment 228 :Ms2ger (⌚ UTC+1/+2) 2013-02-01 00:49:23 PST
Comment on attachment 708793 [details] [diff] [review]
Part 13: Use moz.build files to build the tree, v1

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +53,5 @@
>          self.fh.close()
>  
> +        # Always update the mtime so we ensure the output files are always
> +        # newer than the input files.
> +        os.utime(self.path, None)

Am I missing a point or doesn't it make sense to use FileAvoidWrite if you're going to touch the file anyway?
Comment 229 Mike Hommey [:glandium] 2013-02-01 01:22:14 PST
Comment on attachment 708793 [details] [diff] [review]
Part 13: Use moz.build files to build the tree, v1

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

::: config/rules.mk
@@ +1186,5 @@
> +# traversal.
> +
> +ifneq ($(BACKEND_OUTPUT_FILES),,)
> +$(BACKEND_OUTPUT_FILES): $(BACKEND_INPUT_FILES)
> +	@$(PYTHON) $(DEPTH)/config.status -n

I'm really not a fan of having everything be refreshed, cf. comment 190 and comment 208.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +53,5 @@
>          self.fh.close()
>  
> +        # Always update the mtime so we ensure the output files are always
> +        # newer than the input files.
> +        os.utime(self.path, None)

Indeed, this defeats using FileAvoidWrite, and will likely lead to many useless recompilations.
Comment 230 Gregory Szorc [:gps] 2013-02-04 16:32:22 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #208)
> Are these numbers from Windows? Please make sure you measure on Windows
> before drawing conclusions from these kinds of measurements.

Just did an apples and oranges comparison between OS X and Windows.

OS X:      1.90s wall time
Windows 7: 2.70s wall time

Both were on Core i7's with SSDs. Windows had a 800MHz advantage.

Anyway, my takeaway is Windows is in the same ballpark as OS X (and presumably Linux). While it sucks Windows isn't as fast, it generally isn't (at least for these types of tasks).

If things slow down over time, we can optimize things. But, I don't think it is worth the time investment at this juncture. Hopefully the next version of my patch will find a more acceptable compromise.
Comment 231 Gregory Szorc [:gps] 2013-02-04 20:11:02 PST
Created attachment 710040 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v2

We now have a SUBSTITUTE_FILES rules for moz.build files to drive *.in conversion. This includes Makefile.in.

BACKEND_INPUT_FILES now pertains to only the moz.build files. There is a backend.mk rule that rebuilds backend.mk if any of the input files change. We still perform a full scan. If we run into perf issues, we can cross that bridge. I think this a fair compromise between performance and complexity.

BACKEND_OUTPUT_FILES has been sacked.

Try at https://tbpl.mozilla.org/?tree=Try&rev=56e5a927a358.
Comment 232 Gregory Szorc [:gps] 2013-02-04 20:50:46 PST
Created attachment 710043 [details] [diff] [review]
Part 13: Use absolute paths in config.status, v1

The "if not os.path.isabs" implies that we should have been converting topsrcdir to an absolute path if it is relative. The old code did not do that. This patch changes that.

If the old comment was wrong, the justification for requiring an absolute path is that a) absolute paths are better b) mozbuild.frontend expects an absolute path and it's much easier to normalize it in config.status then somewhere down the stack.
Comment 233 Mozilla RelEng Bot 2013-02-04 22:30:41 PST
Try run for 56e5a927a358 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=56e5a927a358
Results (out of 155 total builds):
    exception: 43
    success: 67
    warnings: 1
    failure: 44
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-56e5a927a358
Comment 234 Mike Hommey [:glandium] 2013-02-05 01:24:01 PST
Comment on attachment 710040 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v2

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

::: build/autoconf/config.status.m4
@@ +79,5 @@
>  cat > $CONFIG_STATUS <<EOF
>  #!${PYTHON}
>  # coding=$encoding
>  
> +from __future__ import unicode_literals

Could it be what is breaking the packager on windows on your try build?

::: config/rules.mk
@@ +1180,5 @@
> +# Detect when the backend.mk needs rebuilt. This will cause a full scan and
> +# rebuild. While relatively expensive, it should only once per recursion.
> +ifneq ($(BACKEND_INPUT_FILES),,)
> +backend.mk: $(BACKEND_INPUT_FILES) $(DEPTH)/config/autoconf.mk
> +	@$(PYTHON) $(DEPTH)/config.status -n

I still don't like this, but meh.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +103,5 @@
>              out_path = os.path.join(bf.objdir, 'Makefile')
>              self.log(logging.DEBUG, 'create_makefile', {'path': out_path},
>                  'Generating makefile: {path}')
>              self.environment.create_config_file(out_path)
> +            os.utime(out_path, None)

Please remove this (see comments on previous iteration).

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ +82,5 @@
>          backend_path = os.path.join(env.topobjdir, 'backend.mk')
>  
> +        now = time.time()
> +        os.utime(makefile_path, (now - 5, now - 5))
> +        os.utime(backend_path, (now - 5, now - 5))

Ditto
Comment 235 Mozilla RelEng Bot 2013-02-05 02:00:40 PST
Try run for 645cb2432294 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=645cb2432294
Results (out of 270 total builds):
    success: 259
    warnings: 8
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-645cb2432294
Comment 236 Gregory Szorc [:gps] 2013-02-18 16:43:06 PST
There was a DONTBUILD sitting on top of inbound and inbound needed built and I had this patch sitting around and I was lazy. So, part 12:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35a2fd406e4f
Comment 237 Ryan VanderMeulen [:RyanVM] 2013-02-19 05:48:50 PST
https://hg.mozilla.org/mozilla-central/rev/35a2fd406e4f
Comment 238 Gregory Szorc [:gps] 2013-02-19 12:21:26 PST
Created attachment 715638 [details] [diff] [review]
Part 4h: Convert /dom, v2

I updated the various files in /dom/imptests to work with moz.build files. However, when I try to run them, I get weird results. Looking at the Mercurial commit history, it seems Ms2ger is the person who commonly runs these scripts.

Ms2ger: I'd appreciate if you could do the voodoo that you do so well and let me know if my changes have the intended side-effects. Notably, we shouldn't have DIRS in autogenerated Makefile.in and we should now have autogenerated moz.build files alongside the Makefile.in. If you could upload an updated patch with the results of autogeneration, that would be swell.

Note that I changed the arguments to |rm| because apparently OS X's rm doesn't support the long form arguments! Perhaps the scripts could be rewritten to use shutil.rmtree?
Comment 239 Gregory Szorc [:gps] 2013-02-19 12:22:37 PST
Created attachment 715640 [details] [diff] [review]
Part 4e: Convert /browser, v2

Removed bit rot. Mostly additions from /browser/metro.
Comment 240 :Ms2ger (⌚ UTC+1/+2) 2013-02-19 14:13:58 PST
Comment on attachment 715638 [details] [diff] [review]
Part 4h: Convert /dom, v2

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

I've looked a bit... Any chance you could get me a diff of just the python files?

::: dom/imptests/importTestsuite.py
@@ +86,5 @@
>          # Empty directory, i.e., the repository root
>          importDirs(thissrcdir, dest, subdirs)
>  
> +def printMozbuild(dest, directories):
> +    """Create a .mozbuild file to be included into the main moz.build, which lists the

I'd really rather not have mixed two-space/four-space indentation... Could you either stick with two spaces or land a separate patch to reindent all the local files in this dir? (Should be isomorphic to the ones with an MPL header.)

@@ +95,5 @@
> +
> +    with open(path, 'w') as fh:
> +        fh.write('DIRS += [\n')
> +        for d in directories:
> +            fh.write("    '%s\n'" % d)

I think you want the ' before the \n.

@@ +140,5 @@
>      repo, dest, directories = getData(confFile)
>      hgdest = "hg-%s" % (dest, )
>      print("Going to clone %s to %s..." % (repo, hgdest))
>      print("Removing %s..." % dest)
> +    subprocess.check_call(["rm", "-rf", dest])

Lovely.

::: dom/imptests/writeMakefile.py
@@ +19,2 @@
>  
>  ${dirs}

I don't think this should still be here

@@ +27,5 @@
> +mozbuildTemplate = """# THIS FILE IS AUTOGENERATED BY ${caller} - DO NOT EDIT
> +
> +DIRS += [
> +
> +]

And I think there should be a ${dirs} somewhere here

@@ +45,4 @@
>      "files": assignList("MOCHITEST_FILES", files) if files else ""
>    })
> +
> +def substMozbuild(caller, subdirs):

Which makes writeMakefile.py a terrible name... Maybe writeBuildFile? I dunno
Comment 241 Gregory Szorc [:gps] 2013-02-19 14:32:20 PST
Created attachment 715718 [details] [diff] [review]
Part 14: 4-space indent for dom/imptests/*.py, v1

Cleaning up these files before I change them. I used reindent.py from the cpython source tree to perform reformatting. I didn't actually test things, but I highly doubt reindent.py breaks things.
Comment 242 Gregory Szorc [:gps] 2013-02-19 15:02:34 PST
Created attachment 715740 [details] [diff] [review]
Part 14: Produce moz.build files for DOM implementation tests, v1

This patch should be more reasonable. It kinda/sorta seems to work locally. Although, the produced .mozbuild files (e.g. editing.mozbuild) seem to consist of empty lists. I must be missing something obvious...
Comment 243 :Ms2ger (⌚ UTC+1/+2) 2013-02-20 03:18:26 PST
Comment on attachment 715718 [details] [diff] [review]
Part 14: 4-space indent for dom/imptests/*.py, v1

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

rs=me
Comment 244 :Ms2ger (⌚ UTC+1/+2) 2013-02-20 03:43:16 PST
Comment on attachment 715740 [details] [diff] [review]
Part 14: Produce moz.build files for DOM implementation tests, v1

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

DOM imported tests, btw, not implementation.

I'd appreciate it if you could fix these comments and push your queue repo so I can test it locally.

::: dom/imptests/importTestsuite.py
@@ +93,5 @@
> +    path = dest + ".mozbuild"
> +    with open(path, 'w') as fh:
> +        fh.write('DIRS += [\n')
> +        for d in directories:
> +            fh.write("    '%s',\n" % d)

You're missing the makePath call here.

::: dom/imptests/parseFailures.py
@@ +60,5 @@
>  
>      for k, v in pathmap.items():
> +        with open(k + '/Makefile.in', 'wb') as fh:
> +            result = writeBuildFiles.substMakefile('parseFailures.py', v)
> +            result = resultstr.encode('utf-8')

Please use result here or assign to resultstr above.

@@ +66,4 @@
>  
> +        with open(k + '/moz.build', 'wb') as fh:
> +            result = writeBuildFiles.substMozbuild('parseFailures.py', [])
> +            result = resultstr.encode('utf-8')

Same here.

::: dom/imptests/writeMakefile.py
@@ +1,1 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public

Please update the name of this file in dom/imptests/README and fix the description.

@@ +47,5 @@
> +
> +def substMozbuild(caller, dirs):
> +    return string.Template(mozbuild_template).substitute({
> +        "caller": caller,
> +        "dirs": "\n".join(["    '%s'," % d for d in dirs]),

Factor this out and use it in printMozbuildFile in importTestsuite.py? Probably along with the DIRS += [] bit.
Comment 245 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-20 15:44:00 PST
Comment on attachment 715640 [details] [diff] [review]
Part 4e: Convert /browser, v2

I took a quick look at the interdiff from what I reviewed last time and it seems fine (though I don't know what STANDALONE_MAKEFILE or CONFIGURE_SUBST_FILES are).

There's a "dekstop" typo in the metro files. Might be good to have someone familiar with their build system (Jim?) look over those new parts again.
Comment 246 Gregory Szorc [:gps] 2013-02-21 10:48:30 PST
Created attachment 716666 [details] [diff] [review]
Part 15: Produce moz.build files for DOM implementation tests, v2

I believe I addressed all points from the last review.

Ms2ger: Assuming r+, could you please upload a patch with the new state of the autogenerated files?
Comment 248 :Ms2ger (⌚ UTC+1/+2) 2013-02-21 12:07:06 PST
Comment on attachment 716666 [details] [diff] [review]
Part 15: Produce moz.build files for DOM implementation tests, v2

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

r=me. Thanks!

::: dom/imptests/importTestsuite.py
@@ +92,5 @@
> +    print("Creating mozbuild...")
> +    path = dest + ".mozbuild"
> +    with open(path, 'w') as fh:
> +        normalized = [makePath(dest, d) for d in directories]
> +        fh.write(writeBuildFiles.mozbuildDirs(normalized))

Please use substMozbuild here too. (That gives us the comment about it being autogenerated and adds a missing newline at the end with this code.)
Comment 249 :Ms2ger (⌚ UTC+1/+2) 2013-02-21 12:29:06 PST
Created attachment 716710 [details] [diff] [review]
Part 4h (i): dom/imptests (importTestsuite.py)
Comment 250 :Ms2ger (⌚ UTC+1/+2) 2013-02-21 12:30:04 PST
Created attachment 716712 [details] [diff] [review]
Part 4h (ii): dom/imptests (parseFailures.py)
Comment 251 Gregory Szorc [:gps] 2013-02-21 14:57:22 PST
Created attachment 716806 [details] [diff] [review]
Part 4h: Convert /dom, v3

Removed all the files now touched by Ms2ger's patches (which I plan on keeping in their own patches). I also found a few missing directories (due to patch bit rot) that were causing build failures.
Comment 252 Gregory Szorc [:gps] 2013-02-21 17:20:21 PST
My latest push of the entire patch queue to Try is failing on Windows:

https://tbpl.mozilla.org/?tree=Try&rev=d1d8745a00f9

It's failing in packaging with the following:

Executing e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/dist\bin\shlibsign.exe -v -o ..\..\dist\firefox\softokn3.chk -i ../../dist/firefox\softokn3.dll
Traceback (most recent call last):
  File "e:/builds/moz2_slave/try-w32-0000000000000000000000/build/toolkit/mozapps/installer/packager.py", line 366, in <module>
    main()
  File "e:/builds/moz2_slave/try-w32-0000000000000000000000/build/toolkit/mozapps/installer/packager.py", line 360, in main
    copier.copy(args.destination)
  File "e:\builds\moz2_slave\try-w32-0000000000000000000000\build\python\mozbuild\mozpack\copier.py", line 151, in copy
    file.copy(destfile, skip_if_older)
  File "e:/builds/moz2_slave/try-w32-0000000000000000000000/build/toolkit/mozapps/installer/packager.py", line 104, in copy
    if launcher.launch(['shlibsign', '-v', '-o', dest, '-i', self.path]):
  File "e:/builds/moz2_slave/try-w32-0000000000000000000000/build/toolkit/mozapps/installer/packager.py", line 83, in launch
    return subprocess.call(cmd, env=env)
  File "c:\mozilla-build\python27\Lib\subprocess.py", line 493, in call
    return Popen(*popenargs, **kwargs).wait()
  File "c:\mozilla-build\python27\Lib\subprocess.py", line 679, in __init__
    errread, errwrite)
  File "c:\mozilla-build\python27\Lib\subprocess.py", line 896, in _execute_child
    startupinfo)
TypeError: environment can only contain strings
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\toolkit\mozapps\installer\packager.mk:589:0: command 'e:/builds/moz2_slave/try-w32-0000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe e:/builds/moz2_slave/try-w32-0000000000000000000000/build/toolkit/mozapps/installer/packager.py -DNO_NSPR_10_SUPPORT -DAB_CD=en-US -DMOZ_APP_NAME=firefox -DPREF_DIR=defaults/preferences -D_MSC_VER=1600 -DJAREXT= -DMOZ_ANGLE_RENDERER=1 -DMOZ_D3DCOMPILER_DLL=D3DCompiler_43.dll -DMOZ_CHILD_PROCESS_NAME=plugin-container.exe -DMOZ_MSVC_REDIST=1600 -DMOZ_SHARED_MOZGLUE=1 -DMOZ_JSDEBUGGER -DNECKO_WIFI -DDLL_PREFIX= -DDLL_SUFFIX=.dll -DBIN_SUFFIX=.exe -DBINPATH=bin \
	--format omni \
	--removals e:/builds/moz2_slave/try-w32-0000000000000000000000/build/browser/installer/removed-files.in \
	 \
	 \
	 \
	--optimizejars \
	 \
	package-manifest ../../dist ../../dist/firefox \
	' failed, return code 1
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\toolkit\mozapps\installer\packager.mk:616:0: command 'C:/mozilla-build/python27/python.exe e:/builds/moz2_slave/try-w32-0000000000000000000000/build/build/pymake/pymake/../make.py make-package-internal' failed, return code 2
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\config\rules.mk:613:0: command 'C:/mozilla-build/python27/python.exe e:/builds/moz2_slave/try-w32-0000000000000000000000/build/build/pymake/pymake/../make.py libs' failed, return code 2
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\browser\build.mk:25:0: command 'C:/mozilla-build/python27/python.exe e:/builds/moz2_slave/try-w32-0000000000000000000000/build/build/pymake/pymake/../make.py -C browser/installer' failed, return code 2
program finished with exit code 2

I suspect c:\mozilla-build\python27\python.exe is Python 2.7.1 or Python 2.7.2, not Python 2.7.3. I suspect this because those older 2.7 releases have bugs in various modules where they only accept str instances and reject unicode instances. I suspect some change in my patches has introduced a unicode value and this is tickling this Python bug.

I suppose I can figure out where the unicode is coming from and change it back to str. But, I'd prefer to cross the Python 3 "all strings are unicode" bridge while we can. This would make us dependent on the tree in bug 602908. Maybe I'll just rip out unicode...
Comment 253 Gary Kwong [:gkw] [:nth10sd] 2013-02-21 17:28:31 PST
> I suspect c:\mozilla-build\python27\python.exe is Python 2.7.1 or Python
> 2.7.2, not Python 2.7.3.

You are correct. MozillaBuild 1.6.1 has Python 2.7.2. MozillaBuild 1.7 is not yet released, but we've already landed a patch to bump that to Python 2.7.3.

See bug 788911 and the current status at http://hg.mozilla.org/mozilla-build/ (as of the time of writing).
Comment 254 Gregory Szorc [:gps] 2013-02-21 18:55:39 PST
I hypothesized that the unicode was coming from config.status. So, I pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=c3a9a9f33aad

The good news is Windows is working now. The bad news is all of Android and B2G broke. I can't make this stuff up.
Comment 255 Gregory Szorc [:gps] 2013-02-22 08:20:15 PST
Parts 13 and 14:

https://hg.mozilla.org/mozilla-central/rev/31466fd86eb7
https://hg.mozilla.org/mozilla-central/rev/bdf128d09ed6

Note that build-system is currently building a lot of Ubuntu tests that are known orange. And, things like Fedora tests aren't running. This makes it somewhat difficult to ascertain whether build-system is green. I'm pretty confident that despite the orange state of build-system, it isn't actually orange. philor filed bug 843950 to deal with the Ubuntu tests. We may consider making build-system behave more like other project branches. I'm not sure what the historical reasons are...
Comment 256 Ted Mielczarek [:ted.mielczarek] 2013-02-22 08:24:10 PST
We were intentionally not running Talos tests at one point because we didn't think we needed them. Aside from that I suspect any difference is historical accident.
Comment 257 Ted Mielczarek [:ted.mielczarek] 2013-02-22 11:54:44 PST
Comment on attachment 710040 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v2

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

Fix the stuff glandium wants fixed and this should be good.

::: config/rules.mk
@@ +1180,5 @@
> +# Detect when the backend.mk needs rebuilt. This will cause a full scan and
> +# rebuild. While relatively expensive, it should only once per recursion.
> +ifneq ($(BACKEND_INPUT_FILES),,)
> +backend.mk: $(BACKEND_INPUT_FILES) $(DEPTH)/config/autoconf.mk
> +	@$(PYTHON) $(DEPTH)/config.status -n

I suspect this will only be really painful until we can derecursify some.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +33,5 @@
>  
> +        self.fh = StringIO()
> +        self.fh.write('# THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT EDIT.\n')
> +        self.fh.write('\n')
> +        self.fh.write('MOZBUILD_DERIVED := 1\n')

I usually prefer triple-quoted strings for multi-line strings.

@@ +38,3 @@
>  
> +        # SUBSTITUTE_FILES handles Makefile.in -> Makefile conversion.
> +        self.fh.write('NO_MAKEFILE_RULE := 1\n')

Can we just make NO_MAKEFILE_RULE the default? Will anything actually need it when this lands?
Comment 258 Gregory Szorc [:gps] 2013-02-22 18:02:40 PST
Created attachment 717420 [details] [diff] [review]
Part 17: Handle unicode environment variables in packager, v1

Workaround for Python < 2.7.3 bug not allowing unicode in environment variables. I reproduced locally and verified the fix. In-progress Try at https://tbpl.mozilla.org/?tree=Try&rev=81c7e173352a also seems happy.
Comment 259 Gregory Szorc [:gps] 2013-02-22 18:26:49 PST
Created attachment 717428 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v3

I removed the os.utime() abuse per earlier comments. I /think/ this was the only outstanding review comment from your previous r-. I consider ted's r+ carried forward (on condition of your r+).
Comment 260 Gregory Szorc [:gps] 2013-02-22 19:06:16 PST
Masatoshi: I'd like to verify these patches don't regress non-English toolchains (e.g. comment #216). I downloaded the Turkish Visual Studio language pack and was able to successfully build. However, I didn't see any Turkish output when building (I did switch Visual Studio to Turkish and restart the environment before building). So, I might have been doing it wrong.

I'd appreciate if you could grab my patch queue from https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc, apply everything from pybuild-dom-imptests-produce-mozbuild to pybuild-remove-allmakefiles on mozilla-central:7d5884e9ebf5, and let me know if you run into any issues. If not, great. If so, I guess I'll try harder to get the Visual Studio CLI tools to emit localized strings. If you have any suggestions, I'd love to hear them.
Comment 261 Masatoshi Kimura [:emk] 2013-02-22 19:45:18 PST
It look like there is no such a revision.
https://hg.mozilla.org/mozilla-central/file/7d5884e9ebf5
Also, your patch queue has 3000+ revisions. Where to start from?
Comment 262 Gregory Szorc [:gps] 2013-02-22 20:02:59 PST
(In reply to Masatoshi Kimura [:emk] from comment #261)
> It look like there is no such a revision.
> https://hg.mozilla.org/mozilla-central/file/7d5884e9ebf5
> Also, your patch queue has 3000+ revisions. Where to start from?

Sorry. m-c:08a034e1d43a and use the tip revision of the default branch in my patch queue.
Comment 263 Masatoshi Kimura [:emk] 2013-02-22 20:11:59 PST
Probably I'm mistaken, but how to apply patches from your queue?

$ hg update -r 08a034e1d43a
0 files updated, 0 files merged, 0 files removed, 0 files unresolved

$ hg qimp -e 82ad91243619
adding 82ad91243619 to series file

$ hg qpush
applying 82ad91243619
unable to find 'pybuild-fixup' for patching
1 out of 1 hunks FAILED -- saving rejects to file pybuild-fixup.rej
unable to find 'pybuild-use-mozbuild' for patching
4 out of 4 hunks FAILED -- saving rejects to file pybuild-use-mozbuild.rej
unable to find 'series' for patching
1 out of 1 hunks FAILED -- saving rejects to file series.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 82ad91243619

$
Comment 264 Gregory Szorc [:gps] 2013-02-22 20:18:01 PST
$ hg up -r 08a034e1d43a
$ hg qpush pybuild-remove-allmakefiles
<this should push about 40 patches on the stack>

You /may/ have modified my patch queue repository when you ran |qimp| so you may want to revert to its original state.
Comment 265 Masatoshi Kimura [:emk] 2013-02-22 20:40:57 PST
Ah, succeeded! Thank you for your patience.
But the build failed with the following error:

 1:19.53 creating config files and headers...
 1:19.53 Traceback (most recent call last):
 1:19.54   File "./config.status", line 859, in <module>
 1:19.54     config_status(**args)
 1:19.54   File "h:\m\mozilla-central\build\ConfigStatus.py", line 130, in confi
g_status
 1:19.54     backend.consume(definitions)
 1:19.54   File "h:\m\mozilla-central\python\mozbuild\mozbuild\backend\base.py",
 line 58, in consume
 1:19.54     self.consume_object(obj)
 1:19.54   File "h:\m\mozilla-central\python\mozbuild\mozbuild\backend\recursive
make.py", line 87, in consume_object
 1:19.54     self.environment.create_config_file(obj.output_path)
 1:19.54   File "h:\m\mozilla-central\python\mozbuild\mozbuild\backend\configenv
ironment.py", line 144, in create_config_file
 1:19.54     pp.do_include(input)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 447, in do_i
nclude
 1:19.54     self.handleLine(l)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 226, in hand
leLine
 1:19.54     self.write(aLine)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 136, in writ
e
 1:19.54     filteredLine = self.applyFilters(aLine)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 121, in appl
yFilters
 1:19.54     aLine = f[1](aLine)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 407, in filt
er_attemptSubstitution
 1:19.54     return self.filter_substitution(aLine, fatal=False)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 405, in filt
er_substitution
 1:19.54     return self.varsubst.sub(repl, aLine)
 1:19.54   File "h:\m\mozilla-central\config\Preprocessor.py", line 401, in repl

 1:19.54     return str(self.context[varname])
 1:19.55 UnicodeEncodeError: 'ascii' codec can't encode characters in position 3
365-3366: ordinal not in range(128)
 1:19.56 *** Fix above errors and then restart with               "c:/mozilla-bu
ild/python/python.exe h:/m/mozilla-central/build/pymake/pymake/../make.py -f cli
ent.mk build"
 1:19.57 h:\m\mozilla-central\client.mk:320:0: command 'cd h:/m/mozilla-central/
../obj-debug &&  MAKE="c:/mozilla-build/python/python.exe h:/m/mozilla-central/b
uild/pymake/pymake/../make.py"  h:/m/mozilla-central/configure  \
 1:19.57   || ( echo "*** Fix above errors and then restart with\
 1:19.57                \"c:/mozilla-build/python/python.exe h:/m/mozilla-centra
l/build/pymake/pymake/../make.py -f client.mk build\"" && exit 1 )' failed, retu
rn code 1
 1:19.57 h:\m\mozilla-central\client.mk:332:0: command 'c:/mozilla-build/python/
python.exe h:/m/mozilla-central/build/pymake/pymake/../make.py -f h:/m/mozilla-c
entral/client.mk configure' failed, return code 2
 1:19.57 h:\m\mozilla-central\client.mk:160:0: command 'c:/mozilla-build/python/
python.exe h:/m/mozilla-central/build/pymake/pymake/../make.py -f h:/m/mozilla-c
entral/client.mk realbuild' failed, return code 2
 1:19.65 0 compiler warnings present.
Finished building. Built files are in h:\m\mozilla-central/../obj-debug
2

$
Comment 266 Masatoshi Kimura [:emk] 2013-02-22 23:49:05 PST
Created attachment 717479 [details] [diff] [review]
Remove |from __future__ import unicode_literals| from config.status

I needed this patch to build the tree.
Comment 267 Gregory Szorc [:gps] 2013-02-23 08:36:31 PST
Comment on attachment 717479 [details] [diff] [review]
Remove |from __future__ import unicode_literals| from config.status

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

Oh, I know this fixes it for you. The problem is this breaks the build for B2G!

The reason is... because Python.

I wrote all the new Python code in this bug to be Python 3 compatible (or at least had it in mind - I haven't actually tested Python 3 in a while). This means "from __future__ import unicode_literals" needs to be so Python 2 treats literal strings as Unicode (like Python 3). If we fail to do this, there will be many inadvertent conversions between str and unicode. This will work a lot of times in Python 2 (as long as the underlying data is ASCII). Python 3 will flat out refuse to perform the conversion because that's how Python 3 rolls.

Anyway, strings in mozbuild files are assumed to be the unicode type (not str). There is code sprinkled throughout the mozbuild reader that does things like "isinstance(foo, str_type)" (str_type is unicode on Python 2 and str on Python 3). The problem is the data from config.status gets injects into mozbuild files. If we don't have unicode_literals in config.status, these values are the str type. If we concatenate with literal strings during execution of a mozbuild file, Python 3 will blow up. If we assign to another variable, that variable has a str value and the isinstance(foo, str_type) doesn't hold. This is what was causing B2G builds to blow up. Essentially it was assigning CONFIG['MOZ_BRANDING_APP'] (or some similar variable) to a special mozbuild variable and then the whole world blew up.

Long term, I'd like to get config.status to use unicode_literals because this will be needed for Python 3 compatibility. This is why every time I touch this file I add it. Every time it works for buildbot and a majority of developers (developing in English). It breaks for you, probably a good faction of non-English developers, and possibly l10n builds (untested).

While changing config.status to use unicode_literals is the preferred long term solution, it is the most radical. If it worked, I'd take it. But, I have a feeling that even if we chase down the one failure you are having that other components further down the chain would break. We need an alternative.

I think the safest alternative is to have the mozbuild reader convert the contents of CONFIG from str to unicode. It should only need to do this once, when it is instantiated. So, there shouldn't be a performance penalty. A potential downside is we may not apply the proper encoding. We'll likely assume UTF-8. I'm optimistic this will just work (if we don't normalize the encoding in config.status, then we have other problems, as many of these values could creep into source code or generated files and mixed file encodings could cause a world of hurt).

I can also look into what in your config.status contains non-ascii text. If it is something small and likely not widespread, perhaps we can add some validation to config.status and/or configure checks to ensure it doesn't creep into the build. This is slightly more risky, as it means anything grabbing data from config.status will now see unicode instead of str. I bet things would break.

Anyway, this is on me to chase down. I'll figure out something.
Comment 268 Gregory Szorc [:gps] 2013-02-23 09:12:10 PST
OK, I coded up a very hacky patch to help identify the non-ascii strings in your config.status. Please pull my patch queue and then push pybuild-find-unicode-in-config-status on top of the other pybuild-* patches. When you build, you should see some output before the exception. e.g.

String contains non-ascii data: Value from exec_prefix in substs
${prefix}β
creating config files and headers...
Traceback (most recent call last):
  File "./config.status", line 931, in <module>
    config_status(**args)
  File "/home/gps/src/mozilla-central/build/ConfigStatus.py", line 130, in config_status
    backend.consume(definitions)

The good news is that as part of verifying this patch, I actually managed to reproduce your error! Down the rabbit hole I go...
Comment 269 Masatoshi Kimura [:emk] 2013-02-23 09:56:44 PST
 0:54.11 creating ./config.status
 0:56.29 String contains non-ascii data: Value from CL_INCLUDES_PREFIX in substs

 0:56.29 郢晢ス。郢晢ス「: 郢ァ・、郢晢スウ郢ァ・ッ郢晢スォ郢晢スシ郢晢ソス郢晁シ斐<郢ァ・、郢晢スォ
:

But this text is garbled. The actual CL_INCLUDES_PREFIX value is "メモ: インクルード ファイル:" encoded in mbcs (not UTF-8!)
Here is the byte sequence I copied from a binary editor:
83 81 83 82 3A 20 83 43 83 93 83 4E 83 8B 81 5B 83 68 20 83 74 83 40 83 43 83 8B 3A

(In reply to Gregory Szorc [:gps] from comment #267)
> I think the safest alternative is to have the mozbuild reader convert the
> contents of CONFIG from str to unicode. It should only need to do this once,
> when it is instantiated. So, there shouldn't be a performance penalty. A
> potential downside is we may not apply the proper encoding. We'll likely
> assume UTF-8. I'm optimistic this will just work

Unfortunately it doesn't hold. Win32 console sucks.
Comment 270 Gregory Szorc [:gps] 2013-02-23 10:59:03 PST
This is very helpful!

It appears we use CL_INCLUDES_PREFIX to detect included files so we can set up proper build dependencies (https://mxr.mozilla.org/mozilla-central/search?string=CL_INCLUDES_PREFIX). So, it's important for us not to get this value wrong (i.e. wrong encoding): if it is wrong, header dependencies will be broken and incremental builds won't be proper.

I'm reasonably confident that if we perform the str -> unicode conversion as part of mozbuild execution and only as part of mozbuild execution, we should be fine. As long as CL_INCLUDES_PREFIX (or any other value that needs preserved as bytes) doesn't leak into the build part of the build system, we should be fine.

Ted, Mike, et al: Are you aware of any other config.status variables that might be non-ascii?

Long term, we may need to introduce a new AC_* macro that differentiates between byte strings and unicode strings or something. But, we can defer crossing that bridge until we tackle Python 3 compatibility in the overall build system.

Masatoshi: One last thing: what does the CL_INCLUDES_PREFIX line look like in your objdir/config.status?
Comment 271 Gregory Szorc [:gps] 2013-02-23 13:06:22 PST
We already build on 2.7 everywhere. We'll work around the <2.7.3 in patches.
Comment 272 Masatoshi Kimura [:emk] 2013-02-23 13:07:37 PST
(In reply to Gregory Szorc [:gps] from comment #270)
> Masatoshi: One last thing: what does the CL_INCLUDES_PREFIX line look like
> in your objdir/config.status?

    (''' CL_INCLUDES_PREFIX ''', r''' メモ: インクルード ファイル: '''),

in mbcs encoding.
Comment 273 Gregory Szorc [:gps] 2013-02-23 13:31:54 PST
Created attachment 717545 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v4

Removed unicode_literals from config.status and converted to Unicode for the CONFIG variable in mozbuild files.

Try at http://tbpl.mozilla.org/?tree=Try&rev=f060401b256e.
Comment 274 :Ms2ger (⌚ UTC+1/+2) 2013-02-23 13:36:50 PST
Comment on attachment 716806 [details] [diff] [review]
Part 4h: Convert /dom, v3

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

::: dom/indexedDB/moz.build
@@ +3,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['ipc']
> +TEST_DIRS += ['test']

You forgot to remove these from the Makefile.in. Didn't you have code to prevent that?

::: dom/mms/Makefile.in
@@ -16,3 @@
>  ifdef MOZ_B2G_RIL
>  ifdef ENABLE_TESTS
>  XPCSHELL_TESTS = tests

Don't we usually add those to DIRS too?
Comment 275 Masatoshi Kimura [:emk] 2013-02-23 13:38:49 PST
Comment on attachment 717545 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v4

Apparently this is the same file as v3.
Comment 276 Masatoshi Kimura [:emk] 2013-02-23 13:42:12 PST
try-pushed file semms to be correct.
https://hg.mozilla.org/try/raw-rev/ddff1be65f2a
Comment 277 Gregory Szorc [:gps] 2013-02-23 13:42:47 PST
Created attachment 717547 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v5

I accidentally uploaded the previous patch from the wrong VM. #firstworldproblems.
Comment 278 Gregory Szorc [:gps] 2013-02-23 13:57:03 PST
Honeybadger don't care.
Comment 279 Gregory Szorc [:gps] 2013-02-23 15:31:53 PST
Comment on attachment 717547 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v5

We are failing Python unit tests due to behavior of the recursive make backend and mtimes. Back to the drawing board.
Comment 280 Gregory Szorc [:gps] 2013-02-23 16:20:56 PST
Created attachment 717564 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v6

I'm feeling pretty good about this version. I had to insert an os.utime() into the backend code so build dependency behavior was sane. The comprehensive added comment hopefully explains things in sufficient detail.
Comment 281 Gregory Szorc [:gps] 2013-02-23 16:41:40 PST
Created attachment 717566 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v7

I should have read Ms2ger's review comment. This fixes rules.mk not rejecting Makefiles still containing DIRS variables.
Comment 282 Gregory Szorc [:gps] 2013-02-23 16:49:27 PST
(In reply to :Ms2ger from comment #274)
> You forgot to remove these from the Makefile.in. Didn't you have code to
> prevent that?

So I did x 2. Fixed rules.mk and this instance in my patch queue.

> ::: dom/mms/Makefile.in
> @@ -16,3 @@
> >  ifdef MOZ_B2G_RIL
> >  ifdef ENABLE_TESTS
> >  XPCSHELL_TESTS = tests
> 
> Don't we usually add those to DIRS too?

XPCSHELL_TESTS, not yet. I imagine we'll replace this with XPCSHELL_TEST_MANIFESTS or something in a subsequent bug.
Comment 283 Gregory Szorc [:gps] 2013-02-23 18:42:30 PST
Comment on attachment 717566 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v7

This patch doesn't properly handle changes to autoconf.mk. Almost there...
Comment 284 Gregory Szorc [:gps] 2013-02-23 19:19:06 PST
Created attachment 717574 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v8

Handle autoconf.mk within the build backend. Try at https://tbpl.mozilla.org/?tree=Try&rev=e5c384a851ff. Note I have rebased my patch repo against latest m-c.
Comment 285 Gregory Szorc [:gps] 2013-02-23 19:20:32 PST
Created attachment 717575 [details] [diff] [review]
Part 4e: Convert /browser, v3

Please intradiff against v2 and verify I picked up the theme changes properly.
Comment 286 :Ms2ger (⌚ UTC+1/+2) 2013-02-24 03:07:34 PST
Some new dirs from bug 767231 seem to have been lost.
Comment 287 Gregory Szorc [:gps] 2013-02-24 10:23:57 PST
(In reply to :Ms2ger from comment #286)
> Some new dirs from bug 767231 seem to have been lost.

Fixed in my patch queue.
Comment 288 Mike Hommey [:glandium] 2013-02-25 03:57:10 PST
Comment on attachment 717574 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v8

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

::: build/ConfigStatus.py
@@ +127,2 @@
>      if not options.files and not options.headers:
>          print >>sys.stderr, "creating config files and headers..."

Should that go through logging?

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +96,5 @@
> +            try:
> +                return os.path.getmtime(path)
> +            except OSError as e:
> +                # No such file or directory.
> +                if e.errno == 2:

errno.ENOENT
Comment 289 Jared Wein [:jaws] (please needinfo? me) 2013-02-25 09:01:58 PST
Comment on attachment 717575 [details] [diff] [review]
Part 4e: Convert /browser, v3

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

::: browser/themes/moz.build
@@ +10,5 @@
> +else:
> +    DIRS += ['windows']
> +
> +if toolkit in ('gtk2', 'qt'):
> +    DIRS += ['linux']

Comment #112 said to make sure that we don't end up with windows+linux here. It looks like v2 had it fixed but that regressed in v3.
Comment 290 Jeff Hammel 2013-02-25 11:58:36 PST
(In reply to Gregory Szorc [:gps] from comment #267)

> I think the safest alternative is to have the mozbuild reader convert the
> contents of CONFIG from str to unicode. It should only need to do this once,
> when it is instantiated. So, there shouldn't be a performance penalty. A
> potential downside is we may not apply the proper encoding. We'll likely
> assume UTF-8. I'm optimistic this will just work (if we don't normalize the
> encoding in config.status, then we have other problems, as many of these
> values could creep into source code or generated files and mixed file
> encodings could cause a world of hurt).

To subtext -> text the unspoken and obvious, we can of course make utf-8 in these areas a requirement for building vs trying to save the world on this bug.  Not ideal, nor a forever plan, but I'd do that here if it saves time for the reasons suggested elsewhere in comment 267
Comment 291 Gregory Szorc [:gps] 2013-02-25 12:03:13 PST
Created attachment 718007 [details] [diff] [review]
Part ∞: Remove allmakefiles.sh and friends, v1

I don't think I've posted this part yet!

Review should be mostly rubber stamp file removal. There are some parts to configure.in that changed, hence glandium review.
Comment 292 Ed Morley [:emorley] 2013-02-25 12:33:26 PST
Comment on attachment 718007 [details] [diff] [review]
Part ∞: Remove allmakefiles.sh and friends, v1

Glad to see those damn files go - the amount of time I spent trying to keep them up to date..!
Comment 293 Gregory Szorc [:gps] 2013-02-25 12:35:17 PST
(In reply to Mike Hommey [:glandium] from comment #288)
> ::: build/ConfigStatus.py
> @@ +127,2 @@
> >      if not options.files and not options.headers:
> >          print >>sys.stderr, "creating config files and headers..."
> 
> Should that go through logging?

Probably. The relationship between logging and ConfigStatus.py is a little funky right now (e.g. you get double timing output when building through mach). There will already be a follow-up here. I'm fine with deferring.
Comment 294 Gregory Szorc [:gps] 2013-02-25 13:18:47 PST
Comment on attachment 695155 [details] [diff] [review]
Part 4d: Convert /toolkit, v2

Mossop also looked at this way bay when IIRC.
Comment 295 Gregory Szorc [:gps] 2013-02-25 13:23:44 PST
https://hg.mozilla.org/projects/build-system/rev/c1b0d8399be9
https://hg.mozilla.org/projects/build-system/rev/23673de2aa43
https://hg.mozilla.org/projects/build-system/rev/63e2e94efd44
https://hg.mozilla.org/projects/build-system/rev/76d05284e22b
https://hg.mozilla.org/projects/build-system/rev/f4bbd950f0b2
https://hg.mozilla.org/projects/build-system/rev/5cdf008662ee
https://hg.mozilla.org/projects/build-system/rev/3fc89f2888fe
https://hg.mozilla.org/projects/build-system/rev/7fc68ba4fe80
https://hg.mozilla.org/projects/build-system/rev/56b6ff7d20ec
https://hg.mozilla.org/projects/build-system/rev/b9dc9cbefcad
https://hg.mozilla.org/projects/build-system/rev/60edd40b5809
https://hg.mozilla.org/projects/build-system/rev/9358ecd378af
https://hg.mozilla.org/projects/build-system/rev/f1c891577cef
https://hg.mozilla.org/projects/build-system/rev/18c0e3805925
https://hg.mozilla.org/projects/build-system/rev/f58efdc21e14
https://hg.mozilla.org/projects/build-system/rev/008e20cf742a
https://hg.mozilla.org/projects/build-system/rev/bb5481ee141f
https://hg.mozilla.org/projects/build-system/rev/5c4418e37b13
https://hg.mozilla.org/projects/build-system/rev/ba58ecbd2b5b
https://hg.mozilla.org/projects/build-system/rev/4a34004947d1
https://hg.mozilla.org/projects/build-system/rev/36c6535a6352
https://hg.mozilla.org/projects/build-system/rev/d32891bfb1b8
https://hg.mozilla.org/projects/build-system/rev/4a61bea80168
https://hg.mozilla.org/projects/build-system/rev/7f1f7cf07362
https://hg.mozilla.org/projects/build-system/rev/d1a83f27351b
https://hg.mozilla.org/projects/build-system/rev/93e83b5951dc
https://hg.mozilla.org/projects/build-system/rev/5cb8bcab09cc
https://hg.mozilla.org/projects/build-system/rev/de5cf405c384
https://hg.mozilla.org/projects/build-system/rev/b69deb240d9d
https://hg.mozilla.org/projects/build-system/rev/c8cf78ad3f7f
https://hg.mozilla.org/projects/build-system/rev/f861388b829f
https://hg.mozilla.org/projects/build-system/rev/db019bcfd0ad
https://hg.mozilla.org/projects/build-system/rev/0741061e86e7
https://hg.mozilla.org/projects/build-system/rev/c67860b9a9d3
https://hg.mozilla.org/projects/build-system/rev/2e4ea5c2020e
https://hg.mozilla.org/projects/build-system/rev/3cfd3750c162
https://hg.mozilla.org/projects/build-system/rev/02d62b51156a
https://hg.mozilla.org/projects/build-system/rev/0e42b9f6b7d3
https://hg.mozilla.org/projects/build-system/rev/251aa9072a17
https://hg.mozilla.org/projects/build-system/rev/20c33662a25e
https://hg.mozilla.org/projects/build-system/rev/592b31342f74
https://hg.mozilla.org/projects/build-system/rev/17b6856efdbe
https://hg.mozilla.org/projects/build-system/rev/873935822ede
https://hg.mozilla.org/projects/build-system/rev/0dd558c975f0
Comment 296 Gregory Szorc [:gps] 2013-02-25 14:03:24 PST
Recent m-c pull introduced /dom/payment/test. This was turning B2G builds red. I self-reviewed the trivial patch to include it.

https://hg.mozilla.org/projects/build-system/rev/9efbdb63ecbe
Comment 297 Ted Mielczarek [:ted.mielczarek] 2013-02-25 14:08:17 PST
Comment on attachment 718007 [details] [diff] [review]
Part ∞: Remove allmakefiles.sh and friends, v1

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

Whee!

::: configure.in
@@ +9141,5 @@
>    _save_cache_file="$cache_file"
>    cache_file=$_objdir/memory/jemalloc/src/config.cache
> +
> +  if ! test -e memory/jemalloc; then
> +    mkdir -p memory/jemalloc

We should probably just fix AC_OUTPUT_SUBDIRS to do this.
Comment 298 Gregory Szorc [:gps] 2013-02-25 14:11:20 PST
https://hg.mozilla.org/projects/build-system/rev/632f21df7be1

And we're ready to merge as soon as trees go green and we get the go-ahead!
Comment 299 Gregory Szorc [:gps] 2013-02-25 23:14:56 PST
Created attachment 718280 [details] [diff] [review]
Part ∞+1: Support for external build systems, v1

As I discovered from trying to get comm-central working with these patches, I broke external build systems (I think there are comments in here somewhere warning of that). I /think/ this patch is all that is needed from the m-c side to unbork things. If I get r+, I'll probably hold off committing until I'm sure this is the only change required.
Comment 300 Gregory Szorc [:gps] 2013-02-27 20:46:31 PST
Created attachment 719306 [details] [diff] [review]
Part ∞+1: Support for external build systems, v2

I /think/ I finally got comm-central to play well in our new world. And this is what it took. (I'm still churning through builds, so don't be surprised if you see a few minor revisions of this patch.)

There are sub-parts to this bug. I was too lazy and tired to spend my normal diligence to split things into parts:

1) Re-add app libxul static dirs to toolkit.mozbuild. IMO we should eventually replace this hackery with something better. Now that we have a whole world view with moz.build files, this should be doable.

2) Add --with-external-source-dir to configure. moz.build has some cheap security and tries to enforce that build files are in a known directory tree. External projects (like comm-central) are obviously not under the main source tree. My choices were either to remove the check (boo) or pass in external directories to the build system. I chose the latter. It's currently somewhat half-baked. e.g. we should probably add support for multiple values!

3) Add config.status loading code to configenvironment.py. I kinda wrote something similar in the patch waiting review in bug 648681. I like this version better because it doesn't use imp.

4) get_environment() API on the base build backend. This essentially allows you to obtain the correct ConfigEnvironment (essentially config.status interface) from a processed mozbuild file.

5) The RecursiveMakeBackend now uses the appropriate ConfigEnvironment depending for the metadata it is working on. This is needed because when m-c's build system is running, it processes moz.build files from comm-central which use a different config.status! This is confusing, I know. We didn't have to worry about this so much in the old build system because running config.status generated Makefile via allmakefiles.sh. With mozbuild, we rely on the loaded files to generate Makefile. allmakefiles.sh was actually used for something beyond caching!

6) Code to mozbuild reader to allow reads from external source directories.

7) Properly setting topsrcdir, srcdir, objdir, etc in mozbuild sandboxes when the moz.build file is in an external source directory. The topobjdir calculation is really hacky and has a comment to that effect. We should file a follow-up to make this suck less. I probably could have come up with something, but I'm lazy :)

Whew. That's a lot of small changes! The worst part: I didn't write any new tests :( The good news is the existing tests all work without any changes! Checking this in should result in no change in behavior for m-c.
Comment 301 Gregory Szorc [:gps] 2013-02-27 21:45:39 PST
Oh, the last patch goes to whoever gets it first :) I'm only looking for 1 review. And please be lax: I'm content with using follow-ups for the rough edges. Try at https://tbpl.mozilla.org/?tree=Try&rev=728ce6033b58.

As for landing on m-c, Ms2ger has volunteered to shepherd the trees and perform the landing before California wakes up! If that timeline works out, great!

I see 2 scenarios for landing on m-c:

a) build-system as-is, without the last patch on this bug.
b) build-system + the last patch on this bug (+ bug 845089 on c-c?)

m-c will be fine with either. However, c-c won't work until both the final patch and the one in bug 845089 land.

If someone from c-c (Callek?) signs off that it's OK to merge without the patches and break c-c (hopefully for only a few hours), then by all means merge! Else, I guess we have to wait for reviews.

In both timelines, please ensure the following:

* At least 1 build peer is around and aware things are going on. I don't anticipate anything funny. But you never know.
* Tree rules are followed and nobody is objecting loudly. The last I saw on dev-planning, there was a soft and somewhat theoretical objection from Andrew Overhold. My reading of the thread is we're good to go unless someone raises a hard objection. But I'm biased in my interpretation :)

I figure I'll wake up around 8 AM PST. I plan to be around most of the day to assist with any triage, if needed.

Let's land this sucker!
Comment 302 Mike Hommey [:glandium] 2013-02-27 23:25:55 PST
Comment on attachment 719306 [details] [diff] [review]
Part ∞+1: Support for external build systems, v2

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

I think we can just back this out when bug 648979 is fixed, which should happen sooner than later, now that bug 648980 has landed.
Comment 303 Gregory Szorc [:gps] 2013-02-28 09:55:58 PST
https://hg.mozilla.org/mozilla-central/rev/c1b0d8399be9
https://hg.mozilla.org/mozilla-central/rev/23673de2aa43
https://hg.mozilla.org/mozilla-central/rev/63e2e94efd44
https://hg.mozilla.org/mozilla-central/rev/76d05284e22b
https://hg.mozilla.org/mozilla-central/rev/f4bbd950f0b2
https://hg.mozilla.org/mozilla-central/rev/5cdf008662ee
https://hg.mozilla.org/mozilla-central/rev/3fc89f2888fe
https://hg.mozilla.org/mozilla-central/rev/7fc68ba4fe80
https://hg.mozilla.org/mozilla-central/rev/56b6ff7d20ec
https://hg.mozilla.org/mozilla-central/rev/b9dc9cbefcad
https://hg.mozilla.org/mozilla-central/rev/60edd40b5809
https://hg.mozilla.org/mozilla-central/rev/9358ecd378af
https://hg.mozilla.org/mozilla-central/rev/f1c891577cef
https://hg.mozilla.org/mozilla-central/rev/18c0e3805925
https://hg.mozilla.org/mozilla-central/rev/f58efdc21e14
https://hg.mozilla.org/mozilla-central/rev/008e20cf742a
https://hg.mozilla.org/mozilla-central/rev/bb5481ee141f
https://hg.mozilla.org/mozilla-central/rev/5c4418e37b13
https://hg.mozilla.org/mozilla-central/rev/ba58ecbd2b5b
https://hg.mozilla.org/mozilla-central/rev/4a34004947d1
https://hg.mozilla.org/mozilla-central/rev/36c6535a6352
https://hg.mozilla.org/mozilla-central/rev/d32891bfb1b8
https://hg.mozilla.org/mozilla-central/rev/4a61bea80168
https://hg.mozilla.org/mozilla-central/rev/7f1f7cf07362
https://hg.mozilla.org/mozilla-central/rev/d1a83f27351b
https://hg.mozilla.org/mozilla-central/rev/93e83b5951dc
https://hg.mozilla.org/mozilla-central/rev/5cb8bcab09cc
https://hg.mozilla.org/mozilla-central/rev/de5cf405c384
https://hg.mozilla.org/mozilla-central/rev/b69deb240d9d
https://hg.mozilla.org/mozilla-central/rev/c8cf78ad3f7f
https://hg.mozilla.org/mozilla-central/rev/f861388b829f
https://hg.mozilla.org/mozilla-central/rev/db019bcfd0ad
https://hg.mozilla.org/mozilla-central/rev/0741061e86e7
https://hg.mozilla.org/mozilla-central/rev/c67860b9a9d3
https://hg.mozilla.org/mozilla-central/rev/2e4ea5c2020e
https://hg.mozilla.org/mozilla-central/rev/3cfd3750c162
https://hg.mozilla.org/mozilla-central/rev/02d62b51156a
https://hg.mozilla.org/mozilla-central/rev/0e42b9f6b7d3
https://hg.mozilla.org/mozilla-central/rev/251aa9072a17
https://hg.mozilla.org/mozilla-central/rev/20c33662a25e
https://hg.mozilla.org/mozilla-central/rev/592b31342f74
https://hg.mozilla.org/mozilla-central/rev/17b6856efdbe
https://hg.mozilla.org/mozilla-central/rev/873935822ede
https://hg.mozilla.org/mozilla-central/rev/0dd558c975f0
https://hg.mozilla.org/mozilla-central/rev/9efbdb63ecbe
https://hg.mozilla.org/mozilla-central/rev/632f21df7be1
https://hg.mozilla.org/mozilla-central/rev/614fb1e40f6c
https://hg.mozilla.org/mozilla-central/rev/c65d59d33aa8
Comment 304 Gregory Szorc [:gps] 2013-03-02 13:32:16 PST
https://hg.mozilla.org/mozilla-central/rev/0f0745beec38
Comment 305 Ivaylo Dimitrov 2013-03-04 01:50:00 PST
xulrunner/installer does not get build on linux after those changes landed in m-c and I can't find an obvious way besides adding "DIRS += ['installer']" to "xulrunner/moz.build" to overcome that.
Comment 306 Ed Morley [:emorley] 2013-03-05 06:22:09 PST
*** Bug 666439 has been marked as a duplicate of this bug. ***
Comment 307 Gregory Szorc [:gps] 2013-03-20 18:50:51 PDT
*** Bug 389393 has been marked as a duplicate of this bug. ***
Comment 308 Gregory Szorc [:gps] 2013-03-20 18:52:26 PDT
*** Bug 416982 has been marked as a duplicate of this bug. ***
Comment 309 Gregory Szorc [:gps] 2013-05-16 14:12:50 PDT
*** Bug 579494 has been marked as a duplicate of this bug. ***

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