Closed Bug 1042226 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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?
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.
(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?
(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.
(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.
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.
Attachment #8465476 - Flags: feedback?(mshal)
Attachment #8465476 - Flags: feedback?(mh+mozilla)
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?
Attachment #8465476 - Flags: feedback?(mh+mozilla) → feedback-
(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...
Blocks: nomakefiles
Summary: permit DEFINES['foo'] = LiteralThing('$(MAKEFILE_VAR)') → move DEFINES += -DAB_CD=$(AB_CD) pattern into config.mk
Might as well see what a third build person says about this...
Attachment #8465476 - Attachment is obsolete: true
Attachment #8465476 - Flags: feedback?(mshal)
Attachment #8466281 - Flags: review?(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?
Attachment #8466281 - Flags: review?(mshal) → review+
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
(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?
Flags: needinfo?(Ms2ger)
DEFINES is probably evaluated lazily, so might be fine.
Flags: needinfo?(Ms2ger)
Light testing indicates this doesn't break anything on tbpl: https://tbpl.mozilla.org/?tree=Try&rev=fc6e75dd48c1

I'll check this in tomorrow.
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.
Assignee: nobody → nfroyd
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/43529d2194a0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.