Closed Bug 345517 Opened 13 years ago Closed 13 years ago

Build Firefox --enable-libxul (not static) by default

Categories

(Firefox Build System :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla3 alpha3

People

(Reporter: benjamin, Assigned: benjamin)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

In trasitioning to firefox-on-xulrunner, I'd like to start building Firefox --enable-libxul by default. This will track the remaining work to make that happen.
Attachment #230363 - Flags: review?(darin)
Attached patch Browser changes, rev. 1 (obsolete) — Splinter Review
Attachment #230364 - Flags: review?(darin)
I need to write an additional patch to make --enable-libxul the default in configure and fix up the installer packaging, but I can't do that until the spellchecker stuff from bug 339106 is landed.
Comment on attachment 230363 [details] [diff] [review]
Glue additions, rev. 1 [checked in]

+  if (*aASCIIString)
+    return PR_FALSE;
+
+  return PR_TRUE;

  return !*aASCIIString;
Attachment #230363 - Flags: review?(darin) → review+
Attachment #230364 - Flags: review?(darin) → review+
Depends on: 346679
Comment on attachment 230363 [details] [diff] [review]
Glue additions, rev. 1 [checked in]

I've got this completely working on Linux, and startup times are fantastic! Compiling windows/mac now, I should have a patch tomorrow afternoon.
Attachment #230363 - Attachment description: Glue additions, rev. 1 → Glue additions, rev. 1 [checked in]
Attached patch Build system changes, rev. 1 (obsolete) — Splinter Review
This won't do the right thing without the prerequisite patches here and in the unionenumerator bug, but it works!
Attachment #232125 - Flags: review?(mark)
The final set of browser/components changes, including some win/mac fixup and conflict resolution with my own patch in bug 344159
Attachment #230364 - Attachment is obsolete: true
Depends on: 347183
(In reply to comment #1)
> Created an attachment (id=230363) [edit]
> Glue additions, rev. 1

If nspr is avaliable inside nsStringAPI.cpp, isn't it
reasonable to use PR_snprintf(...) instead of _snprintf(...)?

_snprintf is one of dll hell trigers on Windows, so some embedding 
app programmers may fail to link their app to geckosdk static libs,
which are linking to msvcrt/libcmt.

i.e.
http://lxr.mozilla.org/mozilla/source/xpcom/glue/nsStringAPI.cpp

> 59 #ifdef XP_WIN
> 60 #define snprintf _snprintf
> 61 #endif

->

#ifdef XP_WIN
#  ifdef XPCOM_GLUE_AVOID_NSPR
#    define snprintf _snprintf
#  else
#    define snprintf PR_snprintf
#  endif
#endif
Is there something special about _snprintf in particular? We already have the problem of CRT dependencies and mixed CRTs, but that can usually mostly be fixed with /nodefaultlib, right? Anyway, that's a separate bug.
Comment on attachment 232125 [details] [diff] [review]
Build system changes, rev. 1

+# A Firefox static build is really a meta-component build, not truly static
+if test "$MOZ_BUILD_APP" = "browser" -a -n "$BUILD_STATIC_LIBS"; then
+    MOZ_META_COMPONENT=1
+fi

We should probably init MOZ_META_COMPONENT to empty up above.

Something's missing here - I get this when linking libappmeta.

/usr/bin/ld: Undefined symbols:
Apprunner_NSGetModule(nsIComponentManager*, nsIFile*, nsIModule**)
_NS_NewUnionEnumerator
NS_NewArrayEnumerator(nsISimpleEnumerator**, nsCOMArray_base const&)
_FSGetParentRef
_FSRefValid
_NS_StringGetData_P
_NS_StringGetMutableData_P
nsAString_internal::GetWritableBuffer(unsigned short**, unsigned int)
nsAString_internal::Assign(nsAString_internal const&)
nsAString_internal::Length() const
nsDefaultStringComparator::operator()(unsigned short const*, unsigned short const*, unsigned int) const
vtable for nsDefaultStringComparator

And in a shared build, when linking libbrowserdirprovider:

/usr/bin/ld: Undefined symbols:
_NS_NewUnionEnumerator
NS_NewArrayEnumerator(nsISimpleEnumerator**, nsCOMArray_base const&)
I forgot toolkit/xre/Makefile.in in the patch. The remaining linkage changes are covered in bug 347183, which is a prerequisite (I've built this patch successfully on all three platforms).
Attachment #232125 - Attachment is obsolete: true
Attachment #232594 - Flags: review?(mark)
Attachment #232125 - Flags: review?(mark)
Comment on attachment 232594 [details] [diff] [review]
Build system changes, rev. 1.1

These changes look OK then, but I haven't tested this by tracking down the dependencies.  I trust that you have.
Attachment #232594 - Flags: review?(mark) → review+
This was checked in, but is going to be backed out tomorrow morning due to linux perf regressions and a mysterious balsa orange.
regression 080904-080907 [cairo].

"Recently closed Tabs" is gray, and "Undo close tab" on tab-bar disappears.
+/- cursor disappears when mouse-over on auto-resized image. 

are these ceused by this checkin ?
Feedview is broken.
Building with --disable-libxul on OS X gives me the following error in mozilla/browser/app:

c++ -o firefox-bin -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -isysroot /Developer/SDKs/MacOSX10.3.9.sdk -fpascal-strings -no-cpp-precomp -fno-common -fshort-wchar -I/Developer/SDKs/MacOSX10.3.9.sdk/Developer/Headers/FlatCarbon -pipe  -DDEBUG -D_DEBUG -DDEBUG_peterv -DTRACING -g  nsBrowserApp.o            -L../../dist/bin -L../../dist/lib  -L../../dist/bin -lmozjs -L../../dist/bin -lxpcom -lxpcom_core -L../../dist/lib -lplds4 -lplc4 -lnspr4 -framework Cocoa -framework Carbon       
/usr/bin/ld: Undefined symbols:
_XRE_main
collect2: ld returned 1 exit status
This may have caused Firefox to crash on startup, please see bug 348044.
Depends on: 348212
Now building with --enable-libxul does not build with libxul.
*** Bug 349761 has been marked as a duplicate of this bug. ***
This bug is responsible for creating an API inconsistency.

MOZILLA_INTERNAL_API's {String}::AssignLiteral is for literal constants not const pointers.

The flash frozen instance in glue unfortunately accepts a simple const pointer. As a result people are now confused by what "AssignLiteral" means and it is no longer properly portable.

I don't have any recourse other than to say FLASH FREEZING IS BAD. EVEN WHEN YOU THINK YOU ARE NOT FLASH FREEZING.
The glue is not API-frozen. It is safe to link against, but the APIs may change in the future.
This patch is mainly an updated version of the last patches, with some updates for changed trunk code and fixing some regressions I caused last time.
Attachment #244859 - Flags: review?(mano)
Comment on attachment 244859 [details] [diff] [review]
Update patch, makes browser components use frozen linkage without libxul, rev. 1

browser/ changes look OK. please file a bug on  porting bakc EscapeHTML.
Attachment #244859 - Flags: review?(mano) → review+
So... something about this change made string adoption confused _somehow_.  See the leak stats on balsa.
In particular, here's the interesting part from the debug output in the log:

  nsStringStats
...
   => mAdoptCount:           2303
   => mAdoptFreeCount:       2309

That second number should be no greater than the first number...
I've closed the tree until we have reasonable leak numbers on balsa again.
Is it possible that some timing changed somewhere so now we're adopting some strings before we really start keeping track of things?
backing out so we can reopen the tree.
Attachment #245232 - Flags: review? → review?(darin.moz)
Attachment #245232 - Flags: review?(darin.moz) → review+
attachment 245232 [details] [diff] [review] committed on trunk. Relanding the big patch will have to wait until Monday.
Is it just me, or was the string change a 165KB codesize win for Firefox and a 250KB codesize win for Seamonkey?

Did taking this constructor out of line really help that much, or is codesighs confused?  It's claiming small (from 20 to 500 byte) decreases in all sorts of functions all through the codebase.
It was indeed a codesize win... inlining apparently cost a lot in this case.
So bug 188224 is still valid with the new string code...
Looking at the tinderboxes it is just me (hmm, although the OS/2 nightlies are also garbled and just 529K downloads), but somehow attachment 245232 [details] [diff] [review] seems to have caused compile errors like these:

make.exe[6]: Entering directory `m:/trunk/fx_thebes/xpcom/string/src'
nsDependentString.cpp
In file included from ../../../dist/include/string/nsSubstring.h:60,
                 from ../../../dist/include/string/nsString.h:44,
                 from ../../../dist/include/string/nsDependentString.h:43,
                 from G:/trunk/mozilla/xpcom/string/src/nsDependentString.cpp:40:
../../../dist/include/string/nsTSubstring.h:501: error: parse error before `*' token
In file included from ../../../dist/include/string/nsSubstring.h:66,
                 from ../../../dist/include/string/nsString.h:44,
                 from ../../../dist/include/string/nsDependentString.h:43,
                 from G:/trunk/mozilla/xpcom/string/src/nsDependentString.cpp:40:
../../../dist/include/string/nsTSubstring.h:501: error: parse error before `*' token
In file included from ../../../dist/include/string/nsDependentSubstring.h:48,
                 from ../../../dist/include/string/nsString.h:48,
                 from ../../../dist/include/string/nsDependentString.h:43,
                 from G:/trunk/mozilla/xpcom/string/src/nsDependentString.cpp:40:
../../../dist/include/string/nsTDependentSubstring.h: In constructor `nsDependentSubstring::nsDependentSubstring(const PRUnichar*, const PRUnichar*)':
../../../dist/include/string/nsTDependentSubstring.h:74: error: no matching function for call to `nsSubstring::nsSubstring(PRUnichar*&, int, nsSubstring::<anonymous enum>)'
../../../dist/include/string/nsTSubstring.h:542: error: candidates are: nsSubstring::nsSubstring(const nsSubstring&)
../../../dist/include/string/nsTSubstring.h:533: error: nsSubstring::nsSubstring(unsigned int)
../../../dist/include/string/nsTSubstring.h:521: error: nsSubstring::nsSubstring()
../../../dist/include/string/nsTSubstring.h:485: error: nsSubstring::nsSubstring(const nsSubstringTuple&)
[... etc ...]

This is an OS/2 trunk build from a freshly checked out tree, but as I don't see anything OS/2 specific in the patch I am a bit lost... Any ideas?
It was definitely the TSubstring.h change that broke us. No idea why yet. I'm going to do some preprocessor output to see what I can figure out.
Attachment 244859 [details] [diff] committed on trunk. Actually switching to use libxul by default will happen (again) after 1.9alpha1.
(In reply to comment #36)
> It was definitely the TSubstring.h change that broke us. No idea why yet. I'm
> going to do some preprocessor output to see what I can figure out.

Perhaps our OS/2 GCC cannot handle "__attribute__((dllexport))" in front of constructors? Removing NS_COM in front of nsTSubstring_CharT solves the problem. (I am wondering why that is there anyway, the first constructor does not have that decoration.)
That is there because the other is an inline function, and this is not. What is the syntax for marking a constructor dllexport on OS/2?
I don't know, but perhaps Andy does (or can find out).
I talked to Knut and he saie:
#define __declspec(a) __attribute__((a))
BirdWrk>	that's how __declspec is implement, so we should support that dllexport attribute.
Depends on: 360662
Hmm, that would only apply to gcc 3.3.5.  Knut would like a testcase but I am not entirely certain what it is doing so not sure how to create one.
Attachment #245588 - Flags: review?(gavin.sharp)
Attachment #245588 - Flags: review?(gavin.sharp) → review+
The actual landing increased codesize by almost 80KB, with lots of changes to lots of functions...  mostly it looks like libbrowsercomps.so got bigger by 245KB and libbrowserdirprovider.so got bigger by 33KB, while firefox-bin got smaller by 200KB.

Is that about as expected?
Yes, that is expected. It was more than offset by the string uninlining ;-)
Peter put together a testcase and Knut found that the problem we are having on OS/2 is that there is a parsing error in the gcc 3.3.x series that was fixed in 3.4.  As we don't have 3.4 is our best option to ifdef and go without the NS_COM there?
I'd be a little surprised if it linked without NS_COM... it is an imported symbol.
Attached patch Fixes build failure on OS/2 (obsolete) — Splinter Review
As far as I know the ifdef may not be required but I can't test on other systems so I put the ifdef in until/unless someone else tests and confirms it is not necessary.
Got the idea here:
http://gcc.gnu.org/ml/gcc-prs/2002-12/msg00672.html
Setting Mike for Review but not sure who to set for SuperReview.
Attachment #246530 - Flags: review?(mozilla)
Attachment #246530 - Attachment is obsolete: true
Attachment #246540 - Flags: review?
Attachment #246530 - Flags: review?(mozilla)
Attachment #246540 - Flags: review? → review?(mozilla)
should we do a GCC 3 check instead of an OS/2 check?
Well, we can use #if HAVE_GCC_VERSION(3,3) I think.  No one else has apparently seen this (or at least not reported it that I have seen) so I went with the XP_OS2.  I just didn't want to change anyone else if they were working as is, though it should affect at least 3.2 and 3.3 for everyone.
Ok, the previous was wrong... it was pointed out on IRC that that would check at _least_ and not exact.  #if (__GNUC__ == 3 && __GNUC_MINOR__ < 4) should work.
Mike, I haven't tested this with 3.2.2 though it should work fine as far as I can tell.
Attachment #246540 - Flags: review?(mozilla) → review?(benjamin)
Attachment #246540 - Flags: review?(benjamin) → review+
(In reply to comment #52)
> Ok, the previous was wrong... it was pointed out on IRC that that would check
> at _least_ and not exact.  #if (__GNUC__ == 3 && __GNUC_MINOR__ < 4) should
> work.
> Mike, I haven't tested this with 3.2.2 though it should work fine as far as I
> can tell.

Somehow I missed this and I see that no fix for the OS/2 problem was checked in yet. Do you want to make a new patch with the GCC test? Or should I check in the one with XP_OS2 that has r+?
Let's checkin the one with r+ to get us building again.
Comment on attachment 246540 [details] [diff] [review]
OS/2 build break patch... including comment
[Checked into trunk]

OK, done. (Works fine here with GCC 3.2.2, too.)
Attachment #246540 - Attachment description: OS/2 build break patch... including comment → OS/2 build break patch... including comment [Checked into trunk]
Priority: -- → P2
Target Milestone: Firefox 3 alpha1 → Firefox 3 alpha3
Blocks: 326925
wtf? why was this enabled for debug builds?  Having to rebuild/relink libxul takes at least 7 minutes on my windows box.  This needs to be backed out.

On a side note, why is thebes part of this?  We said all along that thebes should not be part of libxul.
This is enabled as the default config because we want the default onfig to math the release config. Per dbaron and I tend to agree options like --enable-debug should not have side-effects.

As for thebes, it has dependencies on xpom and internal linkage, and thus has to live in libxul until those dependencies are broken. But I think it should not be exposed in any case, beause it's not safe to link against in any case because of C++RT mixing.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Imho we should just completely disable libxul across the board if that is your stance.  Leaving it on is just going to cause all developers to disable it which isn't going to get you the coverage that you want.
(In reply to comment #57)
> This is enabled as the default config because we want the default onfig to math
> the release config. Per dbaron and I tend to agree options like --enable-debug
> should not have side-effects.

Normally I'd agree, but in this case, doing a debug build without doing disable-libxul makes it quite impossible to do any work on any C++ code.  I think the number of people who want a debug build with libxul are much smaller.  We already have everyone needing to build with --disable-installer, which is just as bad; let's not add another magic option that you have to build with to be able to do development at a layer lower than toolkit.
I fully support somebody coming up with a --enable-developer-mode meta-flag, but that's not this bug.
Depends on: 384267
Depends on: 384269
Depends on: 384831
This apparently caused critical a11y bug 382626.
Depends on: 392626
Sorry, I meant bug 392626, but bug 382626 is probably a dupe since it shows the same symptoms.
Depends on: 368464
Just one question: Is the ifdef check for MOZ_META_COMPONENT in the code under http://lxr.mozilla.org/seamonkey/source/config/config.mk#288 still needed? It was checked in as part of this bug here, but MOZ_META_COMPONENT is no longer(?) defined anywhere in the code, see http://mxr.mozilla.org/mozilla/search?string=MOZ_META_COMPONENT.
The meta-component bits got removed during an iteration of this patch, so that ifdef is unnecessary.
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 3 alpha3 → mozilla3 alpha3
You need to log in before you can comment on or make changes to this bug.