Closed Bug 306324 Opened 19 years ago Closed 8 years ago

Build/run Thunderbird on top of XULRunner

Categories

(Thunderbird :: Build Config, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: benjamin, Assigned: jhorak)

References

()

Details

Attachments

(7 files, 12 obsolete files)

1.98 KB, patch
mconley
: review+
Details | Diff | Splinter Review
18.11 KB, patch
neil
: review+
Details | Diff | Splinter Review
4.83 KB, patch
jhorak
: review+
Details | Diff | Splinter Review
3.53 KB, patch
Details | Diff | Splinter Review
837 bytes, patch
mconley
: review+
Details | Diff | Splinter Review
2.58 KB, patch
jhorak
: review+
Details | Diff | Splinter Review
1.51 KB, patch
standard8
: review+
Details | Diff | Splinter Review
This is the Thunderbird corollary of bug 299997 (build Firefox on XULRunner).
For the moment I'm going to target the 1.9 cycle, but depending on how hard the
autocomplete stuff is this may get bumped.
Priority: -- → P3
Target Milestone: --- → Thunderbird2.0
Correction:
This is the Thunderbird corollary of bug 315452 (Build Firefox on top of XULRunner).
If there's anything I can do to help, please let me know.
Depends on: 330615
Target Milestone: Thunderbird2.0 → Thunderbird 3
QA Contact: chase → build
/me cackles maniacally and reassigns to mscott, who has been working on making tbird use frozen linkage
Assignee: benjamin → mscott
Thanks Benjamin. I'm currently working on the following bugs in preparation for this:

Bug 379070 --> the great mailnews string cleanup of 2007 (including removing nsXPIDLString)
Bug 377319 --> move mailnews to frozen linkages

Also see:

http://wiki.mozilla.org/Thunderbird:Architecture_Cleanup
Severity: enhancement → normal
Status: NEW → ASSIGNED
Depends on: 377319, 379070
Flags: blocking-thunderbird3+
Summary: Build/run Tbird on top of XULRunner → Build/run Thunderbird on top of XULRunner
Depends on: 395701
QA Contact: build → build-config
Depends on: 425077
Depends on: 239131, 360648, 424641
No longer depends on: 263042
Version: unspecified → Trunk
Due to the amount of work this still requires, this is highly unlikely to make it for the TB 3.0 release. Therefore taking off the blocking 3.0 list.
Assignee: mscott → nobody
Status: ASSIGNED → NEW
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
Target Milestone: Thunderbird 3 → ---
No longer depends on: 379070, 395701
Hardware: PC → All
No longer depends on: 377319
Removing dep that should be covered by the mailnews dep.
No longer depends on: 360648
(In reply to comment #6)
> Removing dep that should be covered by the mailnews dep.

err maybe not, sorry for the bugspam.
Depends on: 360648
Blocks: 468835
Depends on: 582943
Now if there were only a "blocking 3.1" list to put this on.
Depends on: 583222
(In reply to comment #8)
> Now if there were only a "blocking 3.1" list to put this on.

Or blocking 3.2.
(In reply to comment #9)
> (In reply to comment #8)
> > Now if there were only a "blocking 3.1" list to put this on.
> 
> Or blocking 3.2.

We will currently not be blocking any release on this bug, so please don't keep spamming the bug suggesting it.

We need to move Thunderbird in the same direction as Firefox, and whilst Firefox supports xulrunner configurations, they are not looking at shipping in that configuration. Likewise (and partially as a result), there is no significant need/benefit to ship Thunderbird in that configuration.
Depends on: 598646
Attached patch wip patch 1 (obsolete) — Splinter Review
I've managed to build thunderbird with separated libxul with ldap disabled. Attaching WIP patch which works with --enable-incomplete-external-linkage and --with-libxul-sdk set. Please have a little glance at it and let me know what's totally wrong with it.
Attached patch Convert rest to frozen linkages (obsolete) — Splinter Review
There showed up some internal API functions which makes building with libxul impossible. Attaching patch which convert these internal API to frozen linkages.
Attachment #571597 - Flags: review?(neil)
This patch solves wrong URIs when running TB with external libxul. I'm not sure how it affect build without using external libxul, though.
Attachment #570979 - Attachment is obsolete: true
Attachment #571599 - Flags: review?
Attached patch Makefiles and config patch 1 (obsolete) — Splinter Review
This patch contains makefile changes which makes mork a libmork.so component (while it is not part of xulrunner - i. e. libxul). It also adds INCLUDES to db/mork where required and ensure creating of libmail.so component.
Attachment #571599 - Flags: review? → review?(neil)
Attachment #571602 - Flags: review?(neil)
Please also see replacement of NS_RegisterStaticAtoms (NS_RegisterStaticAtoms is missing in frozen linkages).
(In reply to jhorak from comment #11)
> Attaching WIP patch which works with --enable-incomplete-external-linkage
> and --with-libxul-sdk set.
Do you need both? (I don't really want to set up a new build tree if I can get away with just using --enable-incomplete-external-linkage.)
The --enable-incomplete-external-linkage only should be sufficient for 'Convert rest to frozen linkages' and 'js patch' patches. Testing makefiles patch require --with-libxul-sdk=/patch/to/mozilla-central/dist . However it might be comm-central/mozilla/dist if xulrunner was build there before.
Attachment #571599 - Flags: review?(neil) → review?(mconley)
Comment on attachment 571597 [details] [diff] [review]
Convert rest to frozen linkages

>     // Init it with the URI
>-    const nsAFlatCString& flatURI = PromiseFlatCString(aURI);
>+    const nsCString& flatURI = PromiseFlatCString(aURI);
> 
>     rv = directory->Init(flatURI.get());
I think we might as well change this to
rv = directory->Init(PromiseFlatCString(aURI).get());

> #ifdef MOZILLA_INTERNAL_API //FIXME NS_RegisterStaticAtoms
>     NS_RegisterStaticAtoms(folder_atoms, NS_ARRAY_LENGTH(folder_atoms));
> #else
>-    NS_ERROR("NS_RegisterStaticAtoms not implemented in frozen linkage");
>+    kTotalUnreadMessagesAtom = MsgNewPermanentAtom("TotalUnreadMessages");
Can we not use nsMsgDBFolderAtomList.h here? Something like
#define MSGDBFOLDER_ATOM(name_, value_) name = MsgNewPermanentAtom(value_);

>+#ifndef MOZILLA_INTERNAL_API
>+    NS_Free(kTotalUnreadMessagesAtom);
This is wrong, permanent atoms are never deleted.

>-    nsAdoptingString detectorName;
>+    nsDependentString detectorName;
nsString detectorName;

> #include "msgCore.h"
>-#include "nsMsgThread.h"
> #include "nsMsgDatabase.h"
> #include "nsCOMPtr.h"
>+#include "nsMsgThread.h"
> #include "MailNewsTypes2.h"
What effect does this have?
(In reply to neil@parkwaycc.co.uk from comment #18)
> >+#ifndef MOZILLA_INTERNAL_API
> >+    NS_Free(kTotalUnreadMessagesAtom);
> This is wrong, permanent atoms are never deleted.
> 
Even when calling destructor while mInstanceCount is zero? Memory allocated by MsgNewPermanentAtom won't be freed when destroying last nsMsgDBFolder object.

> >-    nsAdoptingString detectorName;
> >+    nsDependentString detectorName;
> nsString detectorName;
> 
> > #include "msgCore.h"
> >-#include "nsMsgThread.h"
> > #include "nsMsgDatabase.h"
> > #include "nsCOMPtr.h"
> >+#include "nsMsgThread.h"
> > #include "MailNewsTypes2.h"
> What effect does this have?
Without this change compilation ends with:
In file included from nsMsgThread.cpp:40:0:
../../../../mozilla/dist/include/nsMsgThread.h:62:3: error: ‘nsCOMPtr’ does not name a type
../../../../mozilla/dist/include/nsMsgThread.h:86:3: error: ‘nsCOMPtr’ does not name a type
../../../../mozilla/dist/include/nsMsgThread.h:87:3: error: ‘nsCOMPtr’ does not name a type
nsMsgThread.cpp: In constructor ‘nsMsgThread::nsMsgThread(nsMsgDatabase*, nsIMdbTable*)’:
nsMsgThread.cpp:56:3: error: ‘m_mdbTable’ was not declared in this scope
nsMsgThread.cpp:57:3: error: ‘m_mdbDB’ was not declared in this scope
nsMsgThread.cpp:68:60: error: ‘m_metaRow’ was not declared in this scope

I know that's not the optimal solution.
Comment on attachment 571599 [details] [diff] [review]
js patch 1 [Checked in]

This looks good to me, thanks!
Attachment #571599 - Flags: review?(mconley) → review+
Comment on attachment 571599 [details] [diff] [review]
js patch 1 [Checked in]

Checked in this patch as http://hg.mozilla.org/comm-central/rev/54ce07c8295e
Attachment #571599 - Attachment description: js patch 1 → js patch 1 [Checked in]
Comment on attachment 571597 [details] [diff] [review]
Convert rest to frozen linkages

>diff --git a/mailnews/imap/src/nsImapProtocol.cpp b/mailnews/imap/src/nsImapProtocol.cpp
>--- a/mailnews/imap/src/nsImapProtocol.cpp
>+++ b/mailnews/imap/src/nsImapProtocol.cpp
>@@ -108,16 +108,17 @@ PRLogModuleInfo *IMAP;
> #include "nsIStreamConverterService.h"
> #include "nsIProxyInfo.h"
> #include "nsISSLSocketControl.h"
> #include "nsProxyRelease.h"
> #include "nsDebug.h"
> #include "nsMsgCompressIStream.h"
> #include "nsMsgCompressOStream.h"
> #include "nsAlgorithm.h"
>+#include "nsMsgUtils.h"
My copy of nsImapProtocol.cpp already seems to include nsMsgUtils.h?
Comment on attachment 571597 [details] [diff] [review]
Convert rest to frozen linkages

>+#endif   
> 
> nsMsgDBFolder::nsMsgDBFolder(void)
Nit: the #endif has trailing spaces.
(In reply to jhorak from comment #19)
> (In reply to comment #18)
> > >+#ifndef MOZILLA_INTERNAL_API
> > >+    NS_Free(kTotalUnreadMessagesAtom);
> > This is wrong, permanent atoms are never deleted.
> Even when calling destructor while mInstanceCount is zero?
class PermanentAtomImpl is a non-refcounted implementation of nsIAtom. They get deleted on shutdown by NS_PurgeAtomTable.
Fixing mentioned issues on review.
Attachment #571597 - Attachment is obsolete: true
Attachment #571973 - Flags: review?(neil)
Attachment #571597 - Flags: review?(neil)
Comment on attachment 571597 [details] [diff] [review]
Convert rest to frozen linkages

[This patch of course does not break normal builds.
 But I still haven't got around to creating a external linkage build yet.]
One note on external linkage builds: you have to disable ldap by --disable-ldap otherwise compilation fails. It's related to autocomplete component, I still have to check it. There might by some bug for it already.
Comment on attachment 571973 [details] [diff] [review]
Convert rest to frozen linkages 2 [Checked in]

Well, I have successfully created a partly external linkage build! That is to say, I have a mail.dll that uses external linkage, although I did it by hacking the build system which means that autocomplete is still part of libxul and I disabled ldap just in case and I also had to disable bits of import and fix external linkage of the outlook address book and windows mail alert (fortunately I'm building SeaMonkey, this isn't possible in Thunderbird...)

nsStopwatch.cpp doesn't build debug though, because it uses nsPrintfCString...
Attachment #571973 - Flags: review?(neil) → review+
Set checking-needed for Attachment #571973 [details] [diff]. Thanks.
Keywords: checkin-needed
Comment on attachment 571602 [details] [diff] [review]
Makefiles and config patch 1

I'm not a build system peer so some of this stuff may be over my head.

>+ifdef LIBXUL_SDK
>+SDK_LIB_DIR = $(LIBXUL_SDK)/sdk/lib
>+SDK_BIN_DIR = $(LIBXUL_SDK)/sdk/bin
>+else
> SDK_LIB_DIR = $(DIST)/sdk/lib
> SDK_BIN_DIR = $(DIST)/sdk/bin
>+endif
This doesn't look right, and it's only used when SDK_LIBRARY or SDK_BINARY is set, which in the case of comm-central, is never.

>+ifdef LIBXUL_SDK
>+XPIDL_DEPS = \
>+  $(LIBXUL_SDK)/sdk/bin/header.py \
>+  $(LIBXUL_SDK)/sdk/bin/typelib.py \
>+  $(LIBXUL_SDK)/sdk/bin/xpidl.py \
>+  $(NULL)
>+else
> XPIDL_DEPS = \
>   $(MOZILLA_DIR)/xpcom/idl-parser/header.py \
>   $(MOZILLA_DIR)/xpcom/idl-parser/typelib.py \
>   $(MOZILLA_DIR)/xpcom/idl-parser/xpidl.py \
>   $(NULL)
>+endif
Are you actually able to build without a mozilla-central subtree?
If it turns out that they work for normal builds too then it might be better to use $(LIBXUL_DIST)/sdk/bin/xpidl.py etc.

>+ifdef MOZ_INCOMPLETE_EXTERNAL_LINKAGE
>+INCLUDES += -I../public
>+endif
I believe we agreed in another bug that these aren't necessary.

> # app is always last as it packages up the built files on mac
> DIRS += base locales extensions steel themes jquery app
> 
>+ifdef LIBXUL_SDK
>+DIRS += $(DEPTH)$(SUBDIR)/mailnews/base \
>+        $(DEPTH)$(SUBDIR)/mailnews/mime/public \
>+        $(DEPTH)$(SUBDIR)/mailnews \
>+        $(DEPTH)$(SUBDIR)/db \
>+        components \
>+        $(NULL)
>+endif
Putting DIRS after app is wrong on the Mac ;-)
I'm not sure that this is the best place to do this though. Possibly the top-level makefile, or maybe bridge.mk is the right place.
Attachment #571602 - Flags: review?(neil) → review-
Comment on attachment 571973 [details] [diff] [review]
Convert rest to frozen linkages 2 [Checked in]

Checked in as http://hg.mozilla.org/comm-central/rev/ae8606c83944
Attachment #571973 - Attachment description: Convert rest to frozen linkages 2 → Convert rest to frozen linkages 2 [Checked in]
Attached patch Makefiles and config patch 2 (obsolete) — Splinter Review
I hope I'm sending better version of Makefile and config patch. The first was far from perfect. It's using bridge.mk now and works fine for me.

This bug also depends on bug #452232 which should remove:
APP_LIBXUL_DIRS += $(DEPTH)$(SUBDIR)/mozilla/xpfe/components/autocomplete
from bridge.mk.
Attachment #571602 - Attachment is obsolete: true
Attachment #576142 - Flags: review?(neil)
Comment on attachment 576142 [details] [diff] [review]
Makefiles and config patch 2

I hope to test this tomorrow, but in the mean time, here are some comments:

>-SDK_LIB_DIR = $(DIST)/sdk/lib
>-SDK_BIN_DIR = $(DIST)/sdk/bin
>+SDK_LIB_DIR = $(LIBXUL_DIST)/sdk/lib
>+SDK_BIN_DIR = $(LIBXUL_DIST)/sdk/bin
Sorry, I still don't understand the point of this change.

>+EXTRA_DSO_LDOPTS += \
>+	$(LIBXUL_DIST)/lib/$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
>+	$(MOZ_COMPONENT_LIBS) \
Shouldn't these two be replaced by $(XPCOM_GLUE_LDOPTS)?

> EXTRA_DSO_LDOPTS += $(MOZ_COMPONENT_LIBS) $(LDAP_LIBS)
>+ifdef MOZ_INCOMPLETE_EXTERNAL_LINKAGE
>+EXTRA_DSO_LDOPTS += \
>+	$(XPCOM_GLUE_LDOPTS) \
>+	$(NULL)
>+endif
Excellent, you're using $(XPCOM_GLUE_LDOPTS), but I'm not sure it's good to unconditionally specify $(MOZ_COMPONENT_LIBS).

> ifndef LIBXUL_SDK
> include $(topsrcdir)/toolkit/toolkit-tiers.mk
>+else
>+tier_app_staticdirs += $(APP_LIBXUL_STATICDIRS)
>+tier_app_dirs += $(APP_LIBXUL_DIRS)
> endif
This is nice, it neatly avoids duplicating the list of directories.

>+	$(LIBXUL_DIST)/lib/$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
>+	$(MOZ_COMPONENT_LIBS) \
Should these be $(XPCOM_GLUE_LDOPTS) again?
(In reply to jhorak from comment #32)
> This bug also depends on bug #452232 which should remove:
> APP_LIBXUL_DIRS += $(DEPTH)$(SUBDIR)/mozilla/xpfe/components/autocomplete
> from bridge.mk.
Ah yes, this is annoying, because there's no way for it to tell whether we want it as a libxul or as a shared component. (Ironically it always builds using the external API, so locally to test these patches I've made it build as a shared component.)

(In reply to comment #33)
>> ifndef LIBXUL_SDK
>> include $(topsrcdir)/toolkit/toolkit-tiers.mk
>>+else
>>+tier_app_staticdirs += $(APP_LIBXUL_STATICDIRS)
>>+tier_app_dirs += $(APP_LIBXUL_DIRS)
>> endif
>This is nice, it neatly avoids duplicating the list of directories.
Although to test this locally I used the unholy hack of including both versions of autoconf.mk so that I could condition it on the external linkage define.
(In reply to neil@parkwaycc.co.uk from comment #33)
> >-SDK_LIB_DIR = $(DIST)/sdk/lib
> >-SDK_BIN_DIR = $(DIST)/sdk/bin
> >+SDK_LIB_DIR = $(LIBXUL_DIST)/sdk/lib
> >+SDK_BIN_DIR = $(LIBXUL_DIST)/sdk/bin
> Sorry, I still don't understand the point of this change.

Keeping 
SDK_LIB_DIR = $(DIST)/sdk/lib
SDK_BIN_DIR = $(DIST)/sdk/bin

leads to build failure:

Entering directory `/home/jhorak/mozilla/3comm-central/ldap/xpcom/public'
/usr/bin/python2.7 ../../../mozilla/dist/sdk/bin/xpt.py link _xpidlgen/mozldap.xpt _xpidlgen/nsILDAPConnection.xpt _xpidlgen/nsILDAPOperation.xpt _xpidlgen/nsILDAPMessage.xpt _xpidlgen/nsILDAPMessageListener.xpt _xpidlgen/nsILDAPURL.xpt _xpidlgen/nsILDAPErrors.xpt _xpidlgen/nsILDAPServer.xpt _xpidlgen/nsILDAPService.xpt _xpidlgen/nsILDAPBERValue.xpt _xpidlgen/nsILDAPControl.xpt _xpidlgen/nsILDAPBERElement.xpt _xpidlgen/nsILDAPModification.xpt _xpidlgen/nsILDAPSyncQuery.xpt
/usr/bin/python2.7: can't open file '../../../mozilla/dist/sdk/bin/xpt.py': [Errno 2] No such file or directory

I think this is the most optimal place to fix that.
Attached patch Makefiles and config patch 3 (obsolete) — Splinter Review
Another round:
- Kept:
  >+SDK_LIB_DIR = $(LIBXUL_DIST)/sdk/lib
  >+SDK_BIN_DIR = $(LIBXUL_DIST)/sdk/bin
- The $(MOZ_COMPONENT_LIBS) replaced by $(NSPR_LIBS) (due to some unresolved dependencies).
- Using $(XPCOM_GLUE_LDOPTS) everywhere
Attachment #576142 - Attachment is obsolete: true
Attachment #576432 - Flags: review?(neil)
Attachment #576142 - Flags: review?(neil)
(In reply to jhorak from comment #35)
> (In reply to comment #33)
> > >-SDK_LIB_DIR = $(DIST)/sdk/lib
> > >-SDK_BIN_DIR = $(DIST)/sdk/bin
> > >+SDK_LIB_DIR = $(LIBXUL_DIST)/sdk/lib
> > >+SDK_BIN_DIR = $(LIBXUL_DIST)/sdk/bin
> > Sorry, I still don't understand the point of this change.
> leads to build failure:
This turns out to be bug 650476, which needs to be ported to comm-central.
Attached patch Makefiles and config patch 4 (obsolete) — Splinter Review
> This turns out to be bug 650476, which needs to be ported to comm-central.
Ah I see...Sending patch with fix from bug #650476.
Attachment #576432 - Attachment is obsolete: true
Attachment #576444 - Flags: review?(neil)
Attachment #576432 - Flags: review?(neil)
(In reply to neil@parkwaycc.co.uk from comment #37)
> This turns out to be bug 650476, which needs to be ported to comm-central.

File as bug 704797.
Comment on attachment 576444 [details] [diff] [review]
Makefiles and config patch 4

In some of the Makefiles I had to add $(MOZ_ZLIB_LIBS) and/or $(MOZ_JS_LIBS) to the extra dso ldopts, but that might be because we have slightly different configure options (you're probably using system zlib and not shared JS).
Attachment #576444 - Flags: review?(neil)
Attachment #576444 - Flags: review+
Attachment #576444 - Flags: feedback?(bugspam.Callek)
Comment on attachment 576444 [details] [diff] [review]
Makefiles and config patch 4

Some minor changes needed, once done, you can take Neil's review as final (no need to re-request)

>diff --git a/config/rules.mk b/config/rules.mk
> XPIDL_DEPS = \
>-  $(MOZILLA_DIR)/xpcom/idl-parser/header.py \
>-  $(MOZILLA_DIR)/xpcom/idl-parser/typelib.py \
>-  $(MOZILLA_DIR)/xpcom/idl-parser/xpidl.py \
>+  $(LIBXUL_DIST)/sdk/bin/header.py \
>+  $(LIBXUL_DIST)/sdk/bin/typelib.py \
>+  $(LIBXUL_DIST)/sdk/bin/xpidl.py \
>   $(NULL)

Sorry, can't do this here. this is a DEPS rule, if it was changed in m-c I would be ok with this, but since its not, I don't want to change it. Shouldn't affect *too* much, assuming you do use a matching LIBXUL_SDK as whatever we are tied to. Feel free to convince me why this is needed. (if it is needed I want to feedback again)


>diff --git a/db/mork/build/Makefile.in b/db/mork/build/Makefile.in
>+ifndef MOZ_INCOMPLETE_EXTERNAL_LINKAGE
>+MOZILLA_INTERNAL_API = 1
>+LIBXUL_LIBRARY	= 1
>+else
>+FORCE_SHARED_LIB = 1	 
>+GRE_MODULE	= 1
>+FORCE_USE_PIC   = 1
>+endif

Here and throughout the patch, do _not_ do FORCE_USE_PIC (its use was removed in Bug 698248) we do have some stragglers throughout the comm- build system right now, but  they don't hurt. I just don't want to add more.
Attachment #576444 - Flags: feedback?(bugspam.Callek) → feedback-
Attached patch Makefiles and config patch 5 (obsolete) — Splinter Review
Thanks for feedback.
>diff --git a/config/rules.mk b/config/rules.mk
> XPIDL_DEPS = \
>-  $(MOZILLA_DIR)/xpcom/idl-parser/header.py \
>-  $(MOZILLA_DIR)/xpcom/idl-parser/typelib.py \
>-  $(MOZILLA_DIR)/xpcom/idl-parser/xpidl.py \
>+  $(LIBXUL_DIST)/sdk/bin/header.py \
>+  $(LIBXUL_DIST)/sdk/bin/typelib.py \
>+  $(LIBXUL_DIST)/sdk/bin/xpidl.py \
>   $(NULL)

This change is not required as I found out finally. The mozilla directory and libxul sdk should have same version anyway, I believe.

The FORCE_USE_PIC removed.
Setting review+ according to previous comments.
Attachment #576444 - Attachment is obsolete: true
Attachment #576727 - Flags: review+
Comment on attachment 576727 [details] [diff] [review]
Makefiles and config patch 5

>-XPIDL_LINK = $(PYTHON) $(SDK_BIN_DIR)/xpt.py link
>+XPIDL_LINK = $(PYTHON) $(LIBXUL_DIST)/sdk/bin/xpt.py link

This will be fixed by bug 704797.
> This will be fixed by bug 704797.
Okay, so attaching patch without issue fixed by bug #704797.
Attachment #576727 - Attachment is obsolete: true
Attachment #576732 - Flags: review+
Keywords: checkin-needed
Comment on attachment 576732 [details] [diff] [review]
Makefiles and config patch 6 [Checked in]

Pushed changeset ef4201e5a838 to comm-central.
Attachment #576732 - Attachment description: Makefiles and config patch 6 → Makefiles and config patch 6 [Checked in]
Depends on: 590494
No longer depends on: 582943
No longer depends on: 583222
Depends on: 707126
Attached patch fix for nsMsgUtils 1 (obsolete) — Splinter Review
While testing thunderbird with libxul I found out some mistakes in nsMsgUtils I've made last time (see bug #377319). Attaching patch which fix that.
Attachment #579267 - Flags: review?(neil)
After deeper testing I've found more missing components which aren't installed during build with --enable-incomplete-external-linkage. This allows to set TB as default MUA, enables migration assistant and fix building import module.
Attachment #579268 - Flags: review?(neil)
Makes gloda working again. Shown up after testing of TB build against libxul.
Attachment #579269 - Flags: review?(mconley)
Comment on attachment 579267 [details] [diff] [review]
fix for nsMsgUtils 1

>+  /* We have to create nsDependentCString from 'what' because there's no
>+   * str.Find(char *what, int offset) but there is
>+   * str.Find(nsACString, int offset) */
I don't know why you would think that. There appear to be several versions:
nsACString::Find(nsACString [, offset], [, comparator])
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#649
nsACString::Find(const char* [, offset], [, comparator])
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#666
Attachment #579267 - Flags: review?(neil)
Comment on attachment 579268 [details] [diff] [review]
Continue on makefiles and config patch 1

>+GRE_MODULE	= 1
Not sure that we need GRE_MODULE?

>-	$(MOZDEPTH)/modules/libreg/src/$(LIB_PREFIX)mozreg_s.$(LIB_SUFFIX) \
I still can't link with this removed.
Attachment #579268 - Flags: review?(neil)
(In reply to neil@parkwaycc.co.uk from comment #49)
> Comment on attachment 579267 [details] [diff] [review] [diff] [details] [review]
> fix for nsMsgUtils 1
> 
> >+  /* We have to create nsDependentCString from 'what' because there's no
> >+   * str.Find(char *what, int offset) but there is
> >+   * str.Find(nsACString, int offset) */
> I don't know why you would think that. There appear to be several versions:
> nsACString::Find(nsACString [, offset], [, comparator])
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#649
> nsACString::Find(const char* [, offset], [, comparator])
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#666

No, there aren't. For nsACString there is only one method with offset:
658   NS_HIDDEN_(PRInt32) Find(const self_type& aStr, PRUint32 aOffset,
659                            ComparatorFunc c = DefaultComparator) const;
where self_type is nsACString.
Version for char * takes only length of string as second parameter:
669   NS_HIDDEN_(PRInt32) Find(const char_type *aStr, PRUint32 aLen,
670                            ComparatorFunc c = DefaultComparator) const;
Don't know whether it helps, but "offset + Find(what + offset)" might be worth considering.
(In reply to neil@parkwaycc.co.uk from comment #50)
> Comment on attachment 579268 [details] [diff] [review] [diff] [details] [review]
> Continue on makefiles and config patch 1
> 
> >+GRE_MODULE	= 1
> Not sure that we need GRE_MODULE?
You're right, not needed.

> >-	$(MOZDEPTH)/modules/libreg/src/$(LIB_PREFIX)mozreg_s.$(LIB_SUFFIX) \
> I still can't link with this removed.
Linux is just fine. I thought libreg is being removed.
Comment on attachment 579269 [details] [diff] [review]
js patch 2 [Checked in]

Looks right to me.  Thanks,

-Mike
Attachment #579269 - Flags: review?(mconley) → review+
Attached patch fix for nsMsgUtils 2 (obsolete) — Splinter Review
(In reply to Karl Tomlinson (:karlt) from comment #52)
> Don't know whether it helps, but "offset + Find(what + offset)" might be
> worth considering.
When 'what' is not found it doesn't return -1 when offset is greater than 0.

Neil, sorry, but this is still valid. Please see my explanation in comment 51. Please also note PRUint32 ->PRInt32 change (and relevant comment).
Attachment #624748 - Flags: review?(neil)
Attachment #579267 - Attachment is obsolete: true
Attached patch Makefiles patch II - 1 (obsolete) — Splinter Review
This patch allows smooth build against libxul (while building with --with-libxul-sdk).
Attachment #624749 - Flags: review?(neil)
Comment on attachment 624749 [details] [diff] [review]
Makefiles patch II - 1

The import change looks fine to be but I can't comment on the mail app.
Attachment #624749 - Flags: review?(neil) → review?(mbanner)
Comment on attachment 624748 [details] [diff] [review]
fix for nsMsgUtils 2

>+  /* We have to create nsDependentCString from 'what' because there's no
>+   * str.Find(char *what, int offset) but there is only
>+   * str.Find(nsACString, int offset) */
So what's wrong with the PRInt32 nsACString::Find(const char_type *aStr, PRUint32 aLen, ComparatorFunc c) version again?

>+  NS_ASSERTION(offset < str.Length(), "Offset is greater than string length");
>   return str.Find(what, offset, ignore_case);
str.Find just returns -1 in this case, does it not?

>+  /* See Find_ComputeSearchRange from nsStringObsolete.cpp */
>+  if (offset < 0) {
>+     offset = 0;
>+  }
Fair enough, but shouldn't you change the nsACString version too?
(In reply to neil@parkwaycc.co.uk from comment #58)
> Comment on attachment 624748 [details] [diff] [review]
> fix for nsMsgUtils 2
> 
> >+  /* We have to create nsDependentCString from 'what' because there's no
> >+   * str.Find(char *what, int offset) but there is only
> >+   * str.Find(nsACString, int offset) */
> So what's wrong with the PRInt32 nsACString::Find(const char_type *aStr,
> PRUint32 aLen, ComparatorFunc c) version again?
Nothing, but when calling (see the patch):
return str.Find(what, offset);
we're actually calling nsACString::Find(const char_type *aStr, PRUint32 aLen, ComparatorFunc c = ...). It mixes offset with string length which is bad.

> >+  NS_ASSERTION(offset < str.Length(), "Offset is greater than string length");
> >   return str.Find(what, offset, ignore_case);
> str.Find just returns -1 in this case, does it not?
> 
> >+  /* See Find_ComputeSearchRange from nsStringObsolete.cpp */
> >+  if (offset < 0) {
> >+     offset = 0;
> >+  }
> Fair enough, but shouldn't you change the nsACString version too?
Yes, I should.
Comment on attachment 624748 [details] [diff] [review]
fix for nsMsgUtils 2

>+  /* We have to create nsDependentCString from 'what' because there's no
>+   * str.Find(char *what, int offset) but there is only
>+   * str.Find(nsACString, int offset) */
Please change this to str.Find(char *what, int length)
This will make it much more obvious that you don't want to use a char* !

>+  if (offset < 0) {
>+     offset = 0;
>+  }
[We don't actually pass in negative offsets, do we? I checked all ten callers.]
Nit: incorrect indentation.

r=me with the assertions removed.
Attachment #624748 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #60)
> Comment on attachment 624748 [details] [diff] [review]
> >+  if (offset < 0) {
> >+     offset = 0;
> >+  }
> [We don't actually pass in negative offsets, do we? I checked all ten
> callers.]
> Nit: incorrect indentation.
We do it at least here:
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapService.cpp#544
when chars '/?&' are not found in string the keyEndSeparator is -1.
Removed assertions, fixed comments, check for offset < 0 kept.
Attachment #624748 - Attachment is obsolete: true
Attachment #625614 - Flags: review+
Attached patch Makefiles patch II - 2 (obsolete) — Splinter Review
Ops, accidentally added blank line. Removed now.
Attachment #624749 - Attachment is obsolete: true
Attachment #625617 - Flags: review?(mbanner)
Attachment #624749 - Flags: review?(mbanner)
Attached patch gloda patch 1 (obsolete) — Splinter Review
A patch which makes gloda search work again with thunderbird+libxul builds.
Attachment #628278 - Flags: review?(mconley)
Comment on attachment 628278 [details] [diff] [review]
gloda patch 1

Sorry. Already uploaded patch.
Attachment #628278 - Attachment is obsolete: true
Attachment #628278 - Flags: review?(mconley)
Setting checkin needed for 'js patch 2' and 'fix for nsMsgUtils 3' patches. Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/6c501efb77ec
https://hg.mozilla.org/comm-central/rev/7a5f6bbdc891

Also, to make life easier for those checking in patches on your behalf, please follow the guidelines below for future patches you submit. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Assignee: nobody → jhorak
Keywords: checkin-needed
Whiteboard: [leave open]
Attachment #579269 - Attachment description: js patch 2 → js patch 2 [Checked in]
Attachment #625614 - Attachment description: fix for nsMsgUtils 3 → fix for nsMsgUtils 3 [Checked in]
Sorry, patch was wrong, I didn't notice it before I checked without --enable-incomplete-external-linkages option.
Attachment #625617 - Attachment is obsolete: true
Attachment #625617 - Flags: review?(mbanner)
Attachment #631896 - Flags: review?(mbanner)
Comment on attachment 631896 [details] [diff] [review]
Makefiles patch II - 3

Sorry for the delay, r=me. The only thing I'd ask is if Linux distros are thinking of shipping libxul based Thunderbird, then they discuss with the Thunderbird release team first as there's various issues I'm concerned about.
Attachment #631896 - Flags: review?(mbanner) → review+
Yeah, that's our goal. At least https://bugzilla.mozilla.org/show_bug.cgi?id=452232 needs to be done first.
(In reply to Mark Banner (:standard8) from comment #69)
> Sorry for the delay, r=me. The only thing I'd ask is if Linux distros are
> thinking of shipping libxul based Thunderbird, then they discuss with the
> Thunderbird release team first as there's various issues I'm concerned about.

I'm undecided but also see some issues. I would prefer to have a small discussion about it (not in bugzilla obviously). Mark, you have the main contacts for Linux distributions AFAIK, maybe you should ask that list?
Should this be marked WONTFIX based on the reply here?
https://bugzilla.mozilla.org/show_bug.cgi?id=336874#c24
(In reply to Worcester12345 from comment #72)
> Should this be marked WONTFIX based on the reply here?
> https://bugzilla.mozilla.org/show_bug.cgi?id=336874#c24

No, because "the web became a platform" is just a buzzword. Firefox, thunderbird and xulrunner use ELFs called libxul.so and libmozalloc.so just like they always did. It is only common sense that they should be shared rather than duplicated.
XULRunner is, to my knowledge, completely dead at this point. The closest replacement is firefox -app (webapprt is I think the internal name for this?), and I don't know the differences between firefox -app and XULRunner. The major Linux distributions appear to be in accord that shipping a built Firefox on top of XULRunner is no longer the way to be packaging Firefox, which means that deduplication of libxul.so in particular is unlikely to be realistically achievable (more so, given the fact that anyone attempting to keep up-to-date with Firefox will invariably end up with differing versions of libxul anyways).

What it really comes down to in the end is if libmail should be linked into libxul (as it has historically been) or be kept entirely separate. There are several issues with distributing a version of Thunderbird based on Firefox's libxul (massive perf hit to string APIs is the big one), and it's not clear to me that the benefits of linking libmail as a separate .so are worth the perf hits and increased maintenance costs of doing so. The cost/benefit ratio looks a lot worse when you factor in that the current tenor of Firefox development appears to be "we'll remove nasty old APIs when Firefox stops needing them and make Thunderbird worry about it"--and given that many of the potential affected APIs (RDF templates, the recent charset changes) assume a good deal of integration into libxul already, it would be an exceeding amount of work to maintain an external-API-working variant.

While Benjamin has opined that there is some value in keeping libmail separate from libxul, I have also noted that the trend in mozilla-central appears to be one of screwing over binary extensions and generally not caring about the kinds of interfaces that Thunderbird would have to use were it an external library. XPIDL, for example, is no longer sufficient to be able to represent APIs, and I've seen pushback on suggestions to change it, and there is no indication that WebIDL would be usable outside of libxul. This means that I am very strongly minded to kill support for external API altogether.

I've CC'd Benjamin in case there's anything he wants to add.
Well xulrunner still pumps out a new release every month. Even though most Linux distributions have given up trying to convince Mozilla to use shared libraries, I compile firefox and instantbird against xulrunner and they work almost perfectly.

What I got from your comment is that it would be impractical to make thunderbird compatible with the libxul from mozilla-central because those developers regard thunderbird as a second class citizen. This is unfortunate because there was a big push in ~2009 to use frozen linkage and not be stuck with an ancient Mozilla milestone. IIRC, Firefox 3.6 came out before Thunderbird 3.0 was even out.

How about doing the opposite and shipping a libxul with thunderbird that also has the parts needed by firefox? I don't see an easy way to test it with the current build system but it would be great if I could compile firefox 24.0 against the libraries in thunderbird 24.0.
"This means that I am very strongly minded to kill support for external API altogether."

One of the things that has kept Mozilla and Thunderbird relevant in spite of competition with better funded alternatives, is that they have kept a vibrant community of addons. Yet supporting addons requires that you maintain features that are not required by the core code, but are required by a small minority of applications that rely on them. Many times in the zeal to make life a little easier for the core, the need to support minority users and developers (which is where the real strength of Mozilla is) can be lost. Thunderbird itself is starting to feel this as the drive to optimize Firefox OS is making our needs much less relevant.

Certainly in my case the external API is absolutely critical to what I do. One of the frustrating parts of working with Mozilla are continuing rumors, that arise from conversations like this, that "Mozilla is going to kill the external API" or "Mozilla is going to kill binary addons", or "Mozilla is going to kill (insert here the critical thing that you need for your project)" Sometimes the real decision to actually kill something seems to occur rather suddenly.

Unfortunately there are few ways to make long-term decisions in Mozilla projects, so these issues just languish. I wish Joshua though that you would agree that there needs to be an EXTREMELY STRONG win that arises before killing key features that are absolutely critical to a minority of users. So far, I am not convinced that the current issue of allowing Promises to be freely interchangable between javascript and C++ code is such a critical win (though I admit it would be cool). Before you propose to kill external API in Thunderbird, I'd like to see the critical issue you are trying to solve by doing so. I wish there was some way to get a decision "no, we are not going to kill binary addons and external API for the foreseeable future" but I suppose that is too much to ask.
(In reply to Worcester12345 from comment #72)
> Should this be marked WONTFIX based on the reply here?
> https://bugzilla.mozilla.org/show_bug.cgi?id=336874#c24

So, it is sounding more like a "Yes" answer to me here.
(In reply to Worcester12345 from comment #77)
> (In reply to Worcester12345 from comment #72)
> > Should this be marked WONTFIX based on the reply here?
> > https://bugzilla.mozilla.org/show_bug.cgi?id=336874#c24
> 
> So, it is sounding more like a "Yes" answer to me here.

Going on 2 years. 

Please close this with WONTFIX.
Yes this not going to happen (and support for binary extensions is being phased out).
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
If this is WONTFIX then [leave open] has no purpose.
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.