Closed Bug 852814 Opened 7 years ago Closed 5 years ago

Moving variables to moz.build always requires a clobber

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: gps, Assigned: bokeefe)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

If we move a variable from Makefile.in to moz.build, part of that involves sticking the variable in a blacklist in rules.mk so we can detect Makefile.in not abiding by the rules. We check for presence of that variable before including backend.mk.

So, what happens when we do an incremental build after applying this delta (read: no clobber)?

Well, we go to execute the existing Makefile in the objdir (generated from a before revision). When rules.mk is included, we hit the blacklist and get an $(error). Why does this happen? Because the rule to regenerate Makefile from Makefile.in (or from moz.build) is further down rules.mk, after the blacklist check! Doh.

So, let's just move the blacklist check to after the Makefile regeneration rules. Not so fast. There are multiple ways to generate Makefile. You can config.status --file. You can config.status. Or, maybe there is no Makefile.in or moz.build file (like GYP). The mechanism used is determined by the presence of some variables (like NO_MAKEFILE_RULE). Where do these variables come from? backend.mk is one place. If we include backend.mk, we can't accurately test the blacklist. If we don't include backend.mk, we don't know how to regenerate the Makefile. Derp.

I'm sure there is a creative solution in here somewhere. Until we find it, we're forced to touch CLOBBER every time we move a variable out of Makefile.in. Not cool.
Why not just force a config.status run from the toplevel Makefile?
(In reply to Mike Hommey [:glandium] from comment #1)
> Why not just force a config.status run from the toplevel Makefile?

Always? Sure, we /could/ do that. There might be a perf loss. Meh? Although, I suppose the side-effect of config.status populating all the Makefile would page them all into memory. Doesn't seem too bad (assuming there is no cache eviction).
We could also do it only for the first build after such a migration. Something like the CLOBBER file, but that would only trigger config.status.
Duplicate of this bug: 911197
Duplicate of this bug: 931030
Blocks: clobber
Attached file MozReview Request: bz://852814/bokeefe (obsolete) —
/r/6323 - Bug 852814 - Move the last remaining EXTRA_DSO_LDOPTS to moz.build; r=gps
/r/6325 - Bug 852814 - Move mozbuild variable blacklist logic into the backend; r=gps

Pull down these commits:

hg pull review -r 2a80372b019683965bfa1b6f56fa59955b340491
Attachment #8585668 - Flags: review?(gps)
I have a handful of patches nearly ready that move things out of Makefile.ins, and I suspect everyone would prefer not to need a clobber for each of them, so I'm going to attempt to fix this first.

I'm proposing that we move the blacklisting out of rules.mk and friends, and put it into the backend itself. It's not quite the same, but I think it's close enough for our purposes. The key differences:
* The check only happens on files that are processed by the backend, versus anything that includes config.mk/rules.mk/recurse.mk.
* The way I did it, every branch in the Makefile.in is checked, not just the paths that are hit. That was the reason for the media/omx-plugin/Makefile.in change - apparently it isn't run for gonk.
* I didn't do a very thorough job detecting comments. Right now, "#CPP_UNIT_TESTS = TestCSSPropertyLookup.cpp" (c.f. layout/style/test/Makefile.in) is allowed, but "# CPP_UNIT_TESTS = ..." would be rejected. Allowing one space shouldn't be too bad, but allowing arbitrary things between the beginning of the comment and the rejected token made building the backend take a lot longer.

For comparison purposes, on my laptop:
Inbound: roughly 107s, 57s, and 36s building the backend (3 separate runs - the first with a cold disk cache)
With patches: 37s
With patches + arbitary comments: ~180s

I pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b0c17665c70
and then again to fix android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=412835040e3d
Assignee: nobody → bokeefe
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/6323/#review5531

::: media/omx-plugin/moz.build
(Diff revision 1)
> +    EXTRA_DSO_LDOPTS += [

I /think/ we can switch to OS_LIBS and omit the "-l". But doing a 1:1 port is acceptable.
https://reviewboard.mozilla.org/r/6325/#review5533

I'm not super crazy about using regular expressions to parse for variable assigmnents in Makefile.in.

However, I do like us moving the validation from build time to config.status time. Moving errors forward is almost always a user win.

I'm curious if this changes our no-op build time. I wouldn't be surprised if iterating all these variables for hundreds of Makefiles added measurable overhead. (I wouldn't be surprised if it wasn't measurable either.)
Comment on attachment 8585668 [details]
MozReview Request: bz://852814/bokeefe

https://reviewboard.mozilla.org/r/6321/#review5535

Ship It!
Attachment #8585668 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #9)
> I'm not super crazy about using regular expressions to parse for variable
> assigmnents in Makefile.in.

Wouldn't it be feasible to use the in-tree pymake remnants for that?
(In reply to Joshua Cranmer [:jcranmer] from comment #11)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > I'm not super crazy about using regular expressions to parse for variable
> > assigmnents in Makefile.in.
> 
> Wouldn't it be feasible to use the in-tree pymake remnants for that?

Not worth it due to performance reasons. I'm fine with a 95% accurate regex-based filtering solution.
(In reply to Gregory Szorc [:gps] from comment #8)
> https://reviewboard.mozilla.org/r/6323/#review5531
> 
> ::: media/omx-plugin/moz.build
> (Diff revision 1)
> > +    EXTRA_DSO_LDOPTS += [
> 
> I /think/ we can switch to OS_LIBS and omit the "-l". But doing a 1:1 port
> is acceptable.

I couldn't tell if that would affect anything else (it would change where those libraries appear on the build command line, at least), so I'll leave this alone for now.

(In reply to Gregory Szorc [:gps] from comment #9)
> I'm curious if this changes our no-op build time. I wouldn't be surprised if
> iterating all these variables for hundreds of Makefiles added measurable
> overhead. (I wouldn't be surprised if it wasn't measurable either.)

I did a handful of builds locally, and got some numbers, but they're not the most scientific results:
Without this patch, no-op builds took an average of 1400 seconds, with σ = 75.7s (as reported by mach).
With this patch, no-op builds took an average of 1346 seconds, with σ = 58.8s.
That may or may not be a real 54s (or 4%) improvement, considering how
For comparison, a clobber build took me 13786s (or, almost 4 hours).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/84d9ae40699e
https://hg.mozilla.org/mozilla-central/rev/7f73bbf6df76
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Was using EXTERNALLY_MANAGED_MAKE_FILE to ignore the restrictions on Makefile.in variables [1] purposefully broken by this patch? I'm unsure if what I'm seeing is a bug or a "feature". We used this for Instantbird to define EXTRA_DSO_LDOPTS in a particular situation [2]. Now we're getting the error that EXTRA_DSO_LDOPTS cannot be defined in a Makefile.in.

Also the checking of blacklisted variables in Makefile.in doesn't properly ignore comments [3]. I'm unsure if it is supposed to or not. I had to fix a trivial bustage from this [4].

[1] https://hg.mozilla.org/mozilla-central/rev/7f73bbf6df76#l2.1
[2] http://hg.mozilla.org/users/florian_queze.net/purple/file/b5a332f9b9b9/purplexpcom/src/Makefile.in#l40
[3] https://hg.mozilla.org/mozilla-central/rev/7f73bbf6df76#l5.97
[4] http://hg.mozilla.org/users/florian_queze.net/purple/rev/b5a332f9b9b9
(In reply to Patrick Cloke [:clokep] from comment #16)
> Was using EXTERNALLY_MANAGED_MAKE_FILE to ignore the restrictions on
> Makefile.in variables [1] purposefully broken by this patch? I'm unsure if
> what I'm seeing is a bug or a "feature". We used this for Instantbird to
> define EXTRA_DSO_LDOPTS in a particular situation [2]. Now we're getting the
> error that EXTRA_DSO_LDOPTS cannot be defined in a Makefile.in.

That was an oversight on my part. I'll file a follow-up to put that back in.
Depends on: 1154687
FWIW, I think the MOZBUILD_VARIABLES and DEPRECATED_VARIABLES definitions should go in context.py.
Attachment #8585668 - Attachment is obsolete: true
Attachment #8618008 - Flags: review+
Attachment #8618009 - Flags: review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.