Closed
Bug 1186782
Opened 10 years ago
Closed 10 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)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 42.0
People
(Reporter: ewong, Assigned: iannbugzilla)
References
Details
(Keywords: dogfood, regression)
Attachments
(2 files, 6 obsolete files)
26.21 KB,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
12.93 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
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....
Reporter | ||
Updated•10 years ago
|
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'
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Updated•10 years ago
|
Product: MailNews Core → Thunderbird
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Updated•10 years ago
|
OS: Unspecified → Linux
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Comment 43•10 years ago
|
||
This went through try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f77164f048cc
Reporter | ||
Comment 44•10 years ago
|
||
Attachment #8638356 -
Attachment is obsolete: true
Attachment #8638356 -
Flags: review?(Pidgeot18)
Attachment #8638385 -
Flags: review?(Pidgeot18)
Updated•10 years ago
|
Keywords: regression
Comment 45•10 years ago
|
||
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)?
Comment 46•10 years ago
|
||
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 47•10 years ago
|
||
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.
Reporter | ||
Comment 48•10 years ago
|
||
(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. :)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 67•10 years ago
|
||
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 68•10 years ago
|
||
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-
Assignee | ||
Comment 69•10 years ago
|
||
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)
Assignee | ||
Comment 70•10 years ago
|
||
Attachment #8639042 -
Flags: review?(florian)
Attachment #8639042 -
Flags: review?(clokep)
Assignee | ||
Comment 71•10 years ago
|
||
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)
Assignee | ||
Comment 72•10 years ago
|
||
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 73•10 years ago
|
||
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+
Assignee | ||
Comment 74•10 years ago
|
||
Added other-licenses changes for TB, carrying forward r=
Attachment #8639044 -
Attachment is obsolete: true
Attachment #8639095 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Assignee: ewong → iann_bugzilla
Assignee | ||
Comment 75•10 years ago
|
||
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)
Assignee | ||
Comment 76•10 years ago
|
||
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]
Comment 77•10 years ago
|
||
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+
Assignee | ||
Comment 78•10 years ago
|
||
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]
Assignee | ||
Comment 79•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•