818789 - Allow build to perform incremental tree builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Add arguments to mach build, v1

+++ This bug was initially created as a clone of Bug #677452 +++

It's not quite smartmake. The implementation isn't perfect. But, it's a commonly requested feature that I feel we're obligated to provide until whole tree incremental builds are measured in seconds, not minutes.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 689079 [details] [diff] [review]
Add arguments to mach build, v1

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +91,5 @@
> +                    return 1
> +                else:
> +                    print('You appear to have specified a path, but it does '
> +                        'not refer to a directory. Only paths that are '
> +                        'existing directories may be specified: ' + target)

Instead of bailing out for files, I'd do make -C $(dirname $target) $(basename $target).

So that one would do e.g. mach build dom/src/json/nsJSON.i, expecting make -C objdir/dom/src/json nsJSON.i to run.

And since there are various places where the Makefile is in a parent directory, you could search the last Makefile in the parents and do make -C that-dir target-relative-to-that-dir, so that mach build media/webrtc/trunk/src/modules/modules_rtp_rtcp/rtp_rtcp/source/rtp_rtcp_impl.o calls make -C objdir/media/webrtc/trunk/src/modules/modules_rtp_rtcp rtp_rtcp/source/rtp_rtcp_impl.o

Comment 2

[Gregory Szorc [:gps]]

Attachment: Add arguments to mach build, v2

I /think/ this does what you asked for in the review. I haven't fully tested and am pretty tired. I wouldn't be surprised if the parent traversal were busted, especially since we don't have strong guarantees on what topobjdir is. (This reminds me that I've been wanting to throw an os.path.normpath(os.path.abspath()) before we store it to ensure it is normalized. Maybe I'll do that as a part 0 in this bug. Otherwise, I worry the traversal could go to /)

Some interesting side-effects of the current implementing - you can do things like |mach build toolkit/clean| and that translates to |make -C toolkit clean|. Cool!

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 689103 [details] [diff] [review]
Add arguments to mach build, v2

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +79,5 @@
> +                        break
> +
> +                    make_path = os.path.join(current, 'Makefile')
> +                    if os.path.exists(make_path):
> +                        return (current[len(self.topobjdir) + 1:], None)

You should not run make -C dir-containing-Makefile rel-path for directories, we don't have rules to handle that so it's worthless to run that.

@@ +102,5 @@
> +
> +            current = os.path.join(self.topobjdir, reldir)
> +            while True:
> +                current = os.path.dirname(current)
> +                if current == self.topobjdir:

Since you're already checking there's a Makefile in topobjdir, you don't need this stopper.

@@ +116,5 @@
> +                return (reldir, reltarget)
> +
> +            print('Could not find a Makefile to execute for the path '
> +                'specified. Please run |mach build| with no arguments: ' +
> +                target)

This will never be reached.

All in all, this can probably simplified like this:

reldir = os.path.dirname(target)
target = os.path.basename(target)
while True:
    if os.path.exists(os.path.join(self.topobjdir, reldir, 'Makefile')):
        return (reldir, target)
    target = os.path.join(os.path.basename(reldir), target)
    reldir = os.path.dirname(reldir)

Comment 4

[Gregory Szorc [:gps]]

Attachment: Add arguments to mach build, v3

Incorporated review comments. I love it when code becomes simpler after review!

Comment 5

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

Comment on attachment 705981 [details] [diff] [review]
Add arguments to mach build, v3

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +59,5 @@
>                  pass
>  
>              self.log(logging.INFO, 'build_output', {'line': line}, '{line}')
>  
> +        def resolve_target_to_make(target):

A little docstring, please

@@ +137,3 @@
>  
> +        dist_path = os.path.join(self.topobjdir, 'dist')
> +        print('Finished building. Built files are in %s' % dist_path)

Not just there...

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 705981 [details] [diff] [review]
Add arguments to mach build, v3

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

Only nits.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +61,5 @@
>              self.log(logging.INFO, 'build_output', {'line': line}, '{line}')
>  
> +        def resolve_target_to_make(target):
> +            if os.path.isabs(target):
> +                print('Absolute paths for make targets are not allowed.')

Why not, if it's under the topobjdir?

@@ +100,5 @@
> +                    return (reldir, target)
> +
> +                target = os.path.join(os.path.basename(reldir), target)
> +                reldir = os.path.dirname(reldir)
> +

# End of resolve_target_to_make

@@ +102,5 @@
> +                target = os.path.join(os.path.basename(reldir), target)
> +                reldir = os.path.dirname(reldir)
> +
> +
> +        # Back to main function.

s/main/build/ ?

@@ +137,3 @@
>  
> +        dist_path = os.path.join(self.topobjdir, 'dist')
> +        print('Finished building. Built files are in %s' % dist_path)

> Not just there...

Or not there at all.

Comment 7

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

Comment on attachment 705981 [details] [diff] [review]
Add arguments to mach build, v3

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +103,5 @@
> +                reldir = os.path.dirname(reldir)
> +
> +
> +        # Back to main function.
> +        if len(what):

Apparently len(None) throws. Am I missing something here?

Comment 8

[Jeff Hammel]

(In reply to :Ms2ger from comment #7)
> Comment on attachment 705981 [details] [diff] [review]
> Add arguments to mach build, v3
> 
> Review of attachment 705981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/mach_commands.py
> @@ +103,5 @@
> > +                reldir = os.path.dirname(reldir)
> > +
> > +
> > +        # Back to main function.
> > +        if len(what):
> 
> Apparently len(None) throws. Am I missing something here?

why not just `if what`, which AIUI is favored anyway

Comment 9

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #6)
> ::: python/mozbuild/mozbuild/mach_commands.py
> @@ +61,5 @@
> >              self.log(logging.INFO, 'build_output', {'line': line}, '{line}')
> >  
> > +        def resolve_target_to_make(target):
> > +            if os.path.isabs(target):
> > +                print('Absolute paths for make targets are not allowed.')
> 
> Why not, if it's under the topobjdir?

Simplicity. We can file a follow-up if anyone requests it.

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6cca454559c8

With review comments addressed.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sorry if I am digging up an old bug, but I think this should be documented somehow, maybe here: https://developer.mozilla.org/en-US/docs/Incremental_Build

Comment 12

[Mike Hommey [:glandium]]

Considering bug 925350, it's not worth.