Closed Bug 784841 Opened 12 years ago Closed 11 years ago

Move DIRS logic out of Makefiles; eliminate allmakefiles

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: gps, Assigned: gps)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(61 files, 50 obsolete files)

5.53 KB, patch
k0scist
: review+
Details | Diff | Splinter Review
14.79 KB, patch
ted
: review+
glandium
: review+
Details | Diff | Splinter Review
99.68 KB, patch
ted
: review+
glandium
: review+
Details | Diff | Splinter Review
4.86 KB, patch
ted
: review+
Details | Diff | Splinter Review
8.91 KB, patch
ted
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
9.19 KB, patch
ted
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
51.79 KB, patch
glandium
: review+
Details | Diff | Splinter Review
5.84 KB, patch
glandium
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
12.71 KB, patch
glandium
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
17.98 KB, patch
glandium
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
16.84 KB, patch
glandium
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
9.60 KB, patch
jrmuizel
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
11.69 KB, patch
jrmuizel
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
24.52 KB, patch
ted
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
9.25 KB, patch
ted
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
15.31 KB, patch
glandium
: review+
Details | Diff | Splinter Review
22.34 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
33.49 KB, patch
glandium
: review+
Details | Diff | Splinter Review
23.49 KB, patch
glandium
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
2.36 KB, patch
khuey
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
12.41 KB, patch
glandium
: review+
Details | Diff | Splinter Review
22.20 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
10.34 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
3.38 KB, patch
glandium
: review+
Details | Diff | Splinter Review
9.35 KB, patch
glandium
: review+
Details | Diff | Splinter Review
12.33 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
2.55 KB, patch
glandium
: review+
Details | Diff | Splinter Review
13.42 KB, patch
ted
: review+
Details | Diff | Splinter Review
5.87 KB, patch
ted
: review+
Details | Diff | Splinter Review
4.16 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
4.48 KB, patch
glandium
: review+
Details | Diff | Splinter Review
9.68 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
27.97 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
10.21 KB, patch
ted
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
9.65 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
137.68 KB, patch
mossop
: feedback+
Details | Diff | Splinter Review
10.85 KB, patch
glandium
: review+
Details | Diff | Splinter Review
12.24 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
6.60 KB, patch
ted
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
19.33 KB, patch
ted
: review+
Details | Diff | Splinter Review
11.48 KB, patch
glandium
: review+
Details | Diff | Splinter Review
41.54 KB, patch
ted
: review+
Details | Diff | Splinter Review
10.63 KB, patch
ted
: review+
Details | Diff | Splinter Review
17.94 KB, patch
ted
: review+
Details | Diff | Splinter Review
11.12 KB, patch
ted
: review+
Details | Diff | Splinter Review
10.77 KB, patch
ted
: review+
Details | Diff | Splinter Review
11.57 KB, patch
ted
: review+
Details | Diff | Splinter Review
18.90 KB, patch
ted
: review+
Details | Diff | Splinter Review
17.44 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.80 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.01 KB, patch
glandium
: review+
Details | Diff | Splinter Review
14.40 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
11.36 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
20.01 KB, patch
Details | Diff | Splinter Review
11.38 KB, patch
Details | Diff | Splinter Review
123.53 KB, patch
Ms2ger
: feedback+
Details | Diff | Splinter Review
1.13 KB, patch
glandium
: review+
Details | Diff | Splinter Review
29.39 KB, patch
ted
: review+
glandium
: review+
Details | Diff | Splinter Review
91.23 KB, patch
jaws
: review+
Details | Diff | Splinter Review
69.93 KB, patch
ted
: review+
Details | Diff | Splinter Review
20.93 KB, patch
glandium
: review+
Details | Diff | Splinter Review
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.
Attached patch Conversion strawman, v1 (obsolete) — Splinter Review
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.
Attachment #657629 - Flags: feedback?(ted.mielczarek)
Attachment #657629 - Flags: feedback?(mh+mozilla)
Attachment #657629 - Flags: feedback?(khuey)
Attachment #657629 - Flags: feedback?(benjamin)
It would be easier if the interesting code was separated from the Makefile.in/mozbuild dance.
(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 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
Attached patch Just the Python bits (obsolete) — Splinter Review
There is also a rules.mk change in there. It's horribly broken but shows what I was thinking of.
(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.
(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.
(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.
(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.
Blocks: 774572
On another note: I noticed "eliminate allmakefiles" in the summary of this bug...
Assignee: nobody → gps
(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.
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.
> 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.
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 :)
Attachment #659417 - Flags: review?(jhammel)
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.
Attachment #659547 - Flags: review?(ted.mielczarek)
Attachment #659547 - Flags: review?(mh+mozilla)
Attachment #659547 - Flags: review?(khuey)
Attachment #659547 - Flags: review?(benjamin)
Attachment #659547 - Flags: feedback?(jhammel)
Attachment #657629 - Flags: feedback?(ted.mielczarek)
Attachment #657629 - Flags: feedback?(mh+mozilla)
Attachment #657629 - Flags: feedback?(khuey)
Attachment #657629 - Flags: feedback?(benjamin)
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.
Depends on: 602908
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.
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.
(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.
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.
Attachment #659547 - Attachment is obsolete: true
Attachment #659547 - Flags: review?(ted.mielczarek)
Attachment #659547 - Flags: review?(mh+mozilla)
Attachment #659547 - Flags: review?(khuey)
Attachment #659547 - Flags: review?(benjamin)
Attachment #659547 - Flags: feedback?(jhammel)
Attachment #659604 - Flags: review?(ted.mielczarek)
Attachment #659604 - Flags: review?(mh+mozilla)
Attachment #659604 - Flags: review?(benjamin)
Attachment #659604 - Flags: feedback?(jhammel)
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)
Attachment #659417 - Flags: review?(jhammel) → review+
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 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'
Attachment #659604 - Flags: feedback?(jhammel) → feedback+
(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.
s/interpretter/interpreter/g, please.
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.
Attachment #659604 - Flags: review?(benjamin)
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.
Attachment #659604 - Flags: review?(ted.mielczarek) → review+
> > +        # 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.
That was what I had in mind as well.
(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.
I suspect we could probably put all TEST_DIRS into TOOL_DIRS and not break anything.
(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!
(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".
Blocks: 800635
No longer blocks: 800635
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.
Attachment #659604 - Attachment is obsolete: true
Attachment #659604 - Flags: review?(mh+mozilla)
Attachment #670718 - Flags: review?(ted.mielczarek)
Attachment #670718 - Flags: review?(mh+mozilla)
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?).
(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.
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.)
> 
> > 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
(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?
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.
Attachment #671000 - Flags: review?(ted.mielczarek)
Attachment #671000 - Flags: review?(mh+mozilla)
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,"
Incorporated Ms2ger's feedback. Minor code cleanup.
Attachment #670718 - Attachment is obsolete: true
Attachment #670718 - Flags: review?(ted.mielczarek)
Attachment #670718 - Flags: review?(mh+mozilla)
Attachment #671150 - Flags: review?(ted.mielczarek)
Attachment #671150 - Flags: review?(mh+mozilla)
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?
Attachment #671150 - Flags: review?(ted) → review+
Attachment #671000 - Flags: review?(ted) → review+
(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.
Blocks: 818246
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
Attachment #671150 - Flags: review?(mh+mozilla) → review+
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*
Attachment #671000 - Flags: review?(mh+mozilla) → review+
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...
(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...
Attached patch Makefile conversion (obsolete) — Splinter Review
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).
Attachment #657629 - Attachment is obsolete: true
Attachment #657695 - Attachment is obsolete: true
Attachment #694664 - Flags: feedback?(ted)
Attachment #694664 - Flags: feedback?(mh+mozilla)
Attachment #694664 - Flags: feedback?(Ms2ger)
Depends on: 778236
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 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.
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.
Attachment #695090 - Flags: review?(ted)
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.
Attachment #694664 - Attachment is obsolete: true
Attachment #694664 - Flags: feedback?(ted)
Attachment #694664 - Flags: feedback?(mh+mozilla)
Attachment #694664 - Flags: feedback?(Ms2ger)
Attachment #695107 - Flags: review?(ted)
Attachment #695108 - Flags: review?(ted)
Attached patch Part 4d: Convert /toolkit, v1 (obsolete) — Splinter Review
Attachment #695112 - Flags: review?(ted)
Attached patch Part 4e: Convert /browser, v1 (obsolete) — Splinter Review
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...
Attachment #695113 - Flags: review?(ted)
Attachment #695113 - Flags: feedback?(gavin.sharp)
Attachment #695114 - Flags: review?(mh+mozilla)
Attachment #695115 - Flags: review?(mh+mozilla)
Attached patch Part 4h: Convert /dom, v1 (obsolete) — Splinter Review
This will definitely need a v2 to address the autogenerated Makefile.in files. The bulk of the review should be fine to perform.
Attachment #695116 - Flags: review?(khuey)
Attachment #695117 - Flags: review?(mh+mozilla)
Attachment #695118 - Flags: review?(mh+mozilla)
Attachment #695119 - Flags: review?(mh+mozilla)
Attachment #695120 - Flags: review?(ted)
Attachment #695122 - Flags: review?(ted)
Attachment #695123 - Flags: review?(ted)
Attachment #695126 - Flags: review?(ted)
Attachment #695127 - Flags: review?(mh+mozilla)
Attachment #695128 - Flags: review?(khuey)
Attachment #695129 - Flags: review?(mh+mozilla)
Attachment #695130 - Flags: review?(mh+mozilla)
Averaging out khuey's patch sizes :)
Attachment #695133 - Flags: review?(khuey)
Attachment #695134 - Flags: review?(ted)
Attached patch Part 4v: Convert /mobile/xul, v1 (obsolete) — Splinter Review
Attachment #695135 - Flags: review?(ted)
Attachment #695136 - Flags: review?(mh+mozilla)
Attachment #695137 - Flags: review?(ted)
Attachment #695138 - Flags: review?(khuey)
Attachment #695140 - Flags: review?(mh+mozilla)
Attachment #695142 - Flags: review?(mh+mozilla)
Attached patch Part 4β: Convert /security, v1 (obsolete) — Splinter Review
Attachment #695143 - Flags: review?(mh+mozilla)
Attachment #695144 - Flags: review?(ted)
Attachment #695145 - Flags: review?(mh+mozilla)
Attachment #695146 - Flags: review?(ted)
Attachment #695147 - Flags: review?(ted)
Attachment #695148 - Flags: review?(ted)
Attachment #695149 - Flags: review?(mh+mozilla)
Attachment #695150 - Flags: review?(ted)
Attachment #695151 - Flags: review?(khuey)
Attached patch Part 4λ: Convert /xpfe, v1 (obsolete) — Splinter Review
Attachment #695152 - Flags: review?(ted)
Attachment #695153 - Flags: review?(ted)
And we're done! Poor ξ.
Attachment #695154 - Flags: review?(ted)
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.
Attachment #695112 - Attachment is obsolete: true
Attachment #695112 - Flags: review?(ted)
Attachment #695155 - Flags: review?(ted)
Attachment #695155 - Flags: feedback?(dtownsend+bugmail)
Attachment #695107 - Flags: feedback+
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.
Attachment #695108 - Flags: feedback+
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
Attachment #695111 - Flags: feedback+
Attachment #695113 - Flags: feedback+
Attachment #695115 - Flags: feedback+
Since we require Python 2.7, we no longer need to implement os.path.relpath inside ConfigStatus.py.
Attachment #695271 - Flags: review?(mh+mozilla)
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)?
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.
Attachment #695282 - Flags: review?(ted)
Attachment #695282 - Flags: review?(mh+mozilla)
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.
Attachment #695283 - Flags: review?(ted)
(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.
(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.
(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.
> 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.
> 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 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.
Attachment #695271 - Flags: review?(mh+mozilla) → review+
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.
Attachment #695282 - Flags: review?(mh+mozilla) → feedback-
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.
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).
Attached patch Some fixes (obsolete) — Splinter Review
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
Attachment #695314 - Flags: review?(gps)
(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.
Incorporated Ms2ger's patches (mainly adding security/manager to the mix).
Attachment #695143 - Attachment is obsolete: true
Attachment #695143 - Flags: review?(mh+mozilla)
Attachment #695324 - Flags: review?(ted)
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/).
Attachment #695113 - Flags: feedback?(gavin.sharp) → review+
Attachment #695117 - Flags: feedback+
Attachment #695118 - Flags: feedback+
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.
Attachment #695153 - Flags: feedback+
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.
Attachment #695282 - Attachment is obsolete: true
Attachment #695282 - Flags: review?(ted)
Attachment #695376 - Flags: review?(mh+mozilla)
Incorporated feedback from Ms2ger.
Attachment #695108 - Attachment is obsolete: true
Attachment #695108 - Flags: review?(ted)
Attachment #695379 - Flags: review?(ted)
Attachment #695376 - Attachment is patch: true
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.
Attachment #695314 - Flags: review?(gps) → feedback+
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).
Attachment #695314 - Attachment is obsolete: true
Fixes to |make check|.
Attachment #695379 - Attachment is obsolete: true
Attachment #695379 - Flags: review?(ted)
Attachment #695523 - Flags: review?(ted)
Ported over tests to mozbuild.
Attachment #695283 - Attachment is obsolete: true
Attachment #695283 - Flags: review?(ted)
Attachment #695524 - Flags: review?(ted)
Not requesting review yet because this needs test coverage. Posting it so everyone can see how external projects are handled.
error() is needed to abort termination. I figured I'd provide warning() as well. Not requesting review because this needs tests.
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.
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.
This hooks up moz.build files with config.status \o/. Not ready for review yet, mainly due to missing tests.
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.
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.
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
(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.)
(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...
And we have green builds everywhere except for Windows (2 minor Python unit test failures) \o/

https://tbpl.mozilla.org/?tree=Try&rev=128b2b1ea62a
(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.
No longer depends on: 778236
Attachment #695120 - Flags: review?(ted) → review+
Attachment #695122 - Flags: review?(ted) → review+
Attachment #695155 - Flags: feedback?(dtownsend+bugmail) → feedback+
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 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
(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 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).
Attachment #695114 - Flags: review?(mh+mozilla) → review+
Attachment #695115 - Flags: review?(mh+mozilla) → review+
Attachment #695117 - Flags: review?(mh+mozilla) → review+
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'
Attachment #695118 - Flags: review?(mh+mozilla) → review+
Attachment #695119 - Flags: review?(mh+mozilla) → review+
Attachment #695127 - Flags: review?(mh+mozilla) → review+
Attachment #695140 - Flags: review?(mh+mozilla) → review+
Attachment #695145 - Flags: review?(mh+mozilla) → review+
Attachment #695149 - Flags: review?(mh+mozilla) → review+
Attachment #695142 - Flags: review?(mh+mozilla) → review+
Attachment #695136 - Flags: review?(mh+mozilla) → review+
Attachment #695130 - Flags: review?(mh+mozilla) → review+
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.
Attachment #695129 - Flags: review?(mh+mozilla) → review+
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 :-/
Attachment #695376 - Flags: review?(mh+mozilla) → review-
Attachment #695090 - Flags: review?(ted) → review+
(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.
(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 :))
(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)
(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 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.
Attachment #695123 - Flags: feedback+
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
Attachment #695126 - Flags: feedback+
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.
Attachment #695128 - Flags: feedback+
Attachment #695133 - Flags: feedback+
Attachment #695138 - Flags: feedback+
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.
Attachment #695107 - Flags: review?(ted) → review+
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.
Attachment #695111 - Flags: review?(ted) → review+
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?
Attachment #695126 - Flags: review?(ted) → review+
Attachment #695123 - Flags: review?(ted) → review+
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.
Attachment #695147 - Flags: review?(ted) → review+
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.
Attachment #695146 - Flags: review?(ted) → review+
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.
Attachment #695153 - Flags: review?(ted) → review+
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.
Attachment #695324 - Flags: feedback+
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
Attachment #695154 - Flags: feedback+
Attachment #695152 - Attachment is obsolete: true
Attachment #695152 - Flags: review?(ted)
Attachment #698268 - Flags: review?(ted)
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
Attachment #695151 - Flags: feedback+
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
Attachment #695150 - Flags: feedback+
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
Attachment #695120 - Flags: feedback+
Attachment #695119 - Flags: feedback+
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
Attachment #695122 - Flags: feedback+
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
Attachment #695137 - Flags: feedback+
In the interest of reviewer sanity, I'm happy to treat Ms2ger's feedback+ as build config peer review+ on these patches.
Depends on: 827308
Depends on: 827310
Blocks: buildtup
Attachment #695130 - Flags: feedback+
Attachment #695144 - Flags: feedback+
Attachment #695148 - Flags: feedback+
Attachment #695523 - Attachment is obsolete: true
Attachment #695523 - Flags: review?(ted)
Attachment #698731 - Flags: review?(ted)
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
Attachment #698268 - Flags: feedback+
(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.
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 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).
(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.
Escaped "$PYTHON" and that's it. See previous comment.
Attachment #695376 - Attachment is obsolete: true
Attachment #702668 - Flags: review?(mh+mozilla)
(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.
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.
Whiteboard: [leave open]
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.
Attachment #695524 - Attachment is obsolete: true
Attachment #695524 - Flags: review?(ted)
Attachment #702677 - Flags: review?(ted)
Comment on attachment 702668 [details] [diff] [review]
Part 5: Require virtualenv to build SpiderMonkey, v3

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

Fair enough
Attachment #702668 - Flags: review?(mh+mozilla) → review+
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.
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.
Attachment #703178 - Flags: review?(ted)
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.
Hopeful patch for the BSD multiprocessing issue in bug 808280.
Thanks, that particular issue is now fixed for me
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?
Attachment #695525 - Attachment is obsolete: true
Attachment #703750 - Flags: review?(ted)
Now with more proper error messages and some test coverage.

warning() could use some more love. meh. Follow-up.
Attachment #695526 - Attachment is obsolete: true
Attachment #703770 - Flags: review?(ted)
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 :)
Attachment #695527 - Attachment is obsolete: true
Attachment #703780 - Flags: review?(ted)
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.
Attachment #695528 - Attachment is obsolete: true
Attachment #703793 - Flags: review?(ted)
(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 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
Attachment #695116 - Flags: feedback+
Attachment #702668 - Attachment description: Part 6: Require virtualenv to build SpiderMonkey, v3 → Part 5: Require virtualenv to build SpiderMonkey, v3
Attachment #702677 - Attachment description: Part 7: Move parts of ConfigStatus into mozbuild, v3 → Part 6: Move parts of ConfigStatus into mozbuild, v3
Changed the API slightly to facilitate future magic in the base class.
Attachment #703178 - Attachment is obsolete: true
Attachment #703178 - Flags: review?(ted)
Attachment #704172 - Flags: review?(ted)
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.
Attachment #704173 - Flags: review?(ted)
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.
Flags: needinfo?(mh+mozilla)
See above comment.
Flags: needinfo?(mshal)
This is a long shot... (see comment #187).
Flags: needinfo?(khuey)
> 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.
Status: ASSIGNED → NEW
Flags: needinfo?(mh+mozilla)
Status: NEW → ASSIGNED
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.
Attachment #704172 - Attachment is obsolete: true
Attachment #704172 - Flags: review?(ted)
Attachment #705045 - Flags: review?(ted)
More capture of input and output files for rules.mk.
Attachment #704173 - Attachment is obsolete: true
Attachment #704173 - Flags: review?(ted)
Attachment #705060 - Flags: review?(ted)
Rebased.
Attachment #703750 - Attachment is obsolete: true
Attachment #703750 - Flags: review?(ted)
Attachment #705063 - Flags: review?(ted)
Attachment #703770 - Attachment description: Part 9: Add warning and error functions to moz.build files, v2 → Part 10: Add warning and error functions to moz.build files, v2
Rebased.
Attachment #703780 - Attachment is obsolete: true
Attachment #703780 - Flags: review?(ted)
Attachment #705065 - Flags: review?(ted)
Rebased.
Attachment #703793 - Attachment is obsolete: true
Attachment #703793 - Flags: review?(ted)
Attachment #705067 - Flags: review?(ted)
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.
Attachment #695531 - Attachment is obsolete: true
Flags: needinfo?(mshal)
Flags: needinfo?(khuey)
Attachment #702677 - Flags: review?(ted) → review+
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.
Attachment #705045 - Flags: review?(ted) → review+
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.
Attachment #705060 - Flags: review?(ted) → review+
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.
Attachment #705063 - Flags: review?(ted) → review+
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.
Attachment #703770 - Flags: review?(ted) → review+
(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 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.
Attachment #705065 - Flags: review?(ted)
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".
Attachment #705067 - Flags: review?(ted) → review+
Attachment #698268 - Flags: review?(ted) → review+
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.
Attachment #698731 - Flags: review?(ted) → review+
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.
Attachment #695134 - Flags: review?(ted) → review-
Attachment #695134 - Attachment is obsolete: true
Attachment #707052 - Flags: review?(ted)
Attachment #695135 - Attachment is obsolete: true
Attachment #695135 - Flags: review?(ted)
Attachment #707053 - Flags: review?(ted)
(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.
(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.
(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!
(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 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.
https://hg.mozilla.org/projects/build-system/rev/11658ed5bc17

Fixed Windows test failure in part 11. Self-reviewed because trivial.
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]))
It looks like localized msvc messages bit me once again (following bug 587372 and bug 780430).
Attachment #708109 - Flags: review?(gps)
Forgot to import sys
Attachment #708109 - Attachment is obsolete: true
Attachment #708109 - Flags: review?(gps)
Attachment #708119 - Flags: review?(gps)
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.
Attachment #708109 - Attachment is obsolete: false
(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
Attachment #708119 - Attachment is obsolete: true
Attachment #708119 - Flags: review?(gps)
Attachment #708109 - Attachment is obsolete: true
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 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.
Attachment #705065 - Attachment is obsolete: true
Attachment #705067 - Attachment description: Part 12: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3 → Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3
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.
Attachment #708700 - Flags: review?(ted)
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.
Attachment #708700 - Flags: review?(ted) → review+
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
Attachment #695530 - Attachment is obsolete: true
Attachment #705068 - Attachment is obsolete: true
Attachment #708793 - Flags: review?(ted)
Attachment #708793 - Flags: review?(mh+mozilla)
Attachment #708793 - Flags: feedback?(joey)
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 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.
Attachment #708793 - Flags: review?(mh+mozilla)
(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.
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.
Attachment #708793 - Attachment is obsolete: true
Attachment #708793 - Flags: review?(ted)
Attachment #708793 - Flags: feedback?(joey)
Attachment #710040 - Flags: review?(ted)
Attachment #710040 - Flags: review?(mh+mozilla)
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.
Attachment #710043 - Flags: review?(mh+mozilla)
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
Attachment #710043 - Flags: review?(mh+mozilla) → review+
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
Attachment #710040 - Flags: review?(mh+mozilla) → review-
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
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
Attached patch Part 4h: Convert /dom, v2 (obsolete) — Splinter Review
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?
Attachment #695116 - Attachment is obsolete: true
Attachment #695116 - Flags: review?(khuey)
Attachment #715638 - Flags: review?(khuey)
Attachment #715638 - Flags: feedback?(Ms2ger)
Attached patch Part 4e: Convert /browser, v2 (obsolete) — Splinter Review
Removed bit rot. Mostly additions from /browser/metro.
Attachment #695113 - Attachment is obsolete: true
Attachment #715640 - Flags: review?(gavin.sharp)
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
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.
Attachment #715718 - Flags: review?(Ms2ger)
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...
Attachment #715740 - Flags: review?(Ms2ger)
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
Attachment #715718 - Flags: review?(Ms2ger) → review+
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 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.
Attachment #715640 - Flags: review?(gavin.sharp) → review+
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?
Attachment #715740 - Attachment is obsolete: true
Attachment #715740 - Flags: review?(Ms2ger)
Attachment #716666 - Flags: review?(Ms2ger)
Attachment #710043 - Attachment description: Part 14: Use absolute paths in config.status, v1 → Part 13: Use absolute paths in config.status, v1
Attachment #715718 - Attachment description: Part 13: 4-space indent for dom/imptests/*.py, v1 → Part 14: 4-space indent for dom/imptests/*.py, v1
Attachment #716666 - Attachment description: Part 14: Produce moz.build files for DOM implementation tests, v2 → Part 15: Produce moz.build files for DOM implementation tests, v2
Attachment #710040 - Attachment description: Part 13: Use moz.build files to build the tree, v2 → Part 16: Use moz.build files to build the tree, v2
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.)
Attachment #716666 - Flags: review?(Ms2ger) → review+
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.
Attachment #715638 - Attachment is obsolete: true
Attachment #715638 - Flags: review?(khuey)
Attachment #715638 - Flags: feedback?(Ms2ger)
Attachment #716806 - Flags: review?(khuey)
Attachment #716806 - Flags: feedback?(Ms2ger)
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...
> 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).
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.
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...
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 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?
Attachment #710040 - Flags: review?(ted) → review+
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.
Attachment #717420 - Flags: review?(mh+mozilla)
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+).
Attachment #717428 - Flags: review?(mh+mozilla)
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.
Flags: needinfo?(VYV03354)
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?
Flags: needinfo?(VYV03354)
(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.
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

$
$ 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.
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

$
I needed this patch to build the tree.
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.
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...
Flags: needinfo?(VYV03354)
 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.
Flags: needinfo?(VYV03354)
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?
We already build on 2.7 everywhere. We'll work around the <2.7.3 in patches.
No longer depends on: 602908
(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.
Blocks: 844509
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.
Attachment #717428 - Attachment is obsolete: true
Attachment #717479 - Attachment is obsolete: true
Attachment #717428 - Flags: review?(mh+mozilla)
Attachment #717545 - Flags: review?(ted)
Attachment #717545 - Flags: review?(mh+mozilla)
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?
Attachment #716806 - Flags: feedback?(Ms2ger) → feedback+
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.
try-pushed file semms to be correct.
https://hg.mozilla.org/try/raw-rev/ddff1be65f2a
I accidentally uploaded the previous patch from the wrong VM. #firstworldproblems.
Attachment #717545 - Attachment is obsolete: true
Attachment #717545 - Flags: review?(ted)
Attachment #717545 - Flags: review?(mh+mozilla)
Attachment #717547 - Flags: review?(ted)
Attachment #717547 - Flags: review?(mh+mozilla)
Honeybadger don't care.
No longer depends on: 827310
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.
Attachment #717547 - Flags: review?(ted)
Attachment #717547 - Flags: review?(mh+mozilla)
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.
Attachment #717547 - Attachment is obsolete: true
Attachment #717564 - Flags: review?(ted)
Attachment #717564 - Flags: review?(mh+mozilla)
I should have read Ms2ger's review comment. This fixes rules.mk not rejecting Makefiles still containing DIRS variables.
Attachment #717564 - Attachment is obsolete: true
Attachment #717564 - Flags: review?(ted)
Attachment #717564 - Flags: review?(mh+mozilla)
Attachment #717566 - Flags: review?(ted)
Attachment #717566 - Flags: review?(mh+mozilla)
(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 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...
Attachment #717566 - Flags: review?(ted)
Attachment #717566 - Flags: review?(mh+mozilla)
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.
Attachment #717566 - Attachment is obsolete: true
Attachment #717574 - Flags: review?(ted)
Attachment #717574 - Flags: review?(mh+mozilla)
Please intradiff against v2 and verify I picked up the theme changes properly.
Attachment #715640 - Attachment is obsolete: true
Attachment #717575 - Flags: review?(jAwS)
Some new dirs from bug 767231 seem to have been lost.
(In reply to :Ms2ger from comment #286)
> Some new dirs from bug 767231 seem to have been lost.

Fixed in my patch queue.
Blocks: 844635
Blocks: 844654
Blocks: 844655
Attachment #717420 - Flags: review?(mh+mozilla) → review+
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
Attachment #717574 - Flags: review?(mh+mozilla) → review+
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.
Attachment #717575 - Flags: review?(jAwS) → review+
Attachment #717574 - Flags: review?(ted) → review+
Attachment #695128 - Flags: review?(khuey)
(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
Attachment #695138 - Flags: review?(khuey)
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.
Attachment #718007 - Flags: review?(mh+mozilla)
Attachment #716806 - Flags: review?(khuey)
Attachment #707053 - Flags: review?(ted) → review+
Attachment #707052 - Flags: review?(ted) → review+
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..!
(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.
Attachment #695151 - Flags: review?(khuey)
Comment on attachment 695155 [details] [diff] [review]
Part 4d: Convert /toolkit, v2

Mossop also looked at this way bay when IIRC.
Attachment #695155 - Flags: review?(ted)
Attachment #695533 - Attachment is obsolete: true
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
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 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.
Attachment #718007 - Flags: review?(mh+mozilla) → review+
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!
Blocks: 845089
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.
Attachment #718280 - Flags: review?(ted)
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.
Attachment #718280 - Attachment is obsolete: true
Attachment #718280 - Flags: review?(ted)
Attachment #719306 - Flags: review?(ted)
Attachment #719306 - Flags: review?(mh+mozilla)
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 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.
Attachment #719306 - Flags: review?(ted)
Attachment #719306 - Flags: review?(mh+mozilla)
Attachment #719306 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla22
Blocks: 846425
Blocks: 846463
Depends on: 846523
Blocks: 846524
Depends on: 846609
Attachment #710040 - Attachment is obsolete: true
Blocks: 845247
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.
Depends on: 847369
Depends on: 847382
Depends on: 849668
Blocks: 851975
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/730d4b16a116
Part 18ξ: Convert /xpfe; f=Ms2ger rs=ted
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.