1042226 - move DEFINES += -DAB_CD=$(AB_CD) pattern into config.mk

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

I was looking through the Makefile.in's that we have left in the tree today:

[froydnj@cerebro gecko-dev.git]$ find . -name Makefile.in | grep -v -e ./intl/icu/ -e ./nsprpub -e ./ipc/chromium/src/ | wc -l
411

And noticed that the easiest win would be knocking off some of the */locales Makefile.in's that have just:

DEFINES += -DAB_CD=$(AB_CD)

in them.  I suspect trying to make them all use MOZ_UI_LOCALE would be met with rioting, so moving that DEFINES definition into moz.build needs to be done.

But the DEFINES machinery doesn't really have anything for "some value that will actually be processed by make first", so we need a fourth type of thing for DEFINES keys, to go with numbers, strings, and booleans.  Or we could say that DEFINES keys with an initial character of '$' will be unquoted, but that seems footgunnish.

Any suggestions on a better idea?

Comment 1

[(not currently active) Ted Mielczarek]

These need to be Makefile variables because they're overridden at build-time, right?

Could we just stick this in config.mk instead of having a bunch of Makefiles define it? It seems like it'd be pretty inoffensive.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> These need to be Makefile variables because they're overridden at
> build-time, right?

That's what config.mk seems to suggest for why we have AB_CD instead of MOZ_UI_LOCALE:

http://mxr.mozilla.org/mozilla-central/source/config/config.mk#718

> Could we just stick this in config.mk instead of having a bunch of Makefiles
> define it? It seems like it'd be pretty inoffensive.

We could do that, but maybe guarded on a (currently non-existent) LOCALES_DIRECTORY (or the presence of JAR_MANIFESTS, or something) so some poor C/C++ developer doesn't run into issues?

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to Nathan Froyd (:froydnj) from comment #0)
> But the DEFINES machinery doesn't really have anything for "some value that
> will actually be processed by make first", so we need a fourth type of thing
> for DEFINES keys, to go with numbers, strings, and booleans.  Or we could
> say that DEFINES keys with an initial character of '$' will be unquoted, but
> that seems footgunnish.

Using make syntax in build files is considered very problematic. For example, I want to make a tool that uses the moz.build files to output a compilation database--having make syntax in some DEFINES would render that inoperable.

Almost all of the uses of complex logic in DEFINES rules are either going to be in */installer (which don't use moz.build anyways) or in */locales, which are affected by l10n repacks.

I think we may do heinous things here like make -C <dir> AB_CD=blah.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Joshua Cranmer [:jcranmer] (high latency until August 11) from comment #3)
> (In reply to Nathan Froyd (:froydnj) from comment #0)
> > But the DEFINES machinery doesn't really have anything for "some value that
> > will actually be processed by make first", so we need a fourth type of thing
> > for DEFINES keys, to go with numbers, strings, and booleans.  Or we could
> > say that DEFINES keys with an initial character of '$' will be unquoted, but
> > that seems footgunnish.
> 
> Using make syntax in build files is considered very problematic. For
> example, I want to make a tool that uses the moz.build files to output a
> compilation database--having make syntax in some DEFINES would render that
> inoperable.

Having it be a separate type would at least permit alternate processors to do intelligent things with it.

> I think we may do heinous things here like make -C <dir> AB_CD=blah.

Yes, grepping for AB_CD reveals usages along those lines.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: make it possible to add DEFINES that are build-time values

Here's a crack at doing this, since I think it's not completely wrong to do
this.  I've tried to make things build-system agnostic, even with the ugliness
that needs to take place between emitter.py and recursivemake.py.

I've included some example Makefile.in conversions; there are a number more of
these that can go in if this sort of change is acceptable.

It's possible that what really needs to happen here is a complete overhaul of
how we do locales.  I'm not prepared to do that at this point, and I don't
think holding up the moz.build conversion on doing so is reasonable.  Alternate
opinions welcome.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8465476 [details] [diff] [review]
make it possible to add DEFINES that are build-time values

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

I think it's a footgun, and I'd rather avoid footguns. It's a footgun because you can be sure at some point someone will forget to use the build-time thingy.

Now, with that being said, I'd rather know more on the scope of this. Are there other variables than AB_CD you'd use that for? If yes, which ones are they?

For AB_CD, until locales are done sanely, how about harcoding DEFINES += -DAB_CD=$(AB_CD) in config.mk?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 8465476 [details] [diff] [review]
> make it possible to add DEFINES that are build-time values
> 
> Review of attachment 8465476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it's a footgun, and I'd rather avoid footguns. It's a footgun
> because you can be sure at some point someone will forget to use the
> build-time thingy.

*shrug*  Things will break, just as they'd break if you did DEFINES += -DAB_CD=AB_CD.  Admittedly there's scope to forget things, but I don't think it's worse than the current state.

> Now, with that being said, I'd rather know more on the scope of this. Are
> there other variables than AB_CD you'd use that for? If yes, which ones are
> they?

I don't know.  I'd be willing to bet there are various things related to locales, and maybe some related to branding.  I'm still wading through things.

> For AB_CD, until locales are done sanely, how about harcoding DEFINES +=
> -DAB_CD=$(AB_CD) in config.mk?

That's two votes from build people for this solution.  WFM, I guess...

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: move DEFINES += -DAB_CD=$(AB_CD) pattern into config.mk

Might as well see what a third build person says about this...

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8466281 [details] [diff] [review]
move DEFINES += -DAB_CD=$(AB_CD) pattern into config.mk

Sounds good to me :). Besides, when has changing anything related to l10n ever gone wrong?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8466281 [details] [diff] [review]
move DEFINES += -DAB_CD=$(AB_CD) pattern into config.mk

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

::: browser/app/Makefile.in
@@ +7,5 @@
>  PREF_JS_EXPORTS = $(srcdir)/profile/firefox.js \
>  		  $(NULL)
>  
>  # hardcode en-US for the moment
>  AB_CD = en-US

You may be losing this

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to :Ms2ger from comment #10)
> Comment on attachment 8466281 [details] [diff] [review]
> move DEFINES += -DAB_CD=$(AB_CD) pattern into config.mk
> 
> Review of attachment 8466281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/Makefile.in
> @@ +7,5 @@
> >  PREF_JS_EXPORTS = $(srcdir)/profile/firefox.js \
> >  		  $(NULL)
> >  
> >  # hardcode en-US for the moment
> >  AB_CD = en-US
> 
> You may be losing this

Why do you say that?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

DEFINES is probably evaluated lazily, so might be fine.

Comment 13

[Nathan Froyd [:froydnj]]

Light testing indicates this doesn't break anything on tbpl: https://tbpl.mozilla.org/?tree=Try&rev=fc6e75dd48c1

I'll check this in tomorrow.

Comment 14

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43529d2194a0

Note to sheriffs: this has a definite chance of causing problems with localized builds, so if anything with localized builds looks suspicious, this is the patch to back out.

Comment 15

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/43529d2194a0