602842 - Port bug 522770 - Implement enough of fakelibs to work around MSVC limitations

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

Although we don't gain much from this at the moment (the real perf improvement is in bug 584474), we should do it anyway as it'll clear the way for that second bug to land without hacks and then we can pick it up as well.

I have a patch in progress for this.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix

This does the port. It passes build and (packaged) tests on Thunderbird on try server.

Comment 2

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

Comment on attachment 482820 [details] [diff] [review]
The fix

Callek asked me to take a peek.

>fakelibs try: -b do -p win32 -m none -u all -t none
>
>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>@@ -80,6 +80,11 @@ check-variable = $(if $(filter-out 0 1,$
> 
> core_abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(CURDIR)/$(1)))
> 
>+nullstr :=
>+space :=$(nullstr) # EOL
>+
>+core_winabspath = $(firstword $(subst /, ,$(call core_abspath,$(1)))):$(subst $(space),,$(patsubst %,\\%,$(wordlist 2,$(words $(subst /, ,$(call core_abspath,$(1)))), $(strip $(subst /, ,$(call core_abspath,$(1)))))))
>+

Sfink fixed this up to not be awful in Bug 584473.  You should grab that and do it right the first time (that patch hasn't landed on m-c).
> # FINAL_TARGET specifies the location into which we copy end-user-shipped
> # build products (typelibs, components, chrome).
> #
>@@ -321,6 +326,10 @@ STATIC_LIBRARY_NAME=$(LIBRARY_NAME)
> endif
> endif
> 
>+ifeq (WINNT,$(OS_ARCH))
>+MOZ_FAKELIBS = 1
>+endif
>+
> # This comes from configure
> ifdef MOZ_PROFILE_GUIDED_OPTIMIZE_DISABLE
> NO_PROFILE_GUIDED_OPTIMIZE = 1
>@@ -681,7 +690,7 @@ DEFINES		+= -DOSARCH=$(OS_ARCH)
> 
> ######################################################################
> 
>-GARBAGE		+= $(DEPENDENCIES) $(MKDEPENDENCIES) $(MKDEPENDENCIES).bak core $(wildcard core.[0-9]*) $(wildcard *.err) $(wildcard *.pure) $(wildcard *_pure_*.o) Templates.DB
>+GARBAGE		+= $(DEPENDENCIES) $(MKDEPENDENCIES) $(MKDEPENDENCIES).bak core $(wildcard core.[0-9]*) $(wildcard *.err) $(wildcard *.pure) $(wildcard *_pure_*.o) Templates.DB $(FAKE_LIBRARY)
 
Do we have this on m-c?  I don't think we do.  Could you file a bug if that's the case?

Rest LGTM.

Comment 3

[Justin Wood (:Callek)]

(In reply to comment #2)
> Comment on attachment 482820 [details] [diff] [review]
> The fix
> 
> Callek asked me to take a peek.
> 
> >+nullstr :=
> >+space :=$(nullstr) # EOL
> >+
> >+core_winabspath = $(firstword $(subst /, ,$(call core_abspath,$(1)))):$(subst $(space),,$(patsubst %,\\%,$(wordlist 2,$(words $(subst /, ,$(call core_abspath,$(1)))), $(strip $(subst /, ,$(call core_abspath,$(1)))))))
> >+
> 
> Sfink fixed this up to not be awful in Bug 584473.  You should grab that and do
> it right the first time (that patch hasn't landed on m-c).

I agree this is a mess, but I also think the patch in Bug 584473 is convoluted enough to be nicely done in its own bug instead of this one (If mark really wants to merge it, I want another go at this, but thats ok)

> >-GARBAGE		+= $(DEPENDENCIES) $(MKDEPENDENCIES) $(MKDEPENDENCIES).bak core $(wildcard core.[0-9]*) $(wildcard *.err) $(wildcard *.pure) $(wildcard *_pure_*.o) Templates.DB
> >+GARBAGE		+= $(DEPENDENCIES) $(MKDEPENDENCIES) $(MKDEPENDENCIES).bak core $(wildcard core.[0-9]*) $(wildcard *.err) $(wildcard *.pure) $(wildcard *_pure_*.o) Templates.DB $(FAKE_LIBRARY)
> 
> Do we have this on m-c? 

Yes

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 482820 [details] [diff] [review]
The fix

>+ifdef MOZ_FAKELIBS
>+ifndef SUPPRESS_FAKELIB
>+FAKE_LIBRARY = $(LIBRARY).fake
>+endif # SUPPRESS_FAKELIB
>+endif # MOZ_FAKELIB

nit: # MOZ_FAKELIBS

> ifdef IS_COMPONENT
> 	$(error Shipping static component libs makes no sense.)
> else
>-	$(INSTALL) $(IFLAGS1) $(LIBRARY) $(DIST)/lib
>+	$(INSTALL) $(IFLAGS1) $(LIBRARY) $(FAKE_LIBRARY) $(DIST)/lib

Odd, m-c doesn't add FAKE_LIBRARY here, intended? [I don't see how it would break, but I want sync, if its important get a bug on them, if its not drop it]

>@@ -1140,6 +1160,17 @@ ifneq (,$(BUILD_STATIC_LIBS)$(FORCE_STAT
> LOBJS	+= $(SHARED_LIBRARY_LIBS)
> endif
> else
>+NONFAKE_SHARED_LIBRARY_LIBS = $(filter-out %.fake,$call EXPAND_FAKELIBS,$(SHARED_LIBRARY_LIBS)))

$(call  [missing the open paren]

>+ifeq (,$(NONFAKE_SHARED_LIBRARY_LIBS))
>+# All of our SHARED_LIBRARY_LIBS have fake equivalents. Score!
>+# Just pass the original object files around.
>+# For shared libraries, these are already included in EXTRA_DSO_LDOPTS
>+# above.
>+ifndef SHARED_LIBRARY
>+LOBJS += $(shell cat $(addsuffix .fake,$(SHARED_LIBRARY_LIBS)))
>+endif
>+SKIP_SUB_LOBJS := 1
>+else
> ifneq (,$(filter OSF1 BSD_OS FreeBSD NetBSD OpenBSD SunOS Darwin,$(OS_ARCH)))
> CLEANUP1	:= | egrep -v '(________64ELEL_|__.SYMDEF)'
> CLEANUP2	:= rm -f ________64ELEL_ __.SYMDEF
>@@ -1147,7 +1178,8 @@ else
> CLEANUP2	:= true
> endif
> SUB_LOBJS	= $(shell for lib in $(SHARED_LIBRARY_LIBS); do $(AR_LIST) $${lib} $(CLEANUP1); done;)
>-endif
>+endif # EXPAND_FAKELIBS
>+endif # SHARED_LIBRARY_LIBS

haha, m-c typo'd here.  [SHARED_LIBARY_LIBS] (Keep our fix)



Lets land this, please close tree when you do so we can be sure we have a green tree on SeaMonkey and Thunderbird. [I'd rather be safe]

Comment 5

[Mark Banner (:standard8)]

Checked in with comments addressed:
http://hg.mozilla.org/comm-central/rev/2cff0f45f0d3

Seems to be working fine so far.

Comment 6

[Justin Wood (:Callek)]

(In reply to comment #5)
> Checked in with comments addressed:

Not quite:

(In reply to comment #4)
> >@@ -1140,6 +1160,17 @@ ifneq (,$(BUILD_STATIC_LIBS)$(FORCE_STAT
> > LOBJS	+= $(SHARED_LIBRARY_LIBS)
> > endif
> > else
> >+NONFAKE_SHARED_LIBRARY_LIBS = $(filter-out %.fake,$call EXPAND_FAKELIBS,$(SHARED_LIBRARY_LIBS)))
> 
> $(call  [missing the open paren]

You added *A* paren, just in the wrong spot...

you need to

-NONFAKE_SHARED_LIBRARY_LIBS = $(filter-out %.fake,$call (EXPAND_FAKELIBS,$(SHARED_LIBRARY_LIBS)))
+NONFAKE_SHARED_LIBRARY_LIBS = $(filter-out %.fake,$(call EXPAND_FAKELIBS,$(SHARED_LIBRARY_LIBS)))

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> You added *A* paren, just in the wrong spot...
> 
> you need to
> 
> -NONFAKE_SHARED_LIBRARY_LIBS = $(filter-out %.fake,$call
> (EXPAND_FAKELIBS,$(SHARED_LIBRARY_LIBS)))
> +NONFAKE_SHARED_LIBRARY_LIBS = $(filter-out %.fake,$(call
> EXPAND_FAKELIBS,$(SHARED_LIBRARY_LIBS)))

Fixed: http://hg.mozilla.org/comm-central/rev/1e9ad9328bd9

Comment 8

[Justin Wood (:Callek)]

(In reply to comment #4)
> > ifdef IS_COMPONENT
> > 	$(error Shipping static component libs makes no sense.)
> > else
> >-	$(INSTALL) $(IFLAGS1) $(LIBRARY) $(DIST)/lib
> >+	$(INSTALL) $(IFLAGS1) $(LIBRARY) $(FAKE_LIBRARY) $(DIST)/lib
> 
> Odd, m-c doesn't add FAKE_LIBRARY here, intended? [I don't see how it would
> break, but I want sync, if its important get a bug on them, if its not drop it]
> 

Bug # for the m-c side of this?  And "Why is it important" for us?

Comment 9

[Justin Wood (:Callek)]

(In reply to comment #8)
> (In reply to comment #4)
> > > ifdef IS_COMPONENT
> > > 	$(error Shipping static component libs makes no sense.)
> > > else
> > >-	$(INSTALL) $(IFLAGS1) $(LIBRARY) $(DIST)/lib
> > >+	$(INSTALL) $(IFLAGS1) $(LIBRARY) $(FAKE_LIBRARY) $(DIST)/lib
> > 
> > Odd, m-c doesn't add FAKE_LIBRARY here, intended? [I don't see how it would
> > break, but I want sync, if its important get a bug on them, if its not drop it]
> > 

Err, I stupidly forgot I had your initial patch in my tree when I compared... sorry on this point.