1286286 - Update Suite installer for VS2015. Port bug 1204202

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Frank-Rainer Grahl (:frg)]

The VS2015 runtime DLLs are currently not included in the Seamonkey Windows packages for installation and update.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1286286-VS2015-Runtime.patch

Patch for copying the runtime dlls. I changed XP_WIN32 to XP_WIN. Both are set when doing x86 and x64 builds but I find the 32 confusing with x64 builds.

I encountered a strange error during mozmake installer. 
mozilla\python\mozbuild\mozpack\files.py whould give me a Windows Error access denied 5 on line 277 os.remove(dest). Probably has something to do with my path settings and will not affect offcial builds but I would be glad if someone else could try the patch. I just removed the line to get it going resulting in temp files not removed. 

The resulting installer worked fine when I removed the runtime from the system so the copy steps are ok.

Comment 2

[Edmund Wong (:ewong)]

Comment on attachment 8770241 [details] [diff] [review]
1286286-VS2015-Runtime.patch

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

generally, it looks good.

I'm not entirely comfortable about the XP_WIN32 -> XP_WIN even though they refer to the same thing.  Mainly due
to XP_WIN32 being sprinkled all over the codebase.

I'm wondering if it's better if we just move everything to XP_WIN?

Comment 3

[Frank-Rainer Grahl (:frg)]

>> I'm wondering if it's better if we just move everything to XP_WIN?

I could do a separate patch for this if desired. I only found a few references in the suite directory. The majority is XP_WIN and I think this should be used. If you check with dxr out of 2100 occurences in c-c m-c only 103 are XP_WIN32.

Comment 4

[Frank-Rainer Grahl (:frg)]

Mike,

you are more familar with Windows development. While switching suite installer files to VS2015 I noticed that there are two defs for Windows. I think checking for XP_WIN32 should be phased out. Any insights into this?

Comment 5

[Frank-Rainer Grahl (:frg)]

Attachment: 1286286-VS2015-Runtime-V2.patch

I shuffled the ifdefs around. I think it makes no sense to have only the vc runtime without the ucrtlibs in the package and vice versa. 

I also found a solution for the access denied error. The vc runtime files I tried to include where read only. The python zipper did copy them as temporary files and left the attributes intact. os.remove did then choke. Just removing the attribute cleared the error condition. 

Maybe a bug against files.py should be raised? A possible solution would be to set the filemode before remove with os.chmod(dest, 0777).

Comment 6

[Mike Hommey [:glandium]]

> I think checking for XP_WIN32 should be phased out.

Probably.

Comment 7

[Ian Neal]

Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

>+++ b/suite/installer/Makefile.in
> # Set MSVC dlls version to package, if any.
>+# With VS2015+ it does not make sense to define the ucrt libs without
>+# the base c++ libs and vice versa. 
>+ifdef MOZ_NO_DEBUG_RTL
> ifdef WIN32_REDIST_DIR
>+ifdef WIN_UCRT_REDIST_DIR
> DEFINES += -DMOZ_PACKAGE_MSVC_DLLS=1
> DEFINES += -DMSVC_C_RUNTIME_DLL=$(MSVC_C_RUNTIME_DLL)
> DEFINES += -DMSVC_CXX_RUNTIME_DLL=$(MSVC_CXX_RUNTIME_DLL)
>+DEFINES += -DMOZ_PACKAGE_WIN_UCRT_DLLS=1
>+endif
> endif
> endif
Compared to Firefox's version, the WIN_UCRT additions are done after the WIN32_REDIST_DIR endif. The minimum would be to move the ifdef to the end of the MSVC defines.

f- for the moment

Comment 8

[Frank-Rainer Grahl (:frg)]

>>
Compared to Firefox's version, the WIN_UCRT additions are done after the WIN32_REDIST_DIR endif. The minimum would be to move the ifdef to the end of the MSVC defines.
<<

Yes that is how I did version 1 of the patch. But it's wrong for VS2015. You end up with a non working Seamonkey if one of the two components is not installed/provided. This is the case with the current broken builds which have no ucrt but did pull in the msvc runtime.  They need to be both there. VS2013 didn't have the ucrt so it didn't apply there and at the time they put the patch in m-c VS2013 was also still used so the patch needed a distinction for the ucrt and the libs. For VS2013 builds ucrt was simply left out. Now with VS2015 only we no longer need this.

Comment 9

[Frank-Rainer Grahl (:frg)]

Or we error out if one of the two is not provided and it's a VS2015+ build.

Comment 10

[Ian Neal]

Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

Interesting, so is that a bug for Firefox?

Comment 11

[Frank-Rainer Grahl (:frg)]

>> Interesting, so is that a bug for Firefox?

No. They needed it at the time to still do VS2013 builds with the same package-manifest.in. ewong wants to do 2.45 with VS2015 and 2.44 is still VS2013 only for now so we do not need this.

FRG

Comment 12

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/c7174cd79146

c-c is currently broken. I will ask for c-a and c-b branch approval after a successful build with the patch.

Comment 14

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: Official VS2015 builds will not work without manual modifications of the file dependentlibs.list and installation of the VS2015 runtime.
Testing completed (on m-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: none

Comment 15

[Ian Neal]

Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

a=me

Comment 16

[Edmund Wong (:ewong)]

https://hg.mozilla.org/releases/comm-aurora/rev/57865e89b3ddc77b14f2dd932ae17c72da13b795
Bug 1286286 - Copy VS2015 runtime files for Windows. r+a=IanN

Comment 17

[Edmund Wong (:ewong)]

Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

Pushed to comm-aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/57865e89b3dd

Comment 18

[Edmund Wong (:ewong)]

https://hg.mozilla.org/releases/comm-beta/rev/d37be8bdb8e3ee6d1a124374bbd93db442cf69fc
Bug 1286286 - Copy VS2015 runtime files for Windows. r+a=IanN

Comment 19

[Edmund Wong (:ewong)]

Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

Pushed to comm-beta:
https://hg.mozilla.org/releases/comm-beta/rev/d37be8bdb8e3

Comment 20

[Edmund Wong (:ewong)]

Frank-Rainer: Thanks for the patch!

Comment 21

[Frank-Rainer Grahl (:frg)]

ewong. The latest Windows beta from https://archive.mozilla.org/pub/seamonkey/nightly/latest-comm-beta/ does not contain the VS2015 runtime files. Could you check the build environment.

Comment 22

[Frank-Rainer Grahl (:frg)]

default mozconfig in top builddir still points to vs2013 in beta. This or our build configs needs to change:

. $topsrcdir/build/win32/mozconfig.vs2013-win64

As an alternative we could also use version 1 of the patch which takes builds with VS2013 into account. Reopened for now to track this.

Comment 23

[Edmund Wong (:ewong)]

(In reply to Frank-Rainer Grahl from comment #22)
> default mozconfig in top builddir still points to vs2013 in beta. This or
> our build configs needs to change:
> 
> . $topsrcdir/build/win32/mozconfig.vs2013-win64
> 
> As an alternative we could also use version 1 of the patch which takes
> builds with VS2013 into account. Reopened for now to track this.

Why isn't TB busted?  Shouldn't they too use vs2015-win64?

Comment 24

[Frank-Rainer Grahl (:frg)]

Attachment: 1286286-Runtime-ucrt-c-b.patch

It looks like they still do VS2013 only builds. If we want only VS2015 we can change our Windows mozconfigs to explicitly include mozconfig.vs2015-win64 which is what the common file on c-a and up just now does. 

Firefox 48 will be VS2013 based. Future releases will use VS2015 so no point changing c-a or c-c.

Comment 25

[Edmund Wong (:ewong)]

Comment on attachment 8774708 [details] [diff] [review]
1286286-Runtime-ucrt-c-b.patch

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

lgtm

Comment 26

[Edmund Wong (:ewong)]

Comment on attachment 8774708 [details] [diff] [review]
1286286-Runtime-ucrt-c-b.patch

[Triage Comment]
we need this patch.

Comment 27

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/7ffc144308b6

and bug resolved again hopefully for good this time.

Comment 28

[Rainer Bielefeld]

TM for easing queries.