654611 - Remove WinCE code from NSS

Status: RESOLVED FIXED

(NSS :: Build, defect, P3)

Description

[Ed Morley [:emorley]]

WinCE & Windows Mobile code is being slowly removed by bug 614720 - since it's broken, no longer supported, and only serves to complicate code maintenance. 

This bug is for the NSS parts of the removal:
http://mxr.mozilla.org/mozilla-central/search?string=wince&find=/security/nss/

It's likely some of the instances of WinCE code are in NSS libraries that are taken directly from upstream, in which case they'll obviously not be able to be touched. However, I think it's just going to be easier for me to attach the whole patch I have (apart from sqlite changes, since they were vast and definitely from upstream) - and we can work from there, reducing the scope down as needed.

Comment 1

[Ed Morley [:emorley]]

Attachment: Remove WinCE code from NSS

Removes WinCE/_WIN32_WCE ifdefs, the now redundant DBM_USING_NSPR defines and hg removes security/nss/cmd/lib/wincemain.c

Comment 2

[Ed Morley [:emorley]]

Comment on attachment 529893 [details] [diff] [review]
Remove WinCE code from NSS

Passed try: http://dev.philringnalda.com/tbpl/?tree=Try&rev=357126c2abd1

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 529893 [details] [diff] [review]
Remove WinCE code from NSS

> # can't do this in manifest.mn because OS_ARCH isn't defined there.
>-ifeq (,$(filter-out WINNT WINCE,$(OS_ARCH))) 
>+ifeq ($(OS_ARCH),WINNT)

I don't know if this is right.

I personally would have kept the more complicated syntax, if that's already proven to work.

Please, 
(a) either attach an updated patch that keeps the original syntax and filter just WINNT (needs less reviewing time),

(b) or find an additional reviewer, only for this one change, who can confirm this change is correct.

r=kaie if you address this

Comment 4

[Ed Morley [:emorley]]

Comment on attachment 529893 [details] [diff] [review]
Remove WinCE code from NSS

Ted, please can you confirm the |ifeq filter-out| to |ifeq| conversion in comment 3. Thanks!

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Ed, 
Mozilla's CVS repository _IS_ the "upstream" for NSS.  
We've been trying very hard to avoid divergence between upstream and 
Firefox's downstream copy, since (last I knew) there was no one to maintain
FF's downstream copy.  So, are you proposing a change to the CVS repository?
Or are you proposing to fork Firefox's copy?

Comment 6

[Ed Morley [:emorley]]

I'm proposing a change to upstream NSS. If CVS is where that development takes place, then against CVS rather than m-c.

By "upstream libraries" in comment 0; I was referring to what I thought were additional libraries taken from elsewhere, inside the libs folder of NSS. (eg the sqlite code).

On a related note, is there any documentation anywhere explaining how the NSS CVS -> m-c NSS merge works / how to submit patches against NSS? ie like nanojit have done here: https://developer.mozilla.org/en/NanojitMerge

The closest I could find for NSS was this page:
http://www.mozilla.org/projects/security/pki/nss/
...but it didn't even give the CVS URL - and looked fairly out of date (like many of the MDN pages that talk about CVS instead of Mercurial), so I had (incorrectly) presumed that m-c's hg repo was the correct place to base patches against.

Comment 7

[Ed Morley [:emorley]]

I should just add, that obviously if NSS still needs to support WinCE, then wontfixing this is fine with me. 

This bug was filed purely as a continuation of the m-c WinCE removal that I've been doing in bug 614720 and it's dependants - since I thought it worth seeing if NSS still needed WinCE support, or if the cleanup would be appreciated :-)

Comment 8

[Kai Engert [:KaiE:]]

The following has been discussed in today's NSS conference call:

- in theory, there might be additional users of this code
- we don't want to remove it if there is still someone using it
- in order to find out, a message will be sent to the dev-crypto newsgroup

- if someone actively uses it, we'll keep it
- if not, we'll remove it at some later time.

Which means, despite having a r+ patch,
please don't check in yet.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Is project Fennec dead?

Comment 10

[Ed Morley [:emorley]]

Fennec now refers to the Maemo and Android mobile work (https://wiki.mozilla.org/Mobile/Fennec), so Fennec itself isn't dead.

What is being discussed here is WinCE/Windows Mobile platform support, which was the initial platform for Fennec, but is no longer supported by it.

tl;dr WinCE/Windows Mobile code is already being removed from the rest of mozilla-central, see the 38 dependant bugs of bug 614720 which cover it's removal.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 529893 [details] [diff] [review]
Remove WinCE code from NSS

I'm not an NSS peer, you've got several CCed here, so I think you've got it covered.

Comment 12

[(not currently active) Ted Mielczarek]

Oh, oops, I totally missed your comment. The filter-out -> ifeq replacement from comment 3 is correct, but sounds like you need to sort out some other issues here first.

Comment 13

[Ed Morley [:emorley]]

Thanks Ted :-)

Comment 14

[Ed Morley [:emorley]]

> - in theory, there might be additional users of this code
> - we don't want to remove it if there is still someone using it
> - in order to find out, a message will be sent to the dev-crypto newsgroup

For reference: 
https://groups.google.com/d/topic/mozilla.dev.tech.crypto/Fb7BrL5m9y0/discussion

Comment 15

[Ed Morley [:emorley]]

It's now been two months since the newsgroup posting asking if there were any problems with dropping WinCE support (to which there were no replies).

Can this now proceed?

Comment 16

[Ed Morley [:emorley]]

Kai, can this be brought up during the next NSS call please :-)

Comment 17

[Wan-Teh Chang]

Ed: thank you for your patch.  Removing the WinCE code is
low priority.  If you need this bug fixed in order to close
bug 614720, please remove the dependency between these two
bugs.

Comment 18

[Ed Morley [:emorley]]

(In reply to Wan-Teh Chang from comment #17)
> Ed: thank you for your patch.  Removing the WinCE code is
> low priority.  

I agree this is low priority, however since I've already created the patch I'm curious to know what the long pole is on this? 

ie: If it's just that the patch isn't based off of the CVS/has rotted, then I'm happy to sort that out myself if it helps expedite getting this patch off my open-assigned bug list. If there is another reason (eg WinCE support still needed or not enough time to review the [ifdef-only] changes), then totally understandable, just thought I'd at least ask.

Thanks :-)

Comment 19

[Wan-Teh Chang]

Ed: reviewing a continuous stream of trivial patches takes
time away from, and sometimes the concentration necessary
for, more important work.

I will review and check in your patch.  Note that zlib is
a third-party library, so I will omit your changes to the
zlib files.  You should not remove the WinCE code from
other copies of zlib in the Mozilla source tree either.

Comment 20

[Ed Morley [:emorley]]

Attachment: Remove WinCE code from NSS v2

Patch now in CVS diff format rebased to CVS trunk (as opposed to from hg m-c), to hopefully reduce the amount of hassle. Have also combined the patches from here, bug 654594 / bug 654723, to keep everything together & removed the zlib changes.

(In reply to Wan-Teh Chang from comment #19)
> Ed: reviewing a continuous stream of trivial patches takes
> time away from, and sometimes the concentration necessary
> for, more important work.

Fair point.

Thanks :-)

Comment 21

[Ed Morley [:emorley]]

For some reason, the CVS diff hasn't included the file deletions (sorry more used to hg), so the following files need to be deleted on top:
/security/coreconf/WINCE.mk
/security/nss/cmd/lib/wincemain.c

Comment 24

[Ed Morley [:emorley]]

1 year and 26 days later I'm going to just give up hoping for this review :-(

Luckily since filing this I moved onto other things - but what worries me is that future contributors who end up with an NSS bug as their first bug might not wait around & we then lose potentially valuable Mozilla contributors as a result.

Closing this out to stop it appearing on some of my saved searches.

Comment 25

[Ed Morley [:emorley]]

Comment on attachment 552677 [details] [diff] [review]
Remove WinCE code from NSS v2

Cancelling review due to lack of response & to stop it showing up on the 'My Requests' page.

Looking at https://bugzilla.mozilla.org/request.cgi?action=queue&requester=&product=&type=all&requestee=wtc%40google.com&component=&group=type the experience in this bug seems pretty typical :-(

Comment 26

[Wan-Teh Chang]

Attachment: Remove WinCE code from NSS v3, by Ed Morley

I cleaned up the patch a bit. I included the removal of WINCE.mk and
wincemain.c in the patch. Based on the patch in bug 483653, I removed
some more code.

The DBM_REOPEN_ON_FLUSH and DBM_USING_NSPR code was only used by
WinCE. (I know the DBM_USING_NSPR code was added to support WinCE.)
But I didn't try to remove this code.

Patch checked in on the NSS trunk (NSS 3.14.1).

Checking in mozilla/dbm/include/mcom_db.h;
/cvsroot/mozilla/dbm/include/mcom_db.h,v  <--  mcom_db.h
new revision: 3.47; previous revision: 3.46
done
Checking in mozilla/dbm/include/winfile.h;
/cvsroot/mozilla/dbm/include/winfile.h,v  <--  winfile.h
new revision: 3.4; previous revision: 3.3
done
Checking in mozilla/dbm/src/Makefile.in;
/cvsroot/mozilla/dbm/src/Makefile.in,v  <--  Makefile.in
new revision: 3.23; previous revision: 3.22
done
Checking in mozilla/dbm/src/mktemp.c;
/cvsroot/mozilla/dbm/src/mktemp.c,v  <--  mktemp.c
new revision: 3.10; previous revision: 3.9
done
Checking in mozilla/security/coreconf/Linux.mk;
/cvsroot/mozilla/security/coreconf/Linux.mk,v  <--  Linux.mk
new revision: 1.52; previous revision: 1.51
done
Checking in mozilla/security/coreconf/WIN95.mk;
/cvsroot/mozilla/security/coreconf/WIN95.mk,v  <--  WIN95.mk
new revision: 1.4; previous revision: 1.3
done
Removing mozilla/security/coreconf/WINCE.mk;
/cvsroot/mozilla/security/coreconf/WINCE.mk,v  <--  WINCE.mk
new revision: delete; previous revision: 1.7
done
Checking in mozilla/security/coreconf/WINNT.mk;
/cvsroot/mozilla/security/coreconf/WINNT.mk,v  <--  WINNT.mk
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/security/coreconf/config.mk;
/cvsroot/mozilla/security/coreconf/config.mk,v  <--  config.mk
new revision: 1.33; previous revision: 1.32
done
Checking in mozilla/security/coreconf/rules.mk;
/cvsroot/mozilla/security/coreconf/rules.mk,v  <--  rules.mk
new revision: 1.87; previous revision: 1.86
done
Checking in mozilla/security/nss/Makefile;
/cvsroot/mozilla/security/nss/Makefile,v  <--  Makefile
new revision: 1.43; previous revision: 1.42
done
Checking in mozilla/security/nss/cmd/platlibs.mk;
/cvsroot/mozilla/security/nss/cmd/platlibs.mk,v  <--  platlibs.mk
new revision: 1.72; previous revision: 1.71
done
Checking in mozilla/security/nss/cmd/certutil/keystuff.c;
/cvsroot/mozilla/security/nss/cmd/certutil/keystuff.c,v  <--  keystuff.c
new revision: 1.24; previous revision: 1.23
done
Checking in mozilla/security/nss/cmd/lib/basicutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/basicutil.c,v  <--  basicutil.c
new revision: 1.6; previous revision: 1.5
done
Checking in mozilla/security/nss/cmd/lib/config.mk;
/cvsroot/mozilla/security/nss/cmd/lib/config.mk,v  <--  config.mk
new revision: 1.6; previous revision: 1.5
done
Checking in mozilla/security/nss/cmd/lib/secpwd.c;
/cvsroot/mozilla/security/nss/cmd/lib/secpwd.c,v  <--  secpwd.c
new revision: 1.18; previous revision: 1.17
done
Checking in mozilla/security/nss/cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.124; previous revision: 1.123
done
Removing mozilla/security/nss/cmd/lib/wincemain.c;
/cvsroot/mozilla/security/nss/cmd/lib/wincemain.c,v  <--  wincemain.c
new revision: delete; previous revision: 1.2
done
Checking in mozilla/security/nss/cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.74; previous revision: 1.73
done
Checking in mozilla/security/nss/lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.73; previous revision: 1.72
done
Checking in mozilla/security/nss/lib/ckfw/Makefile;
/cvsroot/mozilla/security/nss/lib/ckfw/Makefile,v  <--  Makefile
new revision: 1.19; previous revision: 1.18
done
Checking in mozilla/security/nss/lib/freebl/Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.123; previous revision: 1.122
done
Checking in mozilla/security/nss/lib/freebl/arcfour.c;
/cvsroot/mozilla/security/nss/lib/freebl/arcfour.c,v  <--  arcfour.c
new revision: 1.21; previous revision: 1.20
done
Checking in mozilla/security/nss/lib/freebl/config.mk;
/cvsroot/mozilla/security/nss/lib/freebl/config.mk,v  <--  config.mk
new revision: 1.29; previous revision: 1.28
done
Checking in mozilla/security/nss/lib/freebl/win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.29; previous revision: 1.28
done
Checking in mozilla/security/nss/lib/freebl/mpi/mpi.h;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi.h,v  <--  mpi.h
new revision: 1.26; previous revision: 1.25
done
Checking in mozilla/security/nss/lib/freebl/mpi/mpmontg.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpmontg.c,v  <--  mpmontg.c
new revision: 1.25; previous revision: 1.24
done
Checking in mozilla/security/nss/lib/softoken/config.mk;
/cvsroot/mozilla/security/nss/lib/softoken/config.mk,v  <--  config.mk
new revision: 1.32; previous revision: 1.31
done
Checking in mozilla/security/nss/lib/softoken/sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.24; previous revision: 1.23
done
Checking in mozilla/security/nss/lib/softoken/legacydb/config.mk;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/config.mk,v  <--  config.mk
new revision: 1.14; previous revision: 1.13
done
Checking in mozilla/security/nss/lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.109; previous revision: 1.108
done
Checking in mozilla/security/nss/lib/ssl/sslnonce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslnonce.c,v  <--  sslnonce.c
new revision: 1.28; previous revision: 1.27
done
Checking in mozilla/security/nss/lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.98; previous revision: 1.97
done
Checking in mozilla/security/nss/lib/util/secder.h;
/cvsroot/mozilla/security/nss/lib/util/secder.h,v  <--  secder.h
new revision: 1.17; previous revision: 1.16
done
Checking in mozilla/security/nss/lib/util/secport.c;
/cvsroot/mozilla/security/nss/lib/util/secport.c,v  <--  secport.c
new revision: 1.31; previous revision: 1.30
done
Checking in mozilla/security/nss/lib/util/secport.h;
/cvsroot/mozilla/security/nss/lib/util/secport.h,v  <--  secport.h
new revision: 1.29; previous revision: 1.28
done
Checking in mozilla/security/nss/lib/util/utilmod.c;
/cvsroot/mozilla/security/nss/lib/util/utilmod.c,v  <--  utilmod.c
new revision: 1.7; previous revision: 1.6
done

Comment 27

[Ed Morley [:emorley]]

Thank you

Comment 28

[Wan-Teh Chang]

Ed: you're welcome. I didn't forget this patch.
I noticed that you are one of the people moving
patchsets from mozilla-inbound to mozilla-central.
That reminded me of your patch. Sorry about the
frustration caused by my long delay in commiting
your patch.

Comment 29

[Wan-Teh Chang]

Attachment: Nelson's patch that added the DBM_USING_NSPR code

This patch is for future reference. I am not sure if we
should spend the time removing the DBM_USING_NSPR code.

Nelson didn't seem to use a bug report for his DBM_USING_NSPR
work. I reconstructed this patch using cvs diff with a date
range (one hour before and after his checkin). His CVS commit
message is (the timestamp is US Pacific Time):

  nelsonb%netscape.com	2002-04-04 19:33	

  On WinCE, DBM's dbopen uses NSPR file open flags rather than libc's
  file open flags (which are not defined on WinCE).

The files in this patch have since been moved from
mozilla/security/nss/lib/softoken to
mozilla/security/nss/lib/softoken/legacydb.

Some of the changes in this patch are code cleanup.
If someone removes the DBM_USING_NSPR code, please
keep the code cleanup changes.

Comment 30

[Wan-Teh Chang]

Attachment: Tweak win_rand.c

I studied win_rand.c, rev. 1.5 and made some additional changes
to restore it a little closer to its pre-WinCE state.

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.30; previous revision: 1.29
done