882968 - Clean up and move DEFINES and friends to moz.build in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Joshua Cranmer [:jcranmer]]

I've looked at moving these once or twice, and have decided that doing this as an automatic migration is unwise, between things like the quotation issue and the use of auxiliary build variables here. The "friends" here is mostly poor use of OS_CXXFLAGS and maybe an odd CXXFLAGS.

The -DNOMINMAX should hopefully be unnecessary; if not, then bug 830801 is a better option. The -DUNICODE as well should be unncessary; -DMOZ_UNICODE is almost certainly not useful these days, etc.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: mailnews/ files [Checkin: see comment 2]

Green try run: <https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=23603fcc864c>.

This moves most of the code under mailnews/, save MAPI code.

Comment 2

[Joshua Cranmer [:jcranmer]]

Comment on attachment 833123 [details] [diff] [review]
Part 1: mailnews/ files [Checkin: see comment 2]

https://hg.mozilla.org/comm-central/rev/0fef4c10f850

[why don't we have the checkin+ flag available for Mailnews Core products?]

Comment 3

[Ian Neal]

Attachment: Part 2: mailnews/mapi files [Checked in: Comment 20]

All of the friends mentioned have been removed by other patches.

Comment 4

[Ian Neal]

Attachment: Part 3: mail/ files

CHROME_DEPS became obsolete due to the build system changes.

Comment 5

[Ian Neal]

Attachment: Part 4: suite/ changes

Much the same as the mail/ changes, the libs change in the classic Makefile.in is due to fixing a DOS line ending.

Comment 6

[Ian Neal]

Attachment: Part 5: im/ files

Much the same as the mail/ changes. CHROME_DEPS was obsoleted by build system changes.

Comment 7

[Ian Neal]

Try server - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e3d4246d40f0

Comment 8

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8474259 [details] [diff] [review]
Part 3: mail/ files

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

I had some problems getting this patch applied due to it not being clear which order everything was supposed to be applied in, and DEFINES changes in general scare me a lot, so I want to look at this again, preferably not when I have to apply 10 patches to get a patch to cleanly apply.

::: mail/app/moz.build
@@ +44,5 @@
>  
>      if CONFIG['MOZ_LINKER']:
>          OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>  
> +DEFINES['APP_VERSION'] = '"%s"' % CONFIG['MOZ_APP_VERSION']

I'm guessing this should just be DEFINES['APP_VERSION'] = CONFIG['MOZ_APP_VERSION'], but I'm honestly having a hard time trying to track down where this actually gets used.

::: mail/base/Makefile.in
@@ +10,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  PRE_RELEASE_SUFFIX := $(shell $(PYTHON) $(topsrcdir)/mozilla/config/printprereleasesuffix.py \
>                        $(MOZ_APP_VERSION))

These lines are dead code as of 3 years ago (bug 669504). If you also port the DEFINES below, this Makefile is dead!

@@ +12,5 @@
>  
>  PRE_RELEASE_SUFFIX := $(shell $(PYTHON) $(topsrcdir)/mozilla/config/printprereleasesuffix.py \
>                        $(MOZ_APP_VERSION))
>  
> +DEFINES += -DPRE_RELEASE_SUFFIX=""

You ought to be able to port this with DEFINES['PRE_RELEASE_SUFFIX'] = ''

Comment 9

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8474260 [details] [diff] [review]
Part 4: suite/ changes

Redirecting to Callek, as I don't feel quite so confident being able to go over this with a fine-tooth comb.

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 8474260 [details] [diff] [review]
Part 4: suite/ changes

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

I'm assuming this all builds locally, with same (binary-compile -diff abstracting) output. but r+=me based on inspection here and comparing with jcranmers review of mail/ in this bug

::: suite/app/moz.build
@@ +39,5 @@
>  
>      if CONFIG['MOZ_LINKER']:
>          OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>  
> +DEFINES['APP_VERSION'] = '"%s"' % CONFIG['MOZ_APP_VERSION']

if you change the mail/ version of this assignment please change this one too

@@ +41,5 @@
>          OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>  
> +DEFINES['APP_VERSION'] = '"%s"' % CONFIG['MOZ_APP_VERSION']
> +
> +DEFINES['NO_BLOCKLIST_CRASHREPORTER'] = True

only use of NO_BLOCKLIST_CRASHREPORTER is in c-c, here, we can probably drop.

@@ +44,5 @@
> +
> +DEFINES['NO_BLOCKLIST_CRASHREPORTER'] = True
> +
> +DEFINES['XPCOM_GLUE'] = True
> +

I don't see it called out in joshua's review of mail/, but won't XPCOM_GLUE defined break the LIBXUL_SDK situation?

::: suite/browser/moz.build
@@ +13,5 @@
>  
>  JAR_MANIFESTS += ['jar.mn']
> +
> +for var in ('MOZ_APP_NAME', 'MOZ_APP_DISPLAYNAME', 'MOZ_APP_VERSION'):
> +    DEFINES[var] = '"%s"' % CONFIG[var]

sure why not ;-)

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8474261 [details] [diff] [review]
Part 5: im/ files

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

::: im/app/moz.build
@@ +40,5 @@
>  
>      if CONFIG['MOZ_LINKER']:
>          OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>  
> +DEFINES['APP_VERSION'] = '"%s"' % CONFIG['MOZ_APP_VERSION']

Let's keep this in sync with the mail/ version (if you change it due to comment 8).

::: im/branding/halloween/locales/Makefile.in
@@ +9,5 @@
>  relativesrcdir = @relativesrcdir@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
> +DEFINES += -DAB_CD=$(AB_CD)

Is there a reason it's still OK to have the AB_CD variable be defined from the Makefile.in file?

Or more generally, how did you decide which defines to move to moz.build and which to keep in Makefile.in?

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 8474261 [details] [diff] [review]
Part 5: im/ files

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

::: im/content/moz.build
@@ +9,5 @@
> +for var in ('MOZ_APP_NAME', 'MOZ_MACBUNDLE_NAME'):
> +    DEFINES[var] = CONFIG[var]
> +
> +#if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('windows', 'gtk2', 'mac', 'cocoa'):
> +#    DEFINES['HAVE_SHELL_SERVICE'] = 1

Is there value in having this commented out? Should we just nuke it?

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> @@ +9,5 @@
> >  relativesrcdir = @relativesrcdir@
> >  
> >  include $(DEPTH)/config/autoconf.mk
> >  
> > +DEFINES += -DAB_CD=$(AB_CD)
> 
> Is there a reason it's still OK to have the AB_CD variable be defined from
> the Makefile.in file?

Locale repacks work by effectively running make -C path/to/bar AB_CD=en-GB or what have you.

Comment 14

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8474258 [details] [diff] [review]
Part 2: mailnews/mapi files [Checked in: Comment 20]

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

That we have to have the defines for UNICODE and _UNICODE makes me sad...

Comment 15

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8474261 [details] [diff] [review]
Part 5: im/ files

Since florian f+'d this, I'm treating that as an r+ from him.

Comment 16

[Ian Neal]

Attachment: Part 3 v2: mail/ files [Checked in: Comment 24]

Changes as suggested plus the XPCOM_GLUE one that Callek spotted

Comment 17

[Ian Neal]

Attachment: Part 4 v2: suite/ changes [Checked in: Comment 21]

Changes as suggested by Callek, carrying forward r+

Comment 18

[Ian Neal]

Attachment: Part 5 v2: im/ files [Checked in: Comment 22]

Changes as suggested plus the XPCOM_GLUE one suggested by Callek. Carrying forward r+

Comment 19

[Ian Neal]

Attachment: Part 6: calendar/ files [Checked in: Comment 23]

There may be a couple I have missed.

Comment 20

[Ian Neal]

Comment on attachment 8474258 [details] [diff] [review]
Part 2: mailnews/mapi files [Checked in: Comment 20]

http://hg.mozilla.org/comm-central/rev/77c907e1b6ba

Comment 21

[Ian Neal]

Comment on attachment 8481589 [details] [diff] [review]
Part 4 v2: suite/ changes [Checked in: Comment 21]

http://hg.mozilla.org/comm-central/rev/7f8be51ebb19

Comment 22

[Ian Neal]

Comment on attachment 8481600 [details] [diff] [review]
Part 5 v2: im/ files [Checked in: Comment 22]

http://hg.mozilla.org/comm-central/rev/f1a862465033

Comment 23

[Ian Neal]

Comment on attachment 8481624 [details] [diff] [review]
Part 6: calendar/ files [Checked in: Comment 23]

http://hg.mozilla.org/comm-central/rev/3fafdae610c9

Comment 24

[Ian Neal]

Comment on attachment 8481585 [details] [diff] [review]
Part 3 v2: mail/ files [Checked in: Comment 24]

http://hg.mozilla.org/comm-central/rev/91c36d25d113