Closed Bug 1280846 Opened 4 years ago Closed 3 years ago

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

Categories

(NSS :: Test, defect, P1)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: elio.maldonado.batiz, Assigned: ueno)

References

Details

Attachments

(3 files, 11 obsolete files)

15.58 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
1.05 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
1006 bytes, patch
franziskus
: review+
Details | Diff | Splinter Review
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.
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.
Severity: normal → major
Priority: -- → P2
Priority: P2 → P1
Attached patch gtests-split.patch (obsolete) — Splinter Review
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
Assignee: nobody → dueno
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.
(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.
Attached patch gtests-split-v2.patch (obsolete) — Splinter Review
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.
Attachment #8791163 - Attachment is obsolete: true
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.
Flags: needinfo?(dueno)
Sure, that makes sense.  I have attached the scripts to that bug.
Flags: needinfo?(dueno)
Depends on: 1300109
Depends on: 1306319
Modular builds are running on TC now. Do you want to fix this so you can run gtests as well?
Flags: needinfo?(dueno)
What's missing to get this completed?
Attached patch gtests-split-v3.patch (obsolete) — Splinter Review
Rebased the v2 patch against the latest master.
Attachment #8791570 - Attachment is obsolete: true
Flags: needinfo?(dueno)
Attached patch gtests-tc-modular.patch (obsolete) — Splinter Review
This patch enables gtests to build and run under the modular build settings in TC.
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
Attachment #8859655 - Flags: review?(franziskuskiefer)
Attachment #8859656 - Flags: review?(franziskuskiefer)
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.
Attachment #8859655 - Flags: review?(franziskuskiefer) → review-
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.
Attachment #8859656 - Flags: review?(franziskuskiefer) → review-
Attached patch gtests-split-v4.patch (obsolete) — Splinter Review
(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
Attachment #8859655 - Attachment is obsolete: true
Attachment #8872642 - Flags: review?(franziskuskiefer)
Attached patch gtests-tc-modular-v2.patch (obsolete) — Splinter Review
(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?
Attachment #8859656 - Attachment is obsolete: true
Attachment #8872645 - Flags: review?(franziskuskiefer)
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.
Attachment #8872642 - Flags: review?(franziskuskiefer) → review+
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.
Attachment #8872645 - Flags: review?(franziskuskiefer) → review-
(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.
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?
Attached patch gtests-split-v5.patch (obsolete) — Splinter Review
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
Attachment #8872642 - Attachment is obsolete: true
Attachment #8873073 - Attachment is obsolete: true
Attachment #8873416 - Flags: review?(franziskuskiefer)
Attached patch gtests-tc-modular-v3.patch (obsolete) — Splinter Review
Removed the scripts to run the tests under modular builds, as suggested.
Attachment #8872645 - Attachment is obsolete: true
Attachment #8873417 - Flags: review?(franziskuskiefer)
nss-try build with gtests-split-v5.patch and gtests-tc-modular-v3.patch:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=d5e00ab0ccbe2c36fcdfd8766901c6ce6e2d470c
Attachment #8873417 - Flags: review?(franziskuskiefer) → review+
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.
Attachment #8873416 - Flags: review?(franziskuskiefer)
(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.
Reverted to use the old scoped_ptrs splitting from the v4 patch.
Attachment #8873416 - Attachment is obsolete: true
Attachment #8873875 - Flags: review?(franziskuskiefer)
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.
Attachment #8873875 - Flags: review?(franziskuskiefer) → review+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Daiki, could you take another look at this?
Flags: needinfo?(dueno)
Attached patch cpputil-modular.patch (obsolete) — Splinter Review
Sorry for the long overdue.  This should fix modular build after:
https://hg.mozilla.org/projects/nss/rev/d20ee6560caf
Flags: needinfo?(dueno)
Attached patch gtests-tc-modular-v4.patch (obsolete) — Splinter Review
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
Attachment #8873417 - Attachment is obsolete: true
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?
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.
What's the next step?
Daiki, if you rebase the patches we can land them (they rotted).
Flags: needinfo?(dueno)
Rebased and adjusted manifest.mn not to build der_gtest twice.
Attachment #8883030 - Attachment is obsolete: true
Flags: needinfo?(dueno)
Attachment #8894471 - Flags: review?(franziskuskiefer)
The previous patch still seems to apply cleanly, but rebased it against the trunk just in case.
Attachment #8883031 - Attachment is obsolete: true
Attachment #8894472 - Flags: review?(franziskuskiefer)
Attachment #8894471 - Flags: review?(franziskuskiefer) → review+
Attachment #8894472 - Flags: review?(franziskuskiefer) → review+
https://hg.mozilla.org/projects/nss/rev/cdba8bea84a477cf2bb71be560c2d4a3856778ae
https://hg.mozilla.org/projects/nss/rev/3c4f0e9f6e45107dc106f709eaa54f5d096f49a7
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: 3.32 → 3.33
You need to log in before you can comment on or make changes to this bug.