Closed Bug 888660 Opened 6 years ago Closed 6 years ago

MOZ_MOVEMAIL ifdef not working for account central page

Categories

(Thunderbird :: Build Config, defect)

x86_64
Linux
defect
Not set

Tracking

(thunderbird24+ fixed)

RESOLVED FIXED
Thunderbird 25.0
Tracking Status
thunderbird24 + fixed

People

(Reporter: alta88, Assigned: alta88)

References

Details

Attachments

(1 file, 1 obsolete file)

3.92 KB, patch
jcranmer
: review+
standard8
: review+
Details | Diff | Splinter Review
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.
What about HAVE_MOVEMAIL ?
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.
Flags: needinfo?(mbanner)
(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.
Flags: needinfo?(mbanner)
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.
(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).
Attached patch patch (obsolete) — Splinter Review
this builds/works; if no issues a suite r will be requested.
Assignee: nobody → alta88
Attachment #771343 - Flags: review?(Pidgeot18)
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.
Attachment #771343 - Flags: review?(Pidgeot18) → review-
Attached patch patch2Splinter Review
Attachment #771880 - Flags: review?(Pidgeot18)
Attachment #771880 - Flags: review?(Pidgeot18) → review+
Attachment #771880 - Flags: review?(neil)
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.
Attachment #771880 - Flags: review?(neil) → review+
this should follow bug 529131.
Attachment #771343 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/7d8d252b7056
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Blocks: 529131
Comment on attachment 771880 [details] [diff] [review]
patch2

[Triage Comment]
Will take to aurora, as bug 529131 landed there
Attachment #771880 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.