Fix issue where travis does not show the ./tests seed… #685

jonasnick · 2019-11-02T19:52:36Z

…by removing stdout buffering and always cat tests.log after a travis run. Fixes #645.

I noticed that according to the doc tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call fflush twice.

Travis is instructed to always show the seed which seems helpful with after_script by cating ./tests.log. In case the tests fail it looks like https://travis-ci.org/jonasnick/secp256k1/jobs/606446234.

real-or-random

Thanks for picking up this issue, looks good!

ACK except nit

real-or-random · 2019-11-03T15:50:48Z

src/tests.c

@@ -5208,6 +5208,7 @@ int main(int argc, char **argv) {
    }
    secp256k1_rand_seed(seed16);

+    setbuf(stdout, NULL);


C89 says setbuf should be called before any operation is performed on the stream. This means that it is currently in the right position but I think it would be less fragile to put at the beginning of main(). (For example, someone could have the idea of changing the fprintf(stderr, ...) in line 5195 to stdout.) Also, I think this call deserves a comment with a rationale.

Good point. I moved the call to the beginning of main and added comments.

real-or-random · 2019-11-04T08:41:29Z

ACK 8de4392 I read the code and I checked that it does not break the tests

real-or-random · 2019-11-04T14:31:06Z

Actually this would not fix the Fedora example in #645 because we still don't output the seed to stderr in case of failure. Maybe it's better to make make check cat the log files to stderr in case of failure.

jonasnick · 2019-11-04T22:25:26Z

Good point, but I'm tempted to blame this on the user. With automake you could run make check with VERBOSE=1 which will print the stdout and stderr of a failed test to stdout. So not capturing stdout seems to be not a good idea when you need diagnostics anyway. Setting VERBOSE=1 should be possible in a build system and is probably useful for other automake based packages too.

If I understand correctly what you're asking for is to define a custom test runner for which my automake skills are insufficient. And it could be argued that such a non-standard customization isn't particularly helpful in the long run.

real-or-random · 2019-11-05T00:16:10Z

Agreed, we don't want to fiddle around with automake if this can't be done easily.

real-or-random · 2019-11-05T12:34:47Z

needs rebase

jonasnick · 2019-11-05T13:19:09Z

rebased

real-or-random

ACK 40458d2 I read the code and I checked that it does not break the tests

gmaxwell · 2019-11-07T00:49:55Z

Very nice change. Might it makes sense to also unbuffer stderr?

real-or-random · 2019-11-07T09:48:38Z

Very nice change. Might it makes sense to also unbuffer stderr?

It is probably unbuffered on most systems (https://stackoverflow.com/a/3746795) but yep, it won't hurt and it's good for consistency.

…ways cat tests.log after a travis run.

jonasnick · 2019-11-25T10:23:58Z

rebased and added commit to disable buffering for stderr as well

real-or-random · 2019-11-25T10:47:08Z

ACK a0771d1 I looked at the diff and checked that it does not break the tests

a0771d1 Explicitly disable buffering for stderr in tests (Jonas Nick) fb424fb Make travis show the ./tests seed by removing stdout buffering and always cat tests.log after a travis run. (Jonas Nick) Pull request description: …by removing stdout buffering and always cat tests.log after a travis run. Fixes #645. I noticed that according to the [doc](https://www.gnu.org/software/automake/manual/html_node/Parallel-Test-Harness.html) tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call `fflush` twice. Travis is instructed to always show the seed which seems helpful with `after_script` by `cat`ing `./tests.log`. In case the tests fail it looks like https://travis-ci.org/jonasnick/secp256k1/jobs/606446234. ACKs for commit a0771d: real-or-random: ACK a0771d1 I looked at the diff and checked that it does not break the tests Tree-SHA512: 3ba37c2d9169867112981bba3d56680000651ef22ef684c3703f26ed3f71bf415fb23875d30059c8247ea9520c9cfad2c9207badf1b33da8fa3b7b7235a8bf16

Summary: ``` I noticed that according to the doc tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call fflush twice. ``` Backport of secp256k1 [[bitcoin-core/secp256k1#685 | PR685]]. Depends on D5433. It slightly varies from the PR by only printing the logs in the event of a failure. There is no need to explicitly cat the logs with the cmake build because they are already printed by the test runner in case of failure. Before: https://travis-ci.org/Fabcien/secp256k1/jobs/658610743 After: https://travis-ci.org/Fabcien/secp256k1/jobs/658631083 Test Plan: Run the Travis build (see https://travis-ci.org/Fabcien/secp256k1/builds/658633612). Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5434

Summary: ``` I noticed that according to the doc tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call fflush twice. ``` Backport of secp256k1 [[bitcoin-core/secp256k1#685 | PR685]]. Depends on D5433. It slightly varies from the PR by only printing the logs in the event of a failure. There is no need to explicitly cat the logs with the cmake build because they are already printed by the test runner in case of failure. Before: https://travis-ci.org/Fabcien/secp256k1/jobs/658610743 After: https://travis-ci.org/Fabcien/secp256k1/jobs/658631083 Test Plan: Run the Travis build (see https://travis-ci.org/Fabcien/secp256k1/builds/658633612). Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5434 Trigger

e10439c scripted-diff: rename privkey with seckey in secp256k1 interface (Pieter Wuille) ca8bc42 Drop --disable-jni from libsecp256k1 configure options (Pieter Wuille) ddc2419 Update MSVC build config for libsecp256k1 (Pieter Wuille) 67f232b Squashed 'src/secp256k1/' changes from b19c000..2ed54da (Pieter Wuille) Pull request description: It's been abound a year since the subtree was updated. Here is a list of the included PRs: * bitcoin-core/secp256k1#755: Recovery signing: add to constant time test, and eliminate non ct operators * bitcoin-core/secp256k1#754: Fix uninit values passed into cmov * bitcoin-core/secp256k1#752: autoconf: Use ":" instead of "dnl" as a noop * bitcoin-core/secp256k1#750: Add macOS to the CI * bitcoin-core/secp256k1#701: Make ec_ arithmetic more consistent and add documentation * bitcoin-core/secp256k1#732: Retry if r is zero during signing * bitcoin-core/secp256k1#742: Fix typo in ecmult_const_impl.h * bitcoin-core/secp256k1#740: Make recovery/main_impl.h non-executable * bitcoin-core/secp256k1#735: build: fix OpenSSL EC detection on macOS * bitcoin-core/secp256k1#728: Suppress a harmless variable-time optimization by clang in memczero * bitcoin-core/secp256k1#722: Context isn't freed in the ECDH benchmark * bitcoin-core/secp256k1#700: Allow overriding default flags * bitcoin-core/secp256k1#708: Constant-time behaviour test using valgrind memtest. * bitcoin-core/secp256k1#710: Eliminate harmless non-constant time operations on secret data. * bitcoin-core/secp256k1#718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 * bitcoin-core/secp256k1#714: doc: document the length requirements of output parameter. * bitcoin-core/secp256k1#682: Remove Java Native Interface * bitcoin-core/secp256k1#713: Docstrings * bitcoin-core/secp256k1#704: README: add a section for test coverage * bitcoin-core/secp256k1#709: Remove secret-dependant non-constant time operation in ecmult_const. * bitcoin-core/secp256k1#703: Overhaul README.md * bitcoin-core/secp256k1#689: Remove "except in benchmarks" exception for fp math * bitcoin-core/secp256k1#679: Add SECURITY.md * bitcoin-core/secp256k1#685: Fix issue where travis does not show the ./tests seed… * bitcoin-core/secp256k1#690: Add valgrind check to travis * bitcoin-core/secp256k1#678: Preventing compiler optimizations in benchmarks without a memory fence * bitcoin-core/secp256k1#688: Fix ASM setting in travis * bitcoin-core/secp256k1#684: Make no-float policy explicit * bitcoin-core/secp256k1#677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var * bitcoin-core/secp256k1#647: Increase robustness against UB in secp256k1_scalar_cadd_bit * bitcoin-core/secp256k1#664: Remove mention of ec_privkey_export because it doesn't exist * bitcoin-core/secp256k1#337: variable sized precomputed table for signing * bitcoin-core/secp256k1#661: Make ./configure string consistent * bitcoin-core/secp256k1#657: Fix a nit in the recovery tests * bitcoin-core/secp256k1#650: secp256k1/src/tests.c: Properly handle sscanf return value * bitcoin-core/secp256k1#654: Fix typo (∞) * bitcoin-core/secp256k1#583: JNI: fix use sig array * bitcoin-core/secp256k1#644: Avoid optimizing out a verify_check * bitcoin-core/secp256k1#652: README.md: update instruction to run tests * bitcoin-core/secp256k1#651: Fix typo in secp256k1_preallocated.h * bitcoin-core/secp256k1#640: scalar_impl.h: fix includes * bitcoin-core/secp256k1#655: jni: Use only Guava for hex encoding and decoding * bitcoin-core/secp256k1#634: Add a descriptive comment for secp256k1_ecmult_const. * bitcoin-core/secp256k1#631: typo in comment for secp256k1_ec_pubkey_tweak_mul () * bitcoin-core/secp256k1#629: Avoid calling _is_zero when _set_b32 fails. * bitcoin-core/secp256k1#630: Note intention of timing sidechannel freeness. * bitcoin-core/secp256k1#628: Fix ability to compile tests without -DVERIFY. * bitcoin-core/secp256k1#627: Guard memcmp in tests against mixed size inputs. * bitcoin-core/secp256k1#578: Avoid implementation-defined and undefined behavior when dealing with sizes * bitcoin-core/secp256k1#595: Allow to use external default callbacks * bitcoin-core/secp256k1#600: scratch space: use single allocation * bitcoin-core/secp256k1#592: Use trivial algorithm in ecmult_multi if scratch space is small * bitcoin-core/secp256k1#566: Enable context creation in preallocated memory * bitcoin-core/secp256k1#596: Make WINDOW_G configurable * bitcoin-core/secp256k1#561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config * bitcoin-core/secp256k1#533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...) * bitcoin-core/secp256k1#617: Pass scalar by reference in secp256k1_wnaf_const() * bitcoin-core/secp256k1#619: Clear a copied secret key after negation * bitcoin-core/secp256k1#612: Allow field_10x26_arm.s to compile for ARMv7 architecture ACKs for top commit: real-or-random: ACK e10439c I verified the diff (subtree matches my local tree, manual inspection of other commits) but I didn't tested the resulting code fanquake: ACK e10439c Sjors: ACK e10439c jonasnick: reACK e10439c Tree-SHA512: eb6284a485da78e9d2ed3f771df85560d47c770ebf480a0d4121ab356ad26be101a2b973efe412f26e6c142bc1dbd2efbb5cc08774233e41918c59fe3dff3387

jonasnick force-pushed the fix-seed-output branch from 39102ad to 1b9ec7a Compare November 2, 2019 19:53

jonasnick mentioned this pull request Nov 2, 2019

Add schnorrsig module which implements BIP-340 compliant signatures #558

Merged

real-or-random reviewed Nov 3, 2019

View reviewed changes

jonasnick force-pushed the fix-seed-output branch from 1b9ec7a to 8de4392 Compare November 3, 2019 22:35

jonasnick force-pushed the fix-seed-output branch from 8de4392 to 40458d2 Compare November 5, 2019 13:18

real-or-random approved these changes Nov 5, 2019

View reviewed changes

jonasnick added 2 commits November 25, 2019 10:23

Make travis show the ./tests seed by removing stdout buffering and al…

fb424fb

…ways cat tests.log after a travis run.

Explicitly disable buffering for stderr in tests

a0771d1

jonasnick force-pushed the fix-seed-output branch from 2b0ac71 to a0771d1 Compare November 25, 2019 10:23

real-or-random merged commit a0771d1 into bitcoin-core:master Nov 25, 2019

sipa mentioned this pull request Jun 9, 2020

Update libsecp256k1 subtree bitcoin/bitcoin#19228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue where travis does not show the ./tests seed… #685

Fix issue where travis does not show the ./tests seed… #685

jonasnick commented Nov 2, 2019 •

edited

Loading

real-or-random left a comment

real-or-random Nov 3, 2019

jonasnick Nov 3, 2019

real-or-random commented Nov 4, 2019

real-or-random commented Nov 4, 2019

jonasnick commented Nov 4, 2019 •

edited

Loading

real-or-random commented Nov 5, 2019

real-or-random commented Nov 5, 2019

jonasnick commented Nov 5, 2019

real-or-random left a comment

gmaxwell commented Nov 7, 2019

real-or-random commented Nov 7, 2019

jonasnick commented Nov 25, 2019 •

edited

Loading

real-or-random commented Nov 25, 2019

Fix issue where travis does not show the ./tests seed… #685

Fix issue where travis does not show the ./tests seed… #685

Conversation

jonasnick commented Nov 2, 2019 • edited Loading

real-or-random left a comment

Choose a reason for hiding this comment

real-or-random Nov 3, 2019

Choose a reason for hiding this comment

jonasnick Nov 3, 2019

Choose a reason for hiding this comment

real-or-random commented Nov 4, 2019

real-or-random commented Nov 4, 2019

jonasnick commented Nov 4, 2019 • edited Loading

real-or-random commented Nov 5, 2019

real-or-random commented Nov 5, 2019

jonasnick commented Nov 5, 2019

real-or-random left a comment

Choose a reason for hiding this comment

gmaxwell commented Nov 7, 2019

real-or-random commented Nov 7, 2019

jonasnick commented Nov 25, 2019 • edited Loading

real-or-random commented Nov 25, 2019

jonasnick commented Nov 2, 2019 •

edited

Loading

jonasnick commented Nov 4, 2019 •

edited

Loading

jonasnick commented Nov 25, 2019 •

edited

Loading