clang-format NSS: lib/ssl

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(firefox43 affected)

Details

Attachments

(10 attachments, 2 obsolete attachments)

326.31 KB, patch
Details | Diff | Splinter Review
2.38 KB, patch
kaie
: review+
Details | Diff | Splinter Review
34.12 KB, patch
Details | Diff | Splinter Review
1.15 KB, patch
Details | Diff | Splinter Review
6.69 KB, patch
kaie
: review+
Details | Diff | Splinter Review
1.71 KB, patch
Details | Diff | Splinter Review
1.76 MB, patch
Details | Diff | Splinter Review
265.12 KB, patch
Details | Diff | Splinter Review
98.04 KB, patch
Details | Diff | Splinter Review
3.14 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8661383 [details] [diff] [review]
Unconditional clang-format
Assignee: nobody → martin.thomson
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Comment on attachment 8661383 [details] [diff] [review]
Unconditional clang-format

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

I need to check the tables next, but this looks mostly OK.  A couple of things:

The ?: operator seems to be messed up in a lot of places.

The syntax for blocks inside of a switch statement is odd (see ssltrace.c).

::: lib/ssl/ssl3ext.c
@@ +1129,5 @@
>      cert_length = (ss->opt.requestCertificate && ss->sec.ci.sid->peerCert) ?
> +                                                                           3 +
> +                                                                               ss->sec.ci.sid->peerCert->derCert.len
> +                                                                           :
> +                                                                           0;

Whacky.

@@ +1431,5 @@
>      rv = ssl3_AppendHandshakeNumber(ss,
> +                                    message_length -
> +                                        sizeof(ticket.ticket_lifetime_hint) -
> +                                        2,
> +                                    2);

Fixme.

@@ +2084,5 @@
>      if (!sender) {
>          sender = ss->version > SSL_LIBRARY_VERSION_3_0 ?
> +                                                       &clientHelloSendersTLS[0]
> +                                                       :
> +                                                       &clientHelloSendersSSL3[0];

Fixme.

@@ +2125,5 @@
> +                           (ss->sec.isServer ?
> +                                             ss->ssl3.hs.finishedBytes *
> +                                                 2
> +                                             :
> +                                             ss->ssl3.hs.finishedBytes);

Whacky.

::: lib/ssl/ssl3prot.h
@@ +269,4 @@
>  typedef SSL3Opaque SSL3MasterSecret[48];
>  
> +typedef enum { implicit,
> +               explicit } SSL3PublicValueEncoding;

We really need to eliminate this enum somehow.  It's internal, so...

::: lib/ssl/sslauth.c
@@ +267,5 @@
>  	PORT_SetError(0);
>  	if (CERT_CacheOCSPResponseFromSideChannel(handle, ss->sec.peerCert, now,
>  						  &certStatusArray->items[0],
> +                                                  ss->pkcs11PinArg) !=
> +            SECSuccess) {

Please check.

::: lib/ssl/sslcon.c
@@ +48,5 @@
>      PRUint8           pubLen;	/* publicly reveal this many bytes of key. */
>      PRUint8           ivLen;	/* length of IV data at *ca.	*/
>  } ssl2Specs;
>  
>  static const ssl2Specs ssl_Specs[] = {

Don't bother protecting this crap.  (SSL2)

@@ +2617,4 @@
>  		certLen,
> +                                                  data +
> +                                                      SSL_HL_CLIENT_CERTIFICATE_HBYTES +
> +                                                      certLen,

Whacky.

@@ +3146,5 @@
>      msg[0] = SSL_MT_CLIENT_HELLO;
>      ss->clientHelloVersion = SSL3_ALL_VERSIONS_DISABLED(&ss->vrange) ?
> +                                                                     SSL_LIBRARY_VERSION_2
> +                                                                     :
> +                                                                     ss->vrange.max;

Whacky.

@@ +3269,5 @@
> +                                       ekLen,
> +                                       data +
> +                                           SSL_HL_CLIENT_MASTER_KEY_HBYTES +
> +                                           ckLen + ekLen,
> +                                       caLen);

Whacky.

::: lib/ssl/sslimpl.h
@@ +518,1 @@
>                                      * it to 712. */

Strange.

@@ +1029,1 @@
>  				 * headers, so slightly larger than expected */

Strange.

@@ +1513,5 @@
>  #define SSL3_BUFFER_FUDGE     100 + SSL3_COMPRESSION_MAX_EXPANSION
>  
> +#define SSL_LOCK_READER(ss) \
> +    if (ss->recvLock)       \
> +    PZ_Lock(ss->recvLock)

do{}while(0) ?

@@ +1711,5 @@
> +                                     ((s <=   \
> +                                       7168)  \
> +                                          ?   \
> +                                          384 \
> +                                          : 521))))

Damn.

::: lib/ssl/sslinfo.c
@@ +165,5 @@
>  
>  static const SSLCipherSuiteInfo suiteInfo[] = {
>  /* <------ Cipher suite --------------------> <auth> <KEA>  <bulk cipher> <MAC> <FIPS> */
> +    {
> +        0, CS(TLS_RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_RSA, C_AESGCM, B_128, M_AEAD_128, 1, 0, 0,

Add #define for 0 and 1 values.

::: lib/ssl/sslsock.c
@@ +279,5 @@
>                      sc->serverCertChain = NULL;
>                  }
>                  sc->serverKeyPair = oc->serverKeyPair ?
> +                                                      ssl3_GetKeyPairRef(oc->serverKeyPair)
> +                                                      : NULL;

These always seem to go poorly.

::: lib/ssl/ssltrace.c
@@ +193,5 @@
>  	    PrintInt(ss, "version (minor)",                    bp[3]);
>  	    PrintBuf(ss, "certificate",          bp+11,        lc);
>  	    PrintBuf(ss, "cipher-specs",         bp+11+lc,     lcs);
>  	    PrintBuf(ss, "connection-id",        bp+11+lc+lcs, lci);
> +        } break;

Strange choice of structure.
(Assignee)

Comment 3

3 years ago
Here's my plan for protection of tables, based on looking at what has been changed by clang-format.

To protect:
 static const ssl3BulkCipherDef bulk_cipher_defs[] = {

To protect, with note that this is only until export ciphers are removed:
 static const ssl3KEADef kea_defs[] =

No protection:
static const ssl3CipherSuiteDef cipher_suite_defs[] = 
static const SECOidTag ecName2OIDTag[] = {
static const PRUint16 curve2bits[] = {
static const Bits2Curve bits2curve [] = {
static const ssl3HelloExtensionHandler clientHelloHandlers[] = {
static const ssl3HelloExtensionHandler serverHelloHandlersTLS[] = {
static const ssl3HelloExtensionHandler serverHelloHandlersSSL3[] = {
ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS] = {

No protection because we should be deleting them:
static const ssl2Specs ssl_Specs[] = {

No protection, but add #defines to make values self-describing:
static const ssl3MACDef mac_defs[] = {
static const SSLCipherSuiteInfo suiteInfo[] = {
(Assignee)

Comment 4

3 years ago
Created attachment 8661543 [details] [diff] [review]
bug1204998-1.patch
Attachment #8661543 - Flags: review?(kaie)
(Assignee)

Comment 5

3 years ago
Created attachment 8661544 [details] [diff] [review]
bug1204998-2.patch
Attachment #8661544 - Flags: review?(ekr)
(Assignee)

Comment 6

3 years ago
Created attachment 8661547 [details] [diff] [review]
bug1204998-3.patch
Attachment #8661547 - Flags: review?(ekr)
(Assignee)

Comment 7

3 years ago
Created attachment 8661548 [details] [diff] [review]
bug1204998-4.patch
Attachment #8661548 - Flags: review?(kaie)
(Assignee)

Comment 8

3 years ago
Created attachment 8661549 [details] [diff] [review]
bug1204998-5.patch
Attachment #8661549 - Flags: review?(ekr)
(Assignee)

Comment 9

3 years ago
Created attachment 8661551 [details] [diff] [review]
bug1204998-6.patch
Attachment #8661551 - Flags: review?(ekr)
(Assignee)

Comment 10

3 years ago
Created attachment 8661552 [details] [diff] [review]
bug1204998-7.patch
Attachment #8661552 - Flags: review?(ekr)
(Assignee)

Comment 11

3 years ago
TODO: check that the binaries are the same.

Updated

3 years ago
Attachment #8661543 - Flags: review?(kaie) → review+
(Assignee)

Comment 14

3 years ago
Comment on attachment 8661549 [details] [diff] [review]
bug1204998-5.patch

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

Hi Kai, Franziskus, do you think that this would help at all?
Attachment #8661549 - Flags: review?(kaie)
Attachment #8661549 - Flags: review?(franziskuskiefer)
Attachment #8661549 - Flags: review?(ekr)
(Assignee)

Updated

3 years ago
Attachment #8661552 - Flags: review?(ekr)
(Assignee)

Updated

3 years ago
Attachment #8661551 - Flags: review?(ekr)
(Assignee)

Updated

3 years ago
Attachment #8661547 - Flags: review?(ekr)
(Assignee)

Updated

3 years ago
Attachment #8661544 - Flags: review?(ekr)

Comment 15

2 years ago
(In reply to Martin Thomson [:mt:] from comment #14)
> 
> Hi Kai, Franziskus, do you think that this would help at all?

Martin, do you intend to use it?

My understanding was, we'd only run reformat occassionally, on selected occassions, but we don't intend to regularly reformat everything.

If that's true, I don't know if it will help anyone.

But if you intend to use it, I don't mind if we check it in.
(Assignee)

Comment 16

2 years ago
I don't know; it's not difficult to run clang-format on its own.
Created attachment 8746477 [details] [diff] [review]
travis-with-clang-format.patch

This patch adds a travis.yml that runs clang-format on libssl as well as builds and gtests on Mac.
I chose travis here so that we don't have to break the circle.ci builds and we can get Mac builds. Who ever has access to the nss-dev github repo (ekr?) would have to enable travis to make this work. An example run is here [1] (the clang-format fails, I'll upload a patch here that should fix that).

[1] https://travis-ci.org/franziskuskiefer/nss/builds/126342639
Attachment #8746477 - Flags: review?(martin.thomson)
Attachment #8746477 - Flags: feedback?(ekr)
Created attachment 8746478 [details] [diff] [review]
libssl-clang-format-3.8.patch

this is clang-format on libssl without any manual changes using 3.8. So after applying this one the travis clang-format run should be green.
Attachment #8746478 - Flags: review?(martin.thomson)
Created attachment 8746523 [details] [diff] [review]
travis-with-clang-format.patch

allow clang-format job to fail
Attachment #8746477 - Attachment is obsolete: true
Attachment #8746477 - Flags: review?(martin.thomson)
Attachment #8746477 - Flags: feedback?(ekr)
Attachment #8746523 - Flags: review?(martin.thomson)
Attachment #8746523 - Flags: review?(ekr)
Comment on attachment 8746523 [details] [diff] [review]
travis-with-clang-format.patch

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

Generally LGTM

::: .travis.yml
@@ +33,5 @@
> +      env: BUILD_OPT=1
> +    - os: linux
> +      env: CLANG_FORMAT=1
> +  allow_failures:
> +    - env: CLANG_FORMAT=1

Can we add ASAN to these?

@@ +46,5 @@
> +  - if [ -n "$CLANG_FORMAT" ];
> +    then
> +      cd nss;
> +      automation/travis/subdir-clang-format.sh lib/ssl;
> +      automation/travis/subdir-clang-format-diff.sh lib/ssl;

I'm surrpised these end up here. How does that happen?

::: automation/travis/subdir-clang-format-diff.sh
@@ +5,5 @@
> +        echo "usage: $0 <Directory to diff *.c/h against *.c./h.orig> <diff args>"
> +        exit
> +fi
> +
> +for file in `(find $1 -type f -name "*.c.orig" && find $1 -type f -name "*.h.orig")`;

Can't you do *.[ch].orig

::: automation/travis/subdir-clang-format.sh
@@ +6,5 @@
> +        exit
> +fi
> +
> +export ORIG=".orig"
> +for file in `(find $1 -type f -name "*.c" && find $1 -type f -name "*.h")`;

*.[ch]
Attachment #8746523 - Flags: review?(ekr) → review+
(Assignee)

Comment 21

2 years ago
Comment on attachment 8746523 [details] [diff] [review]
travis-with-clang-format.patch

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

I think that this needs a little more work.  Note that the r- isn't because this is bad, it's pretty good, but clearing the review with another r+ can be confusing.

::: .travis.yml
@@ +1,1 @@
> +sudo: required

Can we get away with running in a container rather than requiring sudo?  I see that you don't actually need sudo.

@@ +9,5 @@
> +  apt:
> +    sources:
> +    # add PPAs with more up-to-date toolchains
> +    - ubuntu-toolchain-r-test
> +    - llvm-toolchain-precise-3.8

Is there a source for trusty rather than precise?

@@ +31,5 @@
> +    - os: linux
> +      compiler: gcc
> +      env: BUILD_OPT=1
> +    - os: linux
> +      env: CLANG_FORMAT=1

So this config runs the clang-format checks without allowing failures?

@@ +36,5 @@
> +  allow_failures:
> +    - env: CLANG_FORMAT=1
> +
> +before install:
> +  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export CC="gcc-5"; export CXX="g++-5"; fi

CXX or CCC?  I thought it was CCC in NSS.

@@ +37,5 @@
> +    - env: CLANG_FORMAT=1
> +
> +before install:
> +  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export CC="gcc-5"; export CXX="g++-5"; fi
> +  - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew update; brew install llvm38; fi;

nit: trailing semi

@@ +39,5 @@
> +before install:
> +  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export CC="gcc-5"; export CXX="g++-5"; fi
> +  - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew update; brew install llvm38; fi;
> +
> +install: cd ..; hg clone https://hg.mozilla.org/projects/nspr

install:
  - hg clone https://hg.mozilla.org/projects/nspr ../nspr

@@ +46,5 @@
> +  - if [ -n "$CLANG_FORMAT" ];
> +    then
> +      cd nss;
> +      automation/travis/subdir-clang-format.sh lib/ssl;
> +      automation/travis/subdir-clang-format-diff.sh lib/ssl;

I would rather see the set of directories to check in the automation scripts themselves.  Then you only have to do this:

  - [ -n "$CLANG_FORMAT" ] && run_script

Much simpler.

@@ +50,5 @@
> +      automation/travis/subdir-clang-format-diff.sh lib/ssl;
> +      exit $?;
> +    fi
> +  - cd nss
> +  - USE_64=1 NSS_ENABLE_TLS_1_3=1 make nss_build_all

I didn't realize that cd worked like this in travis scripts, I think that the line above this does nothing.

::: automation/travis/subdir-clang-format.sh
@@ +5,5 @@
> +        echo "usage: $0 <Directory to reformat with clang-format>"
> +        exit
> +fi
> +
> +export ORIG=".orig"

don't export this

@@ +6,5 @@
> +        exit
> +fi
> +
> +export ORIG=".orig"
> +for file in `(find $1 -type f -name "*.c" && find $1 -type f -name "*.h")`;

No parens either.

@@ +10,5 @@
> +for file in `(find $1 -type f -name "*.c" && find $1 -type f -name "*.h")`;
> +do
> +  echo $file
> +  mv "$file" "$file$ORIG"
> +  clang-format-3.8 "$file$ORIG" > "$file"

This would be simpler:

find . -type f -name '*.[ch]' -print -exec clang-format-3.8 -i {} \+

However, I think that you want to combine these two scripts into a single one that does the diff check without writing the files to the filesystem.

STATUS=0
for i in $(find . -type f -name '*.[ch]' -print); do
  if ! clang-format $i | diff -q $i - >/dev/null; then
    echo warning....
    STATUS=1
  fi
done
exit $STATUS
Attachment #8746523 - Flags: review?(martin.thomson) → review-
(Assignee)

Comment 22

2 years ago
Comment on attachment 8746478 [details] [diff] [review]
libssl-clang-format-3.8.patch

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

A few things that are a bit ugly, but I won't ask you to fix them.  We can fix them as we modify the code.

::: lib/ssl/ssl3con.c
@@ +5601,5 @@
> +             1 + (((sid == NULL) || sid->version >= SSL_LIBRARY_VERSION_TLS_1_3)
> +                      ? 0
> +                      : sid->u.ssl3.sessionIDLength) +
> +             2 + num_suites * sizeof(ssl3CipherSuite) +
> +             1 + numCompressionMethods + total_exten_len;

This is a terrible way to avoid creating temporaries, but the reformatting is OK.

::: lib/ssl/ssl3ecc.c
@@ +1423,5 @@
>      for (cursor = PR_NEXT_LINK(&ss->serverCerts);
>           cursor != &ss->serverCerts;
>           cursor = PR_NEXT_LINK(cursor)) {
> +        sslServerCert *cert = (sslServerCert *)cursor;
> +        if (cert->certType.authType == ssl_auth_ecdh_rsa && (mutualCurves & (1U << cert->certType.u.namedCurve))) {

This created a line that is too long.  Does moving the && to the end of the line change what clang-format does here?

::: lib/ssl/sslinfo.c
@@ +22,5 @@
>  
> +#define SSL_CHANNEL_INFO_FIELD_SET(info, field, value) \
> +    if (SSL_CHANNEL_INFO_FIELD_EXISTS(info, field)) {  \
> +        info.UseMacroToAccess_##field = value;         \
> +    }

Careful, I think that this change is being backed out.  (Just run clang-format again, you will get the same r+.)

::: lib/ssl/sslt.h
@@ +206,3 @@
>  
> +#define SSL_CHANNEL_INFO_FIELD_GET(info, field) \
> +    (SSL_CHANNEL_INFO_FIELD_EXISTS(info, field) ? info.UseMacroToAccess_##field : -1)

See previous note.

::: lib/ssl/tls13con.c
@@ +2148,5 @@
>          goto alert_loser;
>      }
>  
>      rv = ssl3_FlushHandshake(ss,
> +                             (IS_DTLS(ss) && !ss->sec.isServer) ? ssl_SEND_FLAG_NO_RETRANSMIT : 0);

This is part of why I like using temporaries.
Attachment #8746478 - Flags: review?(martin.thomson) → review+
(In reply to Eric Rescorla (:ekr) from comment #20)
> Comment on attachment 8746523 [details] [diff] [review]
> travis-with-clang-format.patch
> 
> Review of attachment 8746523 [details] [diff] [review]:
> -----------------------------------------------------------------
> Can we add ASAN to these?
Yep, I can try.

> @@ +46,5 @@
> > +  - if [ -n "$CLANG_FORMAT" ];
> > +    then
> > +      cd nss;
> > +      automation/travis/subdir-clang-format.sh lib/ssl;
> > +      automation/travis/subdir-clang-format-diff.sh lib/ssl;
> 
> I'm surrpised these end up here. How does that happen?
They're part of the patch and needs to get checked in. But as mt suggested, I'll move the entire thing into a script.

> ::: automation/travis/subdir-clang-format-diff.sh
> > +for file in `(find $1 -type f -name "*.c.orig" && find $1 -type f -name "*.h.orig")`;
> Can't you do *.[ch].orig
yep I can...

(In reply to Martin Thomson [:mt:] from comment #21)
> Comment on attachment 8746523 [details] [diff] [review]
> travis-with-clang-format.patch
> 
> Review of attachment 8746523 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: .travis.yml
> @@ +1,1 @@
> > +sudo: required
> 
> Can we get away with running in a container rather than requiring sudo?  I
> see that you don't actually need sudo.
we need sudo to install all the things. So at least on Ubuntu we need sudo. But I can look into this.

> @@ +9,5 @@
> > +  apt:
> > +    sources:
> > +    # add PPAs with more up-to-date toolchains
> > +    - ubuntu-toolchain-r-test
> > +    - llvm-toolchain-precise-3.8
> 
> Is there a source for trusty rather than precise?
yep (https://docs.travis-ci.com/user/trusty-ci-environment/), will do

> @@ +31,5 @@
> > +    - os: linux
> > +      compiler: gcc
> > +      env: BUILD_OPT=1
> > +    - os: linux
> > +      env: CLANG_FORMAT=1
> 
> So this config runs the clang-format checks without allowing failures?
Nope, it allows failures. That's a bit confusing, but the |allow_failures| matrix entry says that if CLANG_FORMAT is set it is allowed to fail. This is the |include| entry that say we want to execute our script with this setting.

> @@ +36,5 @@
> > +  allow_failures:
> > +    - env: CLANG_FORMAT=1
> > +
> > +before install:
> > +  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export CC="gcc-5"; export CXX="g++-5"; fi
> 
> CXX or CCC?  I thought it was CCC in NSS.
yes we are... but I just noticed that it still builds with gcc and g++... I have to check which version it actually uses.

> @@ +50,5 @@
> > +      automation/travis/subdir-clang-format-diff.sh lib/ssl;
> > +      exit $?;
> > +    fi
> > +  - cd nss
> > +  - USE_64=1 NSS_ENABLE_TLS_1_3=1 make nss_build_all
> 
> I didn't realize that cd worked like this in travis scripts, I think that
> the line above this does nothing.

if it wouldn't, it wouldn't work... looking at the output I see
> 0.02s$ cd nss
> The command "cd nss" exited with 0.
> 93.76s$ USE_64=1 NSS_ENABLE_TLS_1_3=1 make nss_build_all

> However, I think that you want to combine these two scripts into a single
> one that does the diff check without writing the files to the filesystem.
> 
> STATUS=0
> for i in $(find . -type f -name '*.[ch]' -print); do
>   if ! clang-format $i | diff -q $i - >/dev/null; then
>     echo warning....
>     STATUS=1
>   fi
> done
> exit $STATUS
yeah I could do that. I had those scripts around so thought I use them, but that's indeed nicer.
(Assignee)

Comment 24

2 years ago
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #23)
> we need sudo to install all the things. So at least on Ubuntu we need sudo.
> But I can look into this.

I found that you can still install without sudo: https://github.com/martinthomson/i-d-template/blob/master/template/.travis.yml#L2-L6

But adding sources might need sudo.  Test it and see.  Containers are MUCH faster to start.

> > @@ +31,5 @@
> > > +    - os: linux
> > > +      compiler: gcc
> > > +      env: BUILD_OPT=1
> > > +    - os: linux
> > > +      env: CLANG_FORMAT=1
> > 
> > So this config runs the clang-format checks without allowing failures?
> Nope, it allows failures. That's a bit confusing, but the |allow_failures|
> matrix entry says that if CLANG_FORMAT is set it is allowed to fail. This is
> the |include| entry that say we want to execute our script with this setting.

Oh, that is a very confusing structure.  env appears both above and below the allow_failures line.  Maybe a comment.
 
> > CXX or CCC?  I thought it was CCC in NSS.
> yes we are... but I just noticed that it still builds with gcc and g++... I
> have to check which version it actually uses.

g++ is simply the default.
 
> > @@ +50,5 @@
> > > +      automation/travis/subdir-clang-format-diff.sh lib/ssl;
> > > +      exit $?;
> > > +    fi
> > > +  - cd nss
> > > +  - USE_64=1 NSS_ENABLE_TLS_1_3=1 make nss_build_all
> > 
> > I didn't realize that cd worked like this in travis scripts, I think that
> > the line above this does nothing.
> 
> if it wouldn't, it wouldn't work... looking at the output I see

Ahh, that's it: it doesn't do anything.  You start every command from the nss directory.  You can delete the 'cd nss' line and it will still work.
> But adding sources might need sudo.  Test it and see.  Containers are MUCH faster to start.
yep, but actually it's trusty, not installing that requires sudo (see https://docs.travis-ci.com/user/trusty-ci-environment/)

> g++ is simply the default.
right. I've fixed the gcc version on Linux, still fighting with the OSX builds and the llvm version.

> Ahh, that's it: it doesn't do anything.  You start every command from the nss directory.  You can delete the 'cd nss' line and it will still work.
I noticed... deleted it already.
Created attachment 8747069 [details] [diff] [review]
travis-with-clang-format.patch

I added asan and fixed most of the other things you mentioned. The only thing I'm not sure about is the clang version that's used on mac builds. But it works and looks all green (https://travis-ci.org/franziskuskiefer/nss/builds/126650822)
Attachment #8746523 - Attachment is obsolete: true
Attachment #8747069 - Flags: review?(martin.thomson)
(Assignee)

Comment 27

2 years ago
Comment on attachment 8747069 [details] [diff] [review]
travis-with-clang-format.patch

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

A few nits only.

::: .travis.yml
@@ +53,5 @@
> +install:
> +  - hg clone https://hg.mozilla.org/projects/nspr ../nspr
> +
> +script:
> +  - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update; brew install llvm38; fi

Why is this not in the install phase?

@@ +54,5 @@
> +  - hg clone https://hg.mozilla.org/projects/nspr ../nspr
> +
> +script:
> +  - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update; brew install llvm38; fi
> +  - if [ -n "$CLANG_FORMAT" ]; then automation/travis/subdir-clang-format.sh lib/ssl; exit $?; fi

This is cleaner as:

[ -n "$CLANG_FORMAT" ] && automation/travis/subdir-clang-format.sh lib/ssl

Same for the brew install line.

@@ +55,5 @@
> +
> +script:
> +  - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update; brew install llvm38; fi
> +  - if [ -n "$CLANG_FORMAT" ]; then automation/travis/subdir-clang-format.sh lib/ssl; exit $?; fi
> +  # - gcc --version; g++ --version; $CC --version; $CCC --version

You can drop this one.

@@ +56,5 @@
> +script:
> +  - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update; brew install llvm38; fi
> +  - if [ -n "$CLANG_FORMAT" ]; then automation/travis/subdir-clang-format.sh lib/ssl; exit $?; fi
> +  # - gcc --version; g++ --version; $CC --version; $CCC --version
> +  - USE_64=1 NSS_ENABLE_TLS_1_3=1 make nss_build_all

Can you put the two env vars in the environment instead?

::: automation/travis/subdir-clang-format.sh
@@ +1,1 @@
> +#!/bin/sh

The name of this file needs to be changed.  Also, add a copyright notice and a brief description of why the file exists.

@@ +1,5 @@
> +#!/bin/sh
> +
> +STATUS=0
> +for i in $(find $1 -type f -name '*.[ch]' -print); do
> +  if ! clang-format-3.8 $i | diff $i - > tmpDiff; then

If you want to print differences, then don't write to the file, just let the diff output to the screen.  The error will then appear below the file, which is better in my opinion.
Attachment #8747069 - Flags: review?(martin.thomson) → review+
landed the lib/ssl clang-format-3.8 patch as https://hg.mozilla.org/projects/nss/rev/09756fe49747

I'll close this bug now. Let's see how this works. The travis builds including clang-format are here https://travis-ci.org/nss-dev/nss
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8661549 [details] [diff] [review]
bug1204998-5.patch

clearing r? for now. Let's see how the travis builds are working out. But if you say you'd like get the format target landed, that's fine with me.
Attachment #8661549 - Flags: review?(kaie)
Attachment #8661549 - Flags: review?(franziskuskiefer)
You need to log in before you can comment on or make changes to this bug.