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.
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 .
I would prefer to have this test library named differently to avoid any confusion. libnss3_test.so is fine with me.
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 ?
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.
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.
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 ?
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 .
Assignee: julien.pierre.bugs → wtchang
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
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.
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 .
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
This enhancement is required to merge all the libpkix tests to the tip . Marking P1 for 3.12 .
Priority: -- → P1
Target Milestone: --- → 3.12
Created attachment 266958 [details] [diff] [review] 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
Assignee: wtc → alexei.volkov.bugs
Status: NEW → ASSIGNED
Attachment #266958 - Flags: review?(julien.pierre.boogz)
Created attachment 267294 [details] [diff] [review] Splitting the big patch in to two parts. This is part one: the C code changes
Created attachment 267295 [details] [diff] [review] Splitting the big patch in to two parts. This is part two: makefiles changes
Attachment #267295 - Attachment description: Splitting the big patch in to two parts. This is part one: makefiles changes → Splitting the big patch in to two parts. This is part two: makefiles changes
Created attachment 267452 [details] [diff] [review] 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.
Attachment #267452 - Flags: review?(nelson)
Created attachment 267453 [details] [diff] [review] c files changes
Attachment #267453 - Flags: review?(julien.pierre.boogz)
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.
Suggested name is fine. But please review the patch. Name will be changed later, before integration.
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.
Attachment #267453 - Flags: review?(julien.pierre.boogz) → review+
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 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?
Attachment #267452 - Flags: review?(nelson) → review-
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 on attachment 267453 [details] [diff] [review] c files changes r+ only for the C files in this patch
Isn't this bug fixed now?
Summary: Support for testing of internal functions → Support for testing of internal libpkix functions
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.
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.
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.
LINKS_LIST should be removed from makefiles.
> 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.
Summary: Support for testing of internal libpkix functions → Combine internal libpkix function tests into a single statically linked program
Created attachment 277964 [details] [diff] [review] 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.
Created attachment 277965 [details] [diff] [review] make file changes accourding to review.
Attachment #277964 - Flags: review?(julien.pierre.boogz) → review+
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+.
Attachment #277965 - Flags: review?(nelson) → review+
Committed both patches.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Version: trunk → unspecified
You need to log in before you can comment on or make changes to this bug.