Closed Bug 766685 Opened 12 years ago Closed 12 years ago

Fix USE_STATIC_LIBS so it really works, or remove it

Categories

(Calendar :: Build Config, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rkent, Assigned: rkent)

Details

Attachments

(1 file, 2 obsolete files)

In calendar builds, USE_STATIC_LIBS is currently implemented in some directories, but not in others (like libical). You either want a dependency on the dynamic DLL, or you don't. I don't see the point in mixing the models.  With some simple changes, I can get Moz15 Lightning builds to work event though I don't use VS10. I'd like to at least propose those, as I think it would be a boon to third part builders of Lightning.

I'll post a patch for this soon.
Assignee: nobody → kent
Status: NEW → ASSIGNED
This is the minimal patch that fixes this.
Discussion of the patch:

The goal here is for calbasecomps.dll to not have a dependency to the static library for the VC version used to compile. Since I am running VC 2008 and official builds are VC 2010 for aurora, this is quite clear for my cases. With the fix, I can now load Lightning builds that I make with my local build system into TB 15.0a2 official builds.

The best tool I have found to see this is depends.exe at http://www.dependencywalker.com  When I compile in the standard way without this patch, then I get a dependency shown there to MSVCR90.dll  With the patch, that dependency is gone.

The dependency is coming from the inclusion of -DEFAULTLIB:mozcrt in the linkage. So without the patch, the link statement is:

calBaseModule.cpp
c:/mozilla-build/python/python2.6.exe /c/tb/aurora/src/mozilla/config/pythonpath.py -I../../../mozilla/config /c/tb/auro
ra/src/mozilla/config/expandlibs_exec.py --uselist -- link -NOLOGO -DLL -OUT:calbasecomps.dll -PDB:calbasecomps.pdb -SUB
SYSTEM:WINDOWS  calBaseModule.obj    ./module.res -MANIFESTUAC:NO -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH  -D
EBUG -DEBUGTYPE:CV -DEBUG -OPT:REF   -IMPLIB:fake.lib -LIBPATH:../../../mozilla/dist/lib -NODEFAULTLIB:msvcrt -NODEFAULT
LIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd -DEFAULTLIB:mozcrt  ../../../calendar/base/src/calbase_s.lib ..
/../../calendar/libical/src/libical/mozical.lib  c:/tb/aurora/tb-release/mozilla/dist/lib/mozjs.lib   c:/tb/aurora/tb-re
lease/mozilla/dist/lib/nspr4.lib c:/tb/aurora/tb-release/mozilla/dist/lib/plc4.lib c:/tb/aurora/tb-release/mozilla/dist/
lib/plds4.lib  c:/tb/aurora/tb-release/mozilla/dist/lib/xpcomglue_s.lib c:/tb/aurora/tb-release/mozilla/dist/lib/xpcom.l
ib c:/tb/aurora/tb-release/mozilla/dist/lib/mozalloc.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advap
i32.lib

The key to getting rid of this turned out to be the supposed comments at the end of USE_STATIC_LIBS.  If I simply delete the comment with no other changes, then the link statement becomes:

calBaseModule.cpp
c:/mozilla-build/python/python2.6.exe /c/tb/aurora/src/mozilla/config/pythonpath.py -I../../../mozilla/config /c/tb/auro
ra/src/mozilla/config/expandlibs_exec.py --uselist -- link -NOLOGO -DLL -OUT:calbasecomps.dll -PDB:calbasecomps.pdb -SUB
SYSTEM:WINDOWS  calBaseModule.obj    ./module.res -MANIFESTUAC:NO -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH  -D
EBUG -DEBUGTYPE:CV -DEBUG -OPT:REF   -IMPLIB:fake.lib   ../../../calendar/base/src/calbase_s.lib ../../../calendar/libic
al/src/libical/mozical.lib  c:/tb/aurora/tb-release/mozilla/dist/lib/mozjs.lib   c:/tb/aurora/tb-release/mozilla/dist/li
b/nspr4.lib c:/tb/aurora/tb-release/mozilla/dist/lib/plc4.lib c:/tb/aurora/tb-release/mozilla/dist/lib/plds4.lib  c:/tb/
aurora/tb-release/mozilla/dist/lib/xpcomglue_s.lib c:/tb/aurora/tb-release/mozilla/dist/lib/xpcom.lib c:/tb/aurora/tb-re
lease/mozilla/dist/lib/mozalloc.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib

It now fails because of missing external symbols, which the other changes fix.

I think this patch is the correct thing to do in Moz15 and beyond given bug 732124, but I want to think about it for a day before I ask for a review. I supposed that you could also request that other uses of comments after USE_STATIC_LIBS also be removed, yet they don't seem to be causing any known issues. Also it would be good to migrate the build system changes that were done in that bug to COMM-CENTRAL.
Comment on attachment 635173 [details] [diff] [review]
Don't append comment to USE_STATIC_LIBS

As I said in the discussion, the existing code is simply incorrect (removing a comment should not change the build configuration).

Continuing to use USE_STATIC_LIBS is I believe a good thing, as it increases the ability of unofficial builders to build Lightning with a variety of configurations. I would be in favor of keeping it. But we should do so correctly, which this patch I believe does.

I used a variant of this patch in my "EarlyLight" trial build, which generally worked fine.
Attachment #635173 - Flags: review?(philipp)
Attachment #635173 - Flags: approval-calendar-aurora?
I should note that I have not actually tried this patch yet on comm-central, only on comm-aurora. I supposed I need to do that.
Comment on attachment 635173 [details] [diff] [review]
Don't append comment to USE_STATIC_LIBS

Some fly-by comments:

>+ifndef XPCOM_STATICRUNTIME_GLUE_LDOPTS
>+XPCOM_STATICRUNTIME_GLUE_LDOPTS = $(LIBXUL_DIST)/lib/$(LIB_PREFIX)xpcomglue_staticruntime_s.$(LIB_SUFFIX) $(XPCOM_FROZEN_LDOPTS)
>+else
>+XPCOM_STATICRUNTIME_GLUE_LDOPTS = $(XPCOM_GLUE_LDOPTS)
>+endif

In comm-central XPCOM_STATICRUNTIME_GLUE_LDOPTS will always be undefined currently, and even if it is defined, then you're going to overwrite it with the else part. That doesn't seem to make sense.

So it would seem you need to port that patch properly for this to work.

I'd also be concerned from the Lightning aspect that this will increase the size of the dll and hence the xpi (due to statically including the runtime). That may be acceptable if it isn't a significant increase, but I believe AMO has already commented about the size of some of the lightning xpis. From Benjamin's bug 732124 comment 8:

"Most extensions should however use a static CRT, unless they know that they are compiled against the exact same CRT as Firefox is"

generally this is true for Lightning, so strictly speaking Lightning doesn't really need this patch, but it would mean that developers who choose not to build Thunderbird but Lightning can run with different CRTs, and potentially future-proof Lightning, in the unlikely event that the MSVC version changed again soon before Lightning removed its binary component. The main expense is probably the greater resulting xpi size (assuming I've understood correctly what's going on there).

I'll let Philipp decide what he wants to do here, I'm just offering an explanation of what I believe is happening here.
Attachment #635173 - Flags: feedback-
(In reply to Mark Banner (:standard8) from comment #5)

Thanks for the feedback

> >+ifndef XPCOM_STATICRUNTIME_GLUE_LDOPTS
> >+XPCOM_STATICRUNTIME_GLUE_LDOPTS = $(LIBXUL_DIST)/lib/$(LIB_PREFIX)xpcomglue_staticruntime_s.$(LIB_SUFFIX) $(XPCOM_FROZEN_LDOPTS)
> >+else
> >+XPCOM_STATICRUNTIME_GLUE_LDOPTS = $(XPCOM_GLUE_LDOPTS)
> >+endif
> 
> In comm-central XPCOM_STATICRUNTIME_GLUE_LDOPTS will always be undefined
> currently, and even if it is defined, then you're going to overwrite it with
> the else part. That doesn't seem to make sense.

What I was trying to do was to make this patch work with or without porting of XPCOM_STATICRUNTIME_GLUE_LDOPTS to COMM-CENTRAL. In looking more closely, I think you are correct that I did not quite accomplish that.

> I'd also be concerned from the Lightning aspect that this will increase the
> size of the dll and hence the xpi (due to statically including the runtime).
> That may be acceptable if it isn't a significant increase, but I believe AMO
> has already commented about the size of some of the lightning xpis.

So what you are proposing is that USE_STATIC_LIBS be removed in all of the Calendar binaries. I agree that that is optimal from the perspective of building Lightning from the Mozilla buildbots, but at the expense of making it harder for everyone else. Since I'm "everyone else", and in another bug I did not get much enthusiasm for removing USER_STATIC_LIBS, I thought I would just leave it. I'd also point out that I'm trying to propose the minimal change possible. This module purports to be using USE_STATIC_LIBS, I'm just trying to make it work correctly. Either this patch or an alternate that removes USE_STATIC_LIBS make sense, but the current situation of mixing the two does not.

> Benjamin's bug 732124 comment 8:
> 
> "Most extensions should however use a static CRT, unless they know that they
> are compiled against the exact same CRT as Firefox is"
> 
> generally this is true for Lightning, so strictly speaking Lightning doesn't
> really need this patch,

except that I believe I have showed that USE_STATIC_LIBS followed by a comment, which is the current situation, does not work as you expect, so really that needs to be cleaned up.

> ... The main expense
> is probably the greater resulting xpi size (assuming I've understood
> correctly what's going on there).

If that is the concern, then let's remove USE_STATIC_LIBS everywhere. The usage here is relatively trivial compared to other, larger pieces of code.

I would love to get some feedback as to what to do about USE_STATIC_LIBS before I propose another patch.
I did some experiments on aurora calendar release builds on my local system to test various options. All were tested with --enable-jemalloc on current comm-central aurora

1) Build with no patch: lightning.xpi is 1406kb
2) Build with patch https://bugzilla.mozilla.org/attachment.cgi?id=635173 : lightning.xpi is 1446kb
3) Remove comments from the end of USE_STATIC_LIBS everywhere: won't build, missing references.
4) Build with patch https://bugzilla.mozilla.org/attachment.cgi?id=635173, plus remove comments from ALL instances of USE_STATIC_LIBS: 1446kb
5) Remove all USE_STATIC_LIBS from calendar: lightning.xpi is 1406kb

Discussion:

Completely removing USE_STATIC_LIBS from the Calendar makefiles has no effect on the final build compared to the existing configuration. But this is only because of side effects of a) including a comment at the end when they are used, which partially disables them and b) --enable-jemalloc, which also has its own side effects to partially disable USE_STATIC_LIBS.

When either of the side-effect producers is removed, Lightning builds no longer work (see (case 3) for fixing the comments so that USE_STATIC_LIBS is really used; see bug 753456 where we discussed that --enable-jemalloc is now required).

So it seems to me there are three choices:

1) Do nothing. In that case, the build files are misleading (because it appears that USE_STATIC_LIBS is used when it is not). Also builds without --enable-jemalloc fail (and AFAIK most people do not use --emable-jemalloc on debug builds). Plus you are relying on poorly understood side effects and bugs for the build to work at all.

2) Eliminate USE_STATIC_LIBS everywhere. AFAICT this has no effect on the existing shipped builds, but has the advantages of also working without --enable-jemalloc, and does not rely on the fragile bug in the build system where USE_STATIC_LIBS does not work correctly when followed by a comment.

3) Fix uses of USE_STATIC_LIBS everywhere, and add the reference fixes from bug 732124. This increases the size of the shipped .xpi file by about 3%, but removes the requirement that Lightning is always built with VS 2010.

Personally I would slightly prefer approach 3), but that is just because it makes my development life a little easier. Approach 2) cleans up the build system with no effects AFAICT on existing users, and I would guess would be preferred by the the product drivers.

I'm willing to drive through the patches for either of these approaches.

But given that so far I am mostly talking to myself here, I will have to assume that I am the only one who cares about this, so 1) is the most likely approach.
Summary: Don't link to dynamic crt in calbasecomps.dll → Fix USE_STATIC_LIBS so it really works, or remove it
Sorry for not answering earlier. I haven't really said anything because this is a bit beyond my comfort zone and I would always opt for "whatever works". I think Mark has more experience here and I'd trust his opinion over mine.

I'm fine with adding 3% size since that keeps the build process flexible for developers that don't have vs2010 as long as Thunderbird doesn't "eol" vs2008. The problem with this is not AMO, but rather the addons manager unzipping lightning.xpi synchronous, which causes long script warnings that bascially break the profile if the user cancels them at the wrong moment. Last time we juggled around with how the jars are packaged, maybe we can do so again to counteract (i.e package chrome content into a jar inside the xpi).
Attached patch Include build system changes (obsolete) — Splinter Review
This patch  incorporates the build system changes from mozilla-central, and removes the comments from USE_STATIC_LIBS  it should not do anything, but in fact it changes you from dynamic to static libs since the "comment" in USE_STATIC_LIBS causes that to fail.
Attachment #635173 - Attachment is obsolete: true
Attachment #635173 - Flags: review?(philipp)
Attachment #635173 - Flags: approval-calendar-aurora?
Attachment #640386 - Flags: review?(philipp)
Attachment #640386 - Flags: review?(mbanner)
Comment on attachment 640386 [details] [diff] [review]
Include build system changes

Lets just do this and see what happens on aurora and beta. Its quick to revert and its been sitting here long enough. Please push this on comm-central, it will migrate to aurora next week anyway.
Attachment #640386 - Flags: review?(philipp)
Attachment #640386 - Flags: review?(mbanner)
Attachment #640386 - Flags: review+
Attachment #640386 - Attachment is obsolete: true
Comment on attachment 653789 [details] [diff] [review]
Unbitrotted patch

r=me for the configure.in changes.
Attachment #653789 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.9
You need to log in before you can comment on or make changes to this bug.