353577 - Delete dead PURE_STAN code, code outside NSS_3_4_CODE, and NSS_CLASSIC code

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P5)

Description

[Julien Pierre]

The NSS libraries contain a lot of code conditionally compiled for PURE_STAN_CODE or PURE_STAN_BUILD . I'm not sure we need this code .  Removal patch forthcoming.

Comment 1

[Julien Pierre]

Attachment: Remove dead code

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 239436 [details] [diff] [review]
Remove dead code

1) Do we have consensus among the NSS developers that we really want to 
do this?  This seems like it really shuts the door on ever re-integrating 
any of this Stan code.  review approval needs that consensus, IMO.

2) in dev/manifest.mn, and in pki/manifest.mn, we see changes like this:

-ifndef PURE_STAN_BUILD
 CSRCS += pki3hack.c
 PRIVATE_EXPORTS += pkistore.h pki3hack.h pkitm.h pkim.h
 #DEFINES = -DNSS_3_4_CODE -DDEBUG_CACHE
 DEFINES = -DNSS_3_4_CODE
-endif

In both of those files, CSRCS and PRIVATE_EXPORTS are defined earlier
in the files.  If this part isn't going to be conditional any more,
the file names added to CSRCS and PRIVATE_EXPORTS here should be moved up 
into the main declaration of those shell variables earlier in the file.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 239436 [details] [diff] [review]
Remove dead code

r=nelson

Comment 4

[Julien Pierre]

Attachment: Update with Nelson's input

Comment 5

[Julien Pierre]

Wan-Teh stated in email on 9/27 that Bob's opinion counts the most as far as deciding whether to delete the Stan code or not, but we haven't heard from Bob on this. I can't resolve this bug without this answer, and I will no longer be active on NSS soon, therefore I'm reassigning this bug to Bob.

Comment 6

[Robert Relyea]

Comment on attachment 243540 [details] [diff] [review]
Update with Nelson's input

This has been on the queue for a long time, so I'm clearing it off with the appropriate comment.

I'm not excited about just killing the PURE_STAN ifdefs, even though they likely atrophied by now. I don' t have a good arguement for why they shouldn't be removed, so if someone really feels strongly about it I'll be OK, but I certainly won't go off and do it myself.... therefore there is little  use keeping the review going on this patch.

bob

Comment 7

[Julien Pierre]

Comment on attachment 243540 [details] [diff] [review]
Update with Nelson's input

Bob,

I really don't like to have dead code lying around, since I end up spending time patching sometimes. Do you think there is a chance we will use this PURE_STAN code again ? AFAIK the Stan project is dead.

Comment 8

[Robert Relyea]

I'm not really excited about completely removing it. It continues to leave us in limbo between two APIs. I've had to many times when some feature we thought was dead suddenly becomes top priority again....

So while I'm guardedly ok with the removal, it's not a priority for me, nor something I would actively push.

Comment 9

[Julien Pierre]

Bob,

If the day comes that we need to resurrect Stan, the source code will not be lost, it will still be available in CVS to restore. Until that day comes, I don't want to be wasting time looking or trying to maintain that dead source code. This is why I would like to delete it.

Comment 10

[Kai Engert [:KaiE:]]

Just some very general thoughts:

source code only available in cvs is very hard to find.
you must know exactly where it is located.
I don't think people will try out all possible revisions of files in order to find code.

I use a modern editor that uses syntax highlighting and displays all comments and all code in "#if 0" as light gray.

Comment 11

[Julien Pierre]

The code is #ifdef XXX, not if 0 . Not every editor does this, anyway. And this isn't only about viewing the code in an editor, it is about doing recursive grep's in the source tree and being misled that some calls are being made when they are really found in dead code only. I often work that way to figure out if some interfaces can be changed. I don't think dead code belongs in our tree.

In the case of stan code, I think we will know what to look for since this bug will have a checkin comment that shows the exact files and revisions from which the stan code is expunged. But anyway, any revision since 3.4 would probably do, as I don't expect the stan code changed much since.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 243540 [details] [diff] [review]
Update with Nelson's input

r=nelson 
(You could have made those minor makefile changes without asking for another review :)

Comment 13

[Wan-Teh Chang]

Comment on attachment 243540 [details] [diff] [review]
Update with Nelson's input

r=wtc.

In dev/manifest.mn, we have:

> # here is where the 3.4 glue code is added
>-ifndef PURE_STAN_BUILD
> DEFINES = -DNSS_3_4_CODE
>-PRIVATE_EXPORTS += devm.h devtm.h
>-endif

Please update the comment to say "enable the 3.4 glue code".
I suggest that you prepare another patch to remove the
NSS_3_4_CODE macro.

Comment 14

[Julien Pierre]

Attachment: Refresh patch for trunk. Also delete NSS_3_4_CODE macro

Nelson, Wan-Teh, thanks for the reviews. It turns out the last patch was bitrotten. Here is an updated one that also deals with NSS_3_4_CODE . I will also attach a separate one for the branch.

Comment 15

[Julien Pierre]

Attachment: Patch for branch

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288938 [details] [diff] [review]
Refresh patch for trunk. Also delete NSS_3_4_CODE macro

While we're getting rid of dead code, I wonder if we should get rid of all the code that is #ifdef NSS_CLASSIC, too.

Comment 17

[Wan-Teh Chang]

Comment on attachment 288944 [details] [diff] [review]
Patch for branch

Removing dead code is a cosmetic change.
Do you really want to make cosmetic changes
on our stable release maintenance branch?
This will produce a huge diff when our customers
analyze the changes in NSS 3.11.9.

In lib/dev/manifest.mn:

>-# here is where the 3.4 glue code is added
>-ifndef PURE_STAN_BUILD
>-DEFINES = -DNSS_3_4_CODE
> PRIVATE_EXPORTS += devm.h devtm.h
>-endif

You should add devm.h and devtm.h to the PRIVATE_EXPORTS
list above, as you did in a previous patch.

In lib/pki/certificate.c:

>-    /* XXX This breaks code for which NSS_3_4_CODE is not defined (pure
>-     *     4.0 builds).  That won't affect the tip.  But be careful
>-     *     when merging 4.0!!!
>+    /* XXX This breaks pure 4.0 builds Be careful when merging 4.0!!!
>      */

This comment should be deleted.  It doesn't make sense without
any pure Stan (4.0) code left in the source tree.

In lib/pki/manifest.mn:
 
>-ifndef PURE_STAN_BUILD
> CSRCS += pki3hack.c
> PRIVATE_EXPORTS += pkistore.h pki3hack.h pkitm.h pkim.h
>-#DEFINES = -DNSS_3_4_CODE -DDEBUG_CACHE
>-DEFINES = -DNSS_3_4_CODE
>-endif
>+#DEFINES = -DDEBUG_CACHE

The files should be added to the CSRCS and PRIVATE_EXPORTS lists
above, as you did in a previous patch.

Comment 18

[Julien Pierre]

Wan-Teh, thanks for the review. Yes, I would like to check this in to the stable branch, since we are actively maintaining it. I noticed you gave r+ to the branch patch but not the trunk patch. Did you mean it the other way around ?

Comment 19

[Wan-Teh Chang]

Comment on attachment 288938 [details] [diff] [review]
Refresh patch for trunk. Also delete NSS_3_4_CODE macro

r=wtc.

See my previous comment 17 for suggested changes to
lib/dev/manifest.mn and lib/pki/certificate.c.

Comment 20

[Julien Pierre]

Thanks . I checked in the patch with your suggested changes to NSS_3_11_BRANCH :

Checking in lib/manifest.mn;
/cvsroot/mozilla/security/nss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.16.2.2; previous revision: 1.16.2.1
done
Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.76.2.4; previous revision: 1.76.2.3
done
Checking in lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.67.28.6; previous revision: 1.67.28.5
done
Checking in lib/certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.34.2.5; previous revision: 1.34.2.4
done
Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.44.10.8; previous revision: 1.44.10.7
done
Checking in lib/dev/ckhelper.c;
/cvsroot/mozilla/security/nss/lib/dev/ckhelper.c,v  <--  ckhelper.c
new revision: 1.34.28.1; previous revision: 1.34
done
Checking in lib/dev/dev.h;
/cvsroot/mozilla/security/nss/lib/dev/dev.h,v  <--  dev.h
new revision: 1.37.28.1; previous revision: 1.37
done
Removing lib/dev/devmod.c;
/cvsroot/mozilla/security/nss/lib/dev/devmod.c,v  <--  devmod.c
new revision: delete; previous revision: 1.7.28.2
done
Checking in lib/dev/devslot.c;
/cvsroot/mozilla/security/nss/lib/dev/devslot.c,v  <--  devslot.c
new revision: 1.22.2.1; previous revision: 1.22
done
Checking in lib/dev/devt.h;
/cvsroot/mozilla/security/nss/lib/dev/devt.h,v  <--  devt.h
new revision: 1.22.28.1; previous revision: 1.22
done
Checking in lib/dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.39.28.1; previous revision: 1.39
done
Checking in lib/dev/devutil.c;
/cvsroot/mozilla/security/nss/lib/dev/devutil.c,v  <--  devutil.c
new revision: 1.26.28.1; previous revision: 1.26
done
Checking in lib/dev/manifest.mn;
/cvsroot/mozilla/security/nss/lib/dev/manifest.mn,v  <--  manifest.mn
new revision: 1.11.28.1; previous revision: 1.11
done
Checking in lib/pk11wrap/dev3hack.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/dev3hack.c,v  <--  dev3hack.c
new revision: 1.21.28.1; previous revision: 1.21
done
Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.143.2.12; previous revision: 1.143.2.11
done
Checking in lib/pk11wrap/pk11nobj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11nobj.c,v  <--  pk11nobj.c
new revision: 1.6.2.3; previous revision: 1.6.2.2
done
Checking in lib/pk11wrap/secmodti.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v  <--  secmodti.h
new revision: 1.24.2.1; previous revision: 1.24
done
Checking in lib/pki/certdecode.c;
/cvsroot/mozilla/security/nss/lib/pki/certdecode.c,v  <--  certdecode.c
new revision: 1.16.28.1; previous revision: 1.16
done
Checking in lib/pki/certificate.c;
/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.56.2.4; previous revision: 1.56.2.3
done
Checking in lib/pki/cryptocontext.c;
/cvsroot/mozilla/security/nss/lib/pki/cryptocontext.c,v  <--  cryptocontext.c
new revision: 1.14.28.3; previous revision: 1.14.28.2
done
Checking in lib/pki/manifest.mn;
/cvsroot/mozilla/security/nss/lib/pki/manifest.mn,v  <--  manifest.mn
new revision: 1.14.28.1; previous revision: 1.14
done
Checking in lib/pki/pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.25.28.2; previous revision: 1.25.28.1
done
Checking in lib/pki/pkim.h;
/cvsroot/mozilla/security/nss/lib/pki/pkim.h,v  <--  pkim.h
new revision: 1.27.2.2; previous revision: 1.27.2.1
done
Checking in lib/pki/pkistore.c;
/cvsroot/mozilla/security/nss/lib/pki/pkistore.c,v  <--  pkistore.c
new revision: 1.26.2.3; previous revision: 1.26.2.2
done
Checking in lib/pki/pkit.h;
/cvsroot/mozilla/security/nss/lib/pki/pkit.h,v  <--  pkit.h
new revision: 1.17.28.2; previous revision: 1.17.28.1
done
Checking in lib/pki/pkitm.h;
/cvsroot/mozilla/security/nss/lib/pki/pkitm.h,v  <--  pkitm.h
new revision: 1.14.2.1; previous revision: 1.14
done
Checking in lib/pki/trustdomain.c;
/cvsroot/mozilla/security/nss/lib/pki/trustdomain.c,v  <--  trustdomain.c
new revision: 1.51.28.3; previous revision: 1.51.28.2
done

And to the trunk :
Checking in lib/manifest.mn;
/cvsroot/mozilla/security/nss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.19; previous revision: 1.18
done
Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.85; previous revision: 1.84
done
Checking in lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.77; previous revision: 1.76
done
Checking in lib/certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.41; previous revision: 1.40
done
Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.57; previous revision: 1.56
done
Checking in lib/dev/ckhelper.c;
/cvsroot/mozilla/security/nss/lib/dev/ckhelper.c,v  <--  ckhelper.c
new revision: 1.36; previous revision: 1.35
done
Checking in lib/dev/dev.h;
/cvsroot/mozilla/security/nss/lib/dev/dev.h,v  <--  dev.h
new revision: 1.39; previous revision: 1.38
done
Removing lib/dev/devmod.c;
/cvsroot/mozilla/security/nss/lib/dev/devmod.c,v  <--  devmod.c
new revision: delete; previous revision: 1.9
done
Checking in lib/dev/devslot.c;
/cvsroot/mozilla/security/nss/lib/dev/devslot.c,v  <--  devslot.c
new revision: 1.23; previous revision: 1.22
done
Checking in lib/dev/devt.h;
/cvsroot/mozilla/security/nss/lib/dev/devt.h,v  <--  devt.h
new revision: 1.23; previous revision: 1.22
done
Checking in lib/dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.42; previous revision: 1.41
done
Checking in lib/dev/devutil.c;
/cvsroot/mozilla/security/nss/lib/dev/devutil.c,v  <--  devutil.c
new revision: 1.29; previous revision: 1.28
done
Checking in lib/dev/manifest.mn;
/cvsroot/mozilla/security/nss/lib/dev/manifest.mn,v  <--  manifest.mn
new revision: 1.13; previous revision: 1.12
done
Checking in lib/pk11wrap/dev3hack.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/dev3hack.c,v  <--  dev3hack.c
new revision: 1.23; previous revision: 1.22
done
Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.161; previous revision: 1.160
done
Checking in lib/pk11wrap/pk11nobj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11nobj.c,v  <--  pk11nobj.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/pk11wrap/secmodti.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodti.h,v  <--  secmodti.h
new revision: 1.25; previous revision: 1.24
done
Checking in lib/pki/certdecode.c;
/cvsroot/mozilla/security/nss/lib/pki/certdecode.c,v  <--  certdecode.c
new revision: 1.17; previous revision: 1.16
done
Checking in lib/pki/certificate.c;
/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.63; previous revision: 1.62
done
Checking in lib/pki/cryptocontext.c;
/cvsroot/mozilla/security/nss/lib/pki/cryptocontext.c,v  <--  cryptocontext.c
new revision: 1.18; previous revision: 1.17
done
Checking in lib/pki/manifest.mn;
/cvsroot/mozilla/security/nss/lib/pki/manifest.mn,v  <--  manifest.mn
new revision: 1.16; previous revision: 1.15
done
Checking in lib/pki/pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.28; previous revision: 1.27
done
Checking in lib/pki/pkim.h;
/cvsroot/mozilla/security/nss/lib/pki/pkim.h,v  <--  pkim.h
new revision: 1.30; previous revision: 1.29
done
Checking in lib/pki/pkistore.c;
/cvsroot/mozilla/security/nss/lib/pki/pkistore.c,v  <--  pkistore.c
new revision: 1.30; previous revision: 1.29
done
Checking in lib/pki/pkit.h;
/cvsroot/mozilla/security/nss/lib/pki/pkit.h,v  <--  pkit.h
new revision: 1.19; previous revision: 1.18
done
Checking in lib/pki/pkitm.h;
/cvsroot/mozilla/security/nss/lib/pki/pkitm.h,v  <--  pkitm.h
new revision: 1.15; previous revision: 1.14
done
Checking in lib/pki/tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--  tdcache.c
new revision: 1.47; previous revision: 1.46
done
Checking in lib/pki/trustdomain.c;
/cvsroot/mozilla/security/nss/lib/pki/trustdomain.c,v  <--  trustdomain.c
new revision: 1.55; previous revision: 1.54
done

Comment 21

[Julien Pierre]

Attachment: Incremental patch to remove NSS_CLASSIC code, for both trunk and branch

Comment 22

[Wan-Teh Chang]

Overwhelming diffs with cosmetic changes is also counter-productive.
I'll let you make the right judgment.

Comment 23

[Robert Relyea]

I'm all for removing NSS_CLASSIC.

Comment 24

[Julien Pierre]

Thanks for the reviews on both patches. Something was missing from my patch on the NSS_3_11_BRANCH, which caused a regression - bug 404610 . Wan-Teh traced it down to two files that hadn't been patched and just checked in the fix.

I will wait a few hours for tinderbox to go green on the stable branch on the platforms that experienced the regression, before I check in the patch for NSS_CLASSIC .

Comment 25

[Julien Pierre]

The tinderbox went green on all platform after Wan-Teh's checkin.

I checked in attachment 288966 [details] [diff] [review] on the trunk :

Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.87; previous revision: 1.86
done
Checking in certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.41; previous revision: 1.40
done
Checking in certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.42; previous revision: 1.41
done
Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.61; previous revision: 1.60
done

and on NSS_3_11_BRANCH :
Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.76.2.5; previous revision: 1.76.2.4
done
Checking in certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.33.2.4; previous revision: 1.33.2.3
done
Checking in certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.34.2.6; previous revision: 1.34.2.5
done
Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.44.10.9; previous revision: 1.44.10.8
done

I am marking this bug fixed.