Remove temporary BORINGSSL_YYYYMM #ifdefs.

[davidben]

We can assume a new enough BoringSSL now.

[pphaneuf]

It seems to have compilation problems with BoringSSL:

cpp/log/cert.cc:385:18: error: use of undeclared identifier
      'ASN1_R_UNKNOWN_SIGNATURE_ALGORITHM'
       reason == ASN1_R_UNKNOWN_SIGNATURE_ALGORITHM)) {

And a plain old test failing with OpenSSL? There's actually multiple issues, it seems...

[Martin2112]

That's maybe because I merged the DSA related PR yesterday? I think this
might conflict.

Martin

On 5 May 2016 at 11:09, Pierre Phaneuf notifications@github.com wrote:

It seems to have compilation problems with BoringSSL:

cpp/log/cert.cc:385:18: error: use of undeclared identifier
'ASN1_R_UNKNOWN_SIGNATURE_ALGORITHM'
reason == ASN1_R_UNKNOWN_SIGNATURE_ALGORITHM)) {

And a plain old test failing with OpenSSL? There's actually multiple
issues, it seems...

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1166 (comment)

[pphaneuf]

His branch is from right before. He's going to have to rebase it and fix the conflicts, for sure.

[davidben]

Yeah, I can rebase it. The patch pretty clearly cannot affect OpenSSL though. The failure seems to be

�[31;1mFAILED: tar -Pzcf /Users/travis/.casher/push.tgz /Users/travis/.cache/pip /Users/travis/.ccache /Users/travis/build/google/ct /Users/travis/build/google/depot_tools /Users/travis/build/google/install�[0m

tar: /Users/travis/.ccache: Cannot stat: No such file or directory

tar: Error exit delayed from previous errors.



�[32;1muploading archive�[0m

which looks like an infrastructure flake. Unless there's a failure elsewhere I can't find. (Your build logs are rather insane long... you might want to improve the error-reporting here.)

The BoringSSL failure is because you're pinned against a much older BoringSSL than the internal repository. What else uses this code with BoringSSL? If you legitimately need to support something older, we can keep the temporarily #ifdefs around for you. If not, we need to point DEPS to something else so we do not have to unnecessarily keep CT-specific hacks in BoringSSL for longer than necessary.

[pphaneuf]

The logs are rather insane long "because gclient (and OpenSSL and BoringSSL and libICU)". 😞

If you search for "exited with", you'll find the The command "${MAKE} -j$(getconf _NPROCESSORS_ONLN) check VERBOSE=1 V=${ENV_VERBOSE}" exited with 2. in https://travis-ci.org/google/certificate-transparency/jobs/126699938

A bit above, you'll find some relevant error messages.

Use the same "exited with" technique with this other one, it has a build failure somewhere a bit before: https://travis-ci.org/google/certificate-transparency/jobs/126699939

We use gclient to pick the "correct" version of BoringSSL, if this is to follow changes to BoringSSL, then something will need updating in DEPS as well.

You shouldn't need to maintain anything in BoringSSL, because we're just "stuck" on some arbitrary older version that happened to work when we added it. 😉

[davidben]

I see. Thanks! So it's

[ RUN      ] EtcdConsistentStoreTest.TestRejectsAddsWhenOverCapacity

cpp/log/etcd_consistent_store_test.cc:787: Failure

Expected: (2) < (GetNumEtcdEntries()), actual: 2 vs 0

cpp/log/etcd_consistent_store_test.cc:791: Failure

Value of: store_->AddPendingEntry(&cert)

Expected: util::Status(code is equal to 8 and message is anything)

  Actual: OK (of type util::Status)

[  FAILED  ] EtcdConsistentStoreTest.TestRejectsAddsWhenOverCapacity (2204 ms)

Has that test flaked before, or is this the first time? The CL should only have an effect in OPENSSL_IS_BORINGSSL is defined.

Alright, well, if you all don't mind my bumping DEPS forward, I'll go ahead and do that when I'm rebasing then. The context here is you got caught in an multi-sided change. This BORINGSSL_YYYYMM hack is just meant as a temporary thing when we can't update code atomically. Ideally we take them away fairly quickly, but we can't do that until all relevant consumers are past the transition.

(To be honest, that arrangement hasn't worked out that well because of this kind of skew. You actually aren't on inherently that old of a BoringSSL---what Chrome is shipping---but Chrome snapshots the consumer atomically with BoringSSL. It might be worth adding a single BORINGSSL_VERSION macro that we just bump whenever is convenient. But we don't really want to be in the versioning business, so I dunno.)

[Martin2112]

I've seen tests in etcd flake because they rely on hardcoded sleeps. I
don't remember seeing that one but I'd try running it again.

The massive build log is a real pain but not sure there's an easy solution.

Martin

On 5 May 2016 at 14:55, David Benjamin notifications@github.com wrote:

I see. Thanks! So it's

[ RUN ] EtcdConsistentStoreTest.TestRejectsAddsWhenOverCapacity

cpp/log/etcd_consistent_store_test.cc:787: Failure

Expected: (2) < (GetNumEtcdEntries()), actual: 2 vs 0

cpp/log/etcd_consistent_store_test.cc:791: Failure

Value of: store_->AddPendingEntry(&cert)

Expected: util::Status(code is equal to 8 and message is anything)

Actual: OK (of type util::Status)

[ FAILED ] EtcdConsistentStoreTest.TestRejectsAddsWhenOverCapacity (2204 ms)

Has that test flaked before, or is this the first time? The CL should only
have an effect in OPENSSL_IS_BORINGSSL is defined.

Alright, well, if you all don't mind my bumping DEPS forward, I'll go
ahead and do that when I'm rebasing then. The context here is you got
caught in an multi-sided change. This BORINGSSL_YYYYMM hack is just meant
as a temporary thing when we can't update code atomically. Ideally we take
them away fairly quickly, but we can't do that until all relevant consumers
are past the transition.

(To be honest, that arrangement hasn't worked out that well because of
this kind of skew. You actually aren't on inherently that old of a
BoringSSL---what Chrome is shipping---but Chrome snapshots the consumer
atomically with BoringSSL. It might be worth adding a single
BORINGSSL_VERSION macro that we just bump whenever is convenient. But we
don't really want to be in the versioning business, so I dunno.)

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#1166 (comment)

[pphaneuf]

The build failure of the ASan one was that flaky test, I just re-ran it and it passed.

[eranmes]

LGTM for the code changes, assuming they haven't broken Travis (seems like it was flakiness).

[eranmes]

David, mind re-basing ? If the rebased commit passes travis and nobody else has any objections, I'll merge.

[eranmes]

@davidben ping - please rebase...

[davidben]

Hrmf. So I rebased and bumped BoringSSL up to a new enough revision, but Travis is unhappy:
https://travis-ci.org/google/certificate-transparency/jobs/181836538

It looks like it's hitting the case for where your DEPS file says to:

# If you change this in an existing client, you should probably rm -fr
# all the deps and rebuild everything from scratch.

But since you all cache the build directory, it doesn't end up doing that. That said, I'm rather puzzled because we don't have this problem in Chromium. Are you all doing something weird to depot_tools?

[daviddrysdale]

I cleared the Travis caches and triggered a rebuild, which looks to have worked OK.

[eranmes]

@davidben happy for me to merge, then?

[davidben]

If the tests pass, sure. Sorry I took to long to revisit this CL.