852814 - Moving variables to moz.build always requires a clobber

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Mike Hommey [:glandium]]

Why not just force a config.status run from the toplevel Makefile?

Comment 2

[Gregory Szorc [:gps]]

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

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 6

[Brian O'Keefe [:bokeefe]]

Attachment: MozReview Request: bz://852814/bokeefe

/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

Comment 7

[Brian O'Keefe [:bokeefe]]

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

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Gregory Szorc [:gps]]

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 10

[Gregory Szorc [:gps]]

Comment on attachment 8585668 [details]
MozReview Request: bz://852814/bokeefe

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

Ship It!

Comment 11

[Joshua Cranmer [:jcranmer]]

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

Comment 12

[Gregory Szorc [:gps]]

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

Comment 13

[Brian O'Keefe [:bokeefe]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84d9ae40699e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f73bbf6df76

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84d9ae40699e
https://hg.mozilla.org/mozilla-central/rev/7f73bbf6df76

Comment 16

[Patrick Cloke [:clokep]]

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

Comment 17

[Brian O'Keefe [:bokeefe]]

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

Comment 18

[Mike Hommey [:glandium]]

FWIW, I think the MOZBUILD_VARIABLES and DEPRECATED_VARIABLES definitions should go in context.py.

Comment 20

[Brian O'Keefe [:bokeefe]]

Attachment: MozReview Request: Bug 852814 - Move the last remaining EXTRA_DSO_LDOPTS to moz.build; r=gps

Comment 21

[Brian O'Keefe [:bokeefe]]

Attachment: MozReview Request: Bug 852814 - Move mozbuild variable blacklist logic into the backend; r=gps