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

RESOLVED FIXED in Thunderbird 35.0

Status

MailNews Core
Build Config
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jcranmer, Assigned: Ian Neal)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 35.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 833123 [details] [diff] [review]
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.
Attachment #833123 - Flags: review?(mbanner)
Attachment #833123 - Flags: review?(mbanner) → review+
(Reporter)

Comment 2

4 years ago
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?]
Attachment #833123 - Attachment description: Part 1: mailnews/ files → Part 1: mailnews/ files [Checkin: see comment 2]
(Assignee)

Comment 3

3 years ago
Created attachment 8474258 [details] [diff] [review]
Part 2: mailnews/mapi files [Checked in: Comment 20]

All of the friends mentioned have been removed by other patches.
Assignee: Pidgeot18 → iann_bugzilla
Attachment #8474258 - Flags: review?(Pidgeot18)
(Assignee)

Updated

3 years ago
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 4

3 years ago
Created attachment 8474259 [details] [diff] [review]
Part 3: mail/ files

CHROME_DEPS became obsolete due to the build system changes.
Attachment #8474259 - Flags: review?(Pidgeot18)
(Assignee)

Comment 5

3 years ago
Created attachment 8474260 [details] [diff] [review]
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.
Attachment #8474260 - Flags: review?(Pidgeot18)
(Assignee)

Comment 6

3 years ago
Created attachment 8474261 [details] [diff] [review]
Part 5: im/ files

Much the same as the mail/ changes. CHROME_DEPS was obsoleted by build system changes.
Attachment #8474261 - Flags: review?(Pidgeot18)
(Assignee)

Comment 7

3 years ago
Try server - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e3d4246d40f0
(Reporter)

Comment 8

3 years ago
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'] = ''
Attachment #8474259 - Flags: review?(Pidgeot18) → review-
(Reporter)

Comment 9

3 years ago
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.
Attachment #8474260 - Flags: review?(Pidgeot18) → review?(bugspam.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 ;-)
Attachment #8474260 - Flags: review?(bugspam.Callek) → review+
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?
Attachment #8474261 - Flags: feedback+
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?
(Reporter)

Comment 13

3 years ago
(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.
(Reporter)

Comment 14

3 years ago
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...
Attachment #8474258 - Flags: review?(Pidgeot18) → review+
(Reporter)

Comment 15

3 years ago
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.
Attachment #8474261 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 16

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

Changes as suggested plus the XPCOM_GLUE one that Callek spotted
Attachment #8474259 - Attachment is obsolete: true
Attachment #8481585 - Flags: review?(Pidgeot18)
(Assignee)

Comment 17

3 years ago
Created attachment 8481589 [details] [diff] [review]
Part 4 v2: suite/ changes [Checked in: Comment 21]

Changes as suggested by Callek, carrying forward r+
Attachment #8474260 - Attachment is obsolete: true
Attachment #8481589 - Flags: review+
(Assignee)

Comment 18

3 years ago
Created attachment 8481600 [details] [diff] [review]
Part 5 v2: im/ files [Checked in: Comment 22]

Changes as suggested plus the XPCOM_GLUE one suggested by Callek. Carrying forward r+
Attachment #8474261 - Attachment is obsolete: true
Attachment #8481600 - Flags: review+
(Assignee)

Comment 19

3 years ago
Created attachment 8481624 [details] [diff] [review]
Part 6: calendar/ files [Checked in: Comment 23]

There may be a couple I have missed.
Attachment #8481624 - Flags: review?(philipp)
Attachment #8481624 - Flags: review?(philipp) → review+
(Assignee)

Comment 20

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

http://hg.mozilla.org/comm-central/rev/77c907e1b6ba
Attachment #8474258 - Attachment description: Part 2: mailnews/mapi files → Part 2: mailnews/mapi files [Checked in: Comment 20]
(Assignee)

Comment 21

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

http://hg.mozilla.org/comm-central/rev/7f8be51ebb19
Attachment #8481589 - Attachment description: Part 4 v2: suite/ changes → Part 4 v2: suite/ changes [Checked in: Comment 21]
(Assignee)

Comment 22

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

http://hg.mozilla.org/comm-central/rev/f1a862465033
Attachment #8481600 - Attachment description: Part 5 v2: im/ files → Part 5 v2: im/ files [Checked in: Comment 22]
(Assignee)

Comment 23

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

http://hg.mozilla.org/comm-central/rev/3fafdae610c9
Attachment #8481624 - Attachment description: Part 6: calendar/ files → Part 6: calendar/ files [Checked in: Comment 23]
(Reporter)

Updated

3 years ago
Attachment #8481585 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 24

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

http://hg.mozilla.org/comm-central/rev/91c36d25d113
Attachment #8481585 - Attachment description: Part 3 v2: mail/ files → Part 3 v2: mail/ files [Checked in: Comment 24]
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 35.0
You need to log in before you can comment on or make changes to this bug.