1.29 KB, patch
Benjamin Smedberg: review+
|Details | Diff | Splinter Review|
652 bytes, patch
Nelson Bolyard (seldom reads bugmail): review+
|Details | Diff | Splinter Review|
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 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+
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-
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.
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
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.
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+
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.