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

Status: RESOLVED FIXED

(NSS :: Build, defect, P3)

Description

[Julien Pierre]

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.

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Julien Pierre]

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.

Comment 3

[timeless]

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

Comment 4

[Julien Pierre]

Yes, NSPR might be a good place for these files, since it is used by both NSS 
and the client.

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Mike Kaply [:mkaply]]

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.

Comment 7

[Wan-Teh Chang]

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.

Comment 8

[Mike Kaply [:mkaply]]

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.

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Julien Pierre]

Taking ownership since there is an OS/2 fan in the NSS group now :).

Comment 11

[Julien Pierre]

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.

Comment 12

[Mike Kaply [:mkaply]]

This will definitely break the browser build. We need the dirent files to build 
nsinstall in config.

Comment 13

[Keyser Sose]

Marking NEW.

Comment 14

[Julien Pierre]

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.

Comment 15

[Julien Pierre]

Attachment: proposed patch

Comment 16

[Julien Pierre]

Checking in the proposed patches to DBM and NSS on the tip to resolve this. And
yes, the browser build still works with it.

Comment 17

[Wan-Teh Chang]

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.

Comment 18

[Julien Pierre]

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.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

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 :-)

Comment 21

[Mike Kaply [:mkaply]]

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.

Comment 22

[Mike Kaply [:mkaply]]

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?

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 24

[Mike Kaply [:mkaply]]

Can we get this change made in the client as well?

Comment 25

[Wan-Teh Chang]

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.

Comment 26

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 27

[Julien Pierre]

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.

Comment 28

[Julien Pierre]

Note that the standalone NSS build does work currently, with the hacked headers
that are in the tree. 

Comment 29

[Mike Kaply [:mkaply]]

What's the status on this please?

Comment 30

[Julien Pierre]

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.

Comment 31

[Mike Kaply [:mkaply]]

Nelson, I'd like to use the NSPR DBM for OS/2.

Can we #ifdef it in?

Comment 32

[Julien Pierre]

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 .

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 34

[Julien Pierre]

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.

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 36

[Julien Pierre]

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.

Comment 37

[Mike Kaply [:mkaply]]

Attachment: Remove dirent stuff from dbm

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.

Comment 38

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 39

[Mike Kaply [:mkaply]]

Attachment: New patch

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!

Comment 40

[Wan-Teh Chang]

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.

Comment 41

[Mike Kaply [:mkaply]]

Attachment: Patch with wtc comments addressed

Sorry about that removal thing. Don't know how I missed it.

And I fixed the nesting issue.

Thanks

Comment 42

[Wan-Teh Chang]

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.

Comment 43

[Mike Kaply [:mkaply]]

Attachment: OK, really this is the patch

OK, I have double and triple checked. We only touch OS/2 stuff here.

Sorry about that.

Comment 44

[Wan-Teh Chang]

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?

Comment 45

[jhp (no longer active)]

Comment on attachment 111872 [details] [diff] [review]
OK, really this is the patch

Looks good to me
r=pedemont

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 111872 [details] [diff] [review]
OK, really this is the patch

rs=dmose

Comment 47

[Mike Kaply [:mkaply]]

Fix checked in.