Closed Bug 180293 Opened 22 years ago Closed 22 years ago

OpenVMS build -> GNV: NSPR build Changes

Categories

(NSPR :: NSPR, defect, P1)

DEC
OpenVMS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: colin, Assigned: wtc)

References

Details

Attachments

(2 files, 3 obsolete files)

This bug report will track the NSPR build changes with the "move OpenVMS build
to GNV" work (meta-bug 180288).
Attached patch NSPR build changes (obsolete) — Splinter Review
I also have three new files which reside in a new directory:

nsprpub/build/unix
nsprpub/build/unix/libnspr4_symvec.opt
nsprpub/build/unix/libplc4_symvec.opt
nsprpub/build/unix/libplds4_symvec.opt

Wan-Teh, should I just post these files as attachments for review? 

Is there any other work involved in adding new directories and files to the
repository?
Blocks: vmsgnv
No longer blocks: 180290
Keywords: review
Priority: -- → P1
Attachment #106354 - Flags: review?(wtc)
This is blocking my work on Mozilla 1.3. Setting severity field according.
Severity: normal → blocker
Comment on attachment 106354 [details] [diff] [review]
NSPR build changes

Colin,

Sorry about the late code review.

You can either attach the new files to this bug or generate a
new patch with "cvs diff -uN".	The -N flag for cvs diff includes
new files in the patch.

Below I will give a lot of comments.  I am tentatively marking
this patch rejected.

>Index: nsprpub/configure.in
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/configure.in,v
>retrieving revision 1.83.2.39
>diff -u -r1.83.2.39 configure.in
>--- nsprpub/configure.in	6 Nov 2002 00:54:49 -0000	1.83.2.39
>+++ nsprpub/configure.in	15 Nov 2002 12:16:37 -0000
>@@ -541,8 +541,8 @@
>     OS_RELEASE=`echo $OS_RELEASE | awk -F\. '{ print $1 "." $2 }'`
> fi
> 
>-if test "$OS_ARCH" = "POSIX_for_OpenVMS_AXP"; then
>-    OS_ARCH=OpenVMS
>+if test "$OS_ARCH" = "OpenVMS"; then
>+    OS_RELEASE=
> fi

This can be simply deleted.  It is not necessary to unset OS_RELEASE.

>@@ -1473,24 +1473,16 @@
> 
> *-openvms*)
>     AC_DEFINE(XP_UNIX)
>-    AC_DEFINE(VMS)

We probably can't remove the VMS macro define on the compile command
line.  Files that include the "prcpucfg.h" header will have VMS defined,
but files like nsprpub/config/nsinstall.c, which do not include any
NSPR headers, won't have VMS defined, and I noticed that you are adding
code to nsprpub/config/nsinstall.c that depends on the VMS macro.

>-    _HAVE_PTHREADS=1

If you remove this, you will be relying on the configure script to
detect whether OpenVMS has the pthreads library.  Is this your
intention?

>-    HOST_CC=c89
>-    HOST_CXX=cxx
>-    HOST_CFLAGS=-O
>-    HOST_CXXFLAGS=-O
>-    CC=ccc
>-    CXX=ccc
>-    CFLAGS="$CFLAGS -Wc,names=\(short,as\)"
>-    CXXFLAGS="$CXXFLAGS -Wc,names=\(short,as\)"

It seems that you are not setting CC and CXX.  Is the default value
acceptable?  I think it is a good idea to set CC and CXX explicitly.
If you want to do that, you should set them in the sections marked
with the comments "Set the default C compiler" and
"Set the default C++ compiler".

>+    DSO_LDOPTS='-shared -auto_symvec $(LDFLAGS) $(OPTIMIZER)'
>+    if test -n "$MOZ_DEBUG"; then
>+      OPTIMIZER=$_DEBUG_FLAGS
>+    else
>+      OPTIMIZER=$_OPTIMIZE_FLAGS
>+    fi

I think it is better to set DSO_LDOPTS this way and avoid the use
of OPTIMIZER:

>+    DSO_LDOPTS='-shared -auto_symvec $(LDFLAGS)'
>+    if test -n "$MOZ_DEBUG"; then
>+      DSO_LDOPTS="$DSO_LDOPTS $_DEBUG_FLAGS"
>+    else
>+      DSO_LDOPTS="$DSO_LDOPTS $_OPTIMIZE_FLAGS"
>+    fi 

>Index: nsprpub/config/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/config/Makefile.in,v
>retrieving revision 1.12.2.1
>diff -u -r1.12.2.1 Makefile.in
>--- nsprpub/config/Makefile.in	13 Jun 2002 22:36:20 -0000	1.12.2.1
>+++ nsprpub/config/Makefile.in	15 Nov 2002 12:16:38 -0000
>@@ -94,6 +94,10 @@
>     endif
> endif
> 
>+ifeq ($(OS_ARCH), OpenVMS)
>+    XLDOPTS += $(LDFLAGS)
>+endif

Why do you need this?  What LDFLAGS are you passing to
configure on the command line?

>Index: nsprpub/config/OpenVMS.mk

This file is obsolete.	You don't need to change it.

>Index: nsprpub/config/nsinstall.c
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/config/nsinstall.c,v
>retrieving revision 3.17.2.1
>diff -u -r3.17.2.1 nsinstall.c
>--- nsprpub/config/nsinstall.c	26 Oct 2001 04:53:57 -0000	3.17.2.1
>+++ nsprpub/config/nsinstall.c	15 Nov 2002 12:16:38 -0000
>@@ -365,6 +365,7 @@
> 
> 	    if (ftruncate(tofd, sb.st_size) < 0)
> 		fail("cannot truncate %s", toname);
>+#if !defined(VMS)
> 	    if (dotimes) {
> 		utb.actime = sb.st_atime;
> 		utb.modtime = sb.st_mtime;
>@@ -377,6 +378,7 @@
> 	    if (chmod(toname, mode) < 0)
> #endif
> 		fail("cannot change mode of %s", toname);
>+#endif /* !defined(VMS) */
> 	    if ((owner || group) && fchown(tofd, uid, gid) < 0)
> 		fail("cannot change owner of %s", toname);
> 
>@@ -384,6 +386,16 @@
> 	    if (close(tofd) < 0)
> 		fail("cannot write to %s", toname);
> 	    close(fromfd);
>+#if defined(VMS)
>+	    if (chmod(toname, mode) < 0)
>+		fail("cannot change mode of %s", toname);
>+	    if (dotimes) {
>+		utb.actime = sb.st_atime;
>+		utb.modtime = sb.st_mtime;
>+		if (utime(toname, &utb) < 0)
>+		    fail("cannot set times of %s", toname);
>+	    }
>+#endif /* defined(VMS) */
> 	}
> 
> 	free(toname);

It seems that all the changes in this section can be replaced by
undefining HAVE_FCHMOD for VMS.  Look for BEOS in this file as an
example.

Why aren't ino2name and reversepath ported to OpenVMS?	Is it
because the dirent structure does not have the d_ino field?

Note that all the changes you made to this file require -DVMS on
the compile command line.

>Index: nsprpub/config/rules.mk

I suggest that you add the *_symvec.opt files to nsprpub/pr/src,
nsprpub/lib/ds, and nsprpub/lib/libc/src, so that they don't need
to be copied from nsprpub/build/unix/vms during the build.

Does the linker automatically look for *_symvec.opt in the same
directory?
Attachment #106354 - Flags: review?(wtc) → review-
Chris, I'd appreciate your comments on Colin's patch.
Target Milestone: --- → 4.3
Wan-Teh,

Thanks for your comments. Please read my comments (prefixed CRB) and if you
don't have anything further I'll produce another patch.

Colin.

>Index: nsprpub/configure.in
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/configure.in,v
>retrieving revision 1.83.2.39
>diff -u -r1.83.2.39 configure.in
>--- nsprpub/configure.in	6 Nov 2002 00:54:49 -0000	1.83.2.39
>+++ nsprpub/configure.in	15 Nov 2002 12:16:37 -0000
>@@ -541,8 +541,8 @@
>     OS_RELEASE=`echo $OS_RELEASE | awk -F\. '{ print $1 "." $2 }'`
> fi
> 
>-if test "$OS_ARCH" = "POSIX_for_OpenVMS_AXP"; then
>-    OS_ARCH=OpenVMS
>+if test "$OS_ARCH" = "OpenVMS"; then
>+    OS_RELEASE=
> fi

This can be simply deleted.  It is not necessary to unset OS_RELEASE.

CRB: Oh but it is. If I don't, then I get the default of `name -r`,
CRB: and I don't want that!

>@@ -1473,24 +1473,16 @@
> 
> *-openvms*)
>     AC_DEFINE(XP_UNIX)
>-    AC_DEFINE(VMS)

We probably can't remove the VMS macro define on the compile command
line.  Files that include the "prcpucfg.h" header will have VMS defined,
but files like nsprpub/config/nsinstall.c, which do not include any
NSPR headers, won't have VMS defined, and I noticed that you are adding
code to nsprpub/config/nsinstall.c that depends on the VMS macro.

CRB: I have -DVMS=1 in my CFLAGS now. I did this to make sure that its
CRB: defined on ALL compile lines (was easier than having to put it into
CRB: several configure.in files). This requirement will be documented on
CRB: the OpenVMS build page.
CRB:
CRB: I can leave it in if you realy want, but I'd rather remove it since
CRB: its superfluous.

>-    _HAVE_PTHREADS=1

If you remove this, you will be relying on the configure script to
detect whether OpenVMS has the pthreads library.  Is this your
intention?

CRB: Yes, the test works in the new OpenVMS build environment.

>-    HOST_CC=c89
>-    HOST_CXX=cxx
>-    HOST_CFLAGS=-O
>-    HOST_CXXFLAGS=-O
>-    CC=ccc
>-    CXX=ccc
>-    CFLAGS="$CFLAGS -Wc,names=\(short,as\)"
>-    CXXFLAGS="$CXXFLAGS -Wc,names=\(short,as\)"

It seems that you are not setting CC and CXX.  Is the default value
acceptable?  I think it is a good idea to set CC and CXX explicitly.
If you want to do that, you should set them in the sections marked
with the comments "Set the default C compiler" and
"Set the default C++ compiler".

CRB: The defaults work just fine in the new build environment. I
CRB: prefer to only define what's needed.

>+    DSO_LDOPTS='-shared -auto_symvec $(LDFLAGS) $(OPTIMIZER)'
>+    if test -n "$MOZ_DEBUG"; then
>+      OPTIMIZER=$_DEBUG_FLAGS
>+    else
>+      OPTIMIZER=$_OPTIMIZE_FLAGS
>+    fi

I think it is better to set DSO_LDOPTS this way and avoid the use
of OPTIMIZER:

>+    DSO_LDOPTS='-shared -auto_symvec $(LDFLAGS)'
>+    if test -n "$MOZ_DEBUG"; then
>+      DSO_LDOPTS="$DSO_LDOPTS $_DEBUG_FLAGS"
>+    else
>+      DSO_LDOPTS="$DSO_LDOPTS $_OPTIMIZE_FLAGS"
>+    fi 

CRB: I agree. Will change.

>Index: nsprpub/config/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/config/Makefile.in,v
>retrieving revision 1.12.2.1
>diff -u -r1.12.2.1 Makefile.in
>--- nsprpub/config/Makefile.in	13 Jun 2002 22:36:20 -0000	1.12.2.1
>+++ nsprpub/config/Makefile.in	15 Nov 2002 12:16:38 -0000
>@@ -94,6 +94,10 @@
>     endif
> endif
> 
>+ifeq ($(OS_ARCH), OpenVMS)
>+    XLDOPTS += $(LDFLAGS)
>+endif

Why do you need this?  What LDFLAGS are you passing to
configure on the command line?

CRB: Some stuff which is build-environment specific is defined via LDFLAGS.

>Index: nsprpub/config/OpenVMS.mk

This file is obsolete.	You don't need to change it.

CRB: Agreed. (Its only showing up as a diff because I'd made changes to it
CRB: before I realised that its not used any more.)

>Index: nsprpub/config/nsinstall.c
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/config/nsinstall.c,v
>retrieving revision 3.17.2.1
>diff -u -r3.17.2.1 nsinstall.c
>--- nsprpub/config/nsinstall.c	26 Oct 2001 04:53:57 -0000	3.17.2.1
>+++ nsprpub/config/nsinstall.c	15 Nov 2002 12:16:38 -0000
>@@ -365,6 +365,7 @@
> 
> 	    if (ftruncate(tofd, sb.st_size) < 0)
> 		fail("cannot truncate %s", toname);
>+#if !defined(VMS)
> 	    if (dotimes) {
> 		utb.actime = sb.st_atime;
> 		utb.modtime = sb.st_mtime;
>@@ -377,6 +378,7 @@
> 	    if (chmod(toname, mode) < 0)
> #endif
> 		fail("cannot change mode of %s", toname);
>+#endif /* !defined(VMS) */
> 	    if ((owner || group) && fchown(tofd, uid, gid) < 0)
> 		fail("cannot change owner of %s", toname);
> 
>@@ -384,6 +386,16 @@
> 	    if (close(tofd) < 0)
> 		fail("cannot write to %s", toname);
> 	    close(fromfd);
>+#if defined(VMS)
>+	    if (chmod(toname, mode) < 0)
>+		fail("cannot change mode of %s", toname);
>+	    if (dotimes) {
>+		utb.actime = sb.st_atime;
>+		utb.modtime = sb.st_mtime;
>+		if (utime(toname, &utb) < 0)
>+		    fail("cannot set times of %s", toname);
>+	    }
>+#endif /* defined(VMS) */
> 	}
> 
> 	free(toname);

It seems that all the changes in this section can be replaced by
undefining HAVE_FCHMOD for VMS.  Look for BEOS in this file as an
example.

CRB: No, not at all!! There are two problems that I am fixing.
CRB: First, I need to do the chmod AFTER the file is closed,
CRB: otherwise it fails because the file is open.
CRB: Second, I need to do the utime last because a chmod on
CRB: OpenVMS will update the file's timestamps.

Why aren't ino2name and reversepath ported to OpenVMS?	Is it
because the dirent structure does not have the d_ino field?

CRB: No symbolic link support on OpenVMS. NSDISTMODE has to be
CRB: COPY for OpenVMS. Hence the code is never needed.

Note that all the changes you made to this file require -DVMS on
the compile command line.

CRB: Yes, and I have that via CFLAGS (see above) :-)

>Index: nsprpub/config/rules.mk

I suggest that you add the *_symvec.opt files to nsprpub/pr/src,
nsprpub/lib/ds, and nsprpub/lib/libc/src, so that they don't need
to be copied from nsprpub/build/unix/vms during the build.

CRB: I prefer them where I put them, if that's OK with you, because
CRB: (a) I find it easier if they're all in one place, and (b) that's
CRB: how I've done it for Mozilla :-)

Does the linker automatically look for *_symvec.opt in the same
directory?

CRB: Yes.
Wan-Teh,

If you could respond to my comments on your initial reviews, I'll update and
repost the patch.
Colin:

1. What is wrong with defining OS_RELEASE as 'uname -r'
on OpenVMS?  Does it break the build or you just don't
like it?

2. Please leave the AC_DEFINE(VMS) in nsprpub/configure.in.

3. Please define CC and CXX explicitly even if the values
are the same as the default.

4. You said:
> Some stuff which is build-environment specific is defined
> via LDFLAGS

Why can't this be defined in the configure.in script or
some makefile?  Is it because this will be different for
each build machine?

Could you give an example of this build-environment stuff?

5. In nsinstall.c, I suggest we do things in this order
to avoid the VMS ifdef's.

    ftruncate
#ifdef HAVE_FCHMOD
    fchmod
#endif
    fchown
    close
    /*
     * On OpenVMS chmod can't be done while the file
     * is open.
     */
#ifndef HAVE_FCHMOD
    chmod
#endif
    utime

We should ask Brendan Eich to review the nsinstall.c
changes when they are ready.

6. Are the ino2name and reversepath changes necessary?
They are not in your patch for NSS's
mozilla/security/coreconf/nsinstall.

7. I prefer that the *_symvec.opt files be in nsprpub/pr/src,
nsprpub/lib/ds, and nsprpub/lib/libc/src.
> 1. What is wrong with defining OS_RELEASE as 'uname -r'
> on OpenVMS?  Does it break the build or you just don't
> like it?

It doesn't work. It just returns the string "0".

> 2. Please leave the AC_DEFINE(VMS) in nsprpub/configure.in.

Will do.

> 3. Please define CC and CXX explicitly even if the values
> are the same as the default.

Will do.

> 4. You said:
> > Some stuff which is build-environment specific is defined
> > via LDFLAGS
>
> Why can't this be defined in the configure.in script or
> some makefile?  Is it because this will be different for
> each build machine?

Yes.

> Could you give an example of this build-environment stuff?

-Wl,repository=moz_objroot:[vms_cxx]
This tells the OpenVMS linker where the repository (for template instantiations)
is for this project. There is a corresponding definition in CXXFLAGS.

/vms_jackets_root/lib/vms_jackets.opt
The "jackets" are the OpenVMS Porting Library routines, a pre-requisite for
building Mozilla on OpenVMS. This definition tells the Mozilla build where the
jackets are installed on this system (there is no equivalent of /usr/local/bin
or anything like that).

> 5. In nsinstall.c, I suggest we do things in this order
> to avoid the VMS ifdef's.
>
>    ftruncate
>    fchmod
>    fchown
>    close
>    chmod
>    utime

That's fine by me. I didn't know if everyone else could live with this setup.
I'll change the patch.

> We should ask Brendan Eich to review the nsinstall.c
> changes when they are ready.

OK.

> 6. Are the ino2name and reversepath changes necessary?
> They are not in your patch for NSS's
> mozilla/security/coreconf/nsinstall.

Good catch about the changes not being in NSS. Technically, no, the changes are
not necessary since we don't ever use that code. In NSPR I was trying to make it
obvious that these routines are not used on OpenVMS. I forgot to do the same for
NSS. Its your call if I change both or neither (I would prefer both since
documentation is good (plus it clears up some compiler warnings)).

> 7. I prefer that the *_symvec.opt files be in nsprpub/pr/src,
> nsprpub/lib/ds, and nsprpub/lib/libc/src.

Then I shall change it.
Attached patch Revised patch (obsolete) — Splinter Review
Incorporates all the comments.
Attachment #106354 - Attachment is obsolete: true
Attachment #108747 - Flags: review?(wtc)
Brendan, Wan-Teh asked me to ask you to review the nsinstall.c change in the
latest attachment (id=108747)
Comment on attachment 108747 [details] [diff] [review]
Revised patch

If (owner || group), the "to" file will be chowned, possibly to an owner whose
files the current process can then no longer modify via chmod or utime.

Isn't this a problem on OpenVMS?  If not, it will be on Unixes.

/be
Attachment #108747 - Flags: superreview-
Then we have a problem, because I have to utime last, since all the other
operations change the date. 
Who uses the -o and -g options, anyway?
I'm not sure who uses -g and -o, but if only VMS has to utime last, could you
ifdef VMS just that utime-calling if-then block movement?  Thanks,

/be
> I'm not sure who uses -g and -o, but if only VMS has to utime last, could you
> ifdef VMS just that utime-calling if-then block movement? 

Well, yeah. That's the way I originally had it and Wan-Teh asked me to change it :-)
Does OpenVMS really let me change your files' mode and times?  If not, could you
also make the -g and -o options fail when used on OpenVMS?  Thanks.

/be
> Does OpenVMS really let me change your files' mode and times?  If not, could you
> also make the -g and -o options fail when used on OpenVMS? 

Yep. 

I think I need to ifdef it for OpenVMS, so that the order is:

  ftruncate
  fchown
  close
  chmod
  utime

That means it'll be possible to shoot yourself in the foot by changing the owner
or protection so that you no longer have permission to change the date/time, but
that's about as good as it gets, I'm afraid. And in reality -o/-g aren't used
anyways.

How does that sound?
Sorry for coming in late here.  The revised build changes look fine to me.  

Wan-Teh, the issue with the LDFLAGS & nsinstall is the same as the one we hit
and never resolved in bug 88283.  I'm still not sure why nsinstall has a custom
local build rule but we should just make it use $(LDFLAGS) unconditionally.
Attached patch New nsinstall.c patch (obsolete) — Splinter Review
This puts the non-VMS code back to how it was, and does it in the VMS-required
order just for VMS. Is this better?
Comment on attachment 108774 [details] [diff] [review]
New nsinstall.c patch

>+#ifdef VMS
>+	    /* On OpenVMS we can't chmod() until the file is closed, and we */
>+	    /* have to utime() last since fchown/chmod alter the timestamps */

Nit: use the prevailing major (multiline) comment style.

Why over-ifdef and duplicate so much code?  Can't you just ifdef the
utime-calling if-then paragraph to be before for non-VMS, after for VMS, and
use the HAVE_FCHMOD ifdef as wtc suggested in between?

I'm sorry to be so picky, but I have to be.  Nits are not a big deal, the main
point here is to avoid duplicating code gratuitously.

/be
And (just to double-check my understanding), can you *really* chown a file to
some other user, then chmod and touch it to change its mode and times?

/be
> Why over-ifdef and duplicate so much code?  Can't you just ifdef the
> utime-calling if-then paragraph to be before for non-VMS, after for VMS, and
> use the HAVE_FCHMOD ifdef as wtc suggested in between?

I can, but then I'm moving the chmod for all platforms, not just OpenVMS. And I
didn't think that was a desired side-effect.
> And (just to double-check my understanding), can you *really* chown a file to
> some other user, then chmod and touch it to change its mode and times?

It all depends on who you chown the file to, what the current protection is, and
what your current set of privileges are. Most of the time, if you can make the
owner of the file someone else, then you're going to be able to diddle with it
afterwards, yes.
Argh, you're right -- the HAVE_FCHOWN ifdef is used by other platforms, of course.

How about just minimizing duplicated code, then?

/be
> How about just minimizing duplicated code, then?

OK, I'll just move the fchmod/chmod and utime code then.
How about this one?
Attachment #108774 - Attachment is obsolete: true
Comment on attachment 108779 [details] [diff] [review]
Another nsinstall.c patch

r=brendan@mozilla.org, delegated by wtc@netscape.com -- drivers ought to a=
this now, requesting 1.3a approval.

/be
Attachment #108779 - Flags: review+
Attachment #108779 - Flags: approval1.3a?
Comment on attachment 108747 [details] [diff] [review]
Revised patch

1. In mozilla/nsprpub/configure.in, please add a comment
explaining why you want to unset OS_RELEASE.

Is there any other options of the uname command that
return a useful version number?  For example, uname -v?

2. Could you use "cvs diff -uN" to generate the diff?

3. Could you explain the issues cause by my removing
PR_ResumeSet, PR_SuspendSet, etc. in NSPR 4.2?
Attachment #108747 - Flags: review?(wtc) → review-
Colin, I found that in NSS's security/coreconf/arch.mk
we are using 'uname -v' as the OS_RELEASE for OpenVMS.
I guess we can use the same in NSPR.
Comment on attachment 108779 [details] [diff] [review]
Another nsinstall.c patch

a=asa for checkin to 1.3a
Attachment #108779 - Flags: approval1.3a? → approval1.3a+
> Is there any other options of the uname command that
> return a useful version number?  For example, uname -v?

Does OS_RELEASE get used in any directory names, or anything like that? Or is it
just "available" for any future tests that may be required?

> 2. Could you use "cvs diff -uN" to generate the diff?

No. The "-N" option doesn't seem to have any effect on our cvs client!! I'm
using cvs version 1.11.

> 3. Could you explain the issues cause by my removing
> PR_ResumeSet, PR_SuspendSet, etc. in NSPR 4.2?

Shareable images have transfer vectors. When an image links against a shareable
image it remembers the offsets in the transfer vector for each routine it needs
to call. The order of the transfer vector in the shareable image can therefore
never change, otherwise any which linked against it will have to itself be
relinked. So when you removed those three routines in 4.2, I couldn't remove
their entries in the transfer vector, so I had to fill them with "something",
just to keep all the following entries in the transfer vector at the same offset. 

You see what I mean, or do you need more info?
You have to 'cvs add' the file first (or manually edit CVS/Entries) before 'cvs
diff -N' will have any effect.
OS_RELEASE is available for any future tests that may be required.
Its value is substituted in mozilla/nsprpub/config/autoconf.mk.in,
so NSPR's makefiles may use it.

Thanks for explaining the transfer vectors of shareable images.
So you have existing binaries that depend on the order of the
transfer vector in NSPR?  If so, I suggest that we fix this
by defining stubs of those deleted functions (which we can
ifdef with VMS) instead of exporting three internal functions
in their place.  Otherwise, I won't be able to remove those
three internal functions in the future.

The stubs for those three removed functions should be defined
in nsprpub/pr/src/pthreads/ptthread.c.
> The stubs for those three removed functions should be defined
> in nsprpub/pr/src/pthreads/ptthread.c.

I'm a little confused now. Mozilla is using NSPRPUB_PRE_4_2_CLIENT_BRANCH,
right? In there the three routines PR_ResumeSet, PR_ResumeTest, and
PR_SuspendAllSuspended are all still present, so I don't need to remove them
from my symbol vector. Yet.

I guess at some point I must have (accidently?) picked up a later version of
NSPR where those routines were missing.

Is Mozilla 1.3 is really shipping using NSPRPUB_PRE_4_2_CLIENT_BRANCH, I will
put those three entries back into my symbol vector, and then Wan-Teh can
(hopefully) stub those three routines for VMS on the NSPR "mainline". Then when
Mozilla picks up a later version of NSPR, I'll be all set.

How's that sound?
Colin:

PR_ResumeSet, PR_ResumeTest, and PR_SuspendAllSuspended
have been turned into static functions and renamed
pt_ResumeSet, pt_ResumeTest, and pt_SuspendAllSuspended,
with the pt_ prefix.

I can arrange so that they are still exported and have
the old names for OpenVMS.

Do you really have existing binaries depending on the old
NSPR symbol vector that you need to support?  Is it
possible to rebuild those binaries against the new NSPR
symbol vector?
> Do you really have existing binaries depending on the old
> NSPR symbol vector that you need to support?  Is it
> possible to rebuild those binaries against the new NSPR
> symbol vector?

That's strange. I could have sworn I replied to this yesterday. Anyway, yes, I
do have an existing binary, its the Java plugin for Mozilla (so blame Sun!). The
Java plugin uses NSPR, and because the Java plugin is part of the Java SDK, its
not something I have control over (a different group at HP does all the Java
work, controls releases, etc.).
Attached patch Patch v3Splinter Review
Colin, I've checked this patch in.  Please review
it and do a test build.

1. In nsprpub/configure.in, I set OS_RELEASE to
`uname -v`.

2. In nsprpub/config/rules.mk, I changed the definition
of VMS_SYMVEC_FILE_MODULE so that the _symvec.opt files
in the *source tree* are plc_symvec.opt, plds_symvec.opt,
and nspr_symvec.opt.  This is because I don't want the
library version (4) in their names.  VMS_SYMVEC_FILE is
unchanged, so in the *build tree* we still have
libplc4_symvec.opt, libplds4_symvec.opt, and
libnspr4_symvec.opt, which I understand are the names the
linker automatically looks for.

3. The changes for nsprpub/config/Makefile.in and
nsprpub/config/nsinstall.c are not in this patch because
they have already been checked in.

4. I added the stubs PR_VMS_Stub1/2/3 in
nsprpub/pr/src/md/unix/openvms.c and use them in the
fixed section of the symbol table in nspr_symvec.opt.
Attachment #108747 - Attachment is obsolete: true
All the patches (including the nsprpub/config/Makefile.in
patch in bug 88283) have been checked into the trunk and
NSPRPUB_PRE_4_2_CLIENT_BRANCH of NSPR.

The remaining work is for Colin to review and test patch
v3 (attachment 109072 [details] [diff] [review]).
Status: NEW → ASSIGNED
I'll pull the new code now and try building. Will report back here.
Builds fine and runs. Looks good. Thanks Wan-Teh.
Marked the bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 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: