Closed Bug 1214390 Opened 5 years ago Closed 5 years ago

Build NSS gtests by default

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(firefox44 affected)

RESOLVED FIXED
Tracking Status
firefox44 --- affected

People

(Reporter: mt, Assigned: mt)

References

Details

Attachments

(4 files, 1 obsolete file)

Agreed on 2015-10-13: we should just require a C++ compiler.
Attached patch bug1214390.patchSplinter Review
Attachment #8673314 - Flags: review?(ekr)
Assignee: nobody → martin.thomson
Comment on attachment 8673314 [details] [diff] [review]
bug1214390.patch

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

r+ whether you like my nit or not

::: manifest.mn
@@ +14,3 @@
>  
> +ifeq (0,$(NSS_BUILD_GTESTS))
> +DIRS := $(filter-out external_tests,$(DIRS))

Nit: wouldn't it be easier to be ifneq(0, $(NSS_BUILD_GTESTS))
DIRS += ...
Attachment #8673314 - Flags: review?(ekr) → review+
I was flip-flopping.  I made the change you suggested, tested it and checked it in.

https://hg.mozilla.org/projects/nss/rev/8cae181b0330
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8673314 [details] [diff] [review]
bug1214390.patch

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

::: manifest.mn
@@ +14,2 @@
>  
> +ifeq (0,$(NSS_BUILD_GTESTS))

I think we should delete this block and just build external_tests
unconditionally.

@@ +14,3 @@
>  
> +ifeq (0,$(NSS_BUILD_GTESTS))
> +DIRS := $(filter-out external_tests,$(DIRS))

The filter-out approach is appropriate when we want the
directory in the middle of the DIRS list. I also used
the filter-out approach to avoid ifdefs in manifest.mn;
the conditional test is performed in the config.mk file.

Martin, if you copied that design, then please move this
ifeq block to config.mk. But I prefer that we just remove
it.
This broke builds on 9 buildbot slaves that don't have a sufficiently new compiler version, Linux, Mac and Windows machines.

Do you want me to define NSS_BUILD_GTESTS=0 on those machines?
Flags: needinfo?(martin.thomson)
(In reply to Wan-Teh Chang from comment #4)
> @@ +14,2 @@
> >  
> > +ifeq (0,$(NSS_BUILD_GTESTS))
> 
> I think we should delete this block and just build external_tests
> unconditionally.


If we did that, we'd make it impossible to complete the remainder of the test suite on systems that lack the required compiler.
(In reply to Kai Engert (:kaie) from comment #5)
> Do you want me to define NSS_BUILD_GTESTS=0 on those machines?

I made that change on the affected machines.
Thanks Kai, I think that I should move the tweaks to config.mk as Wan-Teh has suggested.  I'll do that shortly.
Flags: needinfo?(martin.thomson)
Martin, Eric:

When I compiled NSS on Windows this morning, I saw two problems:

1. Visual C++ doesn't have the -std=c++0x flag. That results in
an annoying compiler warning for every file:

http://mxr.mozilla.org/nss/search?string=std%3Dc

2. Eventually the compilation failed because Windows doesn't seem
to have the unsetenv() function, used here:

http://mxr.mozilla.org/nss/search?string=unsetenv

Could you please look into these two issues on a Windows computer?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(martin.thomson)
The change you have used causes issues when building older branches, because older branches will build the gtests if the symbol is defined, regardless of its value.

I'd like to suggest that we stop using the old symbol, and introduce a new symbol NSS_DISABLE_GTESTS. If defined, gtests are skipped, and I can use that setting on buildbot builders that cannot build gtests because of their compiler version. And when building older branches, that variable will be ignored.
Attached patch 1214390-v2.patchSplinter Review
Attachment #8674474 - Flags: review?(ekr)
Comment on attachment 8674474 [details] [diff] [review]
1214390-v2.patch

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

This is fine with me
Attachment #8674474 - Flags: review?(ekr) → review+
Attached patch win.patch (obsolete) — Splinter Review
I think that this covers all the outstanding niggles.  I discovered some new problems with MSVC 2015 while working on this.
Flags: needinfo?(martin.thomson)
Attachment #8675118 - Flags: review?(kaie)
Attachment #8675118 - Flags: review?(ekr)
Attachment #8675118 - Flags: feedback?(wtc)
Comment on attachment 8675118 [details] [diff] [review]
win.patch

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

r=wtc.

::: coreconf/WIN32.mk
@@ +191,5 @@
>      # NSS has too many of these to fix, downgrade the warning
>      # Disable C4267: conversion from 'size_t' to 'type', possible loss of data
>      # Disable C4244: conversion from 'type1' to 'type2', possible loss of data
>      # Disable C4018: 'expression' : signed/unsigned mismatch
> +    # Disable C4312: 'type cast': conversion from 'type1' to 'type2' of greater size

Why does the compiler warn about such type casts? This should be safe, right?

::: external_tests/ssl_gtest/Makefile
@@ +57,5 @@
>  
>      # Linking to winsock to get htonl
>      OS_LIBS += Ws2_32.lib
> +else
> +    CXXFLAGS += -std=c++0x

This should ideally be set in coreconf/Linux.mk, Darwin.mk, or even UNIX.mk.

Or do you think the C++11 requirement only applies to ssl_gtest?

::: external_tests/ssl_gtest/ssl_gtest.cc
@@ -17,5 @@
>    g_working_dir_path = ".";
>  
> -  // Temporarily disable asserts for PKCS#11 slot leakage until
> -  // Bug 1168425 is fixed.
> -  unsetenv("NSS_STRICT_SHUTDOWN");

If you really need to unset the environment variable on Windows,
you can try

http://stackoverflow.com/questions/3205197/remove-environmental-variable-programmatically

::: external_tests/ssl_gtest/tls_agent.cc
@@ +236,5 @@
>    EXPECT_EQ(SECFailure, SSL_SignaturePrefSet(ssl_fd_, algorithms, 0))
>        << "setting no algorithms should fail and do nothing";
>  
> +  ASSERT_LE(count, 10) << "Can't have >10 signature algorithms";
> +  SSLSignatureAndHashAlg configuredAlgorithms[10];

We can use std::vector. Are you trying to avoid the STL?

::: tests/ssl_gtests/ssl_gtests.sh
@@ +105,5 @@
>      return
>    fi
>  
> +  # Temporarily disable asserts for PKCS#11 slot leakage (Bug 1168425)
> +  unset NSS_STRICT_SHUTDOWN

Ah, this is a much better solution.
Attachment #8675118 - Flags: feedback?(wtc) → feedback+
Comment on attachment 8675118 [details] [diff] [review]
win.patch

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

LGTM

::: external_tests/ssl_gtest/Makefile
@@ +57,5 @@
>  
>      # Linking to winsock to get htonl
>      OS_LIBS += Ws2_32.lib
> +else
> +    CXXFLAGS += -std=c++0x

So far I think it does. AFAIK gtests are so far the only C++ we have.

::: external_tests/ssl_gtest/tls_agent.cc
@@ +236,5 @@
>    EXPECT_EQ(SECFailure, SSL_SignaturePrefSet(ssl_fd_, algorithms, 0))
>        << "setting no algorithms should fail and do nothing";
>  
> +  ASSERT_LE(count, 10) << "Can't have >10 signature algorithms";
> +  SSLSignatureAndHashAlg configuredAlgorithms[10];

I tend to agree with WTC here.
Attachment #8675118 - Flags: review?(ekr) → review+
(In reply to Wan-Teh Chang from comment #15)
> > +    # Disable C4312: 'type cast': conversion from 'type1' to 'type2' of greater size
> 
> Why does the compiler warn about such type casts? This should be safe, right?

We have a bunch of places where an int is cast to void *.  MSVC hates that.

> Or do you think the C++11 requirement only applies to ssl_gtest?

Yeah, we can reconsider that later.

> We can use std::vector. Are you trying to avoid the STL?

Nah, just being daft.
Attached patch win.patchSplinter Review
Attachment #8675118 - Attachment is obsolete: true
Attachment #8675118 - Flags: review?(kaie)
Attachment #8675148 - Flags: review+
(In reply to Martin Thomson [:mt:] from comment #17)
> (In reply to Wan-Teh Chang from comment #15)
> > > +    # Disable C4312: 'type cast': conversion from 'type1' to 'type2' of greater size
> > 
> > Why does the compiler warn about such type casts? This should be safe, right?
> 
> We have a bunch of places where an int is cast to void *.  MSVC hates that.

That's strange. We probably should have macros for casts that compilers often
warn about. For example, we may be able to fix this compiler warning by first
casting the int to an intptr_t and then casting to void *.
I opened bug 1215700.  I agree that we should be careful about these, and macros are probably the best way to avoid issues.
(In reply to Martin Thomson [:mt:] from comment #19)
> Comment on attachment 8675148 [details] [diff] [review]
> win.patch
> 
> https://hg.mozilla.org/projects/nss/rev/71a3941348a2

This patch didn't work. It broke the build on most platforms (Linux, Mac, Windows).

With the patch, several directories aren't built. For example, lib/util isn't built, which results in compiler errors when attemping to build lib/base.

Here are two Mac build logs for you:

Revision f8c2b5564acc on default branch, just before your commit, which builds fine:
https://bot.nss-crypto.org:8011/builders/1-osx105-x32-DBG/builds/635/steps/shell/logs/stdio

Your revision:
https://bot.nss-crypto.org:8011/builders/1-osx105-x32-DBG/builds/634/steps/shell/logs/stdio


I've backed out:
https://hg.mozilla.org/projects/nss/rev/123decc65cef
Attachment #8675148 - Flags: checked-in+
Comment on attachment 8675148 [details] [diff] [review]
win.patch

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

::: coreconf/config.mk
@@ +190,5 @@
>  DEFINES += -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES
> +
> +ifdef NSS_DISABLE_GTESTS
> +DIRS := $(filter-out external_tests,$(DIRS))
> +endif

Kai, Martin: I'm sorry I missed this issue when I reviewed the patch.
I thought this config.mk file is a local config.mk file that is included
by just the Makefile in the same directory.

Martin: I guess my description of this approach wasn't clear. This
should be done in a local config.mk file. Since the top-level nss/
directory doesn't have a local config.mk file, you can either create
one, or just add this code to nss/Makefile, in the section

   (4) Include "local" platform-dependent assignments (OPTIONAL).

Here is the patch:

diff --git a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -21,17 +21,19 @@ include $(CORE_DEPTH)/coreconf/config.mk
 #######################################################################
 
 
 
 #######################################################################
 # (4) Include "local" platform-dependent assignments (OPTIONAL).      #
 #######################################################################
 
-
+ifdef NSS_DISABLE_GTESTS
+DIRS := $(filter-out external_tests,$(DIRS))
+endif
 
 #######################################################################
 # (5) Execute "global" rules. (OPTIONAL)                              #
 #######################################################################
 
 include $(CORE_DEPTH)/coreconf/rules.mk
 
 #######################################################################
diff --git a/manifest.mn b/manifest.mn
--- a/manifest.mn
+++ b/manifest.mn
@@ -5,13 +5,9 @@
 CORE_DEPTH = .
 DEPTH      = .
 
 IMPORTS =	nspr20/v4.8 \
 		$(NULL)
 
 RELEASE = nss
 
-DIRS = coreconf lib cmd
-
-ifndef NSS_DISABLE_GTESTS
-DIRS += external_tests
-endif
+DIRS = coreconf lib cmd external_tests
Ahh, I misunderstood.  New patch inbound (BTW, I couldn't reproduce any issue locally, but I guess that I have c++ 11 :)
https://hg.mozilla.org/projects/nss/rev/3df239b2f8e8  With Wan-Teh's amendment.  I'll watch buildbot for problems.
Martin: it seems that the directories nss/lib/{util,freebl,softoken}
were not build in the log file that Kai provided:
https://bot.nss-crypto.org:8011/builders/1-osx105-x32-DBG/builds/634/steps/shell/logs/stdio

I think the answer is at the bottom of nss/lib/Makefile:

#######################################################################
# (7) Execute "local" rules. (OPTIONAL).                              #
#######################################################################

ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
# Not included when building nss without softoken
UTIL_SRCDIR =
FREEBL_SRCDIR =
SOFTOKEN_SRCDIR =
else
# default is to include all
UTIL_SRCDIR = util
FREEBL_SRCDIR = freebl
SOFTOKEN_SRCDIR = softoken
endif

I guess it interacts badly with the use of := in the new code:

ifdef NSS_DISABLE_GTESTS
DIRS := $(filter-out external_tests,$(DIRS))
endi

and how nss/lib/manifest.mn defines DIRS:

DIRS = \
        $(UTIL_SRCDIR) \
        $(FREEBL_SRCDIR) \
        $(SQLITE_SRCDIR) \
        $(DBM_SRCDIR) \
        $(SOFTOKEN_SRCDIR) \
        base dev pki \
        libpkix \
        certdb certhigh pk11wrap cryptohi nss \
        $(ZLIB_SRCDIR) ssl \
        pkcs7 pkcs12 smime \
        crmf jar \
        ckfw $(SYSINIT_SRCDIR) \
        $(NULL)

So, the new code is only suitable when the value of
DIRS doesn't contain any variables, which is the case
for nss/manifest.mn.
Ahh, you can't mix lazy-evaluated variables (as in nss/lib/manifest.mn, as shown) with immediately evaluated variables like that.  Limiting this to the Makefile in the root as you suggested avoids that problem.
It appears that as it is, there is no way to disable the building of gtests, on either the trunk or in NSS 3.21 RTM. IMO, this is majorly broken. You were made aware that Oracle didn't have C++11 compilers on all (really, any, at this time) platforms. I am fine with the default being changed to build the C++11 tests, but at least an option to not build them should have remained somehow.  The current approach is hosed. See bug 1225377 .
Blocks: 1225377
I take back the previous comment - looks like my editor did not search all the files - I see that NSS_DISABLE_GTESTS is defined in nss/manifest.mn, and not just in coreconf/Linux.mk . Sorry for the rant.
No longer blocks: 1225377
external_tests/README was not updated and still says that it's disabled by default and that to enable it, you must set NSS_BUILD_GTESTS=1.
Attachment #8692625 - Flags: review?(martin.thomson)
Comment on attachment 8692625 [details] [diff] [review]
readme-1214390.patch

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

Thanks for catching this Kai.
Attachment #8692625 - Flags: review?(martin.thomson) → review+
Martin, Wan-Teh, 

can this bug be closed?

Did you address Wan-Teh's concerns for Windows builds, or does comment 26 and 27 mean there's still work to be done?
Flags: needinfo?(wtc)
Target Milestone: --- → 3.22
Yes, I think that it's OK.  I just forgot to list the commit here: https://hg.mozilla.org/projects/nss/rev/3df239b2f8e8  Clearing Wan-Teh's needinfo.
Flags: needinfo?(wtc)
thanks
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.