648911 - Kill --disable-libxul entirely

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Matheus Kerschbaum]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110409 Firefox/4.2a1pre
Build Identifier: 

No longer a supported build option.

Reproducible: Always

Comment 1

[Matheus Kerschbaum]

Attachment: patch

Kyle, can I rm static-config.mk and static-rules.mk or are they needed by anyone? (i.e Thunderbird)

Comment 2

[Mark Banner (:standard8)]

Last I heard, Ted was thinking of re-using the static-*.mk files for static libxul that would be capable of being linked elsewhere. Not sure if that's still true or appropriate though.

Comment 3

[(not currently active) Ted Mielczarek]

We still might at some point, but you can hg rm them for now. If I need to resurrect them, well, that's why we have VCS.

Comment 4

[Ed Morley [:emorley]]

Hope you don't mind the drive-by...

https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/config/rules.mk_sec2
This (which works out to FORCE_SHARED_LIB && !BUILD_STATIC_LIBS):
> ifneq (_1,$(FORCE_SHARED_LIB)_$(BUILD_STATIC_LIBS))
Was replaced by:
> ifneq (_1,$(FORCE_SHARED_LIB))
Whereas presumably it needs to be:
> ifneq (,$(FORCE_SHARED_LIB))
(Plus in js/src/ equivalent)

https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/config/rules.mk_sec3
This (which I think works out to !FORCE_SHARED_LIB && BUILD_STATIC_LIBS):
> ifeq (_1,$(FORCE_SHARED_LIB)_$(BUILD_STATIC_LIBS))
Was replaced by:
> ifeq (_1,$(FORCE_SHARED_LIB))
Whereas that whole ifeq section should presumably be removed. (Same with js/src/ equivalent)

https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/config/config.mk_sec1
This ifneq has been removed:
> ifneq (,$(BUILD_STATIC_LIBS))
Whereas presumably should resolve false now, so the whole ifneq section should removed? (Same with js/src/ equivalent)

https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/docshell/build/Makefile.in_sec1
> ifeq ($(OS_ARCH)$(MOZ_ENABLE_LIBXUL),WINNT)
Is replaced by:
> ifeq ($(OS_ARCH),WINNT)
Whereas presumably should resolve false now, so the section be removed? (Same with js/src/ and xpinstall/src/ equivalents)

https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/browser/app/Makefile.in_sec1
The consecutive |ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))| sections can presumably be combined.

https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/gfx/Makefile.in_sec1
Consecutive |DIRS += | could be combined.

Anyway, again hope you don't mind the drive-by; only keeping an eye on this since it will rot some of my patches. Good riddance to non-xul cruft! :-)

Comment 5

[Ed Morley [:emorley]]

Also WRAP_LCMS_HEADERS seems to only be defined when |MOZ_ENABLE_LIBXUL!=1|; so presumably the whole |#ifdef WRAP_LCMS_HEADERS| section can be removed too?
https://bugzilla.mozilla.org/attachment.cgi?id=525003&action=diff#a/config/system-headers_sec2

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 525003 [details] [diff] [review]
patch

Sorry, I really should have moved this request over last week, but I'm not going to have time to review anything large until May.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 525003 [details] [diff] [review]
patch

>diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in
>--- a/browser/installer/removed-files.in
>+++ b/browser/installer/removed-files.in
>@@ -1267,32 +1267,24 @@ xpicleanup@BIN_SUFFIX@
>     Microsoft.VC80.CRT.manifest
>     msvcm80.dll
>     msvcp80.dll
>     msvcr80.dll
>   #else
>     mozcrt19.dll
>   #endif
> #endif
>-#ifdef MOZ_ENABLE_LIBXUL
>   @DLL_PREFIX@xpcom_core@DLL_SUFFIX@

Fix the indentation while removing the #ifdefs, (the file is specifically set up such that things inside #ifdefs are indented two spaces).

>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
> # If module is going to be merged into the nsStaticModule, 
> # make sure that the entry points are translated and 
> # the module is built static.
> 
> ifdef IS_COMPONENT
> ifdef EXPORT_LIBRARY
>-ifneq (,$(BUILD_STATIC_LIBS))
> ifdef MODULE_NAME
> DEFINES += -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1
> FORCE_STATIC_LIB=1
> endif
> endif
> endif
>-endif

This hunk is wrong, "ifneq (,$(BUILD_STATIC_LIBS))" is equivalent to "ifdef BUILD_STATIC_LIBS", so you should just remove the whole block.

>diff --git a/config/rules.mk b/config/rules.mk
>--- a/config/rules.mk
>+++ b/config/rules.mk
>-ifneq (_1,$(FORCE_SHARED_LIB)_$(BUILD_STATIC_LIBS))
>+ifneq (_1,$(FORCE_SHARED_LIB))

I realize that conditionals like this are basically impossible to read, but what this means is (in pseudocode):

if !(!FORCE_SHARED_LIB && BUILD_STATIC_LIBS)

which is equivalent to:

if FORCE_SHARED_LIB || !BUILD_STATIC_LIBS

since you're removing the latter, you can just rewrite this as:

ifdef FORCE_SHARED_LIB

>-ifeq (,$(BUILD_STATIC_LIBS)$(FORCE_STATIC_LIB))
>+ifeq (,$(FORCE_STATIC_LIB))

You can just write this as

ifndef FORCE_STATIC_LIB

>-ifeq (_1,$(FORCE_SHARED_LIB)_$(BUILD_STATIC_LIBS))
>+ifeq (_1,$(FORCE_SHARED_LIB))

Similar to the earlier condition this is basically:

if !FORCE_SHARED_LIB && BUILD_STATIC_LIBS

and since you're removing the latter you can just write it as:

ifndef FORCE_SHARED_LIB

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
> MOZ_ARG_WITH_BOOL(system-libxul,
> [  --with-system-libxul    Use system installed libxul SDK],
>     SYSTEM_LIBXUL=1)
> 
>-if test -n "$SYSTEM_LIBXUL" -a -z "$MOZ_ENABLE_LIBXUL"; then
>-    AC_MSG_ERROR([--with-system-libxul needs --with-libxul-sdk])
>-fi

I don't think removing this is correct, this needs to be rewritten somehow. (Unless what it says is incorrect.)

>diff --git a/docshell/build/Makefile.in b/docshell/build/Makefile.in
>--- a/docshell/build/Makefile.in
>+++ b/docshell/build/Makefile.in
>@@ -52,17 +52,17 @@ EXPORT_LIBRARY = 1
> GRE_MODULE	= 1
> LIBXUL_LIBRARY	= 1
> 
> 
> EXPORTS = \
> 		  nsDocShellCID.h \
> 		  $(NULL)
> 
>-ifeq ($(OS_ARCH)$(MOZ_ENABLE_LIBXUL),WINNT)
>+ifeq ($(OS_ARCH),WINNT)

This is not correct, the original test is effectively:

if OS_ARCH == WINNT && !MOZ_ENABLE_LIBXUL

so you can just remove the block entirely.

>diff --git a/gfx/angle/include/GLSLANG/ShaderLang.h b/gfx/angle/include/GLSLANG/ShaderLang.h
>--- a/gfx/angle/include/GLSLANG/ShaderLang.h
>+++ b/gfx/angle/include/GLSLANG/ShaderLang.h

ANGLE is an upstream project. Can you ask bjacob how we handle changes there? These are clearly Mozilla-specific bits, but I don't know if we should patch them locally.

>diff --git a/gfx/cairo/cairo/src/cairo-platform.h b/gfx/cairo/cairo/src/cairo-platform.h
>--- a/gfx/cairo/cairo/src/cairo-platform.h
>+++ b/gfx/cairo/cairo/src/cairo-platform.h

Same with Cairo. Can you ask Joe Drew or Jeff Muizelaar?

>diff --git a/gfx/src/gfxCore.h b/gfx/src/gfxCore.h
>--- a/gfx/src/gfxCore.h
>+++ b/gfx/src/gfxCore.h
>@@ -46,23 +46,13 @@ namespace mozilla {
>     enum Side {eSideTop, eSideRight, eSideBottom, eSideLeft};
>   }
> }
> #define NS_SIDE_TOP     mozilla::css::eSideTop
> #define NS_SIDE_RIGHT   mozilla::css::eSideRight
> #define NS_SIDE_BOTTOM  mozilla::css::eSideBottom
> #define NS_SIDE_LEFT    mozilla::css::eSideLeft
> 
>-#if defined(MOZ_ENABLE_LIBXUL) || !defined(MOZILLA_INTERNAL_API)

Leave the other half of this ifdef in here.

>diff --git a/gfx/tests/Makefile.in b/gfx/tests/Makefile.in
>--- a/gfx/tests/Makefile.in
>+++ b/gfx/tests/Makefile.in
>@@ -49,60 +49,16 @@ MOZILLA_INTERNAL_API = 1
> XPCSHELL_TESTS = unit
> 
> _TEST_FILES = $(addprefix mochitest/, \
> 	test_bug509244.html \
> 	test_bug513439.html \
> 	test_acceleration.html \
> )
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-ifndef BUILD_STATIC_LIBS
>-
>-CPP_UNIT_TESTS	= \

Hmm. I think we should leave this in here, even though it's not possible to build these tests currently. Can you just either comment out this entire region, or make it ifdef BUILD_GFX_CPP_TESTS or something like that? We would like to figure out how to get this stuff to build in the future. (We probably already have a bug on file on figuring out how to make C++ unit tests work with libxul builds.)

>diff --git a/intl/uconv/tests/Makefile.in b/intl/uconv/tests/Makefile.in
>--- a/intl/uconv/tests/Makefile.in
>+++ b/intl/uconv/tests/Makefile.in
>@@ -41,61 +41,22 @@ srcdir		= @srcdir@
> VPATH		= @srcdir@
> relativesrcdir = intl/uconv/tests
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE          = test_intl_uconv
> XPCSHELL_TESTS	= unit
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-MOZILLA_INTERNAL_API = 1
>-
>-
>-CPPSRCS		= \
>-		TestUConv.cpp \
>-		nsconv.cpp \
>-		plattest.cpp \
>-		$(NULL)

Same thing here as with the gfx tests, I'd rather just have this commented out or disabled for now, and not completely lose it.

>diff --git a/js/jsd/Makefile.in b/js/jsd/Makefile.in
>--- a/js/jsd/Makefile.in
>+++ b/js/jsd/Makefile.in
>@@ -43,19 +43,16 @@ topsrcdir	= @top_srcdir@
> VPATH		= @srcdir@
> srcdir		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE		= jsdebug
> LIBRARY_NAME	= jsd
> FORCE_SHARED_LIB= 1
>-ifeq ($(OS_ARCH)$(MOZ_ENABLE_LIBXUL),WINNT)
>-LIBRARY_NAME	= jsd32$(VERSION_NUMBER)
>-endif
> 
> # REQUIRES	= java js
> 
> 
> EXTRA_DSO_LDOPTS += \
> 		$(MOZ_COMPONENT_LIBS) \
> 		$(MOZ_JS_LIBS) \
> 		$(NULL)
>@@ -82,22 +79,20 @@ CSRCS		= \
> ifdef JSD_STANDALONE
> DIRS	+= jsdb
> else
> DIRS	+= idl
> CPPSRCS	 = jsd_xpc.cpp
> IS_COMPONENT = 1
> LIBXUL_LIBRARY = 1
> 
>-ifdef MOZ_ENABLE_LIBXUL
> FORCE_SHARED_LIB=
> MODULE_NAME = JavaScript_Debugger
> EXPORT_LIBRARY = 1
> endif
>-endif

Can you remove the JSD_STANDALONE block (this is the only place in the tree that that variable is mentioned), and merge this with the block above that sets FORCE_SHARED_LIB=1?
>diff --git a/js/src/xpconnect/src/Makefile.in b/js/src/xpconnect/src/Makefile.in
>--- a/js/src/xpconnect/src/Makefile.in
>+++ b/js/src/xpconnect/src/Makefile.in
>@@ -48,20 +48,16 @@ MODULE		= xpconnect
> 
> ifeq (xpconnect, $(findstring xpconnect, $(BUILD_MODULES)))
> LIBRARY_NAME	= xpconnect
> EXPORT_LIBRARY = 1
> SHORT_LIBNAME	= xpconect
> IS_COMPONENT	= 1
> MODULE_NAME	= xpconnect
> GRE_MODULE	= 1
>-ifeq ($(OS_ARCH)$(MOZ_ENABLE_LIBXUL),WINNT)
>-LIBRARY_NAME	= xpc32$(VERSION_NUMBER)
>-SHORT_LIBNAME	= xpc32$(VERSION_NUMBER)
>-endif
> else
> LIBRARY_NAME    = xpconnect_s
> FORCE_STATIC_LIB = 1
> endif
> LIBXUL_LIBRARY = 1
> EXPORTS = xpcpublic.h

We could probably stand another bug on just cleaning up this Makefile. It's pretty awful. (This patch is big enough though, save that for later.)

> 
> CPPSRCS		= \
>diff --git a/js/src/xpconnect/tests/Makefile.in b/js/src/xpconnect/tests/Makefile.in
>--- a/js/src/xpconnect/tests/Makefile.in
>+++ b/js/src/xpconnect/tests/Makefile.in
>@@ -45,20 +45,16 @@ relativesrcdir = js/src/xpconnect/tests
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE		= TestXPC
> SIMPLE_PROGRAMS = TestXPC$(BIN_SUFFIX)
> 
> 
> DIRS		= idl mochitest chrome
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-DIRS += components
>-endif

I'd prefer to have this commented out and have a bug filed on making it work with libxul builds.

>diff --git a/layout/style/test/Makefile.in b/layout/style/test/Makefile.in
>--- a/layout/style/test/Makefile.in
>+++ b/layout/style/test/Makefile.in
>@@ -58,31 +58,16 @@ endif
> 
> HOST_CPPSRCS	= \
> 		ListCSSProperties.cpp \
> 		$(NULL)
> 
> HOST_SIMPLE_PROGRAMS	= $(addprefix host_, $(HOST_CPPSRCS:.cpp=$(HOST_BIN_SUFFIX)))
> 
> 
>-# ParseCSS.cpp used to be built as a test program, but it was not
>-# being used for anything, and recent changes to the CSS loader have
>-# made it fail to link.  Further changes are planned which should make
>-# it buildable again.
>-#
>-# TestCSSPropertyLookup.cpp needs the internal XPCOM APIs and so cannot
>-# be built with libxul enabled.
>-
>-ifndef BUILD_STATIC_LIBS
>-ifndef MOZ_ENABLE_LIBXUL
>-CPP_UNIT_TESTS = TestCSSPropertyLookup.cpp
>-LIBS += ../nsCSSKeywords.$(OBJ_SUFFIX) ../nsCSSProps.$(OBJ_SUFFIX) $(XPCOM_LIBS)
>-endif
>-endif

Again, leave these in but commented out or disabled, especially given the comment block here that says they want to be fixed.
>diff --git a/mobile/app/Makefile.in b/mobile/app/Makefile.in
>--- a/mobile/app/Makefile.in
>+++ b/mobile/app/Makefile.in
>-LIBS += $(APP_XPCOM_LIBS) \
>+LIBS += $(XPCOM_GLUE_LDOPTS) \
> 	$(NSPR_LIBS) \
> 	$(NULL)

Can you reformat this while you're here?

LIBS += \
  $(XPCOM_GLUE_LDOPTS) \
  ...

>diff --git a/netwerk/test/Makefile.in b/netwerk/test/Makefile.in
>--- a/netwerk/test/Makefile.in
>+++ b/netwerk/test/Makefile.in
>@@ -65,30 +65,16 @@ SIMPLE_PROGRAMS	= \
> 		TestBlockingSocket$(BIN_SUFFIX) \
> 		TestDNS$(BIN_SUFFIX) \
> 		TestOpen$(BIN_SUFFIX) \
> 		TestCookie$(BIN_SUFFIX) \
> 		TestServ$(BIN_SUFFIX) \
> 		ReadNTLM$(BIN_SUFFIX) \
> 		$(NULL)
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-SIMPLE_PROGRAMS += \

Again, I'd like to keep this block here.

>diff --git a/parser/htmlparser/tests/Makefile.in b/parser/htmlparser/tests/Makefile.in
>--- a/parser/htmlparser/tests/Makefile.in
>+++ b/parser/htmlparser/tests/Makefile.in
>@@ -43,15 +43,9 @@ VPATH		= @srcdir@
> include $(DEPTH)/config/autoconf.mk
> 
> DIRS = \
> 	grabpage \
> 	html \
> 	mochitest \
> 	$(NULL)
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-DIRS += \
>-	outsinks \
>-	$(NULL)
>-endif

Same here. Maybe file a bug on this as well?

>diff --git a/toolkit/components/url-classifier/tests/Makefile.in b/toolkit/components/url-classifier/tests/Makefile.in
>--- a/toolkit/components/url-classifier/tests/Makefile.in
>+++ b/toolkit/components/url-classifier/tests/Makefile.in
>@@ -51,28 +51,9 @@ MOZILLA_INTERNAL_API = 1
> 
> # mochitest tests
> DIRS += mochitest \
> 	$(NULL)
> 
> # xpcshell tests
> XPCSHELL_TESTS=unit
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-# simple c++ tests (no xpcom)
>-CPP_UNIT_TESTS = \
>-	TestUrlClassifierUtils.cpp \
>-	$(NULL)

Here as well.

>diff --git a/toolkit/crashreporter/test/Makefile.in b/toolkit/crashreporter/test/Makefile.in
>--- a/toolkit/crashreporter/test/Makefile.in
>+++ b/toolkit/crashreporter/test/Makefile.in
>@@ -65,31 +65,17 @@ CPPSRCS = \
> 
> LOCAL_INCLUDES += \
>   -I$(XPIDL_GEN_DIR) \
>   -I$(srcdir)/../google-breakpad/src/ \
>   $(NULL)
> EXTRA_DSO_LIBS += xpcom
> EXTRA_DSO_LDOPTS += $(LIBS_DIR) $(MOZ_COMPONENT_LIBS)
> 
>-ifdef MOZ_ENABLE_LIBXUL
> EXTRA_DSO_LDOPTS += $(XPCOM_GLUE_LDOPTS)

You can just combine this line into the assignment above.

>diff --git a/widget/src/windows/tests/Makefile.in b/widget/src/windows/tests/Makefile.in
>--- a/widget/src/windows/tests/Makefile.in
>+++ b/widget/src/windows/tests/Makefile.in
>@@ -37,33 +37,9 @@
> 
> DEPTH		= ../../../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
>-ifndef BUILD_STATIC_LIBS
>-ifndef MOZ_ENABLE_LIBXUL
>-LOCAL_INCLUDES =  -I$(srcdir)/../   \
>-                  -I$(srcdir)/../../xpwidgets \
>-                  $(NULL)
>-
>-LIBS = ../$(LIB_PREFIX)widget_windows.$(LIB_SUFFIX) \
>-       ../../xpwidgets/$(LIB_PREFIX)xpwidgets_s.$(LIB_SUFFIX) \
>-       $(DIST)/lib/$(LIB_PREFIX)thebes.$(LIB_SUFFIX) \
>-       $(DIST)/lib/$(LIB_PREFIX)gkgfx.$(LIB_SUFFIX) \
>-       $(XPCOM_LIBS) \
>-       $(MOZ_UNICHARUTIL_LIBS) \
>-		   $(QCMS_LIBS) \
>-       $(NULL)
>-
>-EXTRA_DSO_LDOPTS += $(LIBS_DIR)
>-
>-OS_LIBS += $(call EXPAND_LIBNAME,ole32 oleaut32 shell32 comctl32 comdlg32 imm32 shlwapi winspool msimg32)
>-
>-CPP_UNIT_TESTS = TestWinDND.cpp \
>-                 $(NULL)

I'd like to keep this as well.

>diff --git a/xpcom/Makefile.in b/xpcom/Makefile.in
>--- a/xpcom/Makefile.in
>+++ b/xpcom/Makefile.in
>@@ -56,40 +56,28 @@ DIRS		= \
> 		threads \
> 		reflect \
> 		proxy \
>                 system \
> 		../chrome \
> 		build \
> 		$(NULL)
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-DIRS += stub
>-endif

Does this dir get built elsewhere? If not, file a separate bug on removing it.

>-#  Can't build internal xptcall tests that use symbols which are not exported.
>-ifndef MOZ_ENABLE_LIBXUL
>-TOOL_DIRS += \
>-	reflect/xptinfo/tests \
>-	reflect/xptcall/tests \
>-	$(NULL)
>-endif

Please keep these tests as well. (I know I'm repeating myself a lot here.)
>diff --git a/xpcom/build/Makefile.in b/xpcom/build/Makefile.in
>--- a/xpcom/build/Makefile.in
>+++ b/xpcom/build/Makefile.in
>@@ -47,41 +47,33 @@ include $(srcdir)/../glue/objs.mk
> EXTRA_DEPS += $(srcdir)/../glue/objs.mk
> 
> MODULE		= xpcom
> LIBRARY_NAME	= xpcom_core
> SHORT_LIBNAME	= xpcomcor
> LIBXUL_LIBRARY = 1
> 
> # This is only a static library in libxul builds
>-ifdef MOZ_ENABLE_LIBXUL
> EXPORT_LIBRARY = 1
>-endif

Remove the comment you're obsoleting.

>diff --git a/xpcom/io/nsSegmentedBuffer.cpp b/xpcom/io/nsSegmentedBuffer.cpp
>--- a/xpcom/io/nsSegmentedBuffer.cpp
>+++ b/xpcom/io/nsSegmentedBuffer.cpp
>@@ -162,46 +162,9 @@ nsSegmentedBuffer::Empty()
>         }
>         nsMemory::Free(mSegmentArray);
>         mSegmentArray = nsnull;
>     }
>     mSegmentArrayCount = NS_SEGMENTARRAY_INITIAL_COUNT;
>     mFirstSegmentIndex = mLastSegmentIndex = 0;
> }
> 
>-#if !defined(MOZ_ENABLE_LIBXUL) && defined(DEBUG)
>-NS_COM void
>-TestSegmentedBuffer()

Maybe just #if 0 this instead?

>diff --git a/xpcom/tests/Makefile.in b/xpcom/tests/Makefile.in
>--- a/xpcom/tests/Makefile.in
>+++ b/xpcom/tests/Makefile.in
> 
>-ifndef MOZ_ENABLE_LIBXUL
>-CPP_UNIT_TESTS += \

Keep these, please.

>diff --git a/xpcom/tests/TestPipes.cpp b/xpcom/tests/TestPipes.cpp
>--- a/xpcom/tests/TestPipes.cpp
>+++ b/xpcom/tests/TestPipes.cpp
>@@ -466,38 +466,28 @@ RunTests(PRUint32 segSize, PRUint32 segC
>     rv = TP_NewPipe(getter_AddRefs(in), getter_AddRefs(out), segSize, bufSize);
>     NS_ASSERTION(NS_SUCCEEDED(rv), "TP_NewPipe failed");
>     rv = TestShortWrites(in, out);
>     NS_ASSERTION(NS_SUCCEEDED(rv), "TestPipe failed");
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
> 
>-#if !defined(MOZ_ENABLE_LIBXUL) && defined(DEBUG)
>-extern NS_COM void
>-TestSegmentedBuffer();
>-#endif

Ah, here's the other half of that. Just #if 0 this as well.

Thanks for writing this enormous patch! If you fix up those things I mention this should be r+ next time.

Comment 8

[Matheus Kerschbaum]

Attachment: patch

This fixes most review comments and compiles, but breaks a bunch of stuff: http://dev.philringnalda.com/tbpl/?tree=Try&rev=cf59ecc18d7f

Comment 9

[Matheus Kerschbaum]

Attachment: patch

This fixes check-sync-dirs, but doesn't fix all the other test failures.
Looking at the logs it seems that my patch breaks the test-plugin, which then causes all the tests that use it to fail. I still don't know _why_ my patch breaks the test-plugin though.

Comment 10

[Ed Morley [:emorley]]

As always, take my comments with a pinch of salt as I'm still new to this - but I think I've spotted a few things that might be causing the failures... 

(and thought if it meant this passing try sooner, that you wouldn't mind the drive by :-) )

>diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
>--- a/browser/app/Makefile.in
>+++ b/browser/app/Makefile.in
>@@ -90,61 +90,30 @@ else
>-else
>-MOZILLA_INTERNAL_API = 1
>-APP_XPCOM_LIBS = $(XPCOM_LIBS)
>-endif

The MOZILLA_INTERNAL_API = 1 part was removed here (as part of optimising the APP_XPCOM_LIBS bit), but it should presumably still be there, given:
http://mxr.mozilla.org/mozilla-central/search?string=MOZILLA_INTERNAL_API

>diff --git a/js/jsd/Makefile.in b/js/jsd/Makefile.in
>--- a/js/jsd/Makefile.in
>+++ b/js/jsd/Makefile.in
>@@ -43,19 +43,25 @@ topsrcdir	= @top_srcdir@
>+DIRS	+= idl
>+CPPSRCS	 = jsd_xpc.cpp
>+IS_COMPONENT = 1
>+LIBXUL_LIBRARY = 1
>+
>+FORCE_SHARED_LIB=
>+MODULE_NAME = JavaScript_Debugger
>+EXPORT_LIBRARY = 1

This moved code fragment used to be inside a JSD_STANDALONE ifdef, but isn't any more.

>diff --git a/js/src/config/rules.mk b/js/src/config/rules.mk
>--- a/js/src/config/rules.mk
>+++ b/js/src/config/rules.mk
>-ifdef FORCE_SHARED_LIB
>-ifndef FORCE_STATIC_LIB
>-LIBRARY			:= $(NULL)
>-endif
>-endif

I think this whole section shouldn't be removed; but instead just the ifndef FORCE_STATIC_LIB part - however all the FORCE_STATIC_LIB vs FORCE_SHARED_LIB similarities were making my head sore reading this though, so could be wrong!

>diff --git a/xpcom/tests/component/Makefile.in b/xpcom/tests/component/Makefile.in
>--- a/xpcom/tests/component/Makefile.in
>+++ b/xpcom/tests/component/Makefile.in
>@@ -42,16 +42,17 @@ srcdir		= @srcdir@
>+FORCE_SHARED_LIB = 1

Where did this come from?


Just reading through the huge patch took long enough - this must have been quite a mission to do! Nice work :-)

Comment 11

[Matheus Kerschbaum]

Attachment: patch

This should fix all the test failures (try-run: http://dev.philringnalda.com/tbpl/?tree=Try&rev=532c8002887d).

Comment 12

[Matheus Kerschbaum]

Attachment: patch

Updated to tip.

Comment 13

[Matheus Kerschbaum]

Review ping.

Comment 14

[(not currently active) Ted Mielczarek]

Sorry, it's a large patch and I've been a bit behind in reviews. I'll review it again by tomorrow.

Comment 15

[(not currently active) Ted Mielczarek]

I started the review on Friday but ran out of time, sorry about that. I'll finish it up today.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 529269 [details] [diff] [review]
patch

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

Okay, this is pretty close. It has two erroneous changes in rules.mk that need to be fixed, and just a few other minor things. I have also asked Joe Drew for review on the gfx changes. If he signs off on them, and you fix the few things I commented on here, you should be good to go.

Sorry this takes so long, but this is a really big patch. In the future if you want to do a tree-wide cleanup, it would be simpler to split it up into multiple patches by section of code to make reviewing it easier.

::: config/config.mk
@@ +253,3 @@
>  ifneq (,$(FORCE_SHARED_LIB)$(FORCE_USE_PIC))
>  _ENABLE_PIC=1
>  endif

I think you can just remove this block, since you're already unconditionally setting _ENABLE_PIC=1.

::: config/rules.mk
@@ -282,5 @@
> -DEF_FILE		:= $(NULL)
> -IMPORT_LIBRARY		:= $(NULL)
> -endif
> -
> -ifdef FORCE_STATIC_LIB

You shouldn't remove this FORCE_STATIC_LIB. FORCE_STATIC_LIB is not the same as BUILD_STATIC_LIBS.

@@ -294,5 @@
> -ifdef FORCE_SHARED_LIB
> -ifndef FORCE_STATIC_LIB
> -LIBRARY			:= $(NULL)
> -endif
> -endif

This block should stay too.

::: gfx/cairo/cairo/src/cairo-platform.h
@@ -41,5 @@
>  
>  /* we're replacing any definition from cairoint.h etc */
>  #undef cairo_public
>  
> -#if defined(MOZ_ENABLE_LIBXUL)

These cairo changes still need a gfx peer to sign off on.

::: gfx/cairo/libpixman/src/pixman-compiler.h
@@ -63,5 @@
>  #      define noinline
>  #   endif
>  #endif
>  
>  /* In libxul builds we don't ever want to export pixman symbols */

pixman changes need a gfx peer to sign off on as well.

::: gfx/thebes/Makefile.in
@@ -206,5 @@
>  
> -ifndef MOZ_ENABLE_LIBXUL
> -EXTRA_DSO_LIBS	= gkgfx ycbcr
> -ifeq (,$(filter-out WINNT WINCE OS2,$(OS_ARCH)))
> -CPPSRCS += gfxDllDeps.cpp

I think this means we can hg rm gfxDllDeps completely.

::: js/jsd/Makefile.in
@@ +47,5 @@
>  
>  MODULE		= jsdebug
>  LIBRARY_NAME	= jsd
>  FORCE_SHARED_LIB= 1
> +DIRS	+= idl

nit: this can just be "DIRS = idl" now.

@@ +52,5 @@
> +CPPSRCS	 = jsd_xpc.cpp
> +IS_COMPONENT = 1
> +LIBXUL_LIBRARY = 1
> +
> +FORCE_SHARED_LIB=

Just remove this line and the other line that sets FORCE_SHARED_LIB=1.

::: mobile/app/Makefile.in
@@ +62,5 @@
>  NSDISTMODE = copy
>  endif
>  
> +LIBS += \
> +	$(XPCOM_GLUE_LDOPTS) \

nit: can you just fix the indentation of these three lines to use 2 spaces instead of a tab while you're here?

::: modules/plugin/test/testplugin/Makefile.in
@@ +56,5 @@
>  NO_DIST_INSTALL	= 1
>  NO_INSTALL = 1
>  
> +FORCE_SHARED_LIB = 1
> +

Interesting, does this get broken because of some rules.mk changes? I guess this makes sense. Can you put this up by the LIBRARY_NAME lines, though?

::: xpcom/build/Makefile.in
@@ -72,5 @@
>  		$(NULL)
>  
> -ifndef MOZ_ENABLE_LIBXUL
> -ifeq (,$(filter-out WINNT WINCE OS2,$(OS_ARCH)))
> -CPPSRCS += dlldeps.cpp

I think dlldeps.cpp can be removed as well.

Comment 17

[Matheus Kerschbaum]

Attachment: patch

Updated per review comments.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 531258 [details] [diff] [review]
patch

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

Okay, this looks good to me. Have you run this latest patch by the try server?

Comment 19

[Matheus Kerschbaum]

(In reply to comment #18)
> Okay, this looks good to me. Have you run this latest patch by the try
> server?

http://tbpl.mozilla.org/?tree=Try&rev=d337167e5102

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If this is good to go lets get it into the tree ASAP so it doesn't bitrot.

Comment 21

[(not currently active) Ted Mielczarek]

Still needs signoff from a gfx peer, hence the r?joe.

Comment 22

[Joe Drew (not getting mail)]

Comment on attachment 531258 [details] [diff] [review]
patch

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

This needs updates as I mentioned below, but other than that looks fine.

::: gfx/angle/include/GLSLANG/ShaderLang.h
@@ -20,1 @@
>  #  define ANGLE_API  /*nothing*/

You'll also need to regenerate gfx/angle/angle-shared.patch after these changes.

::: gfx/cairo/libpixman/src/pixman-compiler.h
@@ -76,5 @@
> -#elif defined(__SUNPRO_C) && (__SUNPRO_C >= 0x550)
> -#   define PIXMAN_EXPORT __global
> -#else
> -#   define PIXMAN_EXPORT
> -#endif

This stuff that you're removing is in upstream pixman. What you want to do instead is #if 0 out the existing visibility stuff, and then update gfx/cairo/pixman-export.patch.

Comment 23

[Matheus Kerschbaum]

Attachment: patch

angle-shared.patch only seems to be needed for shared builds, which this patch removes support for, so I ended up removing it altogether. Assuming that's true, I think this patch is ready for landing (try-run: http://tbpl.mozilla.org/?tree=Try&rev=d4bee452c016).

Comment 24

[Matheus Kerschbaum]

Attachment: patch for checkin

Updated to tip.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Rotted again... Could you please follow <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f>?

Comment 26

[Matheus Kerschbaum]

Attachment: patch for checkin

Updated again and added commit message.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/8e846d7f21ea

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Whew.  Thanks for this Matheus!

Comment 29

[(not currently active) Ted Mielczarek]

Yeah, thanks for sticking through it, that was a lot of work, but great cleanup!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/66d7bf407cd2
Remove support for non-libxul builds. r=ted,joedrew