1280846 - gtests refactoring prevents building portions downstream where nss is built in stages

Status: RESOLVED FIXED

(NSS :: Test, defect, P1)

Description

[Elio Maldonado]

As of nss-3.24 we can no longer build all of gtest components due to some refactoring and some dependencies on static libraries which unfortunately aren't available in our build environment as NSS is built in stages.

For Fedora, and derived distributions, NSS is built in three stages as three separate but dependent packages taking advantage of the NSS build-time environment variables introduced in Bug 835919 and Bug 1186917. 

I should mention Bug 744651 secutil (setool.a) was split into high and low level components which I think could ameliorate some problems if the test could take advantage of such split.

I will offer more details as I continue my analysis.

Comment 1

[Elio Maldonado]

The problem is that some of the tests need to access calls to functions classified as private exports. 
These are available within the rest of nss but not normal client applications. The shared libraries don't export them and to access them you need to statically link with the static libraries. 
We normally install only shared libraries, static linking is frowned up by our package guidelines
and those of of most distribi=tions.

We do make an exception to this rule with freebl.a for the sake of glibc and some pkcs #11 modules. (parts of OpenJDK's java that do crypto are considered such privileged client). Some tests link with sectool.a, a library used by NSS security tools and we have discussed making that library available downstream as an unsupported tool so that's may help some. See my previous reference to Bug 744651 and that split could be modified some. Other tests statically link the libnssutil.a instead of dynamically linking with the shared library and that is even more problematic as we build tree separate packages. 

In https://hg.mozilla.org/projects/nss/file/tip/external_tests/common/gtests.cc
the #include "ssl.h" is also a preblem. The more basic test should need to refers to headers from the higher layers of nss. I notice that main is not a static function so perhaps be could avial ourselves of inheritance here and have a minimalist common main for the basic tests and another for the higher level functionality tests.

In https://hg.mozilla.org/projects/nss/file/tip/external_tests/ssl_gtest/manifest.mn the ssl test depend of the softoken static library. Some refactoring may help on this.

I should point out that though I have the requirement and run all of gtests as part of the build(s) how this is accomplished doesn't matter. I could run the portions that I need as soon I'm building that package that needs them  or defer until the build of nss higher layers. The important thing not to miss any testing.

Comment 2

[Daiki Ueno [:ueno]]

Attachment: gtests-split.patch

Allow util_gtest and der_gtest to run with NSS_BUILD_UTIL_ONLY

gtest based unit tests require access to internal symbols only
available at build time or static libs.  This could be problematic for
downstream packaging where a source tarball is split into multiple
stages: util, softoken, and the rest.  This patch isolates those unit
tests according to the splitting and allows them to run in the
corresponding sub packages.

To test, I did the following:
- make nss_build_all NSS_BUILD_UTIL_ONLY=1 USE_64=1
- NSS_CYCLES=standard NSS_TESTS='gtests' USE_64=1 NSS_BUILD_UTIL_ONLY=1 ./all.sh

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8791163 [details] [diff] [review]
gtests-split.patch

Review of attachment 8791163 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we could probably just dynamically link to util rather than have this much complication.

::: tests/all.sh
@@ +316,4 @@
>  fi
>  
>  if [ ! -f ${DIST}/${OBJDIR}/bin/${LAST_FILE_BUILT}${PROG_SUFFIX} ]; then
> +  if [ "${NSS_BUILD_UTIL_ONLY}" != "1" ]; then

Rather than make this change, why not change the gtests.sh script so that it skips running (and records a single success) if the binary wasn't built.

Comment 4

[Daiki Ueno [:ueno]]

(In reply to Martin Thomson [:mt:] from comment #3)
> Comment on attachment 8791163 [details] [diff] [review]
> gtests-split.patch
> 
> Review of attachment 8791163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that we could probably just dynamically link to util rather than
> have this much complication.

Unfortunately, this is not feasible, because util_gtest refers to the symbols such as sec_port_ucs2_utf8_conversion_function, which are not exported from the shared library:
http://hg.mozilla.org/projects/nss/file/b5bc473195a4/lib/util/nssutil.def

> ::: tests/all.sh
> @@ +316,4 @@
> >  fi
> >  
> >  if [ ! -f ${DIST}/${OBJDIR}/bin/${LAST_FILE_BUILT}${PROG_SUFFIX} ]; then
> > +  if [ "${NSS_BUILD_UTIL_ONLY}" != "1" ]; then
> 
> Rather than make this change, why not change the gtests.sh script so that it
> skips running (and records a single success) if the binary wasn't built.

That sounds like a good idea.  I will update the patch soon.

Comment 5

[Daiki Ueno [:ueno]]

Attachment: gtests-split-v2.patch

Changes from the previous version:
- check the existence of *_gtest binary in gtests.sh, instead of $NSS_BUILD_UTIL_ONLY
- in external_tests/common, create libgtestutil.a instead of the dummy "gtests" executable
- in external_tests/ssl_gtest, link with -lsoftokn3 instead of libsoftokn.a
- use SECTOOL_LIB extensively in cmd/platlibs.mk
- other minor fixes

I have tried to build nss-util and nss Fedora packages with this patch.  For nss-util, it builds successfully and all the tests passed:

https://kojipkgs.fedoraproject.org//work/tasks/921/15640921/build.log

However, for nss, while the build phase succeeded, ssl_gtests had a few failures:

https://koji.fedoraproject.org/koji/getfile?taskID=15641281&name=build.log&offset=-4000
> [  FAILED  ] GenericDatagram/TlsConnectGeneric.ClientAuth/0, where GetParam() = ("DTLS", 771)
...

I am not sure if this is related to this splitting.

Comment 6

[Franziskus Kiefer [:franziskus]]

I think I'd like to see bug 1300109 fixed before this one. That way we can check that our modular builds are working and that the gtests don't break it.
Daiki, do you think you can come up with a way (bash script?) to automate the build process that's done for RedHat? It doesn't have to be the exact same way, but a build that respects the library boundaries you have.

Comment 7

[Daiki Ueno [:ueno]]

Sure, that makes sense.  I have attached the scripts to that bug.

Comment 8

[Franziskus Kiefer [:franziskus]]

Modular builds are running on TC now. Do you want to fix this so you can run gtests as well?

Comment 9

[Kai Engert [:KaiE:]]

What's missing to get this completed?

Comment 10

[Daiki Ueno [:ueno]]

Attachment: gtests-split-v3.patch

Rebased the v2 patch against the latest master.

Comment 11

[Daiki Ueno [:ueno]]

Attachment: gtests-tc-modular.patch

This patch enables gtests to build and run under the modular build settings in TC.

Comment 12

[Daiki Ueno [:ueno]]

I am sorry for the delay.  The patches attached to comment 10 and comment 11 should enable gtests running under modular builds.

For simplicity, there are the following limitations:
- the phases of building and running tests are not separated
- only gtests (not the entire tests with all.sh) are enabled
- pk11_gtests is built as part of nss, while it should be part of nss-softokn

Comment 13

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8859655 [details] [diff] [review]
gtests-split-v3.patch

Review of attachment 8859655 [details] [diff] [review]:
-----------------------------------------------------------------

The changes to manifest.mn fils have to go into the corresponding gyp file as well. At least where names change. Otherwise this won't build.

Comment 14

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8859656 [details] [diff] [review]
gtests-tc-modular.patch

Review of attachment 8859656 [details] [diff] [review]:
-----------------------------------------------------------------

The way TC works is that you have a task that runs the build and then depending tasks that run tests and fetch the build artefact from the build task. So you shouldn't call tests in the build job but make them individual tasks.

::: automation/taskcluster/scripts/build_nss.sh
@@ +32,5 @@
>  rm -rf dist
>  make -C nss-nss nss_build_all
>  
> +# Run tests.
> +(cd nss-nss/tests && ./gtests/gtests.sh)

This script is only building NSS. To run tests on this build you should add it depending on the task that does the modular build [1].

[1] https://searchfox.org/nss/source/automation/taskcluster/graph/src/extend.js#269

::: automation/taskcluster/scripts/build_util.sh
@@ +18,5 @@
>  rm -rf dist
>  make -C nss-util nss_build_all
>  
> +# Run tests.
> +(cd nss-util/tests && ./gtests/gtests.sh)

Same here.

Comment 15

[Daiki Ueno [:ueno]]

Attachment: gtests-split-v4.patch

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)

> The changes to manifest.mn fils have to go into the corresponding gyp file
> as well. At least where names change. Otherwise this won't build.

Thank you for pointing that.  The attached version should build with gyp:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=ddb37d69a2dc9e7d6f42c975788014b00b5b5a57

Comment 16

[Daiki Ueno [:ueno]]

Attachment: gtests-tc-modular-v2.patch

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #14)

> This script is only building NSS. To run tests on this build you should add
> it depending on the task that does the modular build [1].
> 
> [1]
> https://searchfox.org/nss/source/automation/taskcluster/graph/src/extend.
> js#269

Yes, but I don't think I can test such change locally by myself.  Instead, I added separate scripts (run_tests_util.sh, run_tests_softoken.sh, run_tests_nss.sh) and modified run_tests.sh to call them.

Could you integrate it to extend.js, if this is acceptable?

Comment 17

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8872642 [details] [diff] [review]
gtests-split-v4.patch

Review of attachment 8872642 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not really happy about moving the tests and scoped_ptrs around. But I guess there's no other way.

::: gtests/common/gtests-util.cc
@@ +1,1 @@
> +#include "nspr.h"

Missing license.

Comment 18

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8872645 [details] [diff] [review]
gtests-tc-modular-v2.patch

Review of attachment 8872645 [details] [diff] [review]:
-----------------------------------------------------------------

With the other patch we'll get modular builds for everything. We won't run tests though. I think having the build is good enough for us. If you want to run the tests as well, you could make a new modular platform for TC and move the modular extra build there. But I don't think it's necessary.

Comment 19

[Daiki Ueno [:ueno]]

Attachment: 0001-cpputil-split-scoped_ptrs.h-according-to-public-head.patch

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #17)
> Comment on attachment 8872642 [details] [diff] [review]
> gtests-split-v4.patch
> 
> Review of attachment 8872642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not really happy about moving the tests and scoped_ptrs around. But I
> guess there's no other way.

I don't know much about C++, but I am wondering if scoped_ptrs.h is a bit modular so that it can be included separately.  For example, if only ScopedSECItem is needed, one could include "scoped_ptrs_secitem.h", without pulling other unrelated headers.  This way, the "moving" wouldn't be necessary in the future.

I am attaching a patch allowing this, for what it's worth.

Comment 20

[Kai Engert [:KaiE:]]

Daiki, did you intend to set a review flag on your latest patch?

Will you attach a patch addresses the "missing license" text mentioned in comment 17?

Comment 21

[Daiki Ueno [:ueno]]

Attachment: gtests-split-v5.patch

Changes from v4:
- added missing license notice to gtests-util.cc as suggested
- switched scoped_ptrs.h to use template specialization instead of overloading to make file splitting easier (this supersedes the patch at comment 17)
- added ifndef NSS_BUILD_UTIL_ONLY ... endif block in cmd/platlibs.mk, to avoid linking with -lssl3, etc on the system

Comment 22

[Daiki Ueno [:ueno]]

Attachment: gtests-tc-modular-v3.patch

Removed the scripts to run the tests under modular builds, as suggested.

Comment 23

[Daiki Ueno [:ueno]]

nss-try build with gtests-split-v5.patch and gtests-tc-modular-v3.patch:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=d5e00ab0ccbe2c36fcdfd8766901c6ce6e2d470c

Comment 24

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8873416 [details] [diff] [review]
gtests-split-v5.patch

Review of attachment 8873416 [details] [diff] [review]:
-----------------------------------------------------------------

I'd prefer the scoped_ptr version from the earlier patch. (That's not great either, but more readable than the templated version imho.)

::: cpputil/scoped_ptrs_base.h
@@ +8,5 @@
> +#define scoped_ptrs_base_h__
> +
> +#include <memory>
> +
> +template <class T>

Why do we need templates? Can't we just use the struct as before?
This makes the actual scoped ptr definition more complex than before.
I don't think I see how this is better than the scoped_ptr version we had before.

Comment 25

[Daiki Ueno [:ueno]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #24)

> Why do we need templates? Can't we just use the struct as before?
> This makes the actual scoped ptr definition more complex than before.
> I don't think I see how this is better than the scoped_ptr version we had
> before.

The primary reason for using template here is to avoid code duplication.  In the previous version, we had the same deleter definitions for SECItem, SECAlgorithmID, etc, in both scoped_ptrs.h and scoped_ptrs_util.h.

Anyway, even if there are code duplication, those files are still small.  I will update the patch with the old scoped_ptr definition.

Comment 26

[Daiki Ueno [:ueno]]

Attachment: gtests-split-v6.patch

Reverted to use the old scoped_ptrs splitting from the v4 patch.

Comment 27

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8873875 [details] [diff] [review]
gtests-split-v6.patch

Review of attachment 8873875 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, let's get this stuff landed.

Comment 28

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/a15755b99b544dc9643f6e3e7c3b36825112c5b2
https://hg.mozilla.org/projects/nss/rev/8445f247021a7cb13c08046698730aec77069b9f

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Backed out the smaller of the two patches:

https://hg.mozilla.org/projects/nss/rev/c87a857c4f8033680c74967a6a177b410fb504bb

Building gtests in the libutil part probably won't work, we should maybe do in the libnss build? It seems that try push didn't actually build the gtests.

Comment 30

[Franziskus Kiefer [:franziskus]]

Daiki, could you take another look at this?

Comment 31

[Daiki Ueno [:ueno]]

Attachment: cpputil-modular.patch

Sorry for the long overdue.  This should fix modular build after:
https://hg.mozilla.org/projects/nss/rev/d20ee6560caf

Comment 32

[Daiki Ueno [:ueno]]

Attachment: gtests-tc-modular-v4.patch

This re-lands the backed out change which enable compilation of gtests portion under modular builds:
https://hg.mozilla.org/projects/nss/rev/c87a857c4f80

nss-try build:
https://hg.mozilla.org/projects/nss-try/rev/8d1c33f49cd2ea59d5d70d19dd71031628cf1786

Comment 33

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8883031 [details] [diff] [review]
gtests-tc-modular-v4.patch

Review of attachment 8883031 [details] [diff] [review]:
-----------------------------------------------------------------

So we do der_gtest and util_gtest in nss-util and the rest (and der_gtest again) in nss-nss now? How does this cope with new gtests folders being added?

Comment 34

[Daiki Ueno [:ueno]]

The difference between those two tests is that util_gtest uses internal symbols in libnssutil3, while der_gtest doesn't (which only uses external symbols of libnssutil3).  I put der_gtest in nss-util because it doesn't depend on any other part of NSS, but it might be simpler to only have util_gtest in nss-util.

Comment 35

[Kai Engert [:KaiE:]]

What's the next step?

Comment 36

[Franziskus Kiefer [:franziskus]]

Daiki, if you rebase the patches we can land them (they rotted).

Comment 37

[Daiki Ueno [:ueno]]

Attachment: cpputil-modular-v2.patch

Rebased and adjusted manifest.mn not to build der_gtest twice.

Comment 38

[Daiki Ueno [:ueno]]

Attachment: gtests-tc-modular-v5.patch

The previous patch still seems to apply cleanly, but rebased it against the trunk just in case.

Comment 39

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/cdba8bea84a477cf2bb71be560c2d4a3856778ae
https://hg.mozilla.org/projects/nss/rev/3c4f0e9f6e45107dc106f709eaa54f5d096f49a7