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.

Green try run: <https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=23603fcc864c>. This moves most of the code under mailnews/, save MAPI code.

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

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

CHROME_DEPS became obsolete due to the build system changes.

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

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

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

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

(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 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 on attachment 8474261 [details] [diff] [review] Part 5: im/ files Since florian f+'d this, I'm treating that as an r+ from him.

Changes as suggested plus the XPCOM_GLUE one that Callek spotted

Changes as suggested by Callek, carrying forward r+

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

There may be a couple I have missed.

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 on attachment 8481589 [details] [diff] [review] Part 4 v2: suite/ changes [Checked in: Comment 21] http://hg.mozilla.org/comm-central/rev/7f8be51ebb19

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 on attachment 8481624 [details] [diff] [review] Part 6: calendar/ files [Checked in: Comment 23] http://hg.mozilla.org/comm-central/rev/3fafdae610c9

Comment on attachment 8481585 [details] [diff] [review] Part 3 v2: mail/ files [Checked in: Comment 24] http://hg.mozilla.org/comm-central/rev/91c36d25d113