677452 - Add smartmake to the tree

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Josh Matthews [:jdm]]

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

[(no longer active)]

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

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

[(no longer active)]

(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

[Gregory Szorc [:gps]]

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.

Comment 5

[Marco Bonardo [:mak]]

(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

[Gregory Szorc [:gps]]

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.

Comment 7

[Marco Bonardo [:mak]]

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

[Gregory Szorc [:gps]]

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.

Comment 9

[Josh Matthews [:jdm]]

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.

Comment 10

[Quentin Headen [:qheaden]]

Should smartmake's timestamp checking functionality also be included? Or should I only include manually entering paths where source files were modified?

Comment 11

[Josh Matthews [:jdm]]

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.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: 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?

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: v2: forgot the dumbmake-depspecs file.

Comment 14

[Nick Alexander :nalexander [he/him]]

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.

Comment 15

[Josh Matthews [:jdm]]

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.

Comment 16

[Till Schneidereit [:till]]

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.

Comment 17

[Gregory Szorc [:gps]]

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.

Comment 18

[Nick Alexander :nalexander [he/him]]

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 19

[(not currently active) Ted Mielczarek]

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.

Comment 20

[Till Schneidereit [:till]]

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

Comment 21

[Nick Alexander :nalexander [he/him]]

Hi Quentin, do you have a comment on this?  I don't want to steal your ASSIGNED without your input.

Comment 22

[Nick Alexander :nalexander [he/him]]

Attachment: 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.

Comment 23

[Quentin Headen [:qheaden]]

@Nick

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

Comment 24

[Nick Alexander :nalexander [he/him]]

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

Comment 25

[Gregory Szorc [:gps]]

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.

Comment 26

[Nick Alexander :nalexander [he/him]]

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

Comment 27

[Gregory Szorc [:gps]]

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.

Comment 28

[Nick Alexander :nalexander [he/him]]

Attachment: 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!

Comment 29

[Nick Alexander :nalexander [he/him]]

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

Comment 30

[Josh Matthews [:jdm]]

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

Comment 31

[Till Schneidereit [:till]]

(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 32

[Gregory Szorc [:gps]]

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

Comment 33

[Till Schneidereit [:till]]

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

Comment 34

[Nick Alexander :nalexander [he/him]]

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

Comment 35

[(not currently active) Ted Mielczarek]

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.

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 37

[Nick Alexander :nalexander [he/him]]

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

Comment 38

[Josh Matthews [:jdm]]

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.

Comment 39

[Gregory Szorc [:gps]]

Y U NO LAND PATCH?

Comment 40

[Nick Alexander :nalexander [he/him]]

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

Comment 41

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d90d0479cc

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5d90d0479cc

Comment 43

[(no longer active)]

This deserves a post to dev.platform, and opening a bottle of champagne!

Comment 44

[Till Schneidereit [:till]]

\o/, I'll definitely drink to this!

Comment 45

[Nick Alexander :nalexander [he/him]]

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.