Closed Bug 1286286 Opened 8 years ago Closed 8 years ago

Update Suite installer for VS2015. Port bug 1204202

Categories

(SeaMonkey :: Installer, defect)

Product:
SeaMonkey
Component:
Installer
Version:
SeaMonkey 2.47 Branch
Platform:
Unspecified
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.44 unaffected, seamonkey2.45 affected, seamonkey2.46 affected, seamonkey2.47 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.47
Tracking Flags:
Tracking Status
seamonkey2.44 --- unaffected
seamonkey2.45 --- affected
seamonkey2.46 --- affected
seamonkey2.47 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(2 files, 1 obsolete file)

1286286-VS2015-Runtime-V2.patch
1.91 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-aurora+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1286286-Runtime-ucrt-c-b.patch
1.61 KB, patch
ewong
: review+
ewong
: approval-comm-beta+
Details | Diff | Splinter Review 
The VS2015 runtime DLLs are currently not included in the Seamonkey Windows packages for installation and update.
Attached patch 1286286-VS2015-Runtime.patch (obsolete) — — Splinter Review 
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.
Attachment #8770241 - Flags: review?(philip.chee)
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?
Attachment #8770241 - Flags: feedback+
>> 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.
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?
Flags: needinfo?(mh+mozilla)
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).
Attachment #8770241 - Attachment is obsolete: true
Attachment #8770241 - Flags: review?(philip.chee)
Attachment #8771683 - Flags: review?(philip.chee)
> I think checking for XP_WIN32 should be phased out.

Probably.
Flags: needinfo?(mh+mozilla)
Attachment #8771683 - Flags: feedback?(iann_bugzilla)
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
Attachment #8771683 - Flags: feedback?(iann_bugzilla) → feedback-
>>
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.
Flags: needinfo?(iann_bugzilla)
Or we error out if one of the two is not provided and it's a VS2015+ build.
Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

Interesting, so is that a bug for Firefox?
Flags: needinfo?(iann_bugzilla)
Attachment #8771683 - Flags: feedback- → review+
>> 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
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.
Attachment #8771683 - Flags: review?(philip.chee)
Whiteboard: [leave open for branches]
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
Attachment #8771683 - Flags: approval-comm-beta?
Attachment #8771683 - Flags: approval-comm-aurora?
Comment on attachment 8771683 [details] [diff] [review]
1286286-VS2015-Runtime-V2.patch

a=me
Attachment #8771683 - Flags: approval-comm-beta?
Attachment #8771683 - Flags: approval-comm-beta+
Attachment #8771683 - Flags: approval-comm-aurora?
Attachment #8771683 - Flags: approval-comm-aurora+
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
Frank-Rainer: Thanks for the patch!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open for branches]
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.
Flags: needinfo?(ewong)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
Flags: needinfo?(ewong)
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.
Attachment #8774708 - Flags: review?(ewong)
Comment on attachment 8774708 [details] [diff] [review]
1286286-Runtime-ucrt-c-b.patch

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

lgtm
Attachment #8774708 - Flags: review?(ewong) → review+
Status: REOPENED → ASSIGNED
Comment on attachment 8774708 [details] [diff] [review]
1286286-Runtime-ucrt-c-b.patch

[Triage Comment]
we need this patch.
Attachment #8774708 - Flags: approval-comm-beta+
https://hg.mozilla.org/releases/comm-beta/rev/7ffc144308b6

and bug resolved again hopefully for good this time.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Depends on: 1291426
TM for easing queries.
Target Milestone: --- → seamonkey2.47
Depends on: 1364497
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: