1207882 - Initial partial implementation for a new faster build backend

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Context:
https://groups.google.com/forum/#!topic/mozilla.dev.builds/GF6GjygF7ZY

Comment 1

[Mike Hommey [:glandium]]

Attachment: Use source searchplugin files directly for en-US locale

Comment 2

[Mike Hommey [:glandium]]

Attachment: Use a separate build target for config/buildid

Comment 3

[Mike Hommey [:glandium]]

Attachment: Ensure chrome manifests are created in a directory that exists when processing jar manifests

This currently works because other things in the build system are creating
those directories, but it's not a safe thing to rely on.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Associate an install target with every ContextDerived object

Since reading context['FINAL_TARGET'] can have side effects, use it lazily.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Add an initial partial implementation of a new, faster, build backend

Comment 6

[Mike Hommey [:glandium]]

Attachment: Add an initial partial implementation of a new, faster, build backend

Forgot to git add a file.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8665360 [details] [diff] [review]
Add an initial partial implementation of a new, faster, build backend

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

Great start!

I'm not done with review, so no change on review flag.

Install manifests support preprocessor entries. You can even define per-file DEFINES. Is there a reason you are doing all the preprocessor foo in make/PP_TARGETS instead of using install manifests?

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #7)
> Install manifests support preprocessor entries. You can even define per-file
> DEFINES. Is there a reason you are doing all the preprocessor foo in
> make/PP_TARGETS instead of using install manifests?

Yes there is: the fact that some use options that are not supported by install manifests. The end game is to use install manifests for everything, but I didn't want to block on that.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8665360 [details] [diff] [review]
Add an initial partial implementation of a new, faster, build backend

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

::: config/faster/rules.mk
@@ +68,5 @@
> +# fallback
> +$(TOPOBJDIR)/faster/%: ;
> +
> +# And files under dist/ are meant to be copied from their first dependency
> +# if there is not other rule.

There is a typo here: s/not/no/

@@ +70,5 @@
> +
> +# And files under dist/ are meant to be copied from their first dependency
> +# if there is not other rule.
> +$(TOPOBJDIR)/dist/%:
> +	cp $< $@

I'll add a rm -f $@ before the cp, because it is necessary when running `mach build faster` after a standard `mach build`.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8665360 [details] [diff] [review]
Add an initial partial implementation of a new, faster, build backend

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

Great work! This is pretty much an r+. But I'll wait to look at the other patches in my queue before formally granting r+. Feel free to upload the final revision for review so I can heave peace of mind w.r.t. the make voodoo.

If I had one higher-level critique, it would be that a lot of the optimizations in the new backend could be applied to the existing backend. For example, processing of jar manifests is currently processed in the libs tier, which isn't optimally traversed. We could give jar manifests their own .mk file and process them in one batch. Of course there are l10n repack implications for JAR manifests, so maybe that isn't the best of examples :)

::: config/faster/rules.mk
@@ +148,5 @@
> +# $(TOPSRCDIR)/path/to/jar.mn, the default would be $(TOPOBJDIR)/path/to.
> +# In case a different path must be used for the object directory, the `objdir`
> +# variable can be set. For example:
> +#   jar-foo: objdir=/some/other/path
> +jar-%: objdir ?= $(dir $(<:$(TOPSRCDIR)%=$(TOPOBJDIR)%))

You managed to confuse 2 build peers and induce copious amounts of swearing with this line. And I'm still not convinced I fully grok what is going on. Please add a comment on the make wizardry, specifically the "$(<:".

::: python/mozbuild/mozbuild/backend/fastermake.py
@@ +42,5 @@
> +        # We currently ignore a lot of object types, so just acknowledge
> +        # everything.
> +        obj.ack()
> +
> +        if not isinstance(obj, Defines) and isinstance(obj, ContextDerived):

It seems fragile to pull out the defines this way. Makes me realize that we should probably be setting defines on a per-object basis in case we ever add per-object define adjustment.

@@ +72,5 @@
> +                    mozpath.join('components', mozpath.basename(f))
> +                )
> +                if f.endswith('.manifest'):
> +                    self._manifest_entries[
> +                        mozpath.join(obj.install_target, 'chrome.manifest')] \

I prefer using a variable to hold the key to increase readability.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8665360 [details] [diff] [review]
Add an initial partial implementation of a new, faster, build backend

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

The other patches all seem reasonably sane. This looks good enough for something that isn't part of the build (yet). Let's ship it and see what our early adopters say.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #10)
> If I had one higher-level critique, it would be that a lot of the
> optimizations in the new backend could be applied to the existing backend.
> For example, processing of jar manifests is currently processed in the libs
> tier, which isn't optimally traversed. We could give jar manifests their own
> .mk file and process them in one batch. Of course there are l10n repack
> implications for JAR manifests, so maybe that isn't the best of examples :)

That is exactly why this can't be done with the existing backend ;)
 
> ::: config/faster/rules.mk
> @@ +148,5 @@
> > +# $(TOPSRCDIR)/path/to/jar.mn, the default would be $(TOPOBJDIR)/path/to.
> > +# In case a different path must be used for the object directory, the `objdir`
> > +# variable can be set. For example:
> > +#   jar-foo: objdir=/some/other/path
> > +jar-%: objdir ?= $(dir $(<:$(TOPSRCDIR)%=$(TOPOBJDIR)%))
> 
> You managed to confuse 2 build peers and induce copious amounts of swearing
> with this line. And I'm still not convinced I fully grok what is going on.
> Please add a comment on the make wizardry, specifically the "$(<:".

It's not /that/ magic. it's the $(FOO:old%=new%) form applied to $<, with old = $(TOPSRCDIR) and new = $(TOPOBJDIR)

> ::: python/mozbuild/mozbuild/backend/fastermake.py
> @@ +42,5 @@
> > +        # We currently ignore a lot of object types, so just acknowledge
> > +        # everything.
> > +        obj.ack()
> > +
> > +        if not isinstance(obj, Defines) and isinstance(obj, ContextDerived):
> 
> It seems fragile to pull out the defines this way. Makes me realize that we
> should probably be setting defines on a per-object basis in case we ever add
> per-object define adjustment.

That is indeed one of the shortcomings of the current data model that this patch reveals. Another one being the number of different data types indicating file copies and preprocessing in subtly different ways.

Comment 13

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #12)
> It's not /that/ magic. it's the $(FOO:old%=new%) form applied to $<, with
> old = $(TOPSRCDIR) and new = $(TOPOBJDIR)

I completely forgot about substitution references and their special syntax. This makes sense. But please throw in a comment. Or perhaps use $(patsubst) instead?

Comment 14

[Mike Hommey [:glandium]]

Note the comment just above /is/ about that variable. I'll switch to patsubst.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1769f48b3c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbff48899a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/53df14a71227
https://hg.mozilla.org/integration/mozilla-inbound/rev/d742983b5010
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e58f784dbc

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1769f48b3c0
https://hg.mozilla.org/mozilla-central/rev/8dbff48899a6
https://hg.mozilla.org/mozilla-central/rev/53df14a71227
https://hg.mozilla.org/mozilla-central/rev/d742983b5010
https://hg.mozilla.org/mozilla-central/rev/66e58f784dbc