433791 - Win16 support should be deleted from NSS

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P4)

Description

[Julien Pierre]

Comment 1

[Julien Pierre]

Attachment: Remove win16 support from NSS

Comment 2

[Serge Gautherie (:sgautherie)]

wtc, ping ?

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) </dbm/>

(uncompiled, but trivial removals)

Comment 4

[Wan-Teh Chang]

Comment on attachment 342191 [details] [diff] [review]
(Bv1) </dbm/>

r=wtc.  Since we're not using Watcom's compilers any more,
perhaps we should just remove watcomfx.h from CVS, or make
it an empty file for backward compatibility.  See

http://mxr.mozilla.org/security/search?string=watcomfx.h

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) Remove </dbm/include/watcomfx.h> [Checkin: Comment 17+23]

Bv1, extended per comment 4: /dbm/ part.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Remove </security/nss/lib/util/watcomfx.h> [Checkin: See comment 19+28]

(small part of) Av1, extended per comment 4: /security/ part.

Comment 7

[Wan-Teh Chang]

Comment on attachment 343723 [details] [diff] [review]
(Bv2) Remove </dbm/include/watcomfx.h>
[Checkin: Comment 17+23]

r=wtc.  I didn't realize there are two copies of watcomfx.h in NSS:
one in mozilla/dbm/include and the other in mozilla/security/nss/lib/util.

mozilla/dbm/include/watcomfx.h is only used by mozilla/dbm internally, so
it is straightforward to remove it from CVS.

Comment 8

[Wan-Teh Chang]

Comment on attachment 343726 [details] [diff] [review]
(Cv1) Remove </security/nss/lib/util/watcomfx.h> 
[Checkin: See comment 19+28]

r=wtc.

Christophe, Kai, we don't support the Watcom compilers any more.
mozilla/security/nss/lib/util/watcomfx.h is included in the NSS
packages because secport.h includes it.  I don't think any NSS
user is including watcomfx.h directly, so it should be safe to
remove watcomfx.h from the NSS packages.

If you disagree, we'll take a more conservative approach (keep
watcomfx.h but make it an empty file).

Comment 9

[Serge Gautherie (:sgautherie)]

Wan-Teh, where should this be landed ? (Hg m-c 1.9.1, CVS mozilla 1.9.0, ...)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

The most upstream repository is CVS trunk.
All other repositories are down stream of that.

Comment 11

[Serge Gautherie (:sgautherie)]

Does it "need" to be checked in in both, or will the cvs update be automatically picked up ?

Comment 12

[Wan-Teh Chang]

Serge: this bug is a cosmetic issue.  We'll check in the patches on the NSS trunk
in CVS.  The patches will eventually propagate to mozilla-central in Hg.

Comment 13

[Christophe Ravel]

Comment on attachment 343726 [details] [diff] [review]
(Cv1) Remove </security/nss/lib/util/watcomfx.h> 
[Checkin: See comment 19+28]

Let me first check the impact of removing a file on our Sun patch audit system.

Comment 14

[Christophe Ravel]

I had the answer for removing a file from a Sun patch. It is doable but it is going to take me some time to implement. I can't spend this time right now.
Would you agree to make watcomfx.h an empty file for now and remove it later ?

Comment 15

[Wan-Teh Chang]

Yes, that's fine.

Comment 16

[Serge Gautherie (:sgautherie)]

Still 'checkin-needed'...

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 343723 [details] [diff] [review]
(Bv2) Remove </dbm/include/watcomfx.h>
[Checkin: Comment 17+23]

This patch committed on CVS trunk

include/watcomfx.h; new revision: 3.2; previous revision: 3.1
src/db.c;           new revision: 3.5; previous revision: 3.4
src/h_bigkey.c;     new revision: 3.14; previous revision: 3.13
src/h_func.c;       new revision: 3.3; previous revision: 3.2
src/h_log2.c;       new revision: 3.3; previous revision: 3.2
src/h_page.c;       new revision: 3.19; previous revision: 3.18
src/hash.c;         new revision: 3.25; previous revision: 3.24
src/hash_buf.c;     new revision: 3.10; previous revision: 3.9
src/memmove.c;      new revision: 3.6; previous revision: 3.5
src/mktemp.c;       new revision: 3.8; previous revision: 3.7
src/snprintf.c;     new revision: 3.10; previous revision: 3.9
src/strerror.c;     new revision: 3.4; previous revision: 3.3

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 343726 [details] [diff] [review]
(Cv1) Remove </security/nss/lib/util/watcomfx.h> 
[Checkin: See comment 19+28]

I checked in this patch, except
- I did not cvs remove watcom.fx, but just left it empty, and 
- I did not commit the change to nss/pkg/solaris/SUNWtlsd/prototype

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Oh, the files committed were:

cmd/tests/nonspr10.c;   new revision: 1.2; previous revision: 1.1
lib/util/manifest.mn;   new revision: 1.18; previous revision: 1.17
lib/util/secport.h;     new revision: 1.19; previous revision: 1.18
lib/util/watcomfx.h;    new revision: 1.3; previous revision: 1.2

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

I'm removing checkin-needed.  All the r+ patches are committed, but 
there remains one patch awaiting review.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> Would you agree to make watcomfx.h an empty file for now and remove it later ?

(In reply to comment #18)
> - I did not cvs remove watcom.fx, but just left it empty, and 
> - I did not commit the change to nss/pkg/solaris/SUNWtlsd/prototype

I filed (and updated) bug 480442.


(In reply to comment #20)
> there remains one patch awaiting review.

Julien, could you update it ?
Wan-Teh, could you review it ?

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 343726 [details] [diff] [review]
(Cv1) Remove </security/nss/lib/util/watcomfx.h> 
[Checkin: See comment 19+28]

The last component of this patch is to file
security/nss/pkg/solaris/SUNWtlsd/prototype
It can only be made after Christophe does the necessary 
to remove watcomfx.h from Solaris distros.  
Perhaps that part should be attached to the other bug 
that Serge has filed?

Comment 23

[Wan-Teh Chang]

Comment on attachment 343723 [details] [diff] [review]
(Bv2) Remove </dbm/include/watcomfx.h>
[Checkin: Comment 17+23]

mozilla/dbm/include/watcomfx.h can be removed from CVS.

Removing watcomfx.h;
/cvsroot/mozilla/dbm/include/watcomfx.h,v  <--  watcomfx.h
new revision: delete; previous revision: 3.2
done

Comment 24

[Wan-Teh Chang]

Comment on attachment 343726 [details] [diff] [review]
(Cv1) Remove </security/nss/lib/util/watcomfx.h> 
[Checkin: See comment 19+28]

We only need to keep mozilla/security/nss/lib/util/watcomfx.h
for mozilla/security/nss/pkg/solaris/SUNWtlsd/prototype.  In
fact, watcomfx.h may still need to be listed in the EXPORTS
variable in mozilla/security/nss/lib/util/manifest.mn for it
to be included in the Solaris SUNWtlsd package.

Comment 25

[Wan-Teh Chang]

Comment on attachment 336123 [details] [diff] [review]
Remove win16 support from NSS

I don't have time to review this patch.  Some quick comments:

1. The removal of coreconf/WIN16.mk is fine.

2. Don't bother fixing the files in nss/lib/jar.  We rarely
touch those files.  If we don't fix those files, the size of
this patch will shrink significantly.

3. Don't change nss/lib/softoken/pkcs11.h, which is based on
third-party code.  We should stay as close to the upstream
pkcs11.h from RSA Security as possible.

Comment 26

[Julien Pierre]

Serge,

Why did you obsolete my patch ? It has not been reviewed, and I don't see any other patch that addresses the issues in several of the source files that it touched.

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #26)

Your patch is still wanted, but (just) needs unbitrotting after check-ins and comments since then.

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 343726 [details] [diff] [review]
(Cv1) Remove </security/nss/lib/util/watcomfx.h> 
[Checkin: See comment 19+28]


(In reply to comment #24)
> We only need to keep mozilla/security/nss/lib/util/watcomfx.h
> for mozilla/security/nss/pkg/solaris/SUNWtlsd/prototype.  In

Now removed by bug 480442.

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 336123 [details] [diff] [review]
Remove win16 support from NSS

The coreconf part will be bug 438331 Hv1 patch.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) </security/*> other than /nss [Checkin: Comment 31]

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 366602 [details] [diff] [review]
(Dv1) </security/*> other than /nss
[Checkin: Comment 31]

r=nelson, committed on CVS trunk

Removing coreconf/WIN16.mk;      new revision: delete; previous revision: 1.5
Checking in coreconf/arch.mk;    new revision: 1.21; previous revision: 1.20
Checking in coreconf/headers.mk; new revision: 1.9; previous revision: 1.8
Checking in coreconf/tree.mk;    new revision: 1.10; previous revision: 1.9
Checking in dbm/src/config.mk;   new revision: 1.6; previous revision: 1.5

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

I will attempt to un-bitrot the remaining parts of Julien's original patch.
I *think* that's the only potentially remaining work for this bug.  
Agreed?

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #32)
> I will attempt to un-bitrot the remaining parts of Julien's original patch.
> I *think* that's the only potentially remaining work for this bug.  

Should be.

> Agreed?

(Fine by me.)

Comment 34

[Wan-Teh Chang]

Attachment: Fix the indentation and use WIN% [Checkin: Comment 34]

This patch fixed some nits in Serge Gautherie's patch (attachment 366602 [details] [diff] [review]).

We can replace the "WINNT WIN95 WINCE" list by WIN%.  The original
"list omits WIN16" was meant to explain why we couldn't use WIN%.
Now we can.

Checking in coreconf/arch.mk;
/cvsroot/mozilla/security/coreconf/arch.mk,v  <--  arch.mk
new revision: 1.22; previous revision: 1.21
done
Checking in dbm/src/config.mk;
/cvsroot/mozilla/security/dbm/src/config.mk,v  <--  config.mk
new revision: 1.7; previous revision: 1.6
done

Comment 35

[Julien Pierre]

Attachment: Updated patch [Checkin: Comment 40]

This is the remainder of the patch, brought up to the trunk, less libjar, which is having other changes by Nelson, so I didn't want to conflict.
I hope to get a more timely review than last time - the previous patch sat for over 6 months without any review.

Comment 36

[Julien Pierre]

Thanks for the quick review, Nelson. I will wait for the tree to reopen to commit this patch.

Comment 37

[Wan-Teh Chang]

Comment on attachment 371354 [details] [diff] [review]
Updated patch
[Checkin: Comment 40]

Julien, please remove the changes to lib/util/pkcs11.h from this
patch, because it is based on a third-party file:
ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-11/v2-20/pkcs11.h

You may want to keep the original comments in lib/base/nssbaset.h.
It is useful to know the why the PR_EXTERN and PR_IMPLEMENT macros
were defined to take the type as an argument.  Your change would
make the explanation more vague.

Comment 38

[Julien Pierre]

Wan-Teh,

The comment I changed about "NSPR" but not PR_EXTERN or PR_IMPLEMENT. I'm not really sure if the history matters much at this point. Does NSPR still support WIN16 ?

Re: PKCS#11, I was trying to eliminate every instance of the word WIN16 . If we don't want to modify the PKCS#11 header files, then I probably have to also undo the trademark change to include WIN16 again.

I wonder if we should go to the source and get the PKCS#11 committee to drop WIN16 also. That's been dead a long time.

Comment 39

[Wan-Teh Chang]

It's irrelevant whether NSPR still supports WIN16 now.
If we want to explain why PR_EXTERN and PR_IMPLEMENT
were defined to take type as an argument, our explanation
can certainly mention the historical fact that NSPR
supported WIN16.

It is not necessary to eradicate WIN16 from our source
code because it doesn't cause confusion.  This is
very different from the obsolete XP_MAC code, which
is easily confused with the code for Mac OS X.

In any case I don't want to waste too much of your time
on this, so feel free to do whatever you think is right.

Comment 40

[Julien Pierre]

Comment on attachment 371354 [details] [diff] [review]
Updated patch
[Checkin: Comment 40]

Checking in trademarks.txt;
/cvsroot/mozilla/security/nss/trademarks.txt,v  <--  trademarks.txt
new revision: 1.2; previous revision: 1.1
done
Checking in cmd/modutil/pk11jar.html;
/cvsroot/mozilla/security/nss/cmd/modutil/pk11jar.html,v  <--  pk11jar.html
new revision: 1.3; previous revision: 1.2
done
Checking in lib/base/nssbaset.h;
/cvsroot/mozilla/security/nss/lib/base/nssbaset.h,v  <--  nssbaset.h
new revision: 1.8; previous revision: 1.7
done
Checking in lib/freebl/Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.104; previous revision: 1.103
done
Checking in lib/util/pkcs11.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11.h,v  <--  pkcs11.h
new revision: 1.5; previous revision: 1.4
done
Checking in lib/util/secasn1d.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1d.c,v  <--  secasn1d.c
new revision: 1.39; previous revision: 1.38
done
Checking in lib/util/secport.h;
/cvsroot/mozilla/security/nss/lib/util/secport.h,v  <--  secport.h
new revision: 1.20; previous revision: 1.19
done

Comment 41

[Serge Gautherie (:sgautherie)]

V.Fixed