301213 - Combine internal libpkix function tests into a single statically linked program

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

As part of the libpkix project, over 100 test programs have been written so far
to test several hundred internal functions . We aren't clear yet which of these
functions, if any, will be exported by NSS. That's the topic of bug 294531.
What's clear is that not all of the libpkix functions will be exported. Until
now, the testing has been done by building NSS with a separate build system, but
we are now doing everything with coreconf, and we don't want to throw away all
the test programs written to the internal functions.

While in theory it is possible to test all internal functions through test cases
involving external functions, it is often more difficult to do so. Also, the
lower-level tests offer additional data if a higher-level test is broken, which
can help save time debugging the failures . I submit that even without the
libpkix project, it would be very useful to have the ability to test lower-level
internal functions.

The discussion below will refer to "a shared library". Right now, the main one
we are concerned with is libnss3.so, but the same concept could be applied to
our other shared libraries if we want to test internal functions in them.

Technically, there are several possible solutions to the problem .

I) One possibility is to revive the static libraries expressly for the purpose
of testing, and keep them private. These libraries would be linked with each
internal test program. I don't care much for that approach, because it will
create very large test binaries and take up a lot of disk space on build
machines. Also, on some platforms (Windows, OS/2), the sources may need to be
compiled with different flags depending on whether one is building static or
dynamic libraries, which could make it very painful .

II) The other possibility is simply to build a second shared library for the
express purpose of testing. Only the test programs would link with that shared
library, and it obviously it wouldn't be shipped to anybody.  There are several
options if we build this second shared library.

a) The test shared library would export all the same symbols that the regular
shared library has, plus a list of the internal functions we want to test, in a
section at the end of the def file (maybe called 3.999 ?). This would keep it
binary compatible with the applications that use the public APIs . Obviously,
the list of functions in section 3.999 would be subject to change in case the
internal functions get released. There would be no requirement to maintain any
binary compatibility for that section.
We would probably want to build this test shared library with the same name as
the official shared library in this case, but in a different subdirectory, eg.
under dist/OBJDIR/libtest .

b) The test library could just have all the symbols exported . I'm not sure if
this is easily achievable on all platforms. This alone would make it binary
incompatible on many platforms due to the order of exports changing.
This new test library might have either a different file name, or the same file
name as the official shared library in a different subdirectory as proposed in
II a).

In all cases, the new test programs for the internal functions would be in a new
subdirectory under nss, maybe nss/tests/internal, or nss/internalcmd . I have no
preference for the name. And these binaries for these programs would only be
expected to run against the test shared library from the same NSS build, not
from any other build, which might not have the same list of internal functions.

Comments welcome.

Comment 1

[Julien Pierre]

In today's NSS meeting, it was clarified that:

a) while there is no libnss3.a static lib, programs under cmd can link
statically with USE_STATIC_LIB . This links the individual static libraries from
nss/lib to the executable .

But doing this is not desirable, as long as the libpkix test programs remain
separate executables, due to binary size requirements.

It was suggested to merge all the test programs. I don't know how much work this
is. I believe they each have a command-line interface already, so it may be tedious.

b) It was agreed that a separate NSS shared library (option II) was acceptable.
It was suggested to build it under the cmd subdirectory.

Some details about that 2nd shared library were not discussed, in particular
whether this test library would export all symbols or just the ones needed, and
whether it would be named the same as libnss3.so and be in a separate directory;
or whether it would have a separate name.

I think it's easy to just export the symbols that are needed for the test, and
it will work on all platforms, so that's my inclination.

Regarding the naming, it may be preferrable to keep it the same as the official
libnss3.so, and storing it in a separate diectory, rather than calling it
libnss3_test.so . It doesn't matter for the libpkix tests, because they only use
libnss3_test. But if other unit tests are written in the future that need both
libnss3_test and, say, public functions from libssl3, there would still need to
be a libssl3_test that would link against libnss3_test .

Comment 2

[Christophe Ravel]

I would prefer to have this test library named differently to avoid any confusion.
libnss3_test.so is fine with me.

Comment 3

[Julien Pierre]

Christophe,

This means creating test targets for every single NSS shared library. And
modifying the code that dynamically loads them. That's a lot of work.

Would the confusion be avoided by publishing the libraries to something like
mozilla/dist/$(OBJDIR)/testlibs, and the programs that need them to
mozilla/dist/$(OBJDIR)/testbin, instead of mozilla/dist/$(OBJDIR)/lib and
mozilla/dist/$(OBJDIR)/bin ?

Comment 4

[Wan-Teh Chang]

The least intrusive way to implement this is to add
a new make variable BUILD_INTERNAL_TESTS.  When this
make variable is set (to 1), we:
- use the alternate .def files to export the internal
  functions from our shared libraries; and
- build the tests for internal functions.

In this solution, we don't need to change any library
names or their installed directories under mozilla/dist.

Comment 5

[Julien Pierre]

Wan-Teh,

If I understand correctly, your suggestion means that we are going to need two
full, completely separate builds of NSS, one that includes the internal tests
and one that doesn't.

I don't think that would meet our requirements, because that means doubling the
amount of builds that Christophe has to do, and the amount of QA runs as well.
We would like to be able to run both the internal and public function tests
without having to make a second build.

Besides the disk space and effort that not doing separate builds saves us, there
is greater value in doing the QA as part of the same official build, even if the
official shared library isn't tested. The same object files would get included
in both shared libraries, so this gives us greater confidence because the
internal function get tested on the official object files, not on a completely
different set of object files.

This is the same reason why we are testing NISCC at Sun with two different
builds (hacked client against official server; official client against hacked
server), and not merely with a hacked build against itself - which has little
relevance to the bits that actually get shipped. 

Comment 6

[Christophe Ravel]

Julien,

I thought that the special libnss3 library with more symbols exported was only
for testing some libpkix functions, not all and everything.
Do you need a special version of certutil to run the libpkix test programs for
example ?

Comment 7

[Julien Pierre]

Re: my comment #5

I should note re: our NISCC runs that we only do them on one platform, and that
the hacked build has been made only once at some date in the distant past; thus
we only still do nightly official builds of NSS.

But in this case we need all platform coverage for the internal functions,
especially as the libpkix code doesn't currently compile on anything but some
flavors of Solaris yet, and hasn't been tested on anything else.

Re: comment #6

I explained this to Christophe in person .

Comment 8

[Robert Relyea]

I definately prefer the library name *NOT* be libnss3.so.

The new library should be build from the 'cmd' directory. (much like libsecutil
is built today). It should create a new .def file at build time by
concantenating it's .def file with pkix symbols with the normal nss .def file
form ../../lib/nss/nss.def or probably better $(TOP)/nss/lib/nsss/nss.def.

I'm not sure I understand Julien's issue. Selecting which library to link
against is a build time issue. They can easily be separated into pkix_config.mk
which pkix tests include. The changes are made once there and used by all the
pkix test cases. I don't understand why the other NSS libraries would be
involved. Only libssl and libsmime depend on libnss3, and neither of them are
involved in pkix unit tests are they?

Creating a unit test only version of libnss3.so just seems to ask for someone
copying it to the wrong location and trying to use it.

bob

Comment 9

[Julien Pierre]

Bob,

The new shared library can be built from the cmd directory. I'm planning on
having the Makefile copy the DEF file from lib/nss and appending the internal
symbols needed in a "3.999" section.

Indeed, currently the PKIX tests only need private functions from libnss3, so
this isn't a pressing issue. But in the future it's conceivable some tests may
need, ay, private functions from libnss3 and public functions from libssl3. If
the shared lib for libnss that contains the private functions is named
libnss3_test, then the test program won't be able to link against both
libnss3_test and libssl3 (which depends on libnss3) . It will have to link
against libnss3_test and libssl3_test, even though it has no need for any
private symbols from libssl3 . Ie. renaming one shared library has a domino
effect that may cause additional test shared libraries to be built.

Comment 10

[Julien Pierre]

Now that the libpkix source and tests have been committed, I have taken a closer
look at the sources of some of the test programs. It turns out they are using
sectool functions, from cmd/lib, which depend on libsmime .

So, if we build a test nss3 shared lib under a different name, then we'll have
to build an smime3 shared lib under a different name as well, even though the
pkix tests don't need any internal functions from smime3 .

Comment 11

[Robert Relyea]

We have 2 shared libraries above nss3.dll. I don't see a problem building all of
them with separate names and a special flag testing. It's preferable because:
   1) it saves of having to fake up some funny path for the unit test programs
which is different than all of our other test programs, and
   2) [most importantly] it reduces the changes that a test version will
accidentally be shipped and linked with a real application.

bob

Comment 12

[Julien Pierre]

This enhancement is required to merge all the libpkix tests to the tip . Marking P1 for 3.12 .

Comment 13

[Alexei Volkov]

Attachment: change libpkix testing programs to use static libraries

The changes are based on the idea of using single wrapper executable. All pkix test programs will have symbolic link into it. The patch changes the build system as following:
  * Each subdirectory where pkix test tools are built creates it's own statically linking library and place it in to the nss/cmd/libpkix/$OS../ directory.
  * a link at mozilla/dist/$OS../bin directory gets created for each pkix tool that points into "testwrapper" executable.
  * testwrapper is built using all tools library and it's link is placed into mozilla/dist/$OS../bin directory

Comment 14

[Alexei Volkov]

Attachment: Splitting the big patch in to two parts. This is part one: the C code changes

Comment 15

[Alexei Volkov]

Attachment: Splitting the big patch in to two parts. This is part two: makefiles changes

Comment 16

[Alexei Volkov]

Attachment: changes in makefiles and tests

Symbolic links don't work on windows. The patch consolidates all tests programs into one that is called libpkixutil. Making all test to be run through this one program.

Comment 17

[Alexei Volkov]

Attachment: c files changes

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

The new program is apparently named "libpkixutil".  IMO, only libraries
should have names that start with lib.  I suggest pkixtest, which 
doesn't contain lib, is shorter, and signifies that this is a QA test 
program and not a general purpose utility.

Comment 19

[Alexei Volkov]

Suggested name is fine. But please review the patch. Name will be changed later, before integration.

Comment 20

[Julien Pierre]

Comment on attachment 267453 [details] [diff] [review]
c files changes

This code looks OK.

nit - I would find it cleaner if you had a structure type  with the name of the test and a function in it, and an array of that structure; vs having two separate arrays.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 267452 [details] [diff] [review]
changes in makefiles and tests

Alexei,
In this patch and also in the other patch that Julien reviewed,
in almost every manifest.mn file that it modifies, it adds this 
line:

LINKS_LIST = $(CSRCS:.c=)

But apart from that definition of the symbol LINKS_LIST, 
I cannot find ANY other use for that symbol.  I found no
files that reference it in any way.  A full text search of
the trunk source files show no copies of that string anywhere.

So, please explain what it is intended to do.  
If it really is as dead as it appears, I'd rather not add this
dead line to all those manifest.mn files.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 267452 [details] [diff] [review]
changes in makefiles and tests

I previously emailed the following comments to Alexei.  
These comments, together with my remarks in comments 18 and 21 above
are my complete review for this patch.  There are a number of minor
things to fix.  So r- overall, except
r+ for all the changes to nss/tests/libpkix/pkix_tests/*/*.sh

Some observations about the Makefile changes:

1. manifest.mn is always the very first file to be included in
Makefile.  The symbols it defines are defined before any other symbols.
So it should not contain += assignments.  It should contain = assignments.
Any += assignments should be in config.mk, which is included after rules.mk.

2. You added a new file  mozilla/security/nss/cmd/libpkix/config.mk
It defines 4 symbols.  Those symbols should be defined in manifest.mn,
I think.  (If you think otherwise, tell me why in a comment in the new
file.

3. The new files have these lines:

> > # The Original Code is the Netscape security libraries.

Should say "Network Security Software (NSS)"

> > #
> > # The Initial Developer of the Original Code is
> > # Netscape Communications Corporation.

Should say "Sun Microsystems, Inc."

> > # Portions created by the Initial Developer are Copyright (C) 1994-2000

Should say (C) 2007.

4. What is LINKS_LIST used for?
Where is the rule that uses that symbol?

Comment 23

[Julien Pierre]

Good catch, Nelson. I hadn't noticed the one manifest.mn file in the patch I reviewed. I thought it was all C files, as the title of the patch stated, but it wasn't .

Comment 24

[Julien Pierre]

Comment on attachment 267453 [details] [diff] [review]
c files changes

r+ only for the C files in this patch

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Isn't this bug fixed now?

Comment 26

[Julien Pierre]

Nelson,

Not yet. Alexei did not check in the C file changes. The Makefile patch also needs to be updated, reviewed, and checked in. Until that's done, we will continue to require BUILD_LIBPKIX_TESTS in the environment to build and run libkpix tests.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

My point was that we are able to build and test libPKIX now, and we do NOT 
need to staticly link the test commands with libpkix to do it,  so the 
total size of the NSS cmd binaries doesn't grow 50x like it would if we
did staticly link.

I think someone thinks that this bug is blocking the introduction of testing
of libPKIX into nightly builds and tinderbox builds.  But clearly those 
builds CAN be built and tested today without this change.  I'm not saying 
we want to release 3.12 that way, but we should not let it delay our testing.

Is there an RFE for enabling PKIX testing in nightly and tinderbox builds?
If not, there should be, and it should be P1 for 3.12.

Comment 28

[Julien Pierre]

Nelson,

It was thought that the way to fix the issue was to solve this bug, which merges all the libpkix test programs into one, and links only that one statically. That would have required no changes to tinderbox or nightly. But somehow all activity on this bug stopped 2 months ago. I would suggest waiting until Alexei comes back to resolve it early next week unless this can't wait 2 more business days.

Comment 29

[Alexei Volkov]

LINKS_LIST should be removed from makefiles.

Comment 30

[Alexei Volkov]

> 2. You added a new file  mozilla/security/nss/cmd/libpkix/config.mk
> It defines 4 symbols.  Those symbols should be defined in manifest.mn,
> I think.  (If you think otherwise, tell me why in a comment in the new
> file.
This file should not be referenced from any Makefiles. Will be removed.
All manifest.mn have necessary symbols set. 

> 3. The new files have these lines:
> 
> > > # The Original Code is the Netscape security libraries.
> 
> Should say "Network Security Software (NSS)"
> 
> > > #
> > > # The Initial Developer of the Original Code is
> > > # Netscape Communications Corporation.
> 
> Should say "Sun Microsystems, Inc."
> 
> > > # Portions created by the Initial Developer are Copyright (C) 1994-2000
> 
> Should say (C) 2007.
Nelson has created bug 392696 fix for which should resolve copyright issues.


> 4. What is LINKS_LIST used for?
> Where is the rule that uses that symbol?
An obsolete symbol. Will be removed.

Comment 31

[Alexei Volkov]

Attachment: additional c code changes(pkixutil.c only)

pkixutil.c:
   * define test function name/pointer structure and use an array of such structures instead of two separate arrays.
   * rename the main executable to pkixutil.

Comment 32

[Alexei Volkov]

Attachment: make file changes accourding to review.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 277965 [details] [diff] [review]
make file changes accourding to review.

Bugzilla cannot succesfully diff this patch against the previous version because the set of files being diffed has changed.  :-( 
But it appears that this patch addresses the issues I previous reported, so r+.

Comment 34

[Alexei Volkov]

Committed both patches.