Last Comment Bug 706186 - xulrunner-stub.exe depends on mozutils.dll
: xulrunner-stub.exe depends on mozutils.dll
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit Graveyard
Classification: Graveyard
Component: XULRunner (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Dave Townsend [:mossop]
:
Mentors:
: 723085 (view as bug list)
Depends on: 741456
Blocks: 722810
  Show dependency treegraph
 
Reported: 2011-11-29 11:14 PST by Dave Townsend [:mossop]
Modified: 2016-02-12 08:12 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch rev 1 (1.04 KB, patch)
2012-02-29 13:53 PST, Dave Townsend [:mossop]
mh+mozilla: review-
Details | Diff | Review
patch rev 2 (716 bytes, patch)
2012-02-29 15:38 PST, Dave Townsend [:mossop]
mh+mozilla: review-
Details | Diff | Review
patch rev 3 (736 bytes, patch)
2012-03-01 14:59 PST, Dave Townsend [:mossop]
no flags Details | Diff | Review
patch rev 4 (821 bytes, patch)
2012-03-01 16:25 PST, Dave Townsend [:mossop]
mh+mozilla: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Dave Townsend [:mossop] 2011-11-29 11:14:35 PST
At some point (since Gecko 8 at least) xulrunner-stub.exe started depending on mozcrt19.dll. This means you can't just copy the stub to your app dir to run the app. Probably it works when copying the dll too though I didn't yet test that. Ideally we would just remove this dependency though.
Comment 1 Dave Townsend [:mossop] 2011-12-30 11:13:15 PST
As of XULRunner 9 it now depends on mozutils.dll
Comment 2 Mike Hommey [:glandium] 2012-02-01 06:41:17 PST
*** Bug 723085 has been marked as a duplicate of this bug. ***
Comment 3 Dave Townsend [:mossop] 2012-02-29 13:53:11 PST
Created attachment 601743 [details] [diff] [review]
patch rev 1

This forces static libraries and doesn't link against mozglue, mozutils etc for the stub.
Comment 4 Mike Hommey [:glandium] 2012-02-29 14:05:53 PST
Comment on attachment 601743 [details] [diff] [review]
patch rev 1

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

::: xulrunner/stub/Makefile.in
@@ +43,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  MODULE    = xulrunner
> +USE_STATIC_LIBS = 1

I'm not convinced you need this.

@@ +92,5 @@
>  endif
>  
>  include $(topsrcdir)/config/config.mk
>  
> +MOZ_UTILS_PROGRAM_LDFLAGS =

This effectively removes jemalloc from the stub on linux. You probably want MOZ_UTILS_LDFLAGS =
Comment 5 Dave Townsend [:mossop] 2012-02-29 15:38:22 PST
Created attachment 601802 [details] [diff] [review]
patch rev 2

Should have checked that the patch I wrote a few months ago was still the right fix, it almost was...

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 601743 [details] [diff] [review]
> patch rev 1
> 
> Review of attachment 601743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xulrunner/stub/Makefile.in
> @@ +43,5 @@
> >  
> >  include $(DEPTH)/config/autoconf.mk
> >  
> >  MODULE    = xulrunner
> > +USE_STATIC_LIBS = 1
> 
> I'm not convinced you need this.

Without this the stub includes a dependency on mozglue (http://mxr.mozilla.org/mozilla-central/source/config/config.mk#253) and on the msvcr dll (http://mxr.mozilla.org/mozilla-central/source/config/config.mk#507), neither of which are wanted for the stub.

> @@ +92,5 @@
> >  endif
> >  
> >  include $(topsrcdir)/config/config.mk
> >  
> > +MOZ_UTILS_PROGRAM_LDFLAGS =
> 
> This effectively removes jemalloc from the stub on linux. You probably want
> MOZ_UTILS_LDFLAGS =

Actually turns out this isn't necessary anymore, or I was mistaken about it being necessary in the first place.
Comment 6 Dave Townsend [:mossop] 2012-02-29 15:39:33 PST
Note I've tested this new patch on Windows and Linux, not on OSX yet.
Comment 7 Mike Hommey [:glandium] 2012-03-01 00:00:14 PST
Comment on attachment 601802 [details] [diff] [review]
patch rev 2

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

::: xulrunner/stub/Makefile.in
@@ +43,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  MODULE    = xulrunner
> +USE_STATIC_LIBS = 1

In practice, what this does is "MOZ_GLUE_LDFLAGS=" on windows. Not on mac, where you'd want the same thing. So I think the right fix would be MOZ_GLUE_LDFLAGS= instead.
Comment 8 Dave Townsend [:mossop] 2012-03-01 14:59:15 PST
Created attachment 602134 [details] [diff] [review]
patch rev 3

Ok, last try hopefully. Verified that this removes all non-system dependencies on linux, mac and windows.

USE_STATIC_LIBS to drop the msvc dependencies on windows.
MOZ_GLUE_LDFLAGS to drop the mozglue dependencies on windows and mac.

Linux seems ok by itself for some reason.
Comment 9 Mike Hommey [:glandium] 2012-03-01 16:01:22 PST
Comment on attachment 602134 [details] [diff] [review]
patch rev 3

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

::: xulrunner/stub/Makefile.in
@@ +44,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  MODULE    = xulrunner
> +USE_STATIC_LIBS = 1
> +MOZ_GLUE_LDFLAGS =

Please put a comment as to why these are there.
What is the file size before and after, on windows?
Comment 10 Dave Townsend [:mossop] 2012-03-01 16:25:00 PST
Created attachment 602190 [details] [diff] [review]
patch rev 4

File size before is 17920, after is 98816.
Comment 11 Mike Hommey [:glandium] 2012-03-01 16:31:36 PST
Comment on attachment 602190 [details] [diff] [review]
patch rev 4

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

> File size before is 17920, after is 98816.

Not too bad, I'd say. I wonder how small it would get if the stub was rewritten in C (which it mostly is already). But I don't think it matters for this bug, except if bsmedberg objects to the jump in size.
Comment 12 Dave Townsend [:mossop] 2012-03-01 17:54:56 PST
Certainly cheaper than also having to copy the ~160kb of msvc* dlls alongside the stub in your app.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f622f39e4296
Comment 13 Marco Bonardo [::mak] 2012-03-02 06:31:30 PST
https://hg.mozilla.org/mozilla-central/rev/f622f39e4296
Comment 14 Dave Townsend [:mossop] 2012-03-05 11:08:24 PST
Comment on attachment 602190 [details] [diff] [review]
patch rev 4

[Approval Request Comment]
Regression caused by (bug #): Unknown
User impact if declined: XULRunner apps have to use an older or nightly SDK to get a usable stub exe
Testing completed (on m-c, etc.): On nightly
Risk to taking this patch (and alternatives if risky): No risk to Firefox, isn't part of the regular build.
String changes made by this patch: None
Comment 15 Alex Keybl [:akeybl] 2012-03-05 14:43:07 PST
Comment on attachment 602190 [details] [diff] [review]
patch rev 4

[Triage Comment]
NPOTB for Firefox, so approving for Aurora 12 and Beta 11.

Note You need to log in before you can comment on or make changes to this bug.