Include file case changes for mingw cross compilation on a linux host

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Mook, Assigned: Mook)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
Created attachment 314033 [details] [diff] [review]
change the file case in three lines.

Linux file systems are case sensitive; this means that for #include the file case must match.  In this case that means we're matching w32api when cross compiling from a linux host (mingw).  This seems to be an extension of bug 421255 (I'm just building things that cls didn't hit?)

Things that need changing:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib/ckfw/capi/ckcapi.h&rev=1.2&mark=64,65
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h&rev=1.10&mark=10

Plus things in breakpad (which shouldn't be here, and can't read mingw binaries anyway so there's not much of a point)


For when this gets to approval requests - this should be very, very safe (Windows won't see a change, and if I screw up the whole thing will just fail to build).
Attachment #314033 - Flags: review?(benjamin)

Comment 1

10 years ago
Comment on attachment 314033 [details] [diff] [review]
change the file case in three lines.

security/nss is NSS code and needs review from an NSS peer.

r=me on the nsDownloadScanner.h change
Attachment #314033 - Flags: review?(benjamin) → review+
(Assignee)

Comment 2

10 years ago
Created attachment 314254 [details] [diff] [review]
NSS only part (see comment 0 for details) (checked in on trunk)
Assignee: nobody → mook.moz+mozbz
Status: NEW → ASSIGNED
Attachment #314254 - Flags: review?(nelson)
Comment on attachment 314254 [details] [diff] [review]
NSS only part (see comment 0 for details) (checked in on trunk)

File names are also case sensitive in Windows when using certain file systems,
e.g. NFS.

The names of these header files (including the capitalization) are 
set by Microsoft.  The official and correct names for the header files
in question are "WTypes.h" and "WinCrypt.h".  

If someone has copied these files to Linux and downshifted all their names, 
that was an error.  The solution is not to introduce errors into the file 
names in the NSS sources.  The solution is to restore the proper names to 
these files in whatever Linux repository with which you are building.
Attachment #314254 - Flags: review?(nelson) → review-

Comment 4

10 years ago
I think you're going to be hard pressed to convince the w32api developers to change the case of the headers given that they've been distributing the package with all lowercase headers for years.  And, to clear up any misconceptions, those headers are a clean-room implementation of the Win32 API, not a direct copy of Microsoft's implementation.

The most expedient way to support both implementations of the Win32 API is to use the lowercased names since the vast majority of the time, case doesn't matter for native win32 builds.  IOW, the standard builds lose nothing by using lowercased includes.  Requiring developers to create a custom w32api package just to build Firefox is just going to drive them to some other product.  Building firefox is difficult enough as it is without adding unnecessary roadblocks.
I'd consider an ifdef'ed solution.
(Assignee)

Comment 6

10 years ago
Actually, I decided to just give up and read Windows.h (Vista SDK here, so SDK v6.0).  That says, on line 156:

#include <winbase.h>

Of course, I actually have:

2006-10-30  01:44 AM           308,665 WinBase.h

Somehow I don't think it would compile anyway on a case sensitive file system.

I tried to figure out how to get NTFS to be case sensitive, but it looks like setting that obcaseinsensitive registry key doesn't help (since the win32 layer that cl uses is still case insensitive).  It seems like I need to go find a case sensitive file system driver somehow.
I admit that I find the issue raised in comment 6 to be a compelling 
argument against striving for exactitude of capitalization in MS SDK 
header file names.  When the MS headers themselves are not careful or
consistent about it, what point is there in trying to preserve 
capitalization that they themselves do not preserve?  For a long time,
we tried to ensure that Windows source would build on Windows clients
that used NFS to attach remote Unix file systems, which were (of course)
case sensitive.  It was because of that work (in which I was quite involved
at one time) that I wanted to ensure that we did the right thing for 
file name capitalization.  

But I have recently learned that, due to the slowness of building on Windows 
using files on a remotely mounted NFS file system, all the NSS team's windows 
build systems now pull the sources onto the local file system (which is case 
inssensitive), do the builds locally, and then push the results back. Consequently, case sensitivity is no longer an issue for builds done on 
Windows.  So, I'm not going to strive for file name purity at this time.

However, in the event that MS sees the error of its ways (:-) and fixes
the #include statements to match the file name capitalization they use in
their SDK distributions, I will be inclined to again seek correctness of 
file name capitalization.

The NSS component of this bug is bug 405297.  Please attach the patch
you desire to resolve capitalization issues there.  
Depends on: 405297

Comment 8

10 years ago
It's fine to use lowercase header file names for
consistency:
http://lxr.mozilla.org/security/search?string=windows.h
http://lxr.mozilla.org/nspr/search?string=windows.h
We don't say Windows.h.  Why start now?

The MSDN page for CryptAcquireContext says Wincrypt.h:
http://msdn.microsoft.com/en-us/library/aa379886(VS.85).aspx
But the actual file name is WinCrypt.h, so the case
doesn't matter in the #include directives.

Comment 9

10 years ago
What's the final decision about this patch, then? Should I write a patch with ifdefed lowercase names, or will you accept the patch as attached?
Comment on attachment 314254 [details] [diff] [review]
NSS only part (see comment 0 for details) (checked in on trunk)

Please attach this patch to bug 405297.
Attachment #314254 - Flags: review- → review+

Comment 11

10 years ago
Attached the NSS patch to bug 405297 and landed the mozilla patch to mozilla-central, revision 0ae6f915dd4f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 314254 [details] [diff] [review]
NSS only part (see comment 0 for details) (checked in on trunk)

On trunk
lib/ckfw/capi/ckcapi.h; new revision: 1.3; previous revision: 1.2
Attachment #314254 - Attachment description: NSS only part (see comment 0 for details) → NSS only part (see comment 0 for details) (checked in on trunk)
You need to log in before you can comment on or make changes to this bug.