332088 - Updater should use static libs on Windows

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P2)

Description

[Volkmar Kostka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060328 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060328 Firefox/1.6a1

The partial update tries to replace/update the DLL msvcr80.dll. The problem is that the upadter istself uses that DLL so it can not be updated. This prevents any updates - at least for W2K.

Reproducible: Always

Steps to Reproduce:
1. Download an update (partial for me)
2. Try to install it.

Actual Results:  
Update fails because mscvr80.dll can not be pathced/replaced.

Expected Results:  
Update works.

Trunk builds (cairo + places)

Comment 1

[Benjamin Smedberg]

Updater should be linked USE_STATIC_LIBS = 1

Comment 2

[Darin Fisher]

-> me

Comment 3

[Régis Caspar]

(In reply to comment #1)
> Updater should be linked USE_STATIC_LIBS = 1
Tried that and it's OK only if you have a static version of libmar and libbz2, so there is a need to double compile libmar and libbz2 if they are used by something else.

Comment 4

[Benjamin Smedberg]

Are they?

Comment 5

[Régis Caspar]

(In reply to comment #4)
> Are they?
(according to my buildlog)
libbz2 -> used by updater.exe
libmar -> used by mar.exe and by updater.exe

Comment 6

[Darin Fisher]

I think bsmedberg was asking if those libraries were used by something else.  The answer today is no.  Necko will eventually start linking to libbz2.

Comment 7

[Régis Caspar]

Attachment: add USE_STATIC_LIBS = 1 where needed

patch adding "USE_STATIC_LIBS=1" to updater, libmar and libbz2 Makefile.in if target OS is WINNT.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Manifest for windows trunk 2006-04-10 ---> 2006-04-11 partial update

A more interesting question is why the update system thinks it needs to patch msvcr80.dll and msvcp80.dll in the first place. A partial update generation bug?

Also, the suggested patch presumably increases the size of updater.exe considerably (to the detriment of download size). A better solution would be to copy the dll's needed into <install_dir>\updates\0 along with the updater.exe. On my machine, where the runtime VC8 dll's are not installed at a system level, I need to manually copy in msvcr80.dll and Microsoft.VC80.CRT.manifest for an update to apply cleanly.

Comment 9

[Benjamin Smedberg]

The static CRT is not a big codesize hit. We update the CRT as part of the update because we are shipping the local CRT and it might change in the future (it probably will, in fact).

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #9)
> ... We update the CRT as part of the
> update because we are shipping the local CRT and it might change in the future
> (it probably will, in fact).

You mean that a later version of the runtimes will probably be shipped in the future ? So, unless I was unlucky, why did msvcr80.dll and msvcp80.dll need to be updated in the attached manifest? Are the runtime dll's somehow generated or modified in the build process?

Also, my experience was a little different to the reporter - the updater didn't even run without msvcr80.dll and Microsoft.VC80.CRT.manifest

Comment 11

[Régis Caspar]

(In reply to comment #9)
> The static CRT is not a big codesize hit. We update the CRT as part of the
> update because we are shipping the local CRT and it might change in the future
> (it probably will, in fact).
They already have??
DLLs shipped with firefox: v8.0.50727.42
DLLs in WinSxS here: v8.0.50727.49 (from winupdate?)

Comment 12

[Régis Caspar]

Attachment: add USE_STATIC_LIBS = 1 where needed (2)

There was missing changes for mar.exe

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

The second patch compiles ok with MSVC 6 (the first did not); updater.exe goes from 128KB to 188KB.

bsmedberg, are you best placed to review ? Or darin ? Now that software update for nightlies is back in action on the trunk I suspect that the updater failing is going to start hurting testers.

Comment 14

[Régis Caspar]

(In reply to comment #13)
> The second patch compiles ok with MSVC 6 (the first did not); updater.exe goes
> from 128KB to 188KB.
Thanks for the testing, I tested myself with VC 2005 Express and updater.exe was 209Ko (with common control manifest through, see #330231). 

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Is this really a windows-specific issue? I would think we want the updater to be linked statically on all platforms... Otherwise we may run into a few 'libfoo not found' errors in the future on mac/linux as well.

Updating summary.

Comment 16

[Benjamin Smedberg]

bent, linked statically against what? linux/mac have standardized glibc dependencies that aren't going to change.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Really? So we gain nothing (in terms of stability) from defining USE_STATIC_LIBS on those platforms?

Comment 18

[Benjamin Smedberg]

USE_STATIC_LIBS doesn't do anything except on windows.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

/me slaps forehead.

In that case let's get this fixed up... for Windows :)

Comment 20

[Régis Caspar]

Now that this patch has review (thanks Benjamin), do I need to do something else (request super-review, request check-in)? 

Comment 21

[Darin Fisher]

I got the following error when trying to apply the patch:

mozilla> patch -p0 --dry-run < /c/temp/updater-link-1.diff
(Stripping trailing CRs from patch.)
patching file toolkit/mozapps/update/src/updater/Makefile.in
(Stripping trailing CRs from patch.)
patching file modules/libmar/src/Makefile.in
patch: **** malformed patch at line 32: Index: modules/libbz2/src/Makefile.in

Comment 22

[James Ross]

It's trivial to fix - change:
  @@ -47,6 +47,9 @@
to:
  @@ -47,5 +47,8 @@

Comment 23

[Régis Caspar]

Attachment: add USE_STATIC_LIBS = 1 where needed (3)

(In reply to comment #21)
> I got the following error when trying to apply the patch:
> 
> mozilla> patch -p0 --dry-run < /c/temp/updater-link-1.diff
> (Stripping trailing CRs from patch.)
> patching file toolkit/mozapps/update/src/updater/Makefile.in
> (Stripping trailing CRs from patch.)
> patching file modules/libmar/src/Makefile.in
> patch: **** malformed patch at line 32: Index: modules/libbz2/src/Makefile.in

Oops, seems like I removed a blank line by mistake when doing patches concatenation.. Thanks Darin. Updated version:
$ patch -p0 -R --dry-run < ../patches/bug332088-2.patch 
patching file `toolkit/mozapps/update/src/updater/Makefile.in'
patching file `modules/libmar/src/Makefile.in'
patching file `modules/libbz2/src/Makefile.in'
patching file `modules/libmar/tool/Makefile.in

Comment 24

[Darin Fisher]

fixed-on-trunk

Is this needed for FF2 (i.e., the MOZILLA_1_8_BRANCH)?

Comment 25

[Darin Fisher]

marking FIXED

Comment 26

[Régis Caspar]

(In reply to comment #24)
> fixed-on-trunk
> Is this needed for FF2 (i.e., the MOZILLA_1_8_BRANCH)?
Thanks Darin, 
I don't think this is needed on Branch (this was due to MS VC2005 runtime) because Branch doesn't use this runtime (i.e. msvc*80.dll).
But I'm not 100% sure...

Comment 27

[Nick Thomas [:nthomas] (UTC+12)]

Verified fixed updating from

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060417 Firefox/3.0a1 ID:2006041705 [cairo]

to 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060419 Firefox/3.0a1 ID:2006041905 [cairo]

[Tinderbox configuration glitch on gaius is the reason for the two day jump]