Add macOS to the CI #750

pull elichai wants to merge 5 commits into bitcoin-core:master from elichai:2020-05-travis-osx changing 2 files +94 −32
  1. elichai commented at 8:24 am on May 3, 2020: contributor

    This adds macOS to the travis so we can test all the same configurations under macOS, both with Apple’s clang and proper gcc on macOS. It also runs the ctime test, and valgrind on the tests just like on linux.

    The current travis script is pretty messy because we added more and more configuration over time each one required more complex ifs making it harder to read, and because of the somewhat complex logic it failed[1] on macOS(having an old bash version) so I decided to just throw it into a standalone sh script instead, that way it can be formatted nicely and with -e it will fail on every error without needing to put && everywhere.

    Some changed in the script:

    • Replaced the usage of libtool with the locally generated libtool (https://github.com/bitcoin-core/secp256k1/pull/723#issuecomment-622999449)

    • Added --error-exitcode=42 to the ctime tests because they currently silently fail on -O0 (https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/681571334#L454) and disabled the ctime tests on -O0.

    • Moved the valgrind tests to the matrix so that they’ll run on both gcc and clang and on macOS. (also, now that #710 is merged we always pass -DVALGRIND when the valgrind headers exist but I left the explicit CFLAGS in those tests anyway, there’s no harm in explicitly doing that)

    • Removed the use of EXTRAFLAGS for setting CFLAGS, it’s enough to just set CFLAGS directly and it can cause troubles in sh (the whole EXTRAFLAGS="--disable-openssl-tests CPPFLAGS=-DVALGRIND")

    • We have to explicitly set the gcc version on macOS+gcc because macOS ship with a fake gcc which is basically just an alias to their clang compiler, and installing proper gcc from brew adds a gcc-* binary and doesn’t replace the gcc binary, so we have to explicitly set CC=gcc-9 under that scenario, so I also explicitly install gcc@9 so it shouldn’t break when macOS gets gcc-10.

    • Bumped ubuntu to bionic because of #748 (review) (the end of End of Standard Support is in a year anyway) it’s in a separate commit so that if anyone have concerns I’ll just drop that commit.

    1. https://travis-ci.org/github/elichai/secp256k1/jobs/681663742#L336
  2. real-or-random commented at 10:21 am on May 4, 2020: contributor
    That’s very neat. I’ll have a closer look later. Let’s try to get this merged before I continue to work on #723 because this here solves a bunch of the issues I encountered there.
  3. in .travis.yml:108 in ead13d96c0 outdated 
    134     - cat ./tests.log
135     - cat ./exhaustive_tests.log
136     - cat ./valgrind_ctime_test.log
137     - cat ./bench.log
138+    - $CC --version
139+    - valgrind --version
    real-or-random commented at 12:58 pm on May 4, 2020:
    nit: Can you move these two to before_install? This fits the place where travis outputs all the other version info better.
    elichai commented at 1:31 pm on May 4, 2020:
    I’ll make sure it prints after apt/brew updates and if it does I’ll move.
    elichai commented at 2:29 pm on May 5, 2020:
    Just realized a problem with that. We override CC in the sh script, so if we want to know what compiler is it using we must do it after the script :/
    real-or-random commented at 2:58 pm on May 5, 2020:
    Well, okay, I guess then it’s better not to move it. ^^
  4. in contrib/travis.sh:1 in ead13d96c0 outdated 
    0@@ -0,0 +1,57 @@
1+#!/bin/sh -ex
    real-or-random commented at 1:08 pm on May 4, 2020:

    nit: it may be easier to spot for the reader to write

    0#!/bin/sh
1
2set -e
3set -x
  5. in contrib/travis.sh:41 in ead13d96c0 outdated 
    32+   fi
33+if [ -n "$BENCH" ]
34+then
35+    if [ -n "$VALGRIND" ]
36+    then
37+        EXEC='./libtool --mode=execute valgrind --error-exitcode=42'
    real-or-random commented at 1:09 pm on May 4, 2020:
    Can you explain in a comment why we’re using the internal one?
  6. in contrib/travis.sh:44 in ead13d96c0 outdated 
    35+    if [ -n "$VALGRIND" ]
36+    then
37+        EXEC='./libtool --mode=execute valgrind --error-exitcode=42'
38+    else
39+        EXEC=
40+    fi
    real-or-random commented at 1:15 pm on May 4, 2020:
    I first wanted to write it would be cleaner so set SECP256K1_BENCH_ITERS=2 here, for clarity. But I’m not entirely sure. If we set it here, it won’t be configurable. Bu then we should at least add a comment that the caller should set the ITERS to a low value. What do you think?
    elichai commented at 1:33 pm on May 4, 2020:
    Hmm I think this script is 100% travis oriented anyway. so I’m not sure there’s a meaningful difference. If the script was written in a way that we could also run it locally pretty easily then maybe it would have (although the amount of env vars make it pretty hard to run locally)
    real-or-random commented at 1:38 pm on May 4, 2020:

    Hmm I think this script is 100% travis oriented anyway. so I’m not sure there’s a meaningful difference.

    Sure, and no need to change that right now. (Even though we want to change it in the future.) My main concern is that you don’t see the low iteration count when looking at the script. Env variables are great in hiding details. It looks like we’re just invoking the full benchmarks. My maybe a comment is indeed the best.

  7. real-or-random commented at 1:51 pm on May 4, 2020: contributor

    Fwiw, I think travis_wait does not work for us currently and just hides the timeout. It does not work with forking processes.

    Proof: https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/682613807#L1112

    See

    I had noticed this already when working on #723 but never wrote it down. If you want, you can give it a try. The issues above should provide some pointers, in particular PRs from other projects. But we can do this also in another PR. After this, was broken before this PR already.

  8. real-or-random commented at 1:53 pm on May 4, 2020: contributor
    Any idea if that dsymutil thing here is specific to valgrind on MacOS? https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/682613807#L1057
  9. elichai commented at 1:19 pm on May 5, 2020: contributor

    Any idea if that dsymutil thing here is specific to valgrind on MacOS? https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/682613807#L1057

    Sounds like it: https://llvm.org/docs/CommandGuide/dsymutil.html

    And here: https://valgrind.org/docs/manual/manual-core.html

    This option is only relevant when running Valgrind on Mac OS X.

  10. elichai force-pushed on May 6, 2020
  11. jonasnick cross-referenced this on May 6, 2020 from issue Add usage examples by elichai
  12. elichai force-pushed on May 7, 2020
  13. elichai commented at 10:08 am on May 7, 2020: contributor
    Much better now :) 2-3 minutes per macOS job instead of ~11 minutes. now I hope the valgrind test won’t time out
  14. real-or-random commented at 6:20 pm on May 7, 2020: contributor
    Great, so I think this is ready (from my side at least ^^). One last question: Have you verified that a ct-test failure indeed makes the build fail? I guess this should be tested because this can be broken in subtle ways as I learned in my own PR…
  15. elichai commented at 7:16 pm on May 7, 2020: contributor

    Great, so I think this is ready (from my side at least ^^). One last question: Have you verified that a ct-test failure indeed makes the build fail? I guess this should be tested because this can be broken in subtle ways as I learned in my own PR…

    I applied the following diff:

     0diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h
 1index 71aa550..348cca0 100644
 2--- a/src/field_5x52_impl.h
 3+++ b/src/field_5x52_impl.h
 4@@ -465,13 +465,12 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
 5 }
 6 
 7 static SECP256K1_INLINE void secp256k1_fe_storage_cmov(secp256k1_fe_storage *r, const secp256k1_fe_storage *a, int flag) {
 8-    uint64_t mask0, mask1;
 9-    mask0 = flag + ~((uint64_t)0);
10-    mask1 = ~mask0;
11-    r->n[0] = (r->n[0] & mask0) | (a->n[0] & mask1);
12-    r->n[1] = (r->n[1] & mask0) | (a->n[1] & mask1);
13-    r->n[2] = (r->n[2] & mask0) | (a->n[2] & mask1);
14-    r->n[3] = (r->n[3] & mask0) | (a->n[3] & mask1);
15+    if (flag) {
16+        r->n[0] = a->n[0];
17+        r->n[1] = a->n[1];
18+        r->n[2] = a->n[2];
19+        r->n[3] = a->n[3];
20+    }
21 }

    And as you can see here https://travis-ci.org/github/elichai/secp256k1/builds/684392980 everything failed except the jobs with FIELD=32bit

  16. in contrib/travis.sh:35 in a60387acb8 outdated 
    30+then
31+   make -j2
32+# the `--error-exitcode` is required to make the test fail if valgrind found errors, otherwise it'll return 0 (http://valgrind.org/docs/manual/manual-core.html)
33+   valgrind --error-exitcode=42 ./tests 16
34+   valgrind --error-exitcode=42 ./exhaustive_tests
35+   fi
    jonasnick commented at 8:39 pm on May 7, 2020:
    nit: indentation
  17. in contrib/travis.sh:46 in a60387acb8 outdated 
    41+        EXEC='./libtool --mode=execute valgrind --error-exitcode=42'
42+    else
43+        EXEC=
44+    fi
45+    SECP256K1_BENCH_ITERS="$ITERS" $EXEC ./bench_ecmult >> bench.log 2>&1
46+    SECP256K1_BENCH_ITERS="$ITERS" $EXEC ./bench_internal >> bench.log 2>&1
    jonasnick commented at 8:47 pm on May 7, 2020:

    We can avoid the >>bench.log 2>&1 in every line if we use command grouping:

    0{
1  ./bench...
2  ./bench...
3} >> bench.log 2>&1

    Also how about setting ITERS just once? Repeating it makes unnecessarily reduces readability.

    elichai commented at 10:16 am on May 8, 2020:

    I saw that option in shellcheck, but It didn’t really seemed easier to read, I can do that anyway if you prefer.

    About the ITERS, I do think it’s readable right not (especially compared to before) but I can def export it right before instead.

    jonasnick commented at 12:51 pm on May 11, 2020:
    Yeah, doing both would be easier to read, better abstracted and more idiomatic imo.
  18. jonasnick commented at 8:51 pm on May 7, 2020: contributor
    Thank you for cleaning up. ACK mod nits.
  19. in contrib/travis.sh:60 in a60387acb8 outdated 
    55+        SECP256K1_BENCH_ITERS="$ITERS" $EXEC ./bench_ecdh >> bench.log 2>&1
56+    fi
57+fi
58+if [ -n "$CTIMETEST" ]
59+then
60+   ./libtool --mode=execute valgrind --error-exitcode=42 ./valgrind_ctime_test > valgrind_ctime_test.log 2>&1
    real-or-random commented at 12:05 pm on May 8, 2020:
    nit: indentation
  20. elichai force-pushed on May 9, 2020
  21. Move travis script into a standalone sh file b6807d91d8
  22. Add macOS support to travis 0c5ff9066e
  23. Bump travis Ubuntu from xenial(16.04) to bionic(18.04) bc818b160c
  24. Replace travis_wait with a loop printing "\a" to stdout every minute 99bd661d71
  25. elichai force-pushed on May 11, 2020
  26. real-or-random commented at 9:30 am on May 14, 2020: contributor
    @elichai Is this ready for review?
  27. elichai commented at 9:31 am on May 14, 2020: contributor
    Yes
  28. in contrib/travis.sh:45 in 10921eaa1c outdated 
    40+        # Using the local `libtool` because on macOS the system's libtool has nothing to do with GNU libtool
41+        EXEC='./libtool --mode=execute valgrind --error-exitcode=42'
42+    else
43+        EXEC=
44+    fi
45+    # This limits the iterations in the benchmark underneath to ITER(set in .travis.yml) iterations.
    jonasnick commented at 1:41 pm on May 14, 2020:
    Took me a couple of seconds to understand what you mean by underneath (perhaps because it’s usually only used with an object?). Maybe replace with below. Otherwise PR looks good!
    elichai commented at 9:01 am on May 18, 2020:
    Done
  29. Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh 71757da5cc
  30. elichai force-pushed on May 18, 2020
  31. real-or-random commented at 2:28 pm on May 18, 2020: contributor
    ACK 71757da5ccece100b1eca6c70b4d87e2542cff97 I inspected the diff
  32. jonasnick approved
  33. jonasnick commented at 7:36 pm on May 18, 2020: contributor
    ACK 71757da5ccece100b1eca6c70b4d87e2542cff97
  34. jonasnick merged this on May 18, 2020
  35. jonasnick closed this on May 18, 2020
  36. elichai deleted the branch on May 18, 2020
  37. sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa
  38. fanquake referenced this in commit 8c97780db8 on Jun 13, 2020
  39. sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020
  40. ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020
  41. jasonbcox referenced this in commit 836f5d85b2 on Sep 29, 2020
  42. deadalnix referenced this in commit 7d90f7284d on Sep 30, 2020
  43. elichai cross-referenced this on Mar 24, 2021 from issue Manually test that panicking from C will abort the process by real-or-random
  44. UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021
  45. 5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021
  46. gades referenced this in commit d855cc511d on May 8, 2022


elichai real-or-random jonasnick

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 21:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me