888660 - MOZ_MOVEMAIL ifdef not working for account central page

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[alta88]

the ifdef here
http://mxr.mozilla.org/comm-central/source/mailnews/base/content/msgAccountCentral.xul#193

doesn't result in the Movemail label appearing in linux.

Comment 1

[:aceman]

What about HAVE_MOVEMAIL ?

Comment 2

[alta88]

there's no Makefile.in in mailnews/base/content, hence no such declaration.  since build config is in flux, i don't know what the proper solution is, ie a Makefile.in or putting this in a moz.build.

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to alta88 from comment #2)
> there's no Makefile.in in mailnews/base/content, hence no such declaration. 
> since build config is in flux, i don't know what the proper solution is, ie
> a Makefile.in or putting this in a moz.build.

The msgAccountCentral.xul gets preprocessed from mailnews/' jar.mn, so the DEFINES declaration in mailnews/Makefile.in is the appropriate place.

Now, that said, adding things to DEFINES in makefiles is still Bad Practice™, and given the number of places we ad-hoc add it to defines, plopping an AC_DEFINE in the configure.in files would work much better IMHO.

Comment 4

[alta88]

well, it's all over the place and what should be done is the Right Thing™ but that often leads straight to No Good Deed...™

since there is no mailnews/configure.in 
1. should one be created, for this
2. does mailnews/ pick up root configure.in
3. does mail/ pick up root, making it unnecessary in its configure.in

you have a better idea of where build config is going, so the most effective thing to do is sketch out specifically how best to clean up this particular definition.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to alta88 from comment #4)
> well, it's all over the place and what should be done is the Right Thing™
> but that often leads straight to No Good Deed...™
> 
> since there is no mailnews/configure.in 
> 1. should one be created, for this
> 2. does mailnews/ pick up root configure.in
> 3. does mail/ pick up root, making it unnecessary in its configure.in

4. Put the definitions in mail/configure.in and suite/configure.in, around the other AC_SUBST(MOZ_MOVEMAIL) lines (indeed, suite/configure.in and mail/configure.in are nearly full clones of each other).

Comment 6

[alta88]

Attachment: patch

this builds/works; if no issues a suite r will be requested.

Comment 7

[Joshua Cranmer [:jcranmer]]

Comment on attachment 771343 [details] [diff] [review]
patch

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

The configure.in files seem wrong; HAVE_MOVEMAIL=1 should also be defined beforehand.

Comment 8

[alta88]

Attachment: patch2

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 771880 [details] [diff] [review]
patch2

You want a build config review for this, and as it happens I'm a peer of the SM module, so r=me.

Comment 10

[alta88]

this should follow bug 529131.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7d8d252b7056

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 771880 [details] [diff] [review]
patch2

[Triage Comment]
Will take to aurora, as bug 529131 landed there

Comment 13

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/b2ae1dae3739