Closed Bug 818789 Opened 9 years ago Closed 9 years ago

Allow build to perform incremental tree builds

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [mach])

Attachments

(1 file, 2 obsolete files)

Attached patch Add arguments to mach build, v1 (obsolete) — Splinter Review
+++ 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.
Attachment #689079 - Flags: review?(mh+mozilla)
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
Attachment #689079 - Flags: review?(mh+mozilla) → review+
Attached patch Add arguments to mach build, v2 (obsolete) — Splinter Review
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!
Attachment #689079 - Attachment is obsolete: true
Attachment #689103 - Flags: review?(mh+mozilla)
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)
Attachment #689103 - Flags: review?(mh+mozilla) → review-
Incorporated review comments. I love it when code becomes simpler after review!
Attachment #689103 - Attachment is obsolete: true
Attachment #705981 - Flags: review?(mh+mozilla)
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 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.
Attachment #705981 - Flags: review?(mh+mozilla) → review+
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?
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/6cca454559c8

With review comments addressed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 836208
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
Keywords: dev-doc-needed
Considering bug 925350, it's not worth.
Keywords: dev-doc-needed
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.