Add smartmake to the tree

RESOLVED FIXED in mozilla23

Status

Firefox Build System
General
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: jdm, Assigned: nalexander)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mach], URL)

Attachments

(1 attachment, 3 obsolete attachments)

Ehsan keeps bugging me about getting smartmake into the tree, so I'm filing this bug to humour him. Is this something we want to consider?

Comment 1

7 years ago
Yes!!!!!!! Please attach a patch and ask for review.  Anybody who says no should have a *pretty* convincing counter-argument (I'm very hard to convince!)
My argument would be that it's unnecessary.  If you make whatever you changed and then make -B -C objdir/toolkit/library, that should be enough, no?

Comment 3

7 years ago
(In reply to comment #2)
> My argument would be that it's unnecessary.  If you make whatever you changed
> and then make -B -C objdir/toolkit/library, that should be enough, no?

It is enough, but it's painful.  It's just so much easier to type:

smake editor layout

than it is to type:

make -C objdir/editor && make -C objdir/layout && make objdir/toolkit/library

Comment 4

6 years ago
I think mach would be the perfect place to integrate this:

 $ ./mach build [smake-target]

or

 $ ./mach build --smart [target]

or

 $ ./mach smartmake [target]

would be fine by me, I think.
Depends on: 774108
Whiteboard: [mach]

Updated

6 years ago
Blocks: 774108
No longer depends on: 774108

Updated

6 years ago
Component: Build Config → mach
(In reply to Gregory Szorc [:gps] from comment #4)
>  $ ./mach build [smake-target]

I think it should be like this, it would reduce the number of commands to remember ("smart" can also be a strange word for a non-english mother-tongue, hard to translate and figure out what it does). A better command would be at a maximum
./mach build --target [target] if a command is needed.
Also, in the last 2 days we had a workshop with students in italy, mach definitely helped much, but the lack of support for making single components was a big issue, so this is an high wanted feature, imo.

Comment 6

6 years ago
I agree this would be a handy feature. If someone submits a patch, I'll review it. I don't have the spare cycles to put together the patch myself. 

Since smartmake is jdm's baby, perhaps he should throw something up. See https://developer.mozilla.org/en-US/docs/Developer_Guide/mach for how to author mach commands.

FWIW, I think having this as a separate command (at least initially) is better. smartmake isn't part of the build system and philosophically I have issue with making it such because smartmake is really just an optimizing hack to work around the crappiness of recursive make (which we are trying to replace in the official build system). We can always expose functionality under multiple mach commands easily enough if it comes to that.
Well, I'd already be happy being able to specify a folder like
./mach build toolkit/components/places
./mach build toolkit/library
without going too deep into having specific named targets... Maybe I should file this apart or is it going to be part of this?

Comment 8

5 years ago
With Nick's work in bug 836208, we now have a central function for resolving |mach build| arguments to make targets. We can probably just inject smartmake's target list into this function and seamlessly integrate smartmake into mach.

I think the default behavior should be "build intelligently [with smartmake lookups]." Perhaps we should add a flag to disable intelligent behavior? Or, maybe we do something weird like "if a directory and it has a smart target, use the smart lookup unless the argument has a trailing slash." I dunno. There are many variations.
Depends on: 836208
Assignee: nobody → qheaden
I was originally going to protest that making the default be intelligent lookups could lead to subtle breakage, but if it only applies to building explicit directories, that is already the case. I'm fine with this plan.
Should smartmake's timestamp checking functionality also be included? Or should I only include manually entering paths where source files were modified?
Let's keep it at the manual paths for now. I've always been a bit leery of the timestamp checking, and it's never been totally cross-platform happy.
(Assignee)

Comment 12

5 years ago
Created attachment 740424 [details] [diff] [review]
WIP on a smartmake-like mach command

I looked at merging `smartmake` but I think it's easier to write and test a little new code than reverse engineer smartmake.  How do people feel about the attached patch?  Josh, did I even interpret the depspecs file correctly?
Attachment #740424 - Flags: feedback?(josh)
Attachment #740424 - Flags: feedback?(gps)
(Assignee)

Comment 13

5 years ago
Created attachment 740425 [details] [diff] [review]
v2: forgot the dumbmake-depspecs file.
Attachment #740424 - Attachment is obsolete: true
Attachment #740424 - Flags: feedback?(josh)
Attachment #740424 - Flags: feedback?(gps)
(Assignee)

Comment 14

5 years ago
Comment on attachment 740425 [details] [diff] [review]
v2: forgot the dumbmake-depspecs file.

Plenty of room for better documentation, integration into `mach build`, nits, renaming the depspecs file, preprocessing the depspecs file for cheap comment and ifdef support, etc.
Attachment #740425 - Flags: feedback?(josh)
Attachment #740425 - Flags: feedback?(gps)
Comment on attachment 740425 [details] [diff] [review]
v2: forgot the dumbmake-depspecs file.

I'd like Till's eyes on this if he's interested, since he was the brains behind the refactor of smartmake.
Attachment #740425 - Flags: feedback?(tschneidereit)
Comment on attachment 740425 [details] [diff] [review]
v2: forgot the dumbmake-depspecs file.

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

\o/!
Looks good to me, and your understanding of how the depspecs work is spot-on.

I guess they could live somewhere else in the tree, however: adding them to the already-cluttered top-level directory seems unnecessary to me.

Also, should the O(n^2) loop ever be a problem, we could just cache the result based on a hash of the depspecs file, I guess.
Attachment #740425 - Flags: feedback?(tschneidereit) → feedback+
Comment on attachment 740425 [details] [diff] [review]
v2: forgot the dumbmake-depspecs file.

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

::: dumbmake-depspecs
@@ +1,1 @@
> +toolkit/library

Let's not add a new file to the root directory. build/dumbmake-depspecs perhaps?

Nit: Lots of trailing whitespace in this file.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +521,5 @@
> +    """Interface to build the tree with depencies managed by dumbmake."""
> +
> +    @Command('dumbmake', help='Build the tree with dependencies managed by dumbmake.')
> +    @CommandArgument('what', default=None, nargs='*', help=DUMBMAKE_WHAT_HELP)
> +    def dumbmake(self, what=None):

A new command for dumbmake, eh? What's wrong with incorporating it into |mach build|. While behavior for |build| would change, |mach build X| would "just work." I believe this quality is better than cognitive dissonance associated with multiple commands that do seemingly the same thing.

I'm sure someone out there would complain about the change in behavior. Maybe add a --disable-dumbmake? Presumably some day we'll add a mach settings file so developers can select this as the default behavior if they really want to take away assistance.
Attachment #740425 - Flags: feedback?(gps) → feedback+
(Assignee)

Comment 18

5 years ago
Thanks for the feedback!  Responses below.

(In reply to Gregory Szorc [:gps] from comment #17)
> Comment on attachment 740425 [details] [diff] [review]
> v2: forgot the dumbmake-depspecs file.
> 
> Review of attachment 740425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dumbmake-depspecs
> @@ +1,1 @@
> > +toolkit/library
> 
> Let's not add a new file to the root directory. build/dumbmake-depspecs
> perhaps?
> 
> Nit: Lots of trailing whitespace in this file.

Fine by me.

> ::: python/mozbuild/mozbuild/mach_commands.py
> @@ +521,5 @@
> > +    """Interface to build the tree with depencies managed by dumbmake."""
> > +
> > +    @Command('dumbmake', help='Build the tree with dependencies managed by dumbmake.')
> > +    @CommandArgument('what', default=None, nargs='*', help=DUMBMAKE_WHAT_HELP)
> > +    def dumbmake(self, what=None):
> 
> A new command for dumbmake, eh? What's wrong with incorporating it into
> |mach build|. While behavior for |build| would change, |mach build X| would
> "just work." I believe this quality is better than cognitive dissonance
> associated with multiple commands that do seemingly the same thing.

Only to make it easy to test and get feedback on.

> I'm sure someone out there would complain about the change in behavior.
> Maybe add a --disable-dumbmake? Presumably some day we'll add a mach
> settings file so developers can select this as the default behavior if they
> really want to take away assistance.

I think I saw a place to wire it into |mach build|; let me try that and add a flag to disable.  Thanks again, till and gps!
Comment on attachment 740425 [details] [diff] [review]
v2: forgot the dumbmake-depspecs file.

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

::: dumbmake-depspecs
@@ +22,5 @@
> +  accessible/build
> +    accessible
> +  dom
> +  content
> +  layout

This doesn't seem to account for the gkmedias library in layout/media that's a separate library on Windows.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +521,5 @@
> +    """Interface to build the tree with depencies managed by dumbmake."""
> +
> +    @Command('dumbmake', help='Build the tree with dependencies managed by dumbmake.')
> +    @CommandArgument('what', default=None, nargs='*', help=DUMBMAKE_WHAT_HELP)
> +    def dumbmake(self, what=None):

I agree, I think this should be the default for "mach build X". Developers are already not getting a full correct build when they build a single directory, we might as well help them type less while they're doing that. "mach build" should continue to build the full tree, so that workflow isn't broken.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19)
> Comment on attachment 740425 [details] [diff] [review]
> v2: forgot the dumbmake-depspecs file.
> 
> Review of attachment 740425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dumbmake-depspecs
> @@ +22,5 @@
> > +  accessible/build
> > +    accessible
> > +  dom
> > +  content
> > +  layout
> 
> This doesn't seem to account for the gkmedias library in layout/media that's
> a separate library on Windows.

Oh, I meant to comment on that: The depspecs should absolutely be verified and probably updated in a few places before this is used as the default build mechanism. I know that there were cases that didn't create valid builds.
(Assignee)

Comment 21

5 years ago
Hi Quentin, do you have a comment on this?  I don't want to steal your ASSIGNED without your input.
OS: Linux → All
Hardware: x86 → All
(Assignee)

Comment 22

5 years ago
Created attachment 741506 [details] [diff] [review]
Patch against s-c, updated with review comments

* wired this into |mach build X| -- there's a little wrinkle with |mach DIR1 target DIR2|, see tests for details.
* added option to disable.
* renamed /dumbmake-depspecs to /build/dumbmake-dependencies, might even want /build/extra-make-dependencies to make this technology agnostic.
Attachment #740425 - Attachment is obsolete: true
Attachment #740425 - Flags: feedback?(josh)
Attachment #741506 - Flags: review?(gps)
@Nick

I'm unassigning myself from this bug due to lack of time and experience. You'll do a good job of fixing it.  :-)
Assignee: qheaden → nobody
(Assignee)

Comment 24

5 years ago
(In reply to Quentin Headen from comment #23)
> @Nick
> 
> I'm unassigning myself from this bug due to lack of time and experience.
> You'll do a good job of fixing it.  :-)

Thanks for the quick response.  If you can help test or updating the dependencies file, that would be great.
Assignee: nobody → nalexander
Comment on attachment 741506 [details] [diff] [review]
Patch against s-c, updated with review comments

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

I could probably grant r+. But, there's at least 1 too many nits for my liking.

::: build/dumbmake-dependencies
@@ +64,5 @@
> +testing/tests
> +testing/tinderbox-standalone-tests
> +testing/tools
> +testing/tps
> +testing/xpcshell

What's the point of all of these if they don't have children?

::: python/mozbuild/dumbmake/dumbmake.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/.

Nit: "file," should be on line 3.

@@ +29,5 @@
> +
> +    deps = {}
> +
> +    for i in range(len(pairs)):
> +        indent, target = pairs[i]

I believe you can write this as:

for i, (indent, target) in enumerate(pairs):

You should even be able to omit the parens around the tuple.

@@ +31,5 @@
> +
> +    for i in range(len(pairs)):
> +        indent, target = pairs[i]
> +        if not deps.has_key(target):
> +            deps[target] = list()

Nit: deps[target] = []

You should only use the list() or dict() ctors if you are feeding an iterable in.

@@ +52,5 @@
> +    """
> +    dm = kwargs.pop('dependency_map', None)
> +    if dm is None:
> +        dm = dependency_map(targets)
> +    all_targets = {} # Used as an ordered set, value is ordering, larger is later.

Did you know Python has an ordered dictionary? collections.OrderedDict. There is also a collections.Counter() type you may find useful.

@@ +79,5 @@
> +                yield pair
> +            continue
> +
> +        # Add extra dumbmake dependencies to simple directory targets.
> +        make_dirs = [ make_dir for make_dir, _ in group ]

Nit: cuddle

@@ +83,5 @@
> +        make_dirs = [ make_dir for make_dir, _ in group ]
> +        new_make_dirs = all_dependencies(*make_dirs, dependency_map=dependency_map)
> +
> +        for make_dir in new_make_dirs:
> +            yield (make_dir, None)

Nit: parens aren't required.

::: python/mozbuild/dumbmake/test/test_dumbmake.py
@@ +12,5 @@
> +from dumbmake.dumbmake import (
> +    dependency_map,
> +    indentation,
> +    all_dependencies,
> +    add_extra_dependencies,

Nit: alphabetize imports.
Attachment #741506 - Flags: review?(gps) → feedback+
(Assignee)

Comment 26

5 years ago
(In reply to Gregory Szorc [:gps] from comment #25)
> Comment on attachment 741506 [details] [diff] [review]
> Patch against s-c, updated with review comments
> 
> Review of attachment 741506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I could probably grant r+. But, there's at least 1 too many nits for my
> liking.
> 
> ::: build/dumbmake-dependencies
> @@ +64,5 @@
> > +testing/tests
> > +testing/tinderbox-standalone-tests
> > +testing/tools
> > +testing/tps
> > +testing/xpcshell
> 
> What's the point of all of these if they don't have children?

Best guess: they told smartmake what directories to crawl for modified time stamps.  Happy to kill them.  I'm not the right person to update the deps file for content.

> 
> ::: python/mozbuild/dumbmake/dumbmake.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/.
> 
> Nit: "file," should be on line 3.
> 
> @@ +29,5 @@
> > +
> > +    deps = {}
> > +
> > +    for i in range(len(pairs)):
> > +        indent, target = pairs[i]
> 
> I believe you can write this as:
> 
> for i, (indent, target) in enumerate(pairs):
> 
> You should even be able to omit the parens around the tuple.

Urgh, I'd really rather not.

> @@ +31,5 @@
> > +
> > +    for i in range(len(pairs)):
> > +        indent, target = pairs[i]
> > +        if not deps.has_key(target):
> > +            deps[target] = list()
> 
> Nit: deps[target] = []
> 
> You should only use the list() or dict() ctors if you are feeding an
> iterable in.
> 
> @@ +52,5 @@
> > +    """
> > +    dm = kwargs.pop('dependency_map', None)
> > +    if dm is None:
> > +        dm = dependency_map(targets)
> > +    all_targets = {} # Used as an ordered set, value is ordering, larger is later.
> 
> Did you know Python has an ordered dictionary? collections.OrderedDict.
> There is also a collections.Counter() type you may find useful.

Hahaha.  This started as OrderedDict (that's why the list() constructor, for example).  This is not suitable for Counter.  If you insert an existing key into an OrderedDict, its position does not change.  This wants the opposite, so I maintain my own counter and explicitly sort.  I could delete and re-insert, but I don't like |del| for reasons I can't put a finger on.  If you care, I'll use OrderedDict.

> 
> @@ +79,5 @@
> > +                yield pair
> > +            continue
> > +
> > +        # Add extra dumbmake dependencies to simple directory targets.
> > +        make_dirs = [ make_dir for make_dir, _ in group ]
> 
> Nit: cuddle
> 
> @@ +83,5 @@
> > +        make_dirs = [ make_dir for make_dir, _ in group ]
> > +        new_make_dirs = all_dependencies(*make_dirs, dependency_map=dependency_map)
> > +
> > +        for make_dir in new_make_dirs:
> > +            yield (make_dir, None)
> 
> Nit: parens aren't required.
> 
> ::: python/mozbuild/dumbmake/test/test_dumbmake.py
> @@ +12,5 @@
> > +from dumbmake.dumbmake import (
> > +    dependency_map,
> > +    indentation,
> > +    all_dependencies,
> > +    add_extra_dependencies,
> 
> Nit: alphabetize imports.
Looking at it again, not using Counter() or OrderedDict is acceptable. Although, the sorted() doesn't need a lambda: you can use operator.itemgetter(). If you search the Internets for "sort Python dict by value" you'll find itemgetter() is typically how it's done. I reckon it is faster than a lambda.
(Assignee)

Comment 28

5 years ago
Created attachment 741665 [details] [diff] [review]
Patch against s-c, v3, addressing nits

I went with the |OrderedDict| delete and re-insert -- it's simpler with the explanatory comment.  Huzzah for unit tests!
Attachment #741506 - Attachment is obsolete: true
Attachment #741665 - Flags: review?(gps)
(Assignee)

Comment 29

5 years ago
Fun fact: |make check| fails on my Android tree with:

 0:07.60 cppunittests INFO | Running test TestBloomFilter
 0:07.67 ['/Users/ncalexan/Mozilla/mcgit/objdir-droid2.noindex/mfbt/tests/TestBloomFilter']
 0:07.67 cppunittests ERROR | [Errno 8] Exec format error
 0:07.68 make[2]: *** [check] Error 1
 0:07.68 make[1]: *** [check] Error 2
 0:07.68 make: *** [check] Error 2

Rather than figure out how to do this, or build a fresh Mac OS X objdir, try:

http://tbpl.mozilla.org/?tree=Try&rev=8953f84b461a
(In reply to Gregory Szorc [:gps] from comment #25)
> ::: build/dumbmake-dependencies
> @@ +64,5 @@
> > +testing/tests
> > +testing/tinderbox-standalone-tests
> > +testing/tools
> > +testing/tps
> > +testing/xpcshell
> 
> What's the point of all of these if they don't have children?

The smarter smartmake (looking for modified files) would flip out if it encountered a directory about which it knew nothing.
(In reply to Josh Matthews [:jdm] from comment #30)
> The smarter smartmake (looking for modified files) would flip out if it
> encountered a directory about which it knew nothing.

Doesn't sound too smart if you put it like that. ;)
Comment on attachment 741665 [details] [diff] [review]
Patch against s-c, v3, addressing nits

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

1 step backward for hacks around bad build system dependencies. 2 steps forward for easier developer interaction with the build system following the principles of "it just works."
Attachment #741665 - Flags: review?(gps) → review+
> 1 step backward for hacks around bad build system dependencies. 2 steps
> forward for easier developer interaction with the build system following the
> principles of "it just works."

Note that, to guarantee the "it just works" property, we still have to verify that the depspecs are entirely correct. Otherwise, it's very easy to create non-functional builds with this, I'm afraid.
(Assignee)

Comment 34

5 years ago
(In reply to Till Schneidereit [:till] from comment #33)
> > 1 step backward for hacks around bad build system dependencies. 2 steps
> > forward for easier developer interaction with the build system following the
> > principles of "it just works."
> 
> Note that, to guarantee the "it just works" property, we still have to
> verify that the depspecs are entirely correct. Otherwise, it's very easy to
> create non-functional builds with this, I'm afraid.

I agree that "it just works" is too strong, but since as written the code only adds build targets I don't believe it can ever make thinks worse.  That is, if old |mach build X Y Z| produced a working build, new |mach build X Y Z| will still produce a working build (but possibly will do unnecessary work building an unneeded target W).  If that's not true, our Makefiles are /really/ bad :)
Since this only impacts builds where you're building a single directory at a time you're already not guaranteed that your resulting build will be correct. This just makes it a bit more likely to be correct.
But also makes the build take (possibly a lot) longer, without adding correctness guarantees. In particular, if I'm touching code in content/base/src, I'll often run |./mach build content/base/src|, followed by a full build once that works. With this approach, mach would try to link the code as well, even though I already know that won't work.
(Assignee)

Comment 37

5 years ago
(In reply to :Ms2ger from comment #36)
> But also makes the build take (possibly a lot) longer, without adding
> correctness guarantees. In particular, if I'm touching code in
> content/base/src, I'll often run |./mach build content/base/src|, followed
> by a full build once that works. With this approach, mach would try to link
> the code as well, even though I already know that won't work.

I'm not following your example.  Where is the code linking happening?  If linking is part of |make -C content/base/src|, there's no way to avoid it.  If linking is happening in a target you haven't given in your example but is added as a dependency, and is guaranteed to fail, then I think we need to improve or remove that dependency.
Linking is part of toolkit/library, which is computed as a dependency of any under content/. I think Ms2ger is say that he writes code that will compile in isolation before he tries to compile every other piece and link it together properly.
Y U NO LAND PATCH?

Updated

5 years ago
Component: mach → Build Config
(Assignee)

Comment 40

5 years ago
(In reply to Gregory Szorc [:gps] from comment #39)
> Y U NO LAND PATCH?

There was a little chatter, and I wanted to hear it out, and then I forgot to land it.  Soon...
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/e5d90d0479cc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 43

5 years ago
This deserves a post to dev.platform, and opening a bottle of champagne!
\o/, I'll definitely drink to this!
(Assignee)

Comment 45

5 years ago
Post is out:

https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/6o8OGvF8SrU

Ehsan, I wonder now if this is the part of smartmake that you care about: this patch implemented adding extra dependencies but not Till's timestamp tracking.
Blocks: 868880

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.