Closed Bug 1186782 Opened 5 years ago Closed 5 years ago

nsMailComps.o: In function `DirectoryProvider': mail/components/build/../shell/DirectoryProvider.h:35: undefined reference to `vtable for mozilla::mail::DirectoryProvider'

Categories

(MailNews Core :: Build Config, defect, blocker)

All
Linux
defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 42.0

People

(Reporter: ewong, Assigned: iann_bugzilla)

References

Details

(Keywords: dogfood, regression)

Attachments

(2 files, 6 obsolete files)

On  bld-linux64-spot-201 for TB Linux x86-64 comm-central build,

Getting:

./../mail/components/build/nsMailComps.o: In function `DirectoryProvider':
/builds/slave/tb-c-cen-l64-00000000000000000/build/mail/components/build/../shell/DirectoryProvider.h:35: undefined reference to `vtable for mozilla::mail::DirectoryProvider'
/usr/bin/ld: ../../mail/components/build/nsMailComps.o: relocation R_X86_64_PC32 against undefined hidden symbol `_ZTVN7mozilla4mail17DirectoryProviderE' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
make[5]: *** [libxul.so] Error 1
make[5]: Leaving directory `/builds/slave/tb-c-cen-l64-00000000000000000/build/objdir-tb/toolkit/library'
make[4]: *** [toolkit/library/target] Error 2
make[4]: *** Waiting for unfinished jobs....
Summary: nsMailComps.o: In function `DirectoryProvider': /builds/slave/tb-c-cen-l64-00000000000000000/build/mail/components/build/../shell/DirectoryProvider.h:35: undefined reference to `vtable for mozilla::mail::DirectoryProvider' → nsMailComps.o: In function `DirectoryProvider': mail/components/build/../shell/DirectoryProvider.h:35: undefined reference to `vtable for mozilla::mail::DirectoryProvider'
Product: MailNews Core → Thunderbird
OS: Unspecified → Linux
Attached patch proposed patch(v1) (obsolete) — Splinter Review
This went through try:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f77164f048cc
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8638356 - Flags: review?(Pidgeot18)
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8638356 - Attachment is obsolete: true
Attachment #8638356 - Flags: review?(Pidgeot18)
Attachment #8638385 - Flags: review?(Pidgeot18)
Severity: normal → blocker
Keywords: dogfood
Keywords: regression
Comment on attachment 8638385 [details] [diff] [review]
proposed patch (v2)

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

::: mail/components/shell/moz.build
@@ +13,5 @@
>  
>  if CONFIG['OS_ARCH'] == 'WINNT':
>      SOURCES += ['nsMailWinIntegration.cpp']
>  
> +if CONFIG['MOZ_WIDGET_GTK']:

You have many places where you replaced the == with just a def check. Shouldn't this be instead:

if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3'):

(here and various other places)?
OK I picked a bad spot in the previous comment, but the question still stands. You replaced equality with gtk2 to checking for existence in a number of places. Why?
Comment on attachment 8638385 [details] [diff] [review]
proposed patch (v2)

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

::: mail/app/Makefile.in
@@ +109,5 @@
>  libs:: $(addprefix $(DIST)/branding/,$(BRANDED_ICON_FILES))
>  	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/chrome/icons/default
>  endif
>  
> +ifdef MOZ_WIDGET_TOOLKIT

This spot is a better example.
(In reply to Kent James (:rkent) from comment #45)
> Comment on attachment 8638385 [details] [diff] [review]
> proposed patch (v2)
> 
> Review of attachment 8638385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/shell/moz.build
> @@ +13,5 @@
> >  
> >  if CONFIG['OS_ARCH'] == 'WINNT':
> >      SOURCES += ['nsMailWinIntegration.cpp']
> >  
> > +if CONFIG['MOZ_WIDGET_GTK']:
> 
> You have many places where you replaced the == with just a def check.
> Shouldn't this be instead:
> 
> if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('gtk2', 'gtk3'):
> 
> (here and various other places)?

MOZ_WIDGET_GTK is defined only on platforms that use either
cairo-gtk2 or cairo-gtk3, so it is sufficient (from what I
see) to just check the existence of the value.  It's actually
equivalent to what you put; but cleaner. :)
As I said, my comment 45 was an error, I meant to comment (per my correction in comment 47) about using MOZ_WIDGET_TOOLKIT, such as this change:

-ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
+ifdef MOZ_WIDGET_TOOLKIT

I see lots of code like this:

 elif CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows':

that shows that MOZ_WIDGET_TOOLKIT is not linux-specific.
Comment on attachment 8638385 [details] [diff] [review]
proposed patch (v2)

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

You're missing a few changes where MOZ_WIDGET_GTK2 is used in code:
<https://dxr.mozilla.org/comm-central/search?q=MOZ_WIDGET_GTK2&case=true>. The changes in nsMailComps.cpp are absolutely necessary to get the default application stuff working. The change to enable_drag_images are less clear if they're necessary (although that looks to be bug 450971).

::: mail/branding/aurora/Makefile.in
@@ +20,5 @@
>  	cp $(srcdir)/dsstore           $(DIST)/branding/dsstore
>  	cp $(srcdir)/disk.icns         $(DIST)/branding/disk.icns
>  	cp $(srcdir)/thunderbird.icns  $(DIST)/branding/thunderbird.icns
>  endif
> +ifdef MOZ_WIDGET_TOOLKIT

As rkent says, these lines are busted, since it's selecting these files for all platforms, whereas the desire is only for Linux ones.
Attachment #8638385 - Flags: review?(Pidgeot18) → review-
The only thing I wasn't sure about was whether it was worth making changes to mailnews/build/nsMailModule.cpp

Pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fc221c9a961a
Attachment #8638385 - Attachment is obsolete: true
Attachment #8639041 - Flags: review?(Pidgeot18)
Hardware: Unspecified → All
Version: unspecified → Trunk
Product: Thunderbird → MailNews Core
Attached patch GTK changes for IM (obsolete) — Splinter Review
Attachment #8639042 - Flags: review?(florian)
Attachment #8639042 - Flags: review?(clokep)
Blocks: 882036
Changes since last patch:
* Missed package-manifest entries for mozgtk* in suite/
Attachment #8639041 - Attachment is obsolete: true
Attachment #8639041 - Flags: review?(Pidgeot18)
Attachment #8639044 - Flags: review?(Pidgeot18)
Attached patch GTK changes for IM v1.1 (obsolete) — Splinter Review
Changes since previous patch:
* Missed package-manifest changes for mozgtk* entries.
Attachment #8639042 - Attachment is obsolete: true
Attachment #8639042 - Flags: review?(florian)
Attachment #8639042 - Flags: review?(clokep)
Attachment #8639045 - Flags: review?(florian)
Attachment #8639045 - Flags: review?(clokep)
Comment on attachment 8639044 [details] [diff] [review]
GTK changes for TB, SM and MailNews v1.1

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

You're missing other-licenses/branding/thunderbird/Makefile.in, but otherwise everything looks good.

Although it looks like our removed-files.in could use a bit of pruning.
Attachment #8639044 - Flags: review?(Pidgeot18) → review+
Added other-licenses changes for TB, carrying forward r=
Attachment #8639044 - Attachment is obsolete: true
Attachment #8639095 - Flags: review+
Assignee: ewong → iann_bugzilla
Added changes in other-licenses for InstantBird
Attachment #8639045 - Attachment is obsolete: true
Attachment #8639045 - Flags: review?(florian)
Attachment #8639045 - Flags: review?(clokep)
Attachment #8639096 - Flags: review?(florian)
Attachment #8639096 - Flags: review?(clokep)
Comment on attachment 8639095 [details] [diff] [review]
GTK changes for TB, SM and MailNews v1.2 [Checked in: Comment 76 & Comment 79]

https://hg.mozilla.org/comm-central/rev/ae95dbce7c48
Attachment #8639095 - Attachment description: GTK changes for TB, SM and MailNews v1.2 → GTK changes for TB, SM and MailNews v1.2 [Checked in: Comment 76]
Target Milestone: --- → Thunderbird 42.0
Comment on attachment 8639096 [details] [diff] [review]
GTK changes for IM v1.2 [Checked in: Comment 78]

Thanks!
Attachment #8639096 - Flags: review?(florian)
Attachment #8639096 - Flags: review?(clokep)
Attachment #8639096 - Flags: review+
Comment on attachment 8639096 [details] [diff] [review]
GTK changes for IM v1.2 [Checked in: Comment 78]

https://hg.mozilla.org/comm-central/rev/a875b1e5aa1c
Attachment #8639096 - Attachment description: GTK changes for IM v1.2 → GTK changes for IM v1.2 [Checked in: Comment 78]
Comment on attachment 8639095 [details] [diff] [review]
GTK changes for TB, SM and MailNews v1.2 [Checked in: Comment 76 & Comment 79]

Remove unneeded gtk argument:
https://hg.mozilla.org/comm-central/rev/28bda87ffa1f
Attachment #8639095 - Attachment description: GTK changes for TB, SM and MailNews v1.2 [Checked in: Comment 76] → GTK changes for TB, SM and MailNews v1.2 [Checked in: Comment 76 & Comment 79]
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1188965
You need to log in before you can comment on or make changes to this bug.