Move DIRS logic out of Makefiles; eliminate allmakefiles

RESOLVED FIXED in mozilla22

Status

RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

Trunk
mozilla22
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(61 attachments, 50 obsolete attachments)

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
(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Posted 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
(Assignee)

Comment 5

7 years ago
Posted 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.
(Assignee)

Comment 6

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
(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.
(Assignee)

Updated

7 years ago
Blocks: 774572
On another note: I noticed "eliminate allmakefiles" in the summary of this bug...
Assignee: nobody → gps
(Assignee)

Comment 11

7 years ago
(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.
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 14

7 years ago
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)
(Assignee)

Comment 15

7 years ago
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)
(Assignee)

Updated

7 years ago
Attachment #657629 - Flags: feedback?(ted.mielczarek)
Attachment #657629 - Flags: feedback?(mh+mozilla)
Attachment #657629 - Flags: feedback?(khuey)
Attachment #657629 - Flags: feedback?(benjamin)
(Assignee)

Comment 16

7 years ago
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.
(Assignee)

Comment 19

7 years ago
(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.
(Assignee)

Comment 20

7 years ago
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 21

7 years ago
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+

Comment 22

7 years ago
Also, in the general sense, we might want to start moving out these generic items to packages at the /python level (or mozbase, etc)

Comment 23

7 years ago
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+
(Assignee)

Comment 24

7 years ago
(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.
(Assignee)

Comment 32

7 years ago
(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
(Assignee)

Updated

7 years ago
No longer blocks: 800635

Updated

7 years ago
Duplicate of this bug: 800635
(Assignee)

Comment 35

7 years ago
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)
(Assignee)

Comment 36

7 years ago
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.)

Comment 39

7 years ago
> 
> > 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?
(Assignee)

Comment 41

7 years ago
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,"
(Assignee)

Comment 43

7 years ago
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+
(Assignee)

Comment 45

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

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

I would like to glue in an HTML renderer at some point. As soon as we add an RST -> HTML module in the tree, the mach command could emit HTML and open up your web browser automatically (http://docs.python.org/2/library/webbrowser.html). We could even post HTML to MDN via their new publish API.
Comment 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...
(Assignee)

Comment 49

6 years ago
(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...
(Assignee)

Comment 50

6 years ago
Posted 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)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 53

6 years ago
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)
(Assignee)

Comment 54

6 years ago
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)
(Assignee)

Comment 55

6 years ago
Attachment #695108 - Flags: review?(ted)
(Assignee)

Comment 57

6 years ago
Posted patch Part 4d: Convert /toolkit, v1 (obsolete) — Splinter Review
Attachment #695112 - Flags: review?(ted)
(Assignee)

Comment 58

6 years ago
Posted 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)
(Assignee)

Comment 59

6 years ago
Attachment #695114 - Flags: review?(mh+mozilla)
(Assignee)

Comment 60

6 years ago
Attachment #695115 - Flags: review?(mh+mozilla)
(Assignee)

Comment 61

6 years ago
Posted 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)
(Assignee)

Comment 62

6 years ago
Attachment #695117 - Flags: review?(mh+mozilla)
(Assignee)

Comment 63

6 years ago
Attachment #695118 - Flags: review?(mh+mozilla)
(Assignee)

Comment 64

6 years ago
Attachment #695119 - Flags: review?(mh+mozilla)
(Assignee)

Comment 65

6 years ago
Attachment #695120 - Flags: review?(ted)
(Assignee)

Comment 66

6 years ago
Attachment #695122 - Flags: review?(ted)
(Assignee)

Comment 67

6 years ago
Attachment #695123 - Flags: review?(ted)
(Assignee)

Comment 68

6 years ago
Attachment #695126 - Flags: review?(ted)
(Assignee)

Comment 69

6 years ago
Attachment #695127 - Flags: review?(mh+mozilla)
(Assignee)

Comment 70

6 years ago
Attachment #695128 - Flags: review?(khuey)
(Assignee)

Comment 71

6 years ago
Attachment #695129 - Flags: review?(mh+mozilla)
(Assignee)

Comment 72

6 years ago
Attachment #695130 - Flags: review?(mh+mozilla)
(Assignee)

Comment 73

6 years ago
Averaging out khuey's patch sizes :)
Attachment #695133 - Flags: review?(khuey)
(Assignee)

Comment 74

6 years ago
Attachment #695134 - Flags: review?(ted)
(Assignee)

Comment 75

6 years ago
Attachment #695135 - Flags: review?(ted)
(Assignee)

Comment 76

6 years ago
Attachment #695136 - Flags: review?(mh+mozilla)
(Assignee)

Comment 77

6 years ago
Attachment #695137 - Flags: review?(ted)
(Assignee)

Comment 78

6 years ago
Attachment #695138 - Flags: review?(khuey)
(Assignee)

Comment 79

6 years ago
Attachment #695140 - Flags: review?(mh+mozilla)
(Assignee)

Comment 80

6 years ago
Attachment #695142 - Flags: review?(mh+mozilla)
(Assignee)

Comment 81

6 years ago
Attachment #695143 - Flags: review?(mh+mozilla)
(Assignee)

Comment 82

6 years ago
Attachment #695144 - Flags: review?(ted)
(Assignee)

Comment 83

6 years ago
Attachment #695145 - Flags: review?(mh+mozilla)
(Assignee)

Comment 84

6 years ago
Attachment #695146 - Flags: review?(ted)
(Assignee)

Comment 85

6 years ago
Attachment #695147 - Flags: review?(ted)
(Assignee)

Comment 86

6 years ago
Attachment #695148 - Flags: review?(ted)
(Assignee)

Comment 87

6 years ago
Attachment #695149 - Flags: review?(mh+mozilla)
(Assignee)

Comment 88

6 years ago
Attachment #695150 - Flags: review?(ted)
(Assignee)

Comment 89

6 years ago
Attachment #695151 - Flags: review?(khuey)
(Assignee)

Comment 90

6 years ago
Posted patch Part 4λ: Convert /xpfe, v1 (obsolete) — Splinter Review
Attachment #695152 - Flags: review?(ted)
(Assignee)

Comment 91

6 years ago
Attachment #695153 - Flags: review?(ted)
(Assignee)

Comment 92

6 years ago
And we're done! Poor ξ.
Attachment #695154 - Flags: review?(ted)
(Assignee)

Comment 93

6 years ago
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)
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+
(Assignee)

Comment 96

6 years ago
Since we require Python 2.7, we no longer need to implement os.path.relpath inside ConfigStatus.py.
Attachment #695271 - Flags: review?(mh+mozilla)
(Assignee)

Comment 97

6 years ago
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)?
(Assignee)

Comment 98

6 years ago
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)
(Assignee)

Comment 99

6 years ago
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.
(Assignee)

Comment 101

6 years ago
(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).
Posted 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)
(Assignee)

Comment 110

6 years ago
(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.
(Assignee)

Comment 111

6 years ago
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+
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+
(Assignee)

Comment 114

6 years ago
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)
(Assignee)

Comment 115

6 years ago
Incorporated feedback from Ms2ger.
Attachment #695108 - Attachment is obsolete: true
Attachment #695108 - Flags: review?(ted)
Attachment #695379 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
Attachment #695376 - Attachment is patch: true
(Assignee)

Comment 116

6 years ago
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+
(Assignee)

Comment 117

6 years ago
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
(Assignee)

Comment 118

6 years ago
Fixes to |make check|.
Attachment #695379 - Attachment is obsolete: true
Attachment #695379 - Flags: review?(ted)
Attachment #695523 - Flags: review?(ted)
(Assignee)

Comment 119

6 years ago
Ported over tests to mozbuild.
Attachment #695283 - Attachment is obsolete: true
Attachment #695283 - Flags: review?(ted)
Attachment #695524 - Flags: review?(ted)
(Assignee)

Comment 120

6 years ago
Not requesting review yet because this needs test coverage. Posting it so everyone can see how external projects are handled.
(Assignee)

Comment 121

6 years ago
error() is needed to abort termination. I figured I'd provide warning() as well. Not requesting review because this needs tests.
(Assignee)

Comment 122

6 years ago
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.
(Assignee)

Comment 123

6 years ago
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.
(Assignee)

Comment 124

6 years ago
This hooks up moz.build files with config.status \o/. Not ready for review yet, mainly due to missing tests.
(Assignee)

Comment 125

6 years ago
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.
(Assignee)

Comment 126

6 years ago
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.
(Assignee)

Comment 127

6 years ago
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.)
(Assignee)

Comment 129

6 years ago
(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...
(Assignee)

Comment 130

6 years ago
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.
(Assignee)

Updated

6 years ago
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+
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
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
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.
(Assignee)

Comment 164

6 years ago
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).
(Assignee)

Comment 166

6 years ago
(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.
(Assignee)

Comment 167

6 years ago
Escaped "$PYTHON" and that's it. See previous comment.
Attachment #695376 - Attachment is obsolete: true
Attachment #702668 - Flags: review?(mh+mozilla)
(Assignee)

Comment 168

6 years ago
(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.
(Assignee)

Comment 169

6 years ago
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]
(Assignee)

Comment 170

6 years ago
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+
(Assignee)

Comment 172

6 years ago
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.
(Assignee)

Comment 174

6 years ago
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.
(Assignee)

Comment 177

6 years ago
Hopeful patch for the BSD multiprocessing issue in bug 808280.
Thanks, that particular issue is now fixed for me
(Assignee)

Comment 179

6 years ago
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)
(Assignee)

Comment 180

6 years ago
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)
(Assignee)

Comment 181

6 years ago
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)
(Assignee)

Comment 182

6 years ago
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+
(Assignee)

Updated

6 years ago
Attachment #702668 - Attachment description: Part 6: Require virtualenv to build SpiderMonkey, v3 → Part 5: Require virtualenv to build SpiderMonkey, v3
(Assignee)

Updated

6 years ago
Attachment #702677 - Attachment description: Part 7: Move parts of ConfigStatus into mozbuild, v3 → Part 6: Move parts of ConfigStatus into mozbuild, v3
(Assignee)

Comment 185

6 years ago
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)
(Assignee)

Comment 186

6 years ago
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)
(Assignee)

Comment 187

6 years ago
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)
(Assignee)

Comment 188

6 years ago
See above comment.
Flags: needinfo?(mshal)
(Assignee)

Comment 189

6 years ago
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
(Assignee)

Comment 191

6 years ago
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)
(Assignee)

Comment 192

6 years ago
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)
(Assignee)

Comment 193

6 years ago
Rebased.
Attachment #703750 - Attachment is obsolete: true
Attachment #703750 - Flags: review?(ted)
Attachment #705063 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 194

6 years ago
Rebased.
Attachment #703780 - Attachment is obsolete: true
Attachment #703780 - Flags: review?(ted)
Attachment #705065 - Flags: review?(ted)
(Assignee)

Comment 195

6 years ago
Rebased.
Attachment #703793 - Attachment is obsolete: true
Attachment #703793 - Flags: review?(ted)
Attachment #705067 - Flags: review?(ted)
(Assignee)

Comment 196

6 years ago
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.
(Assignee)

Comment 209

6 years ago
(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.
(Assignee)

Comment 210

6 years ago
(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!
(Assignee)

Comment 211

6 years ago
(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.
(Assignee)

Comment 212

6 years ago
Comment on attachment 705067 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3

Review of attachment 705067 [details]