Move DIRS logic out of Makefiles; eliminate allmakefiles

RESOLVED FIXED in mozilla22

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

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

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(61 attachments, 50 obsolete attachments)

5.53 KB, patch
Jeff Hammel
: 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+
Ms2ger
: checkin+
Details | Diff | Splinter Review
14.40 KB, patch
Ms2ger
: review+
Ms2ger
: checkin+
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

5 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

5 years ago
Created attachment 657629 [details] [diff] [review]
Conversion strawman, v1

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

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

There are no shortage of things to discuss:

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

Other known issues:

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

Other comments:

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

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

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

Let the bikeshedding begin.
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

5 years ago
Created attachment 657695 [details] [diff] [review]
Just the Python bits

There is also a rules.mk change in there. It's horribly broken but shows what I was thinking of.
(Assignee)

Comment 6

5 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

5 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

5 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

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

Comment 11

5 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

5 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

5 years ago
Created attachment 659417 [details] [diff] [review]
Part 1: Generic container classes, v1

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

This code has nothing build specific, so I'm giving to jhammel for review. If anyone with Python knowledge and review power wants to take this, I'm sure jhammel won't mind less work :)
Attachment #659417 - Flags: review?(jhammel)
(Assignee)

Comment 15

5 years ago
Created attachment 659547 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v1

Here we have the new core of the build system.

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

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

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

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

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

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

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

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

I've added jhammel for Python-specific feedback.

If anyone else wants to add feedback, please do. I like eyeballs.
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

5 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

5 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

5 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

5 years ago
Created attachment 659604 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v2

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

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

Also, dev-platform thread started regarding Python 2.6 vs 2.7.
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

5 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

5 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

5 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

5 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

5 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

5 years ago
No longer blocks: 800635
Duplicate of this bug: 800635
(Assignee)

Comment 35

5 years ago
Created attachment 670718 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v3

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

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

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

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

---

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

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

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


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

Defines directories constituting the tier traversal mechanism.

Type: OrderedDict
Default Value: OrderedDict()

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

---

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

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

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

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

I'm pretty happy with the state of this patch and barring any radical feedback from Mike Hommey (Ted already gave r+), I think I'll start giving parts 3+ some love and ready them for review.
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

5 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

5 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

5 years ago
Created attachment 671000 [details] [diff] [review]
Part 3: Convert executed sandboxes into data structures, v1

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

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

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

The end state of this patch effectively constitute the frontend portion of our new build system. We have a stream of objects represents the parsed state of all the moz.build files. The next set of patches will focus on doing something useful with this data.
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

5 years ago
Created attachment 671150 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v4

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

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

Updated

5 years ago
Blocks: 818246
Comment on attachment 671150 [details] [diff] [review]
Part 2: Implement Python sandboxing and tree reading, v4

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

rs=me

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

Missing word before passed?

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

interpreted
Attachment #671150 - Flags: review?(mh+mozilla) → review+
Comment on attachment 671000 [details] [diff] [review]
Part 3: Convert executed sandboxes into data structures, v1

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

rs=me

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

The set (...) constitute*s*
Attachment #671000 - Flags: review?(mh+mozilla) → review+
Question: how will we handle "third-party" sub directories? Currently, we're just putting them in DIRS like anything else, but that won't work for e.g. nsprpub, js/src, media/webrtc...
(Assignee)

Comment 49

5 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

5 years ago
Created attachment 694664 [details] [diff] [review]
Makefile conversion

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

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

I'll see about submitting the rest of the Python patches so people can start applying things locally and experimenting (I know Mike Shal is eager to start converting IDLs).
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

5 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

5 years ago
Created attachment 695090 [details] [diff] [review]
Part 2b: Add parameter to control whether to descend into referenced children, v1

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

I plan to fold this into part 2 on commit.
Attachment #695090 - Flags: review?(ted)
(Assignee)

Comment 54

5 years ago
Created attachment 695107 [details] [diff] [review]
Part 4a: Convert /b2g, v1

So it begins.

I've only partially tested all the patches I'm about to submit. I figure I can get eyes on the bulk of the work. Then, as issues are found, we can upload new revisions of just the subset that needs fixed.
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

5 years ago
Created attachment 695108 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v1
Attachment #695108 - Flags: review?(ted)
(Assignee)

Comment 56

5 years ago
Created attachment 695111 [details] [diff] [review]
Part 4c: Convert /memory, /mfbt, /mozglue, v1
Attachment #695111 - Flags: review?(ted)
(Assignee)

Comment 57

5 years ago
Created attachment 695112 [details] [diff] [review]
Part 4d: Convert /toolkit, v1
Attachment #695112 - Flags: review?(ted)
(Assignee)

Comment 58

5 years ago
Created attachment 695113 [details] [diff] [review]
Part 4e: Convert /browser, v1

Incorporated feedback from Ms2ger. Requesting Gavin to look at things because extra eyes is helpful. Actually, extra eyes on all these patches is a good thing...
Attachment #695113 - Flags: review?(ted)
Attachment #695113 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 59

5 years ago
Created attachment 695114 [details] [diff] [review]
Part 4f: Convert /content, v1
Attachment #695114 - Flags: review?(mh+mozilla)
(Assignee)

Comment 60

5 years ago
Created attachment 695115 [details] [diff] [review]
Part 4g: Convert /docshell, v1
Attachment #695115 - Flags: review?(mh+mozilla)
(Assignee)

Comment 61

5 years ago
Created attachment 695116 [details] [diff] [review]
Part 4h: Convert /dom, v1

This will definitely need a v2 to address the autogenerated Makefile.in files. The bulk of the review should be fine to perform.
Attachment #695116 - Flags: review?(khuey)
(Assignee)

Comment 62

5 years ago
Created attachment 695117 [details] [diff] [review]
Part 4i: Convert /editor, v1
Attachment #695117 - Flags: review?(mh+mozilla)
(Assignee)

Comment 63

5 years ago
Created attachment 695118 [details] [diff] [review]
Part 4j: Convert /embedding, v1
Attachment #695118 - Flags: review?(mh+mozilla)
(Assignee)

Comment 64

5 years ago
Created attachment 695119 [details] [diff] [review]
Part 4k: Convert /extensions, v1
Attachment #695119 - Flags: review?(mh+mozilla)
(Assignee)

Comment 65

5 years ago
Created attachment 695120 [details] [diff] [review]
Part 4l: Convert /gfx, v1
Attachment #695120 - Flags: review?(ted)
(Assignee)

Comment 66

5 years ago
Created attachment 695122 [details] [diff] [review]
Part 4m: Convert /image, v1
Attachment #695122 - Flags: review?(ted)
(Assignee)

Comment 67

5 years ago
Created attachment 695123 [details] [diff] [review]
Part 4n: Convert /intl, v1
Attachment #695123 - Flags: review?(ted)
(Assignee)

Comment 68

5 years ago
Created attachment 695126 [details] [diff] [review]
Part 4o: Convert /ipc, v1
Attachment #695126 - Flags: review?(ted)
(Assignee)

Comment 69

5 years ago
Created attachment 695127 [details] [diff] [review]
Part 4p: Convert /js, v1
Attachment #695127 - Flags: review?(mh+mozilla)
(Assignee)

Comment 70

5 years ago
Created attachment 695128 [details] [diff] [review]
Part 4q: Convert /layout, v1
Attachment #695128 - Flags: review?(khuey)
(Assignee)

Comment 71

5 years ago
Created attachment 695129 [details] [diff] [review]
Part 4r: Convert /media, v1
Attachment #695129 - Flags: review?(mh+mozilla)
(Assignee)

Comment 72

5 years ago
Created attachment 695130 [details] [diff] [review]
Part 4s: Convert /accessible, v1
Attachment #695130 - Flags: review?(mh+mozilla)
(Assignee)

Comment 73

5 years ago
Created attachment 695133 [details] [diff] [review]
Part 4t: Convert /caps, v1

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

Comment 74

5 years ago
Created attachment 695134 [details] [diff] [review]
Part 4u: Convert /mobile/android, v1
Attachment #695134 - Flags: review?(ted)
(Assignee)

Comment 75

5 years ago
Created attachment 695135 [details] [diff] [review]
Part 4v: Convert /mobile/xul, v1
Attachment #695135 - Flags: review?(ted)
(Assignee)

Comment 76

5 years ago
Created attachment 695136 [details] [diff] [review]
Part 4w: Convert /modules, v1
Attachment #695136 - Flags: review?(mh+mozilla)
(Assignee)

Comment 77

5 years ago
Created attachment 695137 [details] [diff] [review]
Part 4x: Convert /netwerk, v1
Attachment #695137 - Flags: review?(ted)
(Assignee)

Comment 78

5 years ago
Created attachment 695138 [details] [diff] [review]
Part 4y: Convert /parser, v1
Attachment #695138 - Flags: review?(khuey)
(Assignee)

Comment 79

5 years ago
Created attachment 695140 [details] [diff] [review]
Part 4z: Convert /profile, v1
Attachment #695140 - Flags: review?(mh+mozilla)
(Assignee)

Comment 80

5 years ago
Created attachment 695142 [details] [diff] [review]
Part 4α: Convert /rdf, v1
Attachment #695142 - Flags: review?(mh+mozilla)
(Assignee)

Comment 81

5 years ago
Created attachment 695143 [details] [diff] [review]
Part 4β: Convert /security, v1
Attachment #695143 - Flags: review?(mh+mozilla)
(Assignee)

Comment 82

5 years ago
Created attachment 695144 [details] [diff] [review]
Part 4γ: Convert /services, v1
Attachment #695144 - Flags: review?(ted)
(Assignee)

Comment 83

5 years ago
Created attachment 695145 [details] [diff] [review]
Part 4δ: Convert /storage, v1
Attachment #695145 - Flags: review?(mh+mozilla)
(Assignee)

Comment 84

5 years ago
Created attachment 695146 [details] [diff] [review]
Part 4ε: Convert /testing, v1
Attachment #695146 - Flags: review?(ted)
(Assignee)

Comment 85

5 years ago
Created attachment 695147 [details] [diff] [review]
Part 4ζ: Convert /tools, v1
Attachment #695147 - Flags: review?(ted)
(Assignee)

Comment 86

5 years ago
Created attachment 695148 [details] [diff] [review]
Part 4η: Convert /uriloader, v1
Attachment #695148 - Flags: review?(ted)
(Assignee)

Comment 87

5 years ago
Created attachment 695149 [details] [diff] [review]
Part 4θ: Convert /webapprt, v1
Attachment #695149 - Flags: review?(mh+mozilla)
(Assignee)

Comment 88

5 years ago
Created attachment 695150 [details] [diff] [review]
Part 4ι: Convert /widget, v1
Attachment #695150 - Flags: review?(ted)
(Assignee)

Comment 89

5 years ago
Created attachment 695151 [details] [diff] [review]
Part 4κ: Convert /xpcom, v1
Attachment #695151 - Flags: review?(khuey)
(Assignee)

Comment 90

5 years ago
Created attachment 695152 [details] [diff] [review]
Part 4λ: Convert /xpfe, v1
Attachment #695152 - Flags: review?(ted)
(Assignee)

Comment 91

5 years ago
Created attachment 695153 [details] [diff] [review]
Part 4μ: Convert /xulrunner, v1
Attachment #695153 - Flags: review?(ted)
(Assignee)

Comment 92

5 years ago
Created attachment 695154 [details] [diff] [review]
Part 4ν: Convert misc remaining, v1

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

Comment 93

5 years ago
Created attachment 695155 [details] [diff] [review]
Part 4d: Convert /toolkit, v2

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

Dave: Feel free to decline and/or redirect. Your participation is not required for landing. But, Toolkit is a large and complex module so extra eyeballs would be appreciated.
Attachment #695112 - Attachment is obsolete: true
Attachment #695112 - Flags: review?(ted)
Attachment #695155 - Flags: review?(ted)
Attachment #695155 - Flags: feedback?(dtownsend+bugmail)

Updated

5 years ago
Attachment #695107 - Flags: feedback+
Comment on attachment 695108 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v1

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

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

You'll want to remove all this too.
Attachment #695108 - Flags: feedback+
Comment on attachment 695111 [details] [diff] [review]
Part 4c: Convert /memory, /mfbt, /mozglue, v1

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

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

Is this one necessary?

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

MALLOC, not MALOC, and you lost the ifndef MOZ_NATIVE_JEMALLOC

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

We'll need to decide if we're going to do 2-space or 4-space indentation
Attachment #695111 - Flags: feedback+

Updated

5 years ago
Attachment #695113 - Flags: feedback+

Updated

5 years ago
Attachment #695115 - Flags: feedback+
(Assignee)

Comment 96

5 years ago
Created attachment 695271 [details] [diff] [review]
Part 5: Use relpath in ConfigStatus.py

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

Comment 97

5 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

5 years ago
Created attachment 695282 [details] [diff] [review]
Part 6: Require virtualenv to build SpiderMonkey, v1

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

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

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

If this patch is declined, it will make work in subsequent parts of this bug more difficult. It won't be impossible. But, code will be a little hackier than I'd like. e.g. we'd have to copy python/mozbuild and possibly python/mach into js/src. So, I hope the small number of people inconvenienced by this understand the larger needs of the overall build system. Of course, if anyone has suggestions for making the developer workflow for standalone SpiderMonkey builds easier, I'm all ears.
Attachment #695282 - Flags: review?(ted)
Attachment #695282 - Flags: review?(mh+mozilla)
(Assignee)

Comment 99

5 years ago
Created attachment 695283 [details] [diff] [review]
Part 7: Move parts of ConfigStatus into mozbuild, v1

The plan is to move core build system foo into the mozbuild Python package so mozbuild is self-contained and doesn't have to call out into other systems. Subsequent patches should make the overall direction clearer.
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

5 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).
Created attachment 695314 [details] [diff] [review]
Some fixes

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

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

https://tbpl.mozilla.org/?tree=Try&rev=1074e0f2d865
Attachment #695314 - Flags: review?(gps)
(Assignee)

Comment 110

5 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

5 years ago
Created attachment 695324 [details] [diff] [review]
Part 4β: Convert /security, v2

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+

Updated

5 years ago
Attachment #695117 - Flags: feedback+

Updated

5 years ago
Attachment #695118 - Flags: feedback+
Comment on attachment 695153 [details] [diff] [review]
Part 4μ: Convert /xulrunner, v1

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

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

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

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

You're losing the 

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

that's still left in toolkit/toolkit-tiers.mk.
Attachment #695153 - Flags: feedback+
(Assignee)

Comment 114

5 years ago
Created attachment 695376 [details] [diff] [review]
Part 6: Require virtualenv to build SpiderMonkey, v2

I refactored the approach a bit. In my opinion, the approach is a bit hacky. But, if someone runs a standalone SpiderMonkey configure from mozilla-central, it will find and use the virtualenv automatically. If there is a source distribution, we'll just need to add /build/virtualenv, /python, and other locations referenced by the virtualenv and things will "just work." I'm not aware of a source distribution. If it exists and the packaging code exists in m-c, I'll happily code up a patch and add it to this bug.
Attachment #695282 - Attachment is obsolete: true
Attachment #695282 - Flags: review?(ted)
Attachment #695376 - Flags: review?(mh+mozilla)
(Assignee)

Comment 115

5 years ago
Created attachment 695379 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v1

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

Updated

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

Comment 116

5 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

5 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

5 years ago
Created attachment 695523 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v2

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

Comment 119

5 years ago
Created attachment 695524 [details] [diff] [review]
Part 7: Move parts of ConfigStatus into mozbuild, v2

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

Comment 120

5 years ago
Created attachment 695525 [details] [diff] [review]
Part 8: Add external make variables to moz.build files, v1

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

Comment 121

5 years ago
Created attachment 695526 [details] [diff] [review]
Part 9: Add warning and error functions to moz.build files, v1

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

Comment 122

5 years ago
Created attachment 695527 [details] [diff] [review]
Part 10: Ability to declare tier directories as external, v1

Not requesting review because this needs tests. This /might/ be redundant with "static" tier directories. I'm not sure what exactly static tier directories are. Someone please enlighten me.
(Assignee)

Comment 123

5 years ago
Created attachment 695528 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v1

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

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

We /may/ want a better way to hook this up with config.status. I'll probably be poking glandium about things as soon as I see him around IRC.
(Assignee)

Comment 124

5 years ago
Created attachment 695530 [details] [diff] [review]
Part 12: Integrate with config.status, v1

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

Comment 125

5 years ago
Created attachment 695531 [details] [diff] [review]
Part 13: rules.mk integration, v1

This hooks up our auto-generated .mk files with rules.mk. I still need some rules to regenerate the moz.build-derived .mk files if the moz.build files have changed.
(Assignee)

Comment 126

5 years ago
Created attachment 695533 [details] [diff] [review]
Part 14: Remove allmakefiles.sh, v1

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

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

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

I'll try to get all patches to r? status by 2013. But, I also need to work on bug 718066, so I may be quiet on this bug for a little while. Ping me if you get nervous about not having enough turnaround time to finish reviews before 21 (Jan 7 I believe). We don't have to land on Jan 7. But, I'd sure like to board the train as soon as it arrives.
(Assignee)

Comment 127

5 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

5 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

5 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

5 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)

Comment 142

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

Updated

5 years ago
Attachment #695133 - Flags: feedback+

Updated

5 years ago
Attachment #695138 - Flags: feedback+
Comment on attachment 695107 [details] [diff] [review]
Part 4a: Convert /b2g, v1

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

Seems a bummer to have to put empty moz.build files in leaf dirs, but I guess it's a temporary measure anyway.
Attachment #695107 - Flags: review?(ted) → review+
Comment on attachment 695111 [details] [diff] [review]
Part 4c: Convert /memory, /mfbt, /mozglue, v1

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

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

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

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

I mentioned this elsewhere, but we should use 4-space indent in these files to follow PEP-8.
Attachment #695111 - Flags: review?(ted) → review+
Comment on attachment 695126 [details] [diff] [review]
Part 4o: Convert /ipc, v1

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

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

thinking out loud: should we use this formatting for all multiple-DIRS assignments?
Attachment #695126 - Flags: review?(ted) → review+
Attachment #695123 - Flags: review?(ted) → review+
Comment on attachment 695147 [details] [diff] [review]
Part 4ζ: Convert /tools, v1

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

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

note bug 823915

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

This code looks dead. We should just "hg rm tools/testy" instead.
Attachment #695147 - Flags: review?(ted) → review+
Comment on attachment 695146 [details] [diff] [review]
Part 4ε: Convert /testing, v1

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

It strikes me that testing/mochitest has a lot of extraneous directory structure.
Attachment #695146 - Flags: review?(ted) → review+
Comment on attachment 695153 [details] [diff] [review]
Part 4μ: Convert /xulrunner, v1

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

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

We should probably just `hg rm winEmbed` while we're at it.
Attachment #695153 - Flags: review?(ted) → review+
Comment on attachment 695324 [details] [diff] [review]
Part 4β: Convert /security, v2

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

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

I think this can be dropped.
Attachment #695324 - Flags: feedback+
Comment on attachment 695154 [details] [diff] [review]
Part 4ν: Convert misc remaining, v1

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

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

4δ (storage) would be a better place for this change
Attachment #695154 - Flags: feedback+
Created attachment 698268 [details] [diff] [review]
Part 4λ: Convert /xpfe, v2
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

Updated

5 years ago
Attachment #695120 - Flags: feedback+

Updated

5 years ago
Attachment #695119 - Flags: feedback+
Comment on attachment 695122 [details] [diff] [review]
Part 4m: Convert /image, v1

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

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

Drop the trailing empty line
Attachment #695122 - Flags: feedback+
Comment on attachment 695137 [details] [diff] [review]
Part 4x: Convert /netwerk, v1

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

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

LIBCONIC: https://hg.mozilla.org/users/Ms2ger_gmail.com/gps-patches/rev/7efc2b32072a#l3.1
Attachment #695137 - Flags: feedback+
In the interest of reviewer sanity, I'm happy to treat Ms2ger's feedback+ as build config peer review+ on these patches.

Updated

5 years ago
Depends on: 827308

Updated

5 years ago
Depends on: 827310

Updated

5 years ago
Blocks: 827343

Updated

5 years ago
Attachment #695130 - Flags: feedback+

Updated

5 years ago
Attachment #695144 - Flags: feedback+

Updated

5 years ago
Attachment #695148 - Flags: feedback+
Created attachment 698731 [details] [diff] [review]
Part 4b: Convert /, /build, /config, v3
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

5 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

5 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

5 years ago
Created attachment 702668 [details] [diff] [review]
Part 5: Require virtualenv to build SpiderMonkey, v3

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

Comment 168

5 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

5 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

5 years ago
Created attachment 702677 [details] [diff] [review]
Part 6: Move parts of ConfigStatus into mozbuild, v3

Moved unit tests as well. Added missing __init__.py file. This builds the tree fine locally! And, previous try builds seemed to work as well. Should be a pretty straightforward rubber stamp.
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

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

Updated

5 years ago
Blocks: 831236
(Assignee)

Comment 173

5 years ago
Ted gave IRC approval for landing the groundwork to the moz.build transition in m-c.

https://hg.mozilla.org/mozilla-central/rev/a9021c50ccf9
https://hg.mozilla.org/mozilla-central/rev/ec072cee0502
https://hg.mozilla.org/mozilla-central/rev/d8ea5b8be44d
https://hg.mozilla.org/mozilla-central/rev/ee04a251a5ef
https://hg.mozilla.org/mozilla-central/rev/d2cce982a7c8
Status: NEW → ASSIGNED
(Assignee)

Comment 174

5 years ago
Created attachment 703178 [details] [diff] [review]
Part 7: Recursive make backend, v1

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

Anyway, this patch has 2 major goals:

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

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

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

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

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

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

I'm not sure if I submitted this patch before. If I haven't, the major change since last time is we now have test coverage. It's pretty basic. But, it's better than nothing.
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.
See http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/637/steps/configure/logs/stdio for the full log.
(Assignee)

Comment 177

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

Comment 179

5 years ago
Created attachment 703750 [details] [diff] [review]
Part 8: Add external make variables to moz.build files, v2

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

  EXTERNAL_MAKE_DIRS
  PARALLEL_EXTERNAL_MAKE_DIRS

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

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

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

It's worth noting explicitly that we need these variables separate from DIRS and PARALLEL_DIRS because the moz.build reader uses DIRS and PARALLEL_DIRS to descend into linked files. This will error if there is no moz.build file present. We don't want to create moz.build files in external projects, so we introduce these "external" variables. Besides, a little extra metadata capture isn't too bad, is it?
Attachment #695525 - Attachment is obsolete: true
Attachment #703750 - Flags: review?(ted)
(Assignee)

Comment 180

5 years ago
Created attachment 703770 [details] [diff] [review]
Part 10: Add warning and error functions to moz.build files, v2

Now with more proper error messages and some test coverage.

warning() could use some more love. meh. Follow-up.
Attachment #695526 - Attachment is obsolete: true
Attachment #703770 - Flags: review?(ted)
(Assignee)

Comment 181

5 years ago
Created attachment 703780 [details] [diff] [review]
Part 10: Ability to declare tier directories as external, v2

Now with tests for RecursiveMakeBackend.

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

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

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

I /think/ behavior of external=True is very similar (possibly identical!) to "static" tier directories. If it is, this patch should be to change "static" so it doesn't incur moz.build processing. Or, we could just rubber stamp this and sort it all out later :)
Attachment #695527 - Attachment is obsolete: true
Attachment #703780 - Flags: review?(ted)
(Assignee)

Comment 182

5 years ago
Created attachment 703793 [details] [diff] [review]
Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v2

Now with tests!

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

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

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

The implementation is simple and the test coverage isn't excellent (there is a whole slew of path normalization we should probably do). We don't use this variable very often. I figure we can staple on additional checks later if it becomes a footgun.
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

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

Updated

5 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

5 years ago
Created attachment 704172 [details] [diff] [review]
Part 7: Recursive make backend, v2

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

5 years ago
Created attachment 704173 [details]
Part 12: Capture moz.build state when feeding into backend, v1

There are essentially two parts to this patch:

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

#2 will be used in the make rules to influence when we need to incur a build backend refresh. This is forthcoming in a subsequent patch.
Attachment #704173 - Flags: review?(ted)
(Assignee)

Comment 187

5 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

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

Comment 189

5 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

5 years ago
Created attachment 705045 [details] [diff] [review]
Part 7: Recursive make backend, v3

Decided to change up .mk generation a bit to facilitate better integration with rules.mk. Instead of dirs.mk, we now write out a single backend.mk file. This means buffering everything in memory until the entire tree metadata is consumed. I don't think it will be an issue.
Attachment #704172 - Attachment is obsolete: true
Attachment #704172 - Flags: review?(ted)
Attachment #705045 - Flags: review?(ted)
(Assignee)

Comment 192

5 years ago
Created attachment 705060 [details] [diff] [review]
Part 8: Capture moz.build state when feeding into backend, v2

More capture of input and output files for rules.mk.
Attachment #704173 - Attachment is obsolete: true
Attachment #704173 - Flags: review?(ted)
Attachment #705060 - Flags: review?(ted)
(Assignee)

Comment 193

5 years ago
Created attachment 705063 [details] [diff] [review]
Part 9: Add external make variables to moz.build files, v3

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

Updated

5 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

5 years ago
Created attachment 705065 [details] [diff] [review]
Part 11: Ability to declare tier directories as external, v3

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

Comment 195

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

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

Comment 196

5 years ago
Created attachment 705068 [details] [diff] [review]
Part 14: rules.mk integration, v2

Newer iteration of rules.mk integration. Not yet complete. Need to fix some tests. Hopefully this gives you an idea of where I think we're going.
Attachment #695531 - Attachment is obsolete: true
Flags: needinfo?(mshal)
Flags: needinfo?(khuey)
Attachment #695133 - Flags: review?(khuey) → review+
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 #695137 - Flags: review?(ted)
Attachment #695144 - Flags: review?(ted)
Attachment #695148 - Flags: review?(ted)
Attachment #695150 - Flags: review?(ted)
Attachment #695154 - Flags: review?(ted)
Attachment #695324 - Flags: review?(ted)
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-
Created attachment 707052 [details] [diff] [review]
Part 4u: Convert /mobile/android, v2
Attachment #695134 - Attachment is obsolete: true
Attachment #707052 - Flags: review?(ted)
Created attachment 707053 [details] [diff] [review]
Part 4v: Convert /mobile/xul, v2
Attachment #695135 - Attachment is obsolete: true
Attachment #695135 - Flags: review?(ted)
Attachment #707053 - Flags: review?(ted)
Attachment #695113 - 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

5 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

5 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

5 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

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

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

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

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

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

Changed locally.
(Assignee)

Comment 213

5 years ago
Parts 6 through 11 with review comments addressed.

https://hg.mozilla.org/projects/build-system/rev/5cc7c4038208
https://hg.mozilla.org/projects/build-system/rev/7c73b5af6247
https://hg.mozilla.org/projects/build-system/rev/115bf99a25db
https://hg.mozilla.org/projects/build-system/rev/9584298d2a6a
https://hg.mozilla.org/projects/build-system/rev/c013d07f7deb
https://hg.mozilla.org/projects/build-system/rev/385ef288dd0d
(Assignee)

Comment 214

5 years ago
https://hg.mozilla.org/projects/build-system/rev/11658ed5bc17

Fixed Windows test failure in part 11. Self-reviewed because trivial.
(Assignee)

Comment 215

5 years ago
Parts 6-11:

https://hg.mozilla.org/mozilla-central/rev/5cc7c4038208
https://hg.mozilla.org/mozilla-central/rev/7c73b5af6247
https://hg.mozilla.org/mozilla-central/rev/115bf99a25db
https://hg.mozilla.org/mozilla-central/rev/9584298d2a6a
https://hg.mozilla.org/mozilla-central/rev/c013d07f7deb
https://hg.mozilla.org/mozilla-central/rev/385ef288dd0d
https://hg.mozilla.org/mozilla-central/rev/11658ed5bc17
https://hg.mozilla.org/mozilla-central/rev/c638b856a171
https://hg.mozilla.org/mozilla-central/rev/677e87c11252
I'm unable to build the tree because of infinite error messages:

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

It looks like localized msvc messages bit me once again (following bug 587372 and bug 780430).
Attachment #708109 - Flags: review?(gps)
Created attachment 708119 [details] [diff] [review]
Use filesystem encoding to decode environment variables

Forgot to import sys
Attachment #708109 - Attachment is obsolete: true
Attachment #708109 - Flags: review?(gps)
Attachment #708119 - Flags: review?(gps)
(Assignee)

Comment 219

5 years ago
Comment on attachment 708109 [details] [diff] [review]
Use filesystem encoding to decode environment variables

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

I have an alternative idea: remove the "from __future__ import unicode_literals" from configenvironment.py. If that works for you, let's r+ and land in inbound.
Attachment #708109 - Attachment is obsolete: false
(In reply to Gregory Szorc [:gps] from comment #219)
> I have an alternative idea: remove the "from __future__ import
> unicode_literals" from configenvironment.py. If that works for you, let's r+
> and land in inbound.

It appears that it works. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/7faec6303aa9
Attachment #708119 - Attachment is obsolete: true
Attachment #708119 - Flags: review?(gps)
Attachment #708109 - Attachment is obsolete: true
With my tree on

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

I get

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

though config/nspr/nspr.mozbuild has

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

Can you please fix that, and add a reference to config/nspr/nspr.mozbuild in the error message?
(Assignee)

Comment 222

5 years ago
Comment on attachment 705065 [details] [diff] [review]
Part 11: Ability to declare tier directories as external, v3

This patch was removed because it is redundant with static tier dirs.
Attachment #705065 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #705067 - Attachment description: Part 12: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3 → Part 11: Add SUBSTITUTE_CONFIG_FILES to moz.build, v3
(Assignee)

Comment 223

5 years ago
Created attachment 708700 [details] [diff] [review]
Part 12: Don't read moz.build files in static tier directories, v1

static tier directories == external make-based build systems. This patch makes it so we no longer attempt to read moz.build files in static tier directories.
Attachment #708700 - Flags: review?(ted)
Comment on attachment 708700 [details] [diff] [review]
Part 12: Don't read moz.build files in static tier directories, v1

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

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

Might be worthwhile to still test the multiply-set thing for static dirs, but if it really complicates things then probably not.
Attachment #708700 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/7faec6303aa9
(Assignee)

Updated

5 years ago
Duplicate of this bug: 836693
(Assignee)

Comment 227

5 years ago
Created attachment 708793 [details] [diff] [review]
Part 13: Use moz.build files to build the tree, v1

This is the last critical patch!

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

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

Try at https://tbpl.mozilla.org/?tree=Try&rev=e2f4ad71bbc9
Attachment #695530 - Attachment is obsolete: true
Attachment #705068 - Attachment is obsolete: true
Attachment #708793 - Flags: review?(ted)
Attachment #708793 - Flags: review?(mh+mozilla)
Attachment #708793 - Flags: feedback?(joey)
Comment on attachment 708793 [details] [diff] [review]
Part 13: Use moz.build files to build the tree, v1

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

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

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

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

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

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

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

Indeed, this defeats using FileAvoidWrite, and will likely lead to many useless recompilations.
Attachment #708793 - Flags: review?(mh+mozilla)
(Assignee)

Comment 230

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #208)
> Are these numbers from Windows? Please make sure you measure on Windows
> before drawing conclusions from these kinds of measurements.

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

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

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

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

If things slow down over time, we can optimize things. But, I don't think it is worth the time investment at this juncture. Hopefully the next version of my patch will find a more acceptable compromise.
(Assignee)

Comment 231

5 years ago
Created attachment 710040 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v2

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

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

BACKEND_OUTPUT_FILES has been sacked.

Try at https://tbpl.mozilla.org/?tree=Try&rev=56e5a927a358.
Attachment #708793 - Attachment is obsolete: true
Attachment #708793 - Flags: review?(ted)
Attachment #708793 - Flags: feedback?(joey)
Attachment #710040 - Flags: review?(ted)
Attachment #710040 - Flags: review?(mh+mozilla)
(Assignee)

Comment 232

5 years ago
Created attachment 710043 [details] [diff] [review]
Part 13: Use absolute paths in config.status, v1

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

If the old comment was wrong, the justification for requiring an absolute path is that a) absolute paths are better b) mozbuild.frontend expects an absolute path and it's much easier to normalize it in config.status then somewhere down the stack.
Attachment #710043 - Flags: review?(mh+mozilla)

Comment 233

5 years ago
Try run for 56e5a927a358 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=56e5a927a358
Results (out of 155 total builds):
    exception: 43
    success: 67
    warnings: 1
    failure: 44
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-56e5a927a358
Attachment #710043 - Flags: review?(mh+mozilla) → review+
Comment on attachment 710040 [details] [diff] [review]
Part 16: Use moz.build files to build the tree, v2

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

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

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

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

I still don't like this, but meh.

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

Please remove this (see comments on previous iteration).

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

Ditto
Attachment #710040 - Flags: review?(mh+mozilla) → review-

Comment 235

5 years ago
Try run for 645cb2432294 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=645cb2432294
Results (out of 270 total builds):
    success: 259
    warnings: 8
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-645cb2432294
(Assignee)

Comment 236

4 years ago
There was a DONTBUILD sitting on top of inbound and inbound needed built and I had this patch sitting around and I was lazy. So, part 12:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35a2fd406e4f
https://hg.mozilla.org/mozilla-central/rev/35a2fd406e4f
Flags: in-testsuite+
(Assignee)

Comment 238

4 years ago
Created attachment 715638 [details] [diff] [review]
Part 4h: Convert /dom, v2

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

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

Note that I changed the arguments to |rm| because apparently OS X's rm doesn't support the long form arguments! Perhaps the scripts could be rewritten to use shutil.rmtree?
Attachment #695116 - Attachment is obsolete: true
Attachment #695116 - Flags: review?(khuey)
Attachment #715638 - Flags: review?(khuey)
Attachment #715638 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 239

4 years ago
Created attachment 715640 [details] [diff] [review]
Part 4e: Convert /browser, v2

Removed bit rot. Mostly additions from /browser/metro.
Attachment #695113 - Attachment is obsolete: true
Attachment #715640 - Flags: review?(gavin.sharp)
Comment on attachment 715638 [details] [diff] [review]
Part 4h: Convert /dom, v2

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

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

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

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

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

I think you want the ' before the \n.

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

Lovely.

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

I don't think this should still be here

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

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

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

Which makes writeMakefile.py a terrible name... Maybe writeBuildFile? I dunno
(Assignee)

Comment 241

4 years ago
Created attachment 715718 [details] [diff] [review]
Part 14: 4-space indent for dom/imptests/*.py, v1

Cleaning up these files before I change them. I used reindent.py from the cpython source tree to perform reformatting. I didn't actually test things, but I highly doubt reindent.py breaks things.
Attachment #715718 - Flags: review?(Ms2ger)
(Assignee)

Comment 242

4 years ago
Created attachment 715740 [details] [diff] [review]
Part 14: Produce moz.build files for DOM implementation tests, v1

This patch should be more reasonable. It kinda/sorta seems to work locally. Although, the produced .mozbuild files (e.g. editing.mozbuild) seem to consist of empty lists. I must be missing something obvious...
Attachment #715740 - Flags: review?(Ms2ger)
Comment on attachment 715718 [details] [diff] [review]
Part 14: 4-space indent for dom/imptests/*.py, v1

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

rs=me
Attachment #715718 - Flags: review?(Ms2ger) → review+
Comment on attachment 715740 [details] [diff] [review]
Part 14: Produce moz.build files for DOM implementation tests, v1

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

DOM imported tests, btw, not implementation.

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

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

You're missing the makePath call here.

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

Please use result here or assign to resultstr above.

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

Same here.

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

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

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

Factor this out and use it in printMozbuildFile in importTestsuite.py? Probably along with the DIRS += [] bit.