351893 - Differentiate Basic and Extended ECC for NSS version string

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Christophe Ravel]

in the libraries version string NSS_VERSION

Comment 1

[Christophe Ravel]

Attachment: from Wan-Teh

Comment 2

[Christophe Ravel]

3 curves ECC = basic
more curves = extended

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh has checked this in on the NSS 3.11 branch.  Thanks Wan-Teh.

I am going to develop another patch that will obviate changing the strings.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Obviate future string changes (context diff)

The purpose of this patch is to change NSS such that, in the future
only the values NSS_VMAJOR NSS_VMINOR NSS_VPATCH and NSS_BETA will
need to be changed, and the strings will automatically follow.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: obviate future string changes (unified diff)

Same change, shown here as a unified diff.

Comment 6

[Wan-Teh Chang]

Comment on attachment 237545 [details] [diff] [review]
obviate future string changes (unified diff)

This patch is a great idea.  But it requires more
changes to the Linux and Solaris package files.
See http://lxr.mozilla.org/security/search?string=NSS_VERSION.
These files grep NSS_VERSION in nss.h to obtain the
product version string.  This patch will break those
package files.

Speaking of which, we should verify that those package
files can handle two or three instances of NSS_VERSION
in nss.h.

Anothe minor problem with this patch is that we sometimes
use "Beta 2", "Beta 3", etc. in the version string.  This
patch will lose the ability to differentiate multiple
Betas by version string.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: fix pkg Makefiles, too

I think this patch also fixes the pkg Makefiles.
I've done limited testing of them, using the new "test" target I added.

This patch adds a new program, nssver, that outputs a simple NSS version string.
During the build, this program is run and the output string put in a file in 
$(DIST).  The content of that file is subsequently used by the pkg Makefiles,
obviating any source file grepping.  

Christophe, please review for Solaris.  
Wan-Teh, please review the whole approach.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I should fix the "initial developer" and copyright years in the license 
boilerplate.  :)

I'm not pushing to get this into 3.11.3, so there's no big rush.

Note that I fixed the "Beta N" problem described in comment 6.

Comment 9

[Christophe Ravel]

Comment on attachment 237630 [details] [diff] [review]
fix pkg Makefiles, too

I haven't tested it but the code looks good w.r.t. packaging.

Comment 10

[Wan-Teh Chang]

Comment on attachment 237630 [details] [diff] [review]
fix pkg Makefiles, too

This patch will complicate Mozilla builds and
cross compilation.

The nssver program needs to be built in the
same way as 'nsinstall' is built.  See how we
use INTERNAL_TOOLS, NATIVE_CC, NATIVE_FLAGS in
http://lxr.mozilla.org/security/source/security/coreconf/nsinstall/Makefile

To shorten build time, Mozilla builds do not
build the entire NSS cmd directory.  So this
patch will also require a change to the PSM
makefile.  Search for "cmd/lib" and "cmd/shlibsign"
in http://lxr.mozilla.org/security/source/security/manager/Makefile.in

The distinction between NSS_VERSION_STRING and
NSS_BUILD_STRING is a good idea.  I also came to
the same conclusion.  But the " (debug)" string in
http://lxr.mozilla.org/security/source/security/nss/lib/nss/nssver.c
and
http://lxr.mozilla.org/security/source/security/nss/lib/nss/nss.rc
(there are instances of these files for the other
shared libraries), which indicates a debug build,
should also be part of NSS_BUILD_STRING.

Comment 11

[Wan-Teh Chang]

Comment on attachment 237630 [details] [diff] [review]
fix pkg Makefiles, too

Many programmers write code like the following to test
a Boolean value:

    if (NSS_BETA == PR_TRUE) {
        ...
    }

See http://lxr.mozilla.org/security/search?string=%3D%3D+PR_TRUE

Since NSS_BETA is a Boolean, changing it to an integer
may break some code.

We've also put "Alpha" and "RC1" in the version string
before.

In any case, I'm fine with using the hardcoded string
"Beta" for all pre-releases.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #11)
> Many programmers write code like the following to test
> a Boolean value:
> 
>     if (NSS_BETA == PR_TRUE) {
>         ...
>     }

I consider such code to be broken, and have said so many times.

The proper test is

     if (NSS_BETA != PR_FALSE) {

Regarding other products that substitute their own makefiles for NSS's,
this is equivalent to using private interfaces.  The other products
complicate themselves in so doing, NSS does not complicate them. 
NSS ought not to be restrained from future development because other
products use private interfaces.

We don't need this enhancement for 3.11.3.  I'm marking this bug fixed.