Closed Bug 95128 Opened 23 years ago Closed 22 years ago

standalone NSS build does not work on OS/2 with VAC++ compiler

Categories

(NSS :: Build, defect, P3)

x86
OS/2
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

References

Details

Attachments

(2 files, 3 obsolete files)

When using the IBM compiler, Visual Age C++ 3.6, the standalone NSS build does not complete. This is done by doing "cd mozilla\security\nss" "gmake nss_build_all" gmake.exe[2]: Entering directory `/dev/mozilla/mozilla/security/dbm/src' icc -FoOS22.45_icc_DBG.OBJ/db.obj -c /Q /qlibansi /Gd /Gm /Su4 /Mp /Tl- /Ti+ -DXP_PC=1 -DXP_OS2=1 -DXP_OS2_VACPP -DTCPV40HDRS -DDEBUG -D_DEBUG -DDEBUGPRINTS -DMEMMOVE -D__DBINTERFACE_PRIVATE -DNSPR20=1 -I../../../dist/include -I../../../dist/OS22.45_icc_DBG.OBJ/include -I../../../dist/public/dbm -I../../../dist/private/dbm -I../include -I../../../dbm/include ..\\..\\..\\dbm\\src\\db.c ../../../dist/public/dbm\mcom_db.h(159:10) : error EDC0296: #include file <dirent.h> not found. gmake.exe[2]: Leaving directory `/dev/mozilla/mozilla/security/dbm/src' gmake.exe[1]: Leaving directory `/dev/mozilla/mozilla/security/dbm' The dirent.h header is part of the browser build on OS/2 but not part of the NSS tree, which is why the standalone NSS build fails. It is not a system header, rather it was created for OS/2 to emulate functionality available on other platforms. There is also a dirent.c corresponding to it. There may be other NSS apps on OS/2 than the browser, so the standalone build should get fixed.
Julien, do you any suggestion on how we can fix this problem? Where are the dirent.h and dirent.c files located in the Mozilla source tree? I'm wondering if we could move them into mozilla/dbm.
I'm not certain what the proper fix is because I haven't looked at the browser build. I assume the browser uses those dirent APIs too, not just the DBM in NSS. I don't like code duplication in general so I'm a little unsure if we want to have two copies of it - one living in the browser and one in the NSS DLL - but it looks like there may be no choice but to do that if we want to be able to build both.
would it be horrible if this moved to nspr? note that we already have 2 files named dirent.h that appear to be entirely unrelated :-) http://lxr.mozilla.org/seamonkey/find?string=dirent.h
Yes, NSPR might be a good place for these files, since it is used by both NSS and the client.
NSPR is not the place for these files. To understand my position, just consider a generalization of your logic. Suppose I noticed that two arbitrary NSPR clients (say, iPlanet Web Server and iPlanet Messaging Server) use a common header file, I would conclude that that file should be moved to NSPR. <dirent.h> is not part of the NSPR API. Therefore NSPR cannot be the provider of this file.
I'm not sure how to resolve this. In reality, we should probably ship these files as part of our build environment. These are custom written version of these headers because VACPP doesn't have them.
I would put dirent.h in mozilla/dbm/include/os2 or mozilla/dbm/src/os2 and add -Imozilla/dbm/include/os2 to the compile command line on OS/2. Since dirent.h is only used inside dbm, it is okay for other parts of Mozilla to use a different dirent.h.
dirent.h is used in a lot more places than that. In particular, it is used to build nsinstall. That is why it is in config/os2.
Sorry I wasn't clear. What I meant by "dirent.h is only used inside dbm" is that dbm only uses dirent.h internally, that is, none of the exported dbm header files include dirent.h. However, I just found that this is not true. mcom_db.h, an exported dbm header file, includes dirent.h (only on OS/2 though). We should be able to fix that. Assuming we fix that, then dbm can use its own version of dirent.h. Its use of dirent.h is hidden underneath the dbm exported API and therefore it is okay for its version of dirent.h to be different from the other versions of dirent.h in the Mozilla source tree. This will lead to code duplication, which means the ability to use dbm standalone is not free.
Taking ownership since there is an OS/2 fan in the NSS group now :).
Assignee: wtc → jpierre
I'm in the process of building standalone NSS on OS/2 now. Basically the fix is to remove the #include "dirent.h" from mcom_db.h , and put the dirent.h / dirent.c as part of DBM. I'm making a lot of progress with the build and expect the whole tree to be done soon, including all the utilities. However my changes might break the OS/2 browser build. I don't have a complete Mozilla browser environment setup yet, I only have what's required for NSS, so I can't check if this will break the browser or not. I will post a proposed patch when available and would appreciate if one of you at IBM with a functional browser build on OS/2 could test it.
This will definitely break the browser build. We need the dirent files to build nsinstall in config.
Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm just making additional copies of the dirent.h / dirent.c and putting them into DBM. My patch will not remove the existing dirent.h / dirent.c files from the browser tree; it will only remove the #include dirent.h from DBM. This means we'll have to live with 2 copies of the code, one in DBM and one in the browser. Wherever you are actually using the dirent functions outside of DBM in the browser, you will have to explicitly include the dirent.h header. I am done with the OS/2 NSS build changes - the whole standalone NSS tree built successfully - so I will attach a patch. Please apply it to a mozilla build tree and clobber/rebuild to figure out the #include you will need to add in the browser code.
Attached patch proposed patchSplinter Review
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → 3.4
Checking in the proposed patches to DBM and NSS on the tip to resolve this. And yes, the browser build still works with it.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
The new files dirent.h and dirent.c should not be added to mozilla/security/dbm. They should be added to mozilla/dbm, preferrably in a separate directory such as mozilla/dbm/src/os2. It would be better to use XP_OS2 in ndbm.c and secpwd.c because that macro is the most commonly used OS/2 macro in dbm and NSS. Consistency is a good thing. I am reopening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Wan-Teh, As far as the XP_OS2 vs XP_OS2_VACPP : The EMX and VACPP compilers are different, notably from the standpoint of header files. Because the OS/2 toolkit header files are copyrighted, they can't be shipped with the open-source EMX compiler. They ship some different headers which are closer to Unix. "sys/param.h" which is included from ndbm.c actually exists in the EMX build, but not in VACPP build. I am not testing the EMX build however. Among other things I believe that the EMX runtime library provides the "directory entry" functions that we need, because they are standard Unix functions, whereas the VACPP compiler which is closer to native OS/2 APIs doesn't. In secpwd I am using a system API OS/2 function from os2.h, DosGetInfoBlocks . I don't know how well that file is emulated in EMX. It appears that that function is present in the EMX header files, so it's probably OK to change the ifdef to an XP_OS2, though again, I'm not at all testing the EMX build (I have an old version of EMX laying around on my home box right now, but the build environment isn't setup). As far as moving the dirent files to dbm, I'm OK with it. I'm not sure about creating an os2 subdirectory under mozilla/dbm/src though. This will require an extra Makefile of its own and complicate things even more. I'd be more comfortable with an ifdef with the cross-platform Makefile.in . There are already many platform-specific ifdef's in it.
I see that when we build dbm as part of NSS, we are now building mozilla/security/dbm/src/dirent.c Why are we doing that? It appears to me that DBM doesn't use any of the functions in dirent.c. Am I missing something? BTW, I'm in the process of porting DBM to use NSPR for all IO. When I'm done, DBM will need FAR fewer system header files than it needs now, and will have far fewer ifdefs! BTW, mozilla/dbm/src/nsres.c contains about 100 bugs. It doesn't check malloc calls for null returns. It doesn't check the value returned from the DB's get method, and just assumes it always succeeded. After calling the DB get method, it releases the lock that protects the DB before it has copied the data out of the DB's page buffers. It uses static variables for values that should be in the object -- better hope there's never more than one RESDATABASE (NSRESHANDLE) object! I've fixed all the bugs I've found in my NSPR-ized version of DBM. But tell me, is this file actually used in mozilla? Or is it all dead code? I hope, for mozilla's sake, that it's dead code.
Clarification of that last comment. I've fixed all the bugs in nsres.c in my NSPR-ized version of DBM. I make no claims about the rest of DBM :-)
Hmmm. I did some quick building without dirent and it would appear MAXPATHLEN in ndbm.c is the only thing needed from dirent.h. I only got the lib though, so I didn't link it to see if it actually worked. So ifwe could define MAXPATHLEN for OS/2, we wouldn't have the dirent dependency at all I believe.
One other unresolved external - S_ISDIR If we could get these two issues fixed, we wouldn't need dirent. Could stat be used intead of S_ISDIR in this case?
Blocks: 126923
I have produced a version of DBM that uses NSPR for all file IO and for stat-like functions. It doesn't make direct libc calls at all. I propose to make this the standard version of DBM used in "stand alone" NSS libraries immediately after NSS 3.4 is released. That change will eliminate any need for S_IS symbols in DBM. I submit that use NSPR for these functions, instead of libc, is the intended direction of all of Netscape products that use NSPR. The purpose of NSPR was to eliminate the platform dependencies on strange header files and library semantics. We should use NSPR to full advantage to achieve that goal.
Can we get this change made in the client as well?
I agree the NSPR-based DBM should be used in both "stand alone" NSS libraries and the Mozilla client. Excellent job, Nelson. Please open a bug and attach your DBM patch.
Set target milestone to NSS 3.5.
Target Milestone: 3.4 → 3.5
Wan-Teh , Nelson, Are we going to migrate to this NSPR-based DBM or not ? If not, what changes should we make here ? Right now there is an extraneous copy of the dirent file.
Note that the standalone NSS build does work currently, with the hacked headers that are in the tree.
Target Milestone: 3.5 → 3.6
What's the status on this please?
No news, except I don't think we are going to move to NSPR-based DBM. We are currently investigating other database options for NSS.
Nelson, I'd like to use the NSPR DBM for OS/2. Can we #ifdef it in?
Mike, It seems there are no longer objections to putting this change for all platforms. We'll discuss it at our NSS meeting tomorrow afternoon for inclusion in NSS 3.7 .
In answer to comment #31, Mike, the NSPR-ized version of DBM is on its own branch. To use it instead of the usual version, simply pull mozilla/DBM with the tag DBM_USING_NSPR_BRANCH and then build NSS (and DBM) normally, that is, in security/lib/nss run the command "gmake build_dbm" I have not tested that code on all platforms, but since it uses NSPR for all I/O, I expect it will work as well as NSPR does on any platform.
Nelson, I just tried to build your code with the NSPR DBM branch on OS/2. I ran into the problem that you use the ENOSYS symbol in a PR_SetError in hash.c and ndbm.c which is not defined on OS/2. I just changed it to 0 in my tree. I then removed the #include "dirent.h" that I had added to DBM on OS/2. I also removed dirent.c / dirent.h in mozilla/security/dbm , and removed dirent.c from the manifest.mn. This allowed building DBM with NSPR on OS/2.
in dbm/include/mcom_db.h I added a definition of ENOSYS (and other errors) conditionally compiled for WinCE. It wouldn't surprise me that OS/2 will also need a definition or two there.
I also had to define DBM_USING_NSPR in mozilla/security/nss/lib/softoken. This was done by removing the ifdef WINCE in config.mk . This allowed NSS to work with the NPSR-enabled DBM on OS/2.
Attached patch Remove dirent stuff from dbm (obsolete) — Splinter Review
OK, I think this patch is the correct step one, and we can get it in quick. NSPR DBM is different. This patch removes the dependencies between DBM and dirent using a similar method to Windows. It cleans up the code for VACPP quite a bit. The reason I am trying to get this done is because we are actually going to try to remove config/os2 and all corresponding files from the tree.
Target Milestone: 3.6 → 3.7
Moved to target milestone 3.8 because the original NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Attached patch New patch (obsolete) — Splinter Review
Anyone want to review this? This completely removed OS/2 dbm dependencies on dirent so it can be removed from the tree. This is an OS/2 only patch!
Attachment #104419 - Attachment is obsolete: true
Comment on attachment 111825 [details] [diff] [review] New patch 1. include/mcom_db.h You deleted the definition of MAXPATHLEN for non-OS/2 platforms. 2. src/mktemp.c Instead of nested ifdef's like this #if defined(_WINDOWS) || defined(XP_OS2_VACPP) #include <process.h> #ifdef _WINDOWS #include "winfile.h" #endif #endif I'd prefer two independent ifdefs #ifdef XP_OS2_VACPP #include <process.h> #endif #ifdef _WINDOWS #include <process.h> #include "winfile.h" #endif The other changes are fine.
Attachment #111825 - Flags: review-
Sorry about that removal thing. Don't know how I missed it. And I fixed the nesting issue. Thanks
Attachment #111825 - Attachment is obsolete: true
Comment on attachment 111840 [details] [diff] [review] Patch with wtc comments addressed >Index: dbm/src/mktemp.c [...] > #ifdef _WINDOWS >-#include <process.h> > #include "winfile.h" > #endif The <process.h> inclusion should not be deleted.
Attachment #111840 - Flags: review-
OK, I have double and triple checked. We only touch OS/2 stuff here. Sorry about that.
Attachment #111840 - Attachment is obsolete: true
Comment on attachment 111872 [details] [diff] [review] OK, really this is the patch r=wtc. Could you get a review from an OS/2 developer?
Attachment #111872 - Flags: review+
Comment on attachment 111872 [details] [diff] [review] OK, really this is the patch Looks good to me r=pedemont
Attachment #111872 - Flags: superreview?(dmose)
Comment on attachment 111872 [details] [diff] [review] OK, really this is the patch rs=dmose
Attachment #111872 - Flags: superreview?(dmose) → superreview+
Fix checked in.
Status: REOPENED → RESOLVED
Closed: 23 years ago22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: