Last Comment Bug 478171 - Consolidate the coreconf/XXX.mk files for Windows
: Consolidate the coreconf/XXX.mk files for Windows
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Build (show other bugs)
: 3.2
: x86 Windows XP
: -- normal (vote)
: 3.12.3
Assigned To: Wan-Teh Chang
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-11 22:37 PST by Wan-Teh Chang
Modified: 2009-02-14 10:48 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use WINNT.mk and WIN95.mk (checked in) (24.36 KB, patch)
2009-02-11 22:37 PST, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review
Move the common code in WIN95.mk and WINNT.mk to WIN32.mk (3.33 KB, patch)
2009-02-13 09:50 PST, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review
Move the common code in WIN95.mk and WINNT.mk to WIN32.mk (as checked in) (3.00 KB, patch)
2009-02-13 21:53 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Wan-Teh Chang 2009-02-11 22:37:54 PST
Created attachment 361946 [details] [diff] [review]
Use WINNT.mk and WIN95.mk (checked in)

Right now we have one coreconf/WINNTx.y.mk file for
each version of Windows.  We also have coreconf/WIN954.0.mk
for OS_TARGET=WIN95.  We should consolidate these files.

The attached patch is the first step towards that goal.

1. WIN954.0.mk is renamed WIN95.mk.  The only change is
that the comment in the file now says:
    Config stuff for OS_TARGET=WIN95
where "OS_TARGET=" is new.

2. WINNT6.0.mk is renamed WINNT.mk.  The only change is
that the comment in the file now says:
    Config stuff for OS_TARGET=WINNT
where "OS_TARGET=" is new, and there is no WINNT version
number.

3. All other WINNTx.y.mk files are removed.  These are
all essentially the same as WINNT6.0.mk.  Only WINNT3.51.mk
differs more, but we don't support Windows NT 3.51 any
more.

4. WIN95 and WINNT are added to TARGET_OSES in
coreconf/config.mk.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-02-12 22:13:49 PST
Comment on attachment 361946 [details] [diff] [review]
Use WINNT.mk and WIN95.mk (checked in)

r=nelson.

Before we consider this bug fully resolved, I'd like it to go further.
I want to remove ALL lines that are duplicated in the new WIN95.mk and
WINNT.mk and move them all to WIN32.mk.  I think that will leave very 
few lines in those files.  

I think the only lines that need to remain in WIN95.mk 
(other than the ubiquitous license header) are:

>+include $(CORE_DEPTH)/coreconf/WIN32.mk
>+DEFINES += -DWIN95
>+
>+# WINNT uses the lib prefix, Win95 and WinCE don't
>+NSPR31_LIB_PREFIX = $(NULL)

and the only lines that need to remain in WINNT.mk are:

>+include $(CORE_DEPTH)/coreconf/WIN32.mk
>+DEFINES += -DWINNT
>+#
>+# Win NT needs -GT so that fibers can work
>+#
>+OS_CFLAGS += -GT
>+
>+# WINNT uses the lib prefix, Win95 and WinCE don't
>+NSPR31_LIB_PREFIX = lib

All the other lines can be moved to WIN32.mk.
One difference between the present WINNT and WIN95 is
that one of them surrounds the lines that define 
OS_DLLFLAGS with an ifndef of NS_USE_GCC, like so:

+ifndef NS_USE_GCC
+OS_DLLFLAGS += -nologo -DLL -SUBSYSTEM:WINDOWS
+ifndef MOZ_DEBUG_SYMBOLS
+	OS_DLLFLAGS += -PDB:NONE
+endif
+endif

and the other one does not bracket those lines with that ifndef.  
I think those lines should be enclosed in that ifndef on both platforms.


Finally, I'd like to see us put something into WIN32.mk that figures out 
the version of the cl compiler, perhaps something like:

CC_VERSION := $(shell $(CC) -v 2>&1 | grep Version | sed -e 's|.* Version ||' -e 's| .*||')
CC_MAJOR_VERSION := $(shell echo $(CC_VERSION) | awk -F\. '{ print $1 }')
CC_MINOR_VERSION := $(shell echo $(CC_VERSION) | awk -F\. '{ print $2 }')
MSC_VER = $(CC_MAJOR_VERSION)$(CC_MINOR_VERSION)

so that Win32.mk can use that test to decide when it is (and is not) 
appropriate to use -PDB:NONE.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-02-12 22:15:56 PST
I wrote:
> I think those lines should be enclosed in that ifndef on both platforms.
I meant that when those lines are moved to WIN32.mk, they should look like
the version that now has the ifndef.
Comment 3 Wan-Teh Chang 2009-02-13 07:44:44 PST
Nelson, your comment 1 exactly matched what I planned to do!  I wanted to
add the CC_VERSION and MSC_VER variables to coreconf, and I found that I
need to consolidate the WIN*.mk files so that I only need to fix the -PDB:NONE
flag in one file.
Comment 4 Wan-Teh Chang 2009-02-13 09:15:06 PST
Comment on attachment 361946 [details] [diff] [review]
Use WINNT.mk and WIN95.mk (checked in)

I checked in this patch on the NSS trunk (NSS 3.12.3).

RCS file: /cvsroot/mozilla/security/coreconf/WIN95.mk,v
done
Checking in WIN95.mk;
/cvsroot/mozilla/security/coreconf/WIN95.mk,v  <--  WIN95.mk
initial revision: 1.1
done
Removing WIN954.0.mk;
/cvsroot/mozilla/security/coreconf/WIN954.0.mk,v  <--  WIN954.0.mk
new revision: delete; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/security/coreconf/WINNT.mk,v
done
Checking in WINNT.mk;
/cvsroot/mozilla/security/coreconf/WINNT.mk,v  <--  WINNT.mk
initial revision: 1.1
done
Removing WINNT3.51.mk;
/cvsroot/mozilla/security/coreconf/WINNT3.51.mk,v  <--  WINNT3.51.mk
new revision: delete; previous revision: 1.5
done
Removing WINNT4.0.mk;
/cvsroot/mozilla/security/coreconf/WINNT4.0.mk,v  <--  WINNT4.0.mk
new revision: delete; previous revision: 1.6
done
Removing WINNT5.0.mk;
/cvsroot/mozilla/security/coreconf/WINNT5.0.mk,v  <--  WINNT5.0.mk
new revision: delete; previous revision: 1.6
done
Removing WINNT5.1.mk;
/cvsroot/mozilla/security/coreconf/WINNT5.1.mk,v  <--  WINNT5.1.mk
new revision: delete; previous revision: 1.5
done
Removing WINNT5.2.mk;
/cvsroot/mozilla/security/coreconf/WINNT5.2.mk,v  <--  WINNT5.2.mk
new revision: delete; previous revision: 1.2
done
Removing WINNT6.0.mk;
/cvsroot/mozilla/security/coreconf/WINNT6.0.mk,v  <--  WINNT6.0.mk
new revision: delete; previous revision: 1.3
done
Checking in config.mk;
/cvsroot/mozilla/security/coreconf/config.mk,v  <--  config.mk
new revision: 1.27; previous revision: 1.26
done
Comment 5 Wan-Teh Chang 2009-02-13 09:50:44 PST
Created attachment 362242 [details] [diff] [review]
Move the common code in WIN95.mk and WINNT.mk to WIN32.mk

Nelson, this does what you outlined in comment 1.  This
finishes the job.

I will deal with the MSC_VER and -PDB:NONE issue in a
separate bug.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2009-02-13 12:13:59 PST
Comment on attachment 362242 [details] [diff] [review]
Move the common code in WIN95.mk and WINNT.mk to WIN32.mk

r=nelson.  Thanks, Wan-Teh.  Some comments.

>+#
>+# Define OS_CFLAGS and OS_DLLFLAGS.  This block comes from WIN95.mk and
>+# WINNT.mk and was simply moved here unchanged.  It should be properly
>+# merged into this file.
>+#

OK, but instead of that comment, I suggest refactoring this part as follows:

>+ifeq ($(CPU_ARCH), x386)
>+ifdef USE_64
>+	DEFINES += -D_AMD64_
>+else
>+	DEFINES += -D_X86_
>+endif
>+endif
>+ifeq ($(CPU_ARCH), ALPHA)
>+	DEFINES += -D_ALPHA_=1
>+endif
>+
>+ifndef NS_USE_GCC
>+OS_CFLAGS += -W3 -nologo
>+OS_DLLFLAGS += -nologo -DLL -SUBSYSTEM:WINDOWS
>+ifndef MOZ_DEBUG_SYMBOLS
>+	OS_DLLFLAGS += -PDB:NONE
>+endif
>+endif
Comment 7 Wan-Teh Chang 2009-02-13 21:53:41 PST
Created attachment 362369 [details] [diff] [review]
Move the common code in WIN95.mk and WINNT.mk to WIN32.mk (as checked in)

Nelson, I took your suggestion and further moved the refactored
code to the appropriate places in WIN32.mk.  If we decide that
we only support Win2k or higher, we can delete the Alpha code.
(Alpha is only supported up to NT 4.0.)

Checking in WIN32.mk;
/cvsroot/mozilla/security/coreconf/WIN32.mk,v  <--  WIN32.mk
new revision: 1.28; previous revision: 1.27
done
Checking in WIN95.mk;
/cvsroot/mozilla/security/coreconf/WIN95.mk,v  <--  WIN95.mk
new revision: 1.2; previous revision: 1.1
done
Checking in WINNT.mk;
/cvsroot/mozilla/security/coreconf/WINNT.mk,v  <--  WINNT.mk
new revision: 1.2; previous revision: 1.1
done
Comment 8 Nelson Bolyard (seldom reads bugmail) 2009-02-14 10:48:23 PST
Excellent!  Thanks, Wan-Teh!

Note You need to log in before you can comment on or make changes to this bug.