249782 - (vs2005) Make Mozilla compile with Microsoft Visual Studio 2005

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Will Levine]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.9
Build Identifier: 

So I downloaded the 'Express Edition' of Microsoft's new beta compiler (from
http://lab.msdn.microsoft.com/express/visualc/default.aspx ) and used it with
the MS platform sdk (available
http://www.microsoft.com/msdownload/platformsdk/sdkupdate ) and tried to build
mozilla. First I had to compile glib, pthreads, and libidl to prevent crash in
xpidl. (I can't provide these binaries since the EULA prohibits it). I also had
to make a few small changes to various files to get a successful build. (I will
attach a patch with these soon).

Reproducible: Always
Steps to Reproduce:
1. Try to build Mozilla

Actual Results:  
Mozilla won't build unless I build external libraries and modify a few files.

Expected Results:  
Mozilla should build without modification.

Comment 1

[Will Levine]

Attachment: Make Mozilla build

The change to nsLocalFileWin.cpp prevents an error C2440: 'initializing' :
cannot convert from 'const unsigned char *' to 'unsigned char *' Conversion
loses qualifiers.
The change to directory/c-sdk/build.mk prevents link error about invalid option
syntax.
I think I'll run a Firefox build now to see if all of its special code works.

Comment 2

[Stephen Walker]

Will, could you try using braden's static glib/libIDL libs on bug 242870?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 152300 [details] [diff] [review]
Make Mozilla build

>             AC_MSG_ERROR([This version of the MSVC compiler, $CC_VERSION , is unsupported.])

I'm somewhat skeptical whether giving an error on an unknown compiler is a good
idea in the first place.  It probably is in practice, though.

>Index: xpcom/io/nsLocalFileWin.cpp
>     // search for first slash after the drive (or volume) name
>-    unsigned char* slash = _mbschr(path, '\\');
>+    unsigned char* slash = NS_CONST_CAST(unsigned char*, _mbschr(path, '\\'));

The initial assignment to |path| should use BeginWriting() instead of get(),
and |path| should also be non-const.  You may need some NS_CONST_CAST
gymnastics because of the C-ish (rather than C++-ish with 2 overloaded forms
varying in const-ness) signature of _mbsstr, but you should give the variables
the right constness.

>@@ -759,7 +759,7 @@
>-        unsigned char * doubleDot = _mbsstr(nodePath, (unsigned char *)"\\..");
>+        unsigned char * doubleDot = NS_CONST_CAST(unsigned char *, _mbsstr(nodePath, (unsigned char *)"\\.."));

doubleDot should be |const unsigned char*|.

>Index: directory/c-sdk/build.mk

A brief explanation of these changes would be useful.  How do you know they
won't break older versions?

Comment 4

[Stephen Walker]

_MSC_VER 1400 checks should also be added to Makefiles that specify deprecated
and/or removed flags. 
http://lab.msdn.microsoft.com/library/default.asp?url=/library/en-us/dv_vccomp/html/aa59fce3-50b8-4f66-9aeb-ce09a7a84cce.asp
lists the current deprecated flgas for VC8.

cl : Command line warning D9035 : option 'GX' has been deprecated and will be
removed in a future release
cl : Command line warning D9036 : use 'EHsc' instead of 'GX'

cl : Command line warning D9002 : ignoring unknown option '/Op'
/fp:precise has replaced /Op as documented on
http://lab.msdn.microsoft.com/library/default.asp?url=/library/en-us/dv_vccomp/html/10469d6b-e68b-4268-8075-d073f4f5d57e.asp

cl : Command line warning D9002 : ignoring unknown option '-GM'
Does anyone have a VC6 reference as to what -GM does?

Comment 5

[Darin Fisher]

Comment on attachment 152300 [details] [diff] [review]
Make Mozilla build

/me waits for revised patch

Comment 6

[Will Levine]

I'm experiencing a couple of crashes on startup with my builds now. I'd like to
try to get these fixed before I start testing anything else

(In reply to comment #3)
> >Index: directory/c-sdk/build.mk
> 
> A brief explanation of these changes would be useful.  How do you know they
> won't break older versions?
> 
Well I really don't know why it use /DEBUGTYPE:BOTH in the first place. An lxr
search for debugtype: shows that throughout most of the tree, :CV is used, but
there are a few places where :BOTH is used, although this is the only place I
ran into it in building.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_core_.2f.debugtype.asp
Some documentation about the /DEBUGTYPE switch in VC6. It looks like MS just
dropped support for COFF debugging info in VC8.

Comment 7

[Stephen Walker]

Attachment: Build system changes

The USE_STATIC_LIBS changes are needed since -ML support has been removed.

USE_NON_MT_LIBS does not do anything on OS/2.

Removing -YX from xpconnect/src might be helpful w.r.t. better build times, but
I do not have any data to verify Microsoft's claim listed on
http://msdn2.microsoft.com/library/h4bcz65t.aspx.

Comment 8

[Stephen Walker]

Attachment: NSPR build change

wtc, does NSPR still support building with VC5?

Comment 9

[Stephen Walker]

Attachment: Build system changes

Fix the two cases where activex Makefiles use /GX.

While building activex, I also ran into the same error as noted in comment 1:
c:/Mozilla\mozilla\embedding\browser\activex\src\common\ControlSite.cpp(279) :
error C2440: 'initializing' : cannot convert from 'const wchar_t *' to
'wchar_t *'
	Conversion loses qualifiers

|wchar_t *szHash| -> |const wchar_t *szHash| fixed the build error.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 159256 [details] [diff] [review]
Build system changes

>--- directory/c-sdk/build.mk	26 Aug 2004 23:03:00 -0000	5.0.2.12
>+++ directory/c-sdk/build.mk	9 Sep 2004 23:00:37 -0000
>@@ -379,7 +379,7 @@
> 
> SUBSYSTEM=CONSOLE
> ifndef MOZ_DEBUG_SYMBOLS
>-DEBUG_FLAGS=/PDB:NONE /DEBUGTYPE:BOTH
>+DEBUG_FLAGS=/PDB:NONE /DEBUGTYPE:CV

/DEBUGTYPE:CV is the default, so I think we can just remove the flag.

>--- widget/src/windows/Makefile.in	18 Apr 2004 22:00:30 -0000	3.19
>+++ widget/src/windows/Makefile.in	9 Sep 2004 23:01:24 -0000
>@@ -108,7 +108,11 @@
> ifdef GNU_CC
> CXXFLAGS        += -fexceptions
> else
>+ifeq (,$(filter-out 1200 1300 1310,$(_MSC_VER)))
> CXXFLAGS        += -GX
>+else
>+CXXFLAGS        += -EHsc
>+endif
> endif

Ok, after wading through a bunch of these, I think it would be better to add a
new variable, ENABLE_CXX_EXCEPTIONS, that's used by rules.mk.

>--- xpinstall/cleanup/Makefile.in	17 Apr 2004 14:37:25 -0000	1.16
>+++ xpinstall/cleanup/Makefile.in	9 Sep 2004 23:01:28 -0000
>@@ -63,8 +63,12 @@
> ifeq ($(OS_ARCH),WINNT)
> CPPSRCS += InstallCleanupWin.cpp
> MOZ_WINCONSOLE = 0
>+ifeq (,$(filter-out 1200 1300 1310,$(_MSC_VER)))
> USE_NON_MT_LIBS = 1
> else
>+USE_STATIC_LIBS = 1
>+endif
>+else

Same thing.  There should just be a single variable that signals the intent,
and that should be expanded approrpiately in config.mk or rules.mk.

Comment 11

[Stephen Walker]

Attachment: NSPR/NSS Build Changes

Comment 12

[Stephen Walker]

Attachment: LDAP Build Changes

Comment 13

[Stephen Walker]

Attachment: Build System Changes

Comment 14

[Wan-Teh Chang]

Comment on attachment 159680 [details] [diff] [review]
NSPR/NSS Build Changes

r=wtc.

Stephen, I consider VC6 the minimum VC version
needed to build NSPR.  This is why nsprpub/configure.in
already uses a MSC_VER of 1200 where I really mean
"VC6 or older".

Recently a colleague of mine told me he's still
using VC5 at home to build NSPR and NSS.  So if
you can add VC5 support (back), I'd appreciate
it.  This would mean fixing the existing use of
"1200" in nsprpub/configure.in as well.

Comment 15

[Stephen Walker]

Attachment: NSPR/NSS build changes with restored VC5 support (checked in)

Comment 16

[Wan-Teh Chang]

Comment on attachment 160000 [details] [diff] [review]
NSPR/NSS build changes with restored VC5 support (checked in)

r=wtc.	Thanks for adding VC5 support back.

Comment 17

[Wan-Teh Chang]

Comment on attachment 160000 [details] [diff] [review]
NSPR/NSS build changes with restored VC5 support (checked in)

I checked in the NSPR changes in this patch
on the NSPR trunk (NSPR 4.6) and the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (post Mozilla
1.8 Alpha 4).

I didn't check in the NSS changes in this
patch because they apply to files that are
not being used by VC 2005.

Comment 18

[timeless]

Comment on attachment 159682 [details] [diff] [review]
Build System Changes

mozilla/xpinstall/wizard/windows/nsinstall/Makefile.in	1.6
mozilla/xpinstall/wizard/windows/ren8dot3/Makefile.in	1.6
mozilla/xpinstall/wizard/windows/setup/Makefile.in	1.11
mozilla/xpinstall/wizard/windows/uninstall/Makefile.in	1.9
mozilla/xpinstall/wizard/windows/ds32/Makefile.in	1.6
mozilla/xpinstall/wizard/windows/nsztool/Makefile.in	1.7
mozilla/xpinstall/wizard/windows/setuprsc/Makefile.in	1.8
mozilla/xpinstall/wizard/windows/palmsync/Makefile.in	1.4
mozilla/xpinstall/wizard/windows/GetShortPathName/Makefile.in	1.5
mozilla/toolkit/mozapps/installer/windows/wizard/setup/Makefile.in	1.3
mozilla/xpcom/tests/Makefile.in 	1.85
mozilla/toolkit/mozapps/installer/windows/wizard/uninstall/Makefile.in	1.2
mozilla/xpinstall/wizard/libxpnet/test/Makefile.in	1.5
mozilla/xpinstall/wizard/libxpnet/src/Makefile.in	1.6
mozilla/widget/src/windows/Makefile.in	3.20
mozilla/mailnews/import/Makefile.in	1.13
mozilla/modules/libreg/standalone/Makefile.in	1.18
mozilla/modules/plugin/base/src/Makefile.in	1.100
mozilla/js/src/xpconnect/src/Makefile.in	1.76
mozilla/modules/zlib/standalone/Makefile.in	1.19
mozilla/xpinstall/cleanup/Makefile.in	1.17
mozilla/xpinstall/wizard/os2/nsinstall/Makefile.in	1.13
mozilla/xpinstall/wizard/os2/setup/Makefile.in	1.13
mozilla/xpinstall/wizard/os2/uninstall/Makefile.in	1.9
mozilla/xpinstall/wizard/os2/setuprsc/Makefile.in	1.7
mozilla/configure.in	1.1383
mozilla/config/autoconf.mk.in	3.325
mozilla/config/config.mk	3.297
mozilla/config/rules.mk 	3.446
mozilla/embedding/browser/activex/src/common/Makefile.in	1.6
mozilla/embedding/browser/activex/src/control/Makefile.in	1.29
mozilla/embedding/browser/activex/src/plugin/Makefile.in	1.32
mozilla/embedding/browser/activex/src/control_kicker/Makefile.in	1.5
mozilla/extensions/xmlextras/tests/Makefile.in	1.13
mozilla/js/src/Makefile.in	3.93
mozilla/modules/plugin/samples/default/windows/Makefile.in	1.14
mozilla/mailnews/import/build/Makefile.in	1.8
mozilla/modules/libjar/standalone/Makefile.in	1.27
mozilla/toolkit/mozapps/installer/windows/wizard/setuprsc/Makefile.in	1.3
mozilla/xpcom/obsolete/nsFileStream.h	1.9

Comment 19

[Peter Weilbacher]

Something has gone wrong with the checkin to config/rules.mk. I checked out the
whole file again, but the new lines all have an extra 0x0d character at the
linebreaks (which appear as 0x0d0d0a in my hex editor). Loaded in vim it looks
like this:

ifdef ENABLE_CXX_EXCEPTIONS
ifdef GNU_CC^M
CXXFLAGS                += -fexceptions^M
else^M
ifeq (,$(filter-out 1200 1300 1310,$(_MSC_VER)))^M
CXXFLAGS                += -GX^M
else^M
CXXFLAGS                += -EHsc^M
endif # _MSC_VER^M
endif # GNU_CC
endif # ENABLE_CXX_EXCEPTIONS
endif # WINNT

It's not that important (at least not for my OS/2 compiles) but now I get lots
of warnings like this:

M:/mozilla/config/rules.mk:217: Extraneous text after `else' directive
M:/mozilla/config/rules.mk:220: Extraneous text after `else' directive

Or is just my CVS broken?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Newlines fixed.  (It wasn't just you.)

Comment 21

[timeless]

sorry, i suppose i should have used |file| and seen that the file was listed as
mixed. note that the patch was inconsistent, some lines were added w/o ^M and
some with :(.

fwiw i also fixed mozilla/js/src/xpconnect/src/Makefile.in which had *1* ^M.

Comment 22

[Piotr Komoda]

Configure and Configure.in files checks for Midl.exe version 6.00.0364, while in
Visual Studio 2005 Beta 2 this file has been updated to version 6.00.0366.
Without modification of those files, compilation doesn't even start. Besides,
changes in nsLocalFileWin.cpp (from attachment 152300 [details] [diff] [review], first in the list of this
bug) are still required in order to successfully finish compilation. In case
someone didn't know, they were required in Beta 1, Beta 1 Refresh and later CTPs
also.

Comment 23

[Piotr Komoda]

Attachment: Remaining things for VS2005 Beta 2

Here's what still needs to be done to successfully finish compilation under
Visual Studio 2005 Beta 2.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: better nsLocalFileWin changes per comment 3 (checked in 2005-04-24 09:53 -0700)

Does this also work, instead of the nsLocalFileWin.cpp changes in the previous
patch?

It looks to me like the change that happened is that the C-style _mbsstr and
_mbschr (const argument, non-const return, which encourages implicit casting
away of const) was replaced by a C++-style overloaded pair (const->const and
nonconst->nonconst), which is good for typesafety.  This patch corrects two
errors where the const-ness of the types was incorrect, and, if my
understanding of what's causing the errors is correct, ought to fix them.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #22)
> Configure and Configure.in files checks for Midl.exe version 6.00.0364

can we maybe stop configure doing that and just using whatever midl is
available? why does it do that at all?

Comment 26

[Piotr Komoda]

(In reply to comment #24)
> Does this also work, instead of the nsLocalFileWin.cpp changes in the previous
> patch? 

It seems to work, I've compiled Firefox without any problems. 

Comment 27

[Darin Fisher]

Comment on attachment 181515 [details] [diff] [review]
better nsLocalFileWin changes per comment 3 (checked in 2005-04-24 09:53 -0700)

so, the _mbsstr change is required, and the other is just for correctness
(i.e., not actually required to make VC2005 happy)?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 181515 [details] [diff] [review]
better nsLocalFileWin changes per comment 3 (checked in 2005-04-24 09:53 -0700)

No, they're both needed, since path is (below the context) assigned to a
non-const variable via _mbschr (and since that didn't compile, I suspect it's
overloaded).

Anyway, very low risk patch for compiler portability...

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 181515 [details] [diff] [review]
better nsLocalFileWin changes per comment 3 (checked in 2005-04-24 09:53 -0700)

a=asa

Comment 30

[cls]

The midl check was added because one of the platform sdks came with a different
version of midl.exe which caused the build to break because we weren't
specifically targetting W2K IIRC.  Rather than adding that limitation, we just
added the midl check.  We've started using /no_robust since then so I'm not sure
if the first midl check is still needed.

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 159681 [details] [diff] [review]
LDAP Build Changes

r=dmose

Note that this will need to be checked in on both the client branch and the
trunk of the LDAP C SDK.

Comment 32

[Stephen Walker]

Attachment: Activex/mfcembed bustage changes

Comment 33

[Stephen Walker]

Attachment: Activex/mfcembed bustage changes (checked in)

Activex broke again now that xpconnect activex support builds.

http://msdn2.microsoft.com/library/ms177253(en-us,vs.80).aspx
Compiler will not inject int as the default type in declarations
    Code that is missing the type in a declaration will no longer default to
type int the compiler will generate Compiler Warning C4430 or Compiler Warning
(level 4) C4431. Standard C++ does not support a default int and this change
will ensure that you get the type you really want.

Comment 34

[Mike Shaver (:shaver emeritus)]

Comment on attachment 182643 [details] [diff] [review]
Activex/mfcembed bustage changes (checked in)

I'll go ahead and r+a this -- it's just good compiler cleanup, obviously
correct.

Comment 35

[Stephen Walker]

Comment on attachment 182643 [details] [diff] [review]
Activex/mfcembed bustage changes (checked in)

checked in by db48x

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 159681 [details] [diff] [review]
LDAP Build Changes

a=asa

Comment 37

[dackz]

(In reply to comment #23)
> Created an attachment (id=181512) [edit]
> Remaining things for VS2005 Beta 2
> 
> Here's what still needs to be done to successfully finish compilation under
> Visual Studio 2005 Beta 2.

What about Midl 6.00.0365? This is the version of Midl that I have, while your
patch only adds support for 6.00.0366.

Comment 38

[Piotr Komoda]

(In reply to comment #37)
> What about Midl 6.00.0365? This is the version of Midl that I have, while your
> patch only adds support for 6.00.0366.

It's not a patch, it's rather some kind of summary for
developers/contributors/people with CVS write access/whatever on what still
needs to be done, or on what they should take a look. Besides, as someone said
in comments to this bug, there's probably no further need of checking midl
version. So if it can be removed, that would be great, because adding check for
every possible midl version from VS2005 beta is IMO stupid.

Comment 39

[dackz]

(In reply to comment #38)
> (In reply to comment #37)
> It's not a patch, it's rather some kind of summary for
> developers/contributors/people with CVS write access/whatever on what still
> needs to be done, or on what they should take a look. Besides, as someone said
> in comments to this bug, there's probably no further need of checking midl
> version. So if it can be removed, that would be great, because adding check for
> every possible midl version from VS2005 beta is IMO stupid.

Well to clarify, the win32 version of midl is 6.00.0366, while the amd64 version
is 6.00.0365.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: LDAP Build Change, v2

Merged LDAP changes to latest tree.  Carrying forward review/approval.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 186243 [details] [diff] [review]
LDAP Build Change, v2

LDAP changes have been checked in on both the client branch and the trunk.

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

note bug 298260

Comment 43

[MCT]

Something else to consider. This may be a difference between the Express Edition
and the Full Blown Edition, not sure. I have built Firefox successfully using
the full version of VS2005 beta 2. However, ran into strife with the debug
builds. The reason is that MS has a new scheme for managing where to find
compiler runtime library dll's. (Called "side by side assemblies").

By default, when an exe is linked, a manifest file also gets created that points
the exe to the correct dll's. The mozilla build scripts don't seem to know about
this, so when they copy certain exe's around during the build, the manifests
don't get copied with them. The exe cannot find the dll's without the manifest,
as the dll's are not on the search path. (That is the "old" way of doing things!)

The workaround, which is what I think people will be tempted to do, is to copy
the crtl dll's out of windows\system32\winsxs tree back into a directory on the
search path. This probably works, but works against the approach that MS wants
people to take.

It is possible to get the manifest file into the exe, but this requires
additional steps after the link.

So, either:

1. The manifest file must be copied around with the exe's (and the runtime libs
must be installed in the correct structure under windows\system32\winsxs, and
I'm not sure quite how they get there if you don't have the compiler installed), or:

2. The manifest file is added into the exe using additional make steps, or

3. The runtime dll's are copied into the dist\bin folder (and for release builds
are distributed in that location?)

From a build point of view, what is the correct way to get a compiler-specific
runtime dll added into the dist?

Comment 44

[Dave Townsend [:mossop]]

Attachment: Embed manifest files

This patch does the work of embedding the manifest files into the exe and dll
files. I needed this to actually complete a build, but the built files still do
not really work well for me so I'm only attaching this patch for reference.

Also I have no experience in changing the makefiles so chances are I've done
this in totally the wrong place!

Comment 45

[Aaron Leventhal]

Visual Studio 2005 is out now. What's currently the best way to get libidl/glib. We need to update the build instructions on developer.mozilla.org which only mention Visual Studio 2003: http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites

Do we need to update our ftp site with vc8 versions?
ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/libraries/win32/ 

Comment 46

[Robert Accettura [:raccettura]]

(In reply to comment #45)
> Visual Studio 2005 is out now. What's currently the best way to get
> libidl/glib. We need to update the build instructions on developer.mozilla.org
> which only mention Visual Studio 2003:
> http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites
> 
> Do we need to update our ftp site with vc8 versions?
> ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/libraries/win32/ 
> 
Yes, we need vc8 versions.

IMHO this is worth some effort, as there is a Free Express version (must download by Nov 2006 if you want it).  A lot cheaper than paying ;-).  I'm trying to compile now.

There are a few user contributed vc8 versions in the mozillazine forums.

Comment 47

[Dave Townsend [:mossop]]

I agree that providing vc8 versions of the libraries would be great. I would just say that the versions that I found on the forums were compiled with beta versions of vc8 and didnt appear to work well with the RC version and so potentially with the final version.

Comment 48

[(not currently active) Ted Mielczarek]

I don't see this as being critical just because it's a free download.  The Visual C++ Toolkit 2003 has been out for quite some time, and it's quite possible to compile Mozilla with that right now.

Comment 49

[Ronny Perinke]

(In reply to comment #47)
> I agree that providing vc8 versions of the libraries would be great. I would
> just say that the versions that I found on the forums were compiled with beta
> versions of vc8 and didnt appear to work well with the RC version and so
> potentially with the final version.
> 

I opened a bug entry a view days ago to address this issue.
bug #315929 provides the necessary patches for glib, libIDL and pthreads as well as pre-compiled binaries (with MSVC++ 2005 EE).

Those files + the sources can also found on my homepage - http://www.sephiroth-j.de/mozilla/moztools.vc8/

At the time writing this comment, I am testing an improved and more completely patch to embed the .manifest files, which is the recommended/prefered way.

http://msdn2.microsoft.com/en-us/library/ms235591(en-US,VS.80).aspx
>
> It is recommended that a C/C++ application have its manifest embedded inside  the final binary because this ensures proper runtime behavior in most scenarios.

Comment 50

[Ronny Perinke]

Attachment: improved patch to embed manifest files

needed for successfull compilation using MSVC++ 2005.
otherwise the manifest files must be copied manually to dist/bin or the build process will fail after compiling xpidl.exe and the final applications executables (and updater) will not start.

Comment 51

[Wan-Teh Chang]

Comment on attachment 202883 [details] [diff] [review]
improved patch to embed manifest files

Thanks a lot for the patch.

Rather than adding "host_os", you should use the
existing HOST_OS_ARCH variable, which has the value
"WINNT" on Windows.  (In mozilla/nsprpub,
mozilla/security/coreconf, mozilla/security/nss,
and mozilla/directory/c-sdk, just use OS_ARCH.)

Since Mozilla supports MSVC 6.0, you should find out
if MSVC 6.0 has mt.exe.  If not, the makefile rule
that uses mt.exe needs to be ifdef'ed for the versions
of MSVC that have mt.exe.

Comment 52

[Ronny Perinke]

(In reply to comment #51)
> (From update of attachment 202883 [details] [diff] [review] [edit])
> Thanks a lot for the patch.
> 
> Rather than adding "host_os", you should use the
> existing HOST_OS_ARCH variable, which has the value
> "WINNT" on Windows.  (In mozilla/nsprpub,
> mozilla/security/coreconf, mozilla/security/nss,
> and mozilla/directory/c-sdk, just use OS_ARCH.)
>
well, ok, I'll add some missing checks fo WINNT, but actually there are no more checks needed, because of host_os.
host_os has the value "msvc" when using MSVC, so we can be shure that we really use MSVC++ and this is only available for Windows.

> Since Mozilla supports MSVC 6.0, you should find out
> if MSVC 6.0 has mt.exe.  If not, the makefile rule
> that uses mt.exe needs to be ifdef'ed for the versions
> of MSVC that have mt.exe.
> 
mt.exe is part of the Platform SDK and now included in MSVC 8.0. MSVC 6 won't contain it AFAIK (I do not have MSVC 6, only the free 2003 Toolkit).
The point is, that on earlier versions of MSVC than 2005 (8.0) those manifest files are not createad automatically and not necessary to run. Therefore the check for existens of manifest files should be sufficient.

What about the following check instead of just check for a manifest file?
@if [ -f $@.manifest ] && [ $(_MSC_VER) -ge 1400 ]; then \
fi

_MSC_VER is 1400 for MSVC 8.0

Comment 53

[Julien Pierre]

The free express version of VC8 doesn't include windows.h or anything like a platform SDK. I browsed the Microsoft forums and it was stated by MS employees that this was an intentional decision - they charge for the version with the windows toolkit. I tried to use headers from older licensed MS compilers I had, unsuccessfully.

Comment 54

[cls]

> well, ok, I'll add some missing checks fo WINNT, but actually there are no more
> checks needed, because of host_os.
> host_os has the value "msvc" when using MSVC, so we can be shure that we really
> use MSVC++ and this is only available for Windows.

host_os only returns msvc when using Netscape's custom uname.  If you use the cygwin uname or msys uname, then they will return cygwin32 & mingw32 respectively.  The result of the uname output does not indicate which compiler is being used.

For what you want, you'll need to check for HOST_OS_ARCH == WINNT && !GNU_CC.  Instead of !GNU_CC, you could try to explicitly check for _MSC_VER >= 1400 using  something like: ifneq (1399, $(firstword $(sort 1399 ${_MSC_VER})))  .

> mt.exe is part of the Platform SDK and now included in MSVC 8.0. MSVC 6 won't
> contain it AFAIK (I do not have MSVC 6, only the free 2003 Toolkit).

Assuming that the first mt.exe in the path is the correct one is a bad idea.  Some cygwin installations may have mt.exe installed.  The cygwin version is based off of the standard unix magnetic tape manipulation utility.  You'll need a configure check to verify that you have the proper version.

Comment 55

[Ronny Perinke]

Attachment: patch to embed manifest files with check for valid mt.exe

Chris, thanks for the explanations. I always thought host_os would be a variable which holds the compiler which is being used.

I modified the patch which now includes a configure check (mozilla/configure /mozilla/nsprpub/configure and /mozila/directory/c-sdk/configure) and sets MSMANIFEST_TOOL to 1, if mt.exe is valid (default is 0).

Comment 56

[Benjamin Smedberg]

Comment on attachment 203439 [details] [diff] [review]
patch to embed manifest files with check for valid mt.exe

This patch includes changes to "configure" but not "configure.in"... it should be the other way 'round.

Please attach NSPR/NSS/main tree patches separately, since they must be reviewed by different people and landed on different branches.

+MSMANIFEST_TOOL=0

Why "0"? Normally we use an empty var for a tool not present/configure option not enabled.

+ifeq (_1,_$(MSMANIFEST_TOOL))

This can just be ifdef MSMANIFEST_TOOL

Comment 57

[Ronny Perinke]

(In reply to comment #56)
> (From update of attachment 203439 [details] [diff] [review] [edit])
> This patch includes changes to "configure" but not "configure.in"... it should
> be the other way 'round.
> 
Where is the difference?

> Please attach NSPR/NSS/main tree patches separately, since they must be
> reviewed by different people and landed on different branches.
> 
So separate patches for NSPR and Security?

> +MSMANIFEST_TOOL=0
> 
> Why "0"? Normally we use an empty var for a tool not present/configure option
> not enabled.
> 
> +ifeq (_1,_$(MSMANIFEST_TOOL))
> 
> This can just be ifdef MSMANIFEST_TOOL
> 
ok

Comment 58

[Ronny Perinke]

(In reply to comment #56)
> (From update of attachment 203439 [details] [diff] [review] [edit])
> This patch includes changes to "configure" but not "configure.in"... it should
> be the other way 'round.
> 
that did not worked at all

Comment 59

[Benjamin Smedberg]

Ronny, "configure" is a generated file, produced from the "configure.in" script. You have to run "autoconf-2.13" to do this generation, but patches against "configure" aren't going to do any good because the next time somebody changes configure.in your changes will get wiped away.

Comment 60

[Ronny Perinke]

(In reply to comment #59)
> Ronny, "configure" is a generated file, produced from the "configure.in"
> script. You have to run "autoconf-2.13" to do this generation, but patches
> against "configure" aren't going to do any good because the next time somebody
> changes configure.in your changes will get wiped away.
> 

Thank you Benjamin, it works now. :) Why does autoconf-2.13 not start autoamtically at the beginning of a build process after configure.in was changed?

Are there any other things I have to consider before I'll attach the new set of patch files?

Comment 61

[Ronny Perinke]

Attachment: embed manifest files with check for mt.exe - patch#1 (main)

patches configure.in and config/rules.mk
diff'ed against TRUNK

Comment 62

[Ronny Perinke]

Attachment: embed manifest files with check for mt.exe - patch#2 (NSPR)

patches nsprpub/configure.in nsprpub/config/autoconf.mk.in nsprpub/config/Makefile.in nsprpub/config/rules.mk

Comment 63

[Ronny Perinke]

Attachment: embed manifest files with check for mt.exe - patch#3 (security/NSS)

security/coreconf/rules.mk security/coreconf/WIN32.mk security/nss/lib/fortcrypt/Makefile

Comment 64

[Ronny Perinke]

Attachment: embed manifest files with check for mt.exe - patch#4 (c-sdk)

Comment 65

[Julien Pierre]

Comment on attachment 204580 [details] [diff] [review]
embed manifest files with check for mt.exe - patch#3 (security/NSS)

This patch does not apply to the tip of NSS or to the current branch, NSS_3_11_BRANCH .

lib/fortcrypt has been removed from NSS in 3.11, so you can forget about patching it.

The rules.mk changes however need to be merged with the current version, as there have been many changes.

Comment 66

[Julien Pierre]

Comment on attachment 204580 [details] [diff] [review]
embed manifest files with check for mt.exe - patch#3 (security/NSS)

The patch to WIN32.mk is incorrect because it only ever sets MSMANIFEST_TOOL if BUILD_TREE is set.

When doing a standalone build of NSS (gmake nss_build_all target in mozilla/security/nss), it must work even if BUILD_TREE is not set.

The manifest still needs to be added to all executables and shared libraries in that case.

Comment 67

[Julien Pierre]

The WIN32.mk patch in coreconf also depends on autoconf in mozilla, which is wrong. I think the right thing to do is to just remove the ifdef for MSMANIFEST_TOOL in the patch for coreconf/rules.mk . This assumes that mt.exe is always the first in the path. When the browser builds NSS, the NSPR build and autoconf are run first, so it should already have checked that. In the standalone NSS build, we would have to make it a requirement that the MS mt.exe is in the path first, and document it in the NSS and mozilla build instructions, since mt.exe from VC2005 may conflict with versions of mt.exe in both cygwin and MKS toolkits .

Comment 68

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #60)
> (In reply to comment #59)
> > Ronny, "configure" is a generated file, produced from the "configure.in"
> > script. You have to run "autoconf-2.13" to do this generation, but patches
> > against "configure" aren't going to do any good because the next time somebody
> > changes configure.in your changes will get wiped away.
> > 
> 
> Thank you Benjamin, it works now. :) Why does autoconf-2.13 not start
> autoamtically at the beginning of a build process after configure.in was
> changed?
> 

I think if you put "mk_add_options RUN_AUTOCONF_LOCALLY=1" in your .mozconfig, it will run autoconf when configure.in changes.

Comment 69

[Aaron Leventhal]

This seems stalled ... are there steps someone can give us to get this working now, before all the last touches are put on it?

Comment 70

[Dave Townsend [:mossop]]

(In reply to comment #69)
> This seems stalled ... are there steps someone can give us to get this working
> now, before all the last touches are put on it?

Using the 4 patches here (with a bit of work to un-bitrot them) and the VC 8 moztools does allow me to compile in VC 8.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted patch (diff -up16)

This is the same as the above four patches, except combined into one patch and unbitrotted. It applies cleanly to the current trunk.  Hopefully others find it useful :)

Comment 72

[Jon Henry]

Ryan, you should request review for your patch from a build system guru, maybe Benjamin Smedberg ([e-mail])?

Comment 73

[Ryan VanderMeulen [:RyanVM]]

I'm assuming the reviews would carry over from the previous patches since nothing's changed except updating for bitrot.

Comment 74

[Aaron Leventhal]

(In reply to comment #73)
> I'm assuming the reviews would carry over from the previous patches since
> nothing's changed except updating for bitrot.
> 
Yes but only the 1st patch had r=+ and we also need sr='s

The old patches should be obsoleted and the new patch should have review request flags set.

Comment 75

[Benjamin Smedberg]

Please keep the patches for mozilla/NSS/NSPR/LDAPCSDK separate: they are maintained by different teams and need separate review and landing.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted embed manifest files with check for mt.exe - patch#1 (main) [fixed on trunk]

Un-bitrotted version of attachment 208970 [details] [diff] [review] split into seperate parts per bsmedberg's request. Carrying over previous r+ since no code has actually changed.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted embed manifest files with check for mt.exe - patch#2 (NSPR)

Un-bitrotted version of attachment 208970 [details] [diff] [review] split into seperate parts per bsmedberg's request. Carrying over previous r?

Comment 78

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted embed manifest files with check for mt.exe - patch#3 (security/NSS)

Un-bitrotted version of attachment 208970 [details] [diff] [review] split into seperate parts per
bsmedberg's request. Carrying over previous sr- since no code has actually
changed.

Comment 79

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted embed manifest files with check for mt.exe - patch#4 (c-sdk)

Un-bitrotted version of attachment 208970 [details] [diff] [review] split into seperate parts per
bsmedberg's request. Carrying over previous r?

Comment 80

[Benjamin Smedberg]

Ryan, what are you planning to do with the NSS patch? It is incorrect as-is since it relies on the mozilla build tree for building NSS.

Comment 81

[Benjamin Smedberg]

Comment on attachment 209050 [details] [diff] [review]
Un-bitrotted embed manifest files with check for mt.exe - patch#1 (main) [fixed on trunk]

Attachment 209050 [details] [diff] landed on trunk.

Comment 82

[Ryan VanderMeulen [:RyanVM]]

All I did was update the old patches for bitrot. I'm not sure how to go about making the changes requested by Julien. With some guidance, I could probably learn how to implement the changes, but that would require someone willing to help me along.

Comment 83

[Aaron Leventhal]

How do I get the *.exe.manifest files?

Doing make -f client.mk doesn't seem to grab them. I have to manually cvs up -C filename to get them.

Comment 84

[Ryan VanderMeulen [:RyanVM]]

Aren't they generated at compile time? The only existing manifest file which isn't generated is Microsoft.VC80.CRT.manifest, which comes with MSVC8.  After the compile finishes, it must be manually copied to the root, components, and plugins directories.

Comment 85

[Robert Kaiser]

Aaron: The current VC8 compiler generates them. This patch integrates them into the executable and libary files again (which is easily possible), so one doesn't need to care about them, like with older compilers.

Comment 86

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Argh, didn't realize there were still outstanding patches for this -- not sure why I didn't run into this in the few builds that I did.  The NSPR patch isn't crucial (because NSPR just builds tests), but the NSS patch is -- it builds the the dll singing executable and then tries to execute it from the dist dir.  I'll see about updating that patch tomorrow to remove the dependency on the top directory's conf script.

Comment 87

[Jon Henry]

FWIW, I can successfully complete a trunk Firefox build using VS 2005 Professional after bsmedberg's checkin of attachment 209050 [details] [diff] [review]. I can't speak to less common build configurations, but I've been dogfooding off my VS 2k5 trunk build for about a week with no troubles.

Comment 88

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I've been doing the same; I think the problem is just release builds, though I could've sworn I did a release build a while ago with a beta to compare build sizes..

Comment 89

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Reworked manifest patch#3 for NSS

Here's an updated patch that removes the top level dir check; it'll just run mt.exe if it finds a .manifest file for the exe.

In my local build, with the exact same config as the tinderbox, shlibsign is executed from the nss directory (and thus always worked fine even without this patch, because the .manifest file was right there).  I build with a separate objdir though, and the tinderboxes don't -- so on the tinderbox it was being executed from dist/bin, and failing, because the .manifest file wasn't there.  I don't know why that distinction is there.

Comment 90

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 210525 [details] [diff] [review]
Reworked manifest patch#3 for NSS

This is currently blocking the VC8 tinderboxes; switching them to objdir builds didn't help, and I'm not sure why.  (Like I said, on all my local builds, shlibsign gets called from nss/, but it insists on running from dist/bin/ on the tinderbox..)

Comment 91

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Workaround for NSS manifest issue

This is a non-NSS workaround until the NSS patch gets reviewed/checked in; this is (I think) the only time we call shlibsign from dist/bin under VC8, and it only happens when we're actually packaging things up -- this is why I never saw it any of my builds.

Comment 92

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: one more NSS workaround

One more shlibsign.exe workaround; I missed the toplevel Makefile.in call to shlibsign.  I just give up and copy in the manifest file if it exists; otherwise I'd have to set up the paths and everything to be able to call shlibsign from somewhere other than dist/bin (which has the dlls).  Tinderbox builds are green with this patch.

Comment 93

[Benjamin Smedberg]

Comment on attachment 210992 [details] [diff] [review]
one more NSS workaround

I'm not sure I understand why you need to copy the manifest file when you subsequently run shlibsign from nss/

Comment 94

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: one more (real) NSS workaround

Er!  Whoops, attached the wrong patch.  This is the correct patch; it's just to copy in the .manifest and still call shlibsign from dist/bin.  (Has to be called from dist/bin, otherwise it can't find various dll's that it needs.)

Comment 95

[Benjamin Smedberg]

Comment on attachment 211035 [details] [diff] [review]
one more (real) NSS workaround

Please add # XXX comment so that this is removed once the NSS build-config patches land.

Comment 96

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in with added comment.

Comment 97

[Michael Daumling]

*** Bug 326203 has been marked as a duplicate of this bug. ***

Comment 98

[Aaron Leventhal]

Has anyone built MOZILLA_1_8_BRANCH with these patches? I still get stuck at nsIConsoleListener.idl because MSVCR80.dll was not found.

Comment 99

[Benjamin Smedberg]

aaronlev, none of this or its followups are on the 1.8 branch. But the staic-libidl patches are, and I highly recommend using the moztools-static package on that branch, because it is compatible with all versions of MSVC.

Comment 100

[Robert Kaiser]

Attachment: make NSS workaround not fail on <VC8

Actually, due to that VC8 NSS workaround, the signnss step now fails on all older compilers than VC8 as the test command returns with error 1 in that case. We need to ignore that error in the Makefile to make those work again, which is what this patch does.

Comment 101

[Wan-Teh Chang]

Comment on attachment 224429 [details] [diff] [review]
make NSS workaround not fail on <VC8

It may be better to use an if statement.  Otherwise
I suspect the '-' will also ignore the failure of the
cp command.

Comment 102

[Benjamin Smedberg]

Comment on attachment 224429 [details] [diff] [review]
make NSS workaround not fail on <VC8

You want ifdef MSMANIFEST_TOOL

Comment 103

[Jonathan Stanley]

After a lot of RTFM-ing and being re-referenced back to this bug from various threads at Mozillazine, it still (?) seems vanilla/un-patched MOZILLA_1_8_BRANCH and building Firefox2 will fail, though TRUNK does now compile without issue.

Comment 104

[Mike Shaver (:shaver emeritus)]

Sounds like this should be closed FIXED, then. (We don't intend to make these changes to the branches, to my knowledge.)

Comment 105

[Julien Pierre]

Mike,

I'm not aware that any NSS changes were ever checked in to the NSS trunk for this, so I don't think the bug can be marked as fixed, unless it was determined that they are not needed.

Comment 106

[Mike Shaver (:shaver emeritus)]

Ugh/sigh.  If we need to distinguish between NSS-trunk and NSS-as-used-by-Firefox-trunk, maybe we need to move the NSS issues off to a separate bug?

Comment 107

[Sergei Dolgov]

Well, as result of packager.mk change it cannot sign libs in SeaMonkey for BeOS,
when running "make" in mozilla/xpinstall/packager:
"run-mozilla.sh cannot execute ../../nss/shlibsign"

and really, i don't see shlibsign executable anywhere besides dist/bin.

Wondering which patches and changes BeOS port of SeaMonkey missed in last 2 months, to perform signing successfully?

P.S. Tested with dynamic build, wondering if it matters. When rolling $(DEPTH)/nss/shlibsign back to $(DIST)/bin/sglibsign
at http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/installer/packager.mk#176
packaging completes without problems.

Comment 108

[Robert Kaiser]

Attachment: make NSS workaround not fail on <VC8, v2 (checked in)

OK, it took too long for me to fix this patch, but here it is. One more time, this makes the signnss step work again in older compilers, using the workaround line only for VC8.

Comment 109

[Wan-Teh Chang]

Comment on attachment 210525 [details] [diff] [review]
Reworked manifest patch#3 for NSS

A new version of this patch (attachment 240450 [details] [diff] [review]) has been checked into NSS 3.11.4 (bug 353475).

Comment 110

[Wan-Teh Chang]

Attachment: Remove NSS workarounds

The NSS patch has been checked in on the Mozilla trunk and MOZILLA_1_8_BRANCH.
So the two NSS workarounds (that I know of) can be removed now.

Please test and check in this patch.  (I can't test it easily.)  Thanks.

Comment 111

[Kai Engert [:KaiE:]]

Comment on attachment 247764 [details] [diff] [review]
Remove NSS workarounds

I think this patch is needed to fix bug 370693 - on Linux!

I'm not sure yet whether it will be sufficient to fix that other bug, but it appears we should get this in soon.

Vlad, can you please review?

Or bsmedberg?
Thanks!

Comment 112

[Kai Engert [:KaiE:]]

Comment on attachment 247764 [details] [diff] [review]
Remove NSS workarounds

I decided to take this patch over to bug 370693, because another change is required.

Comment 113

[(not currently active) Ted Mielczarek]

We're building official nightlies with VC2k5, so I think this is fixed.  I'm going to update those last two patches (NSPR and c-sdk), but I'll do the work in bug 350616.

Comment 114

[cmollis]

is this fixed? newbie checkout and build produced a bunch of errors..  looks like it's missing some projects (medialibrary, playlistsrc..)

Comment 115

[Ryan VanderMeulen [:RyanVM]]

WFM. Are you sure you carefully followed all the directions at the page below?
http://developer.mozilla.org/en/docs/Build_Documentation
http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites

Comment 116

[cmollis]

I'm a moron... 



...thanks.

Comment 117

[Matthew Wilcoxson]

This bug is still referred to on the following web page:
http://publicsvn.songbirdnest.com/trac.cgi/wiki/Build_Instructions#MSVC

It doesn't need to be there any longer but I can't figure out how to edit this wiki page...