678161 - msvc*.dll missing when building with new style jemalloc

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

When building with VC10, the new style jemalloc is used, which requires the CRT to be copied over.
http://mxr.mozilla.org/mozilla-central/source/build/win32/Makefile.in#65 prevents that from happening

Comment 1

[Mike Hommey [:glandium]]

Attachment: Also install msvc*.dll when using new style jemalloc

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 552340 [details] [diff] [review]
Also install msvc*.dll when using new style jemalloc

Need some more

Comment 3

[Mike Hommey [:glandium]]

Attachment: Also install msvc*.dll when using new style jemalloc

This takes care of installing the files in the package, and makes the mobile manifest look like the browser one.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 552377 [details] [diff] [review]
Also install msvc*.dll when using new style jemalloc

the CRT is also missing from the js shell zip.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Also install msvc*.dll when using new style jemalloc

Not sure if it's fine to let zip warn about the missing files when not building with WIN32_REDIST_DIR set.

Comment 6

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

Comment on attachment 554318 [details] [diff] [review]
Also install msvc*.dll when using new style jemalloc

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

A couple comments

::: browser/installer/removed-files.in
@@ +1277,5 @@
>      msvcm80.dll
>      msvcp80.dll
>      msvcr80.dll
>    #else
>      mozcrt19.dll

Shouldn't mozcpp19.dll be here too?

::: mobile/installer/package-manifest.in
@@ +63,5 @@
> +@BINPATH@/mozcpp19.dll
> +#else
> +#ifdef MOZ_MEMORY
> +@BINPATH@/jemalloc.dll
> +#endif

Does mobile not need adjustment in removed-files.in?

::: toolkit/mozapps/installer/packager.mk
@@ +106,5 @@
> +endif
> +ifeq ($(_MSC_VER),1500)
> +JSSHELL_BINS += $(DIST)/bin/msvcr100.dll
> +endif
> +endif

Really sucks that we have to duplicate all this gunk here :-(

Comment 7

[Mike Hommey [:glandium]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> Comment on attachment 554318 [details] [diff] [review]
> Also install msvc*.dll when using new style jemalloc
> 
> Review of attachment 554318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple comments
> 
> ::: browser/installer/removed-files.in
> @@ +1277,5 @@
> >      msvcm80.dll
> >      msvcp80.dll
> >      msvcr80.dll
> >    #else
> >      mozcrt19.dll
> 
> Shouldn't mozcpp19.dll be here too?

mozcpp19.dll isn't there anymore. New style jemalloc builds a mozcrt.lib that helps linking jemalloc with the right hacked static portion of the crt, but that's all. mozcpp19.dll is replaced by jemalloc.dll+the CRT dlls

> Does mobile not need adjustment in removed-files.in?

I was wondering... its removed-files.in is mostly empty.

> ::: toolkit/mozapps/installer/packager.mk
> @@ +106,5 @@
> > +endif
> > +ifeq ($(_MSC_VER),1500)
> > +JSSHELL_BINS += $(DIST)/bin/msvcr100.dll
> > +endif
> > +endif
> 
> Really sucks that we have to duplicate all this gunk here :-(

An alternative could be that we set a variable with the list of crt files somewhere in config/config.mk and use that variable to fill the rest.

Comment 8

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

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> > Comment on attachment 554318 [details] [diff] [review]
> > Also install msvc*.dll when using new style jemalloc
> > 
> > Review of attachment 554318 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > A couple comments
> > 
> > ::: browser/installer/removed-files.in
> > @@ +1277,5 @@
> > >      msvcm80.dll
> > >      msvcp80.dll
> > >      msvcr80.dll
> > >    #else
> > >      mozcrt19.dll
> > 
> > Shouldn't mozcpp19.dll be here too?
> 
> mozcpp19.dll isn't there anymore. New style jemalloc builds a mozcrt.lib
> that helps linking jemalloc with the right hacked static portion of the crt,
> but that's all. mozcpp19.dll is replaced by jemalloc.dll+the CRT dlls

Right, I know that (I wrote that code :-) ).  I'm asking why mozcpp19.dll isn't in removed-files.in.

> > Does mobile not need adjustment in removed-files.in?
> 
> I was wondering... its removed-files.in is mostly empty.

I think that's mostly because mobile hasn't been around that long (there's stuff in Firefox's removed-file.in from the ancient days).
 
> > ::: toolkit/mozapps/installer/packager.mk
> > @@ +106,5 @@
> > > +endif
> > > +ifeq ($(_MSC_VER),1500)
> > > +JSSHELL_BINS += $(DIST)/bin/msvcr100.dll
> > > +endif
> > > +endif
> > 
> > Really sucks that we have to duplicate all this gunk here :-(
> 
> An alternative could be that we set a variable with the list of crt files
> somewhere in config/config.mk and use that variable to fill the rest.

Perhaps.  I wouldn't block this on figuring out what to do here though.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Also install msvc*.dll when using new style jemalloc [Checked in: Comment 12]

As landing

Comment 10

[Mike Hommey [:glandium]]

http://hg.mozilla.org/projects/build-system/rev/9b6e97e91525

Comment 11

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

on toolkit/mozapps/installer/packager.mk,

+ifeq ($(_MSC_VER),1500)
+JSSHELL_BINS += $(DIST)/bin/msvcr100.dll
+endif

but,

+ifeq ($(_MSC_VER),1600)
+JSSHELL_BINS += $(DIST)/bin/msvcr100.dll
+endif

?

Comment 12

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/9b6e97e91525

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to Takeshi Ichimaru from comment #11)
> on toolkit/mozapps/installer/packager.mk,

Fixed in bug 681893.