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

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

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

Forgot to git add a file.

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?

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

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

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

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

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

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