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

RESOLVED FIXED in Thunderbird 3.3a1

Status

MailNews Core
Build Config
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Thunderbird 3.3a1
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 522770
(Assignee)

Comment 1

8 years ago
Created attachment 482820 [details] [diff] [review]
The fix

This does the port. It passes build and (packaged) tests on Thunderbird on try server.
Attachment #482820 - Flags: review?(bugspam.Callek)
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.
(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 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]
Attachment #482820 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Comment 5

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

Seems to be working fine so far.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1

Updated

8 years ago
Depends on: 607032
(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)))
(Assignee)

Comment 7

8 years ago
(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
(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?
(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.
You need to log in before you can comment on or make changes to this bug.