1214390 - Build NSS gtests by default

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Martin Thomson [:mt:]]

Agreed on 2015-10-13: we should just require a C++ compiler.

Comment 1

[Martin Thomson [:mt:]]

Attachment: bug1214390.patch

Comment 2

[Eric Rescorla (:ekr)]

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 += ...

Comment 3

[Martin Thomson [:mt:]]

I was flip-flopping.  I made the change you suggested, tested it and checked it in.

https://hg.mozilla.org/projects/nss/rev/8cae181b0330

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Kai Engert [:KaiE:]]

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?

Comment 6

[Kai Engert [:KaiE:]]

(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.

Comment 7

[Kai Engert [:KaiE:]]

(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.

Comment 8

[Martin Thomson [:mt:]]

Thanks Kai, I think that I should move the tweaks to config.mk as Wan-Teh has suggested.  I'll do that shortly.

Comment 9

[Wan-Teh Chang]

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?

Comment 10

[Kai Engert [:KaiE:]]

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.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: 1214390-v2.patch

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 8674474 [details] [diff] [review]
1214390-v2.patch

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

This is fine with me

Comment 13

[Kai Engert [:KaiE:]]

Thanks!
https://hg.mozilla.org/projects/nss/rev/f8c2b5564acc

Comment 14

[Martin Thomson [:mt:]]

Attachment: win.patch

I think that this covers all the outstanding niggles.  I discovered some new problems with MSVC 2015 while working on this.

Comment 15

[Wan-Teh Chang]

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.

Comment 16

[Eric Rescorla (:ekr)]

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.

Comment 17

[Martin Thomson [:mt:]]

(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.

Comment 18

[Martin Thomson [:mt:]]

Attachment: win.patch

Comment 19

[Martin Thomson [:mt:]]

Comment on attachment 8675148 [details] [diff] [review]
win.patch

https://hg.mozilla.org/projects/nss/rev/71a3941348a2

Comment 20

[Wan-Teh Chang]

(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 *.

Comment 21

[Martin Thomson [:mt:]]

I opened bug 1215700.  I agree that we should be careful about these, and macros are probably the best way to avoid issues.

Comment 22

[Kai Engert [:KaiE:]]

(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

Comment 23

[Wan-Teh Chang]

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

Comment 24

[Martin Thomson [:mt:]]

Ahh, I misunderstood.  New patch inbound (BTW, I couldn't reproduce any issue locally, but I guess that I have c++ 11 :)

Comment 25

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/3df239b2f8e8  With Wan-Teh's amendment.  I'll watch buildbot for problems.

Comment 26

[Wan-Teh Chang]

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.

Comment 27

[Martin Thomson [:mt:]]

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.

Comment 28

[Julien Pierre]

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 .

Comment 29

[Julien Pierre]

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.

Comment 30

[Mike Hommey [:glandium]]

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.

Comment 31

[Kai Engert [:KaiE:]]

Attachment: readme-1214390.patch

Comment 32

[Martin Thomson [:mt:]]

Comment on attachment 8692625 [details] [diff] [review]
readme-1214390.patch

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

Thanks for catching this Kai.

Comment 33

[Kai Engert [:KaiE:]]

Comment on attachment 8692625 [details] [diff] [review]
readme-1214390.patch

https://hg.mozilla.org/projects/nss/rev/61debbbdf411

Comment 34

[Kai Engert [:KaiE:]]

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?

Comment 35

[Martin Thomson [:mt:]]

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.

Comment 36

[Kai Engert [:KaiE:]]

thanks