Add support for Cirrus CI #864

real-or-random · 2021-01-06T22:37:56Z

On top of #862.

This PR adds Cirrus CI integration. For now this is intended to replicate the functionality that we have in Travis currently. These are a lot of builds, we should also rethink the build matrix. Moreover, the CI shell script could have some refactoring and cleaning but these issues tasks are for another PR (see also #707).

Cirrus CI is relies on Docker images for Linux builds. We make use of this extensively by building our own Docker images (with necessary packages) on which CI then runs (https://cirrus-ci.org/guide/docker-builder-vm/#dockerfile-as-a-ci-environment), see the dockerfile setting in .cirrus.yml. The image is recreated for every build and then all "tasks" run on this image.

For the Linux builds, NixOs is used. This is a rather uncommon choice. My intention of this is that it gives you an easy way of opening a shell on your local machine that has the build tools exactly as in the CI environment. (Just install the nix package manager on top of your distro and run the command from the .cirrus.yml. Now after playing around with Nix, I'm not so convinced anymore:

Not many people are familiar with Nix (these is my first usage of Nix), only @jonasnick here knows more.
Support for cross-compilers is somewhat limited.
As we anyway have docker, there's already a nice way to replicate the CI environment locally.

Still, I kept Nix for now since it's working now.

Debian is already used for a s390x big-endian qemu test. I choose s390x over the more common PowerPC and others because s390x is the only officially supported big-endian port of Debian (https://www.debian.org/ports/).

On the MacOS builds, we use a forked valgrind version which support newer macOS versions (https://github.com/LouisBrunner/valgrind-macos). It's not great but it works and this was change was overdue anyway.

Todo (in this PR):

Add build badge
Enable caching of brew binaries for macOS (note that the macOS builds on Cirrus don't use Docker)

Todo (other PRs)

Reduce/reorganize jobs
Tidy cirrus.sh script
Add windows/MSVC
Maybe add "bleeding edge" jobs that are allowed to fail and test against latest compilers.

real-or-random · 2021-01-07T09:41:47Z

Ok I've added a proper description. It's still a draft PR but please have a look.

sipa · 2021-01-07T18:05:16Z

@theuni Care to have a look at the buildsystem stuff here?

real-or-random · 2021-01-08T20:36:32Z

@theuni Care to have a look at the buildsystem stuff here?

If you want to look at this, please look at #862 . All the buildsystem stuff is there, and this PR is not yet rebased on #862.

jonasnick

Thanks! Looks good overall. In particular, should have the same set of tests as before.

For the Linux builds, NixOs is used

It's not NixOS but Alpine with the Nix package manager. That should also be reflected in the new files (e.g. NixOS shell -> Nix shell).

This sounds great but one drawback is that there is currently no way to invalidate the cache except for pushing a change to .cirrus.yml or the Dockerfile

Yeah would be nice to have this and I'm seeing you're experimenting with that in this PR.

Now after playing around with Nix, I'm not so convinced anymore:

There's an advantage of using Nix, namely you can reproduce the exact same dependencies locally by pinning the nixpkgs. When rebuilding the debian docker image you may get newer versions than in CI. Historically we didn't have problems with reproducing test failures. Either way is fine for me, but if we keep nix we should output the nixpkgs commit to actually be able to reproduce it.

I'll probably change the 32-bit i686 builds back to Debian, I had some issues with them.

i686 is only a "limitedSupportedSystem" in Nix which means that the nix cache only contains only a "minimal" package set. Best would be to either just use clang instead of clang_11 or switch to debian for i686.

Since we're on a rolling-release channel, it's rather easy to get the latest compilers for example and see miscompilations early for example.

Not sure about this, but we can try it out.

ci/cirrus.sh

theuni · 2021-01-14T02:26:44Z

If you want to look at this, please look at #862 . All the buildsystem stuff is there, and this PR is not yet rebased on #862.

Roger, will have a look this week.

jonasnick

Since I'm just noticing that Travis is still pending, I'm wondering if this PR wouldn't also be the right time to remove it.

sipa · 2021-01-27T21:10:46Z

Concept ACK. The NixOS approach doesn't seem to add much overhead, and not too hard to replace if it's an issue.

It seems Travis is acting up again, is there something left to get this to an minimal viable mergable state?

real-or-random · 2021-01-28T16:37:35Z

Yes, this slipped out of my focus...

I talked to a few people including @MarcoFalke and I'm considering a different approach for "bleeding edge" builds, namely regular (e.g., nightly) builds (of master) that won't get in the way of contributors working on their PRs. AFAIC this is supported by Cirrus (https://cirrus-ci.org/guide/writing-tasks/#cron-builds).

Also I think that Debian experimental would be a nice platform for this, and this would then probably obsolete the last reason to NixOs, so I'm also considering switching back to Debian for the stable builds.

But all of this can be done in a separate PR. Since this is currently working, let's get this merged. I've added a commit that removes Travis as suggested above.

jonasnick

ACK eb16650 mod nits

ci/cirrus.sh

ci/shell.nix

ci/cirrus.sh

.cirrus.yml

So long, and thanks for all fish!

real-or-random · 2021-01-29T21:27:41Z

Ready for review again (when the checks pass).

sipa · 2021-01-30T00:30:54Z

ACK cc2a545. Tested by introducing bugs: #883, #884, #885, #886, #887.

jonasnick

ACK cc2a545

real-or-random · 2021-02-04T09:50:41Z

Argh, the "clang" builds use GCC in fact [0] because nix-shell redefines $CC. [1] There's a lot of "wrapper" stuff in general. https://nixos.wiki/wiki/C I feel all of this may be great for users but not what we want in CI, where we want a pure environment.

Until now, my position was that nix doesn't really give us the benefits I hoped for but it was working, so I kept it. But now that's a real disadvantage. I think it's better to get of rid, which is somewhat sad given that I spent some time on it.

[0] https://cirrus-ci.com/task/5368525575421952?command=test#L164
[1] https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/setup-hook.sh#L110

real-or-random · 2021-02-04T17:15:52Z

We should also consider implementing the hack from Core (cirruslabs/cirrus-ci-docs#662 (comment)) in order to run CI on the merge commit instead of the source branch (in the case of PRs).

Summary: Switch from Travis to Cirrus as a CI for libsecp256k1. Backport of [[bitcoin-core/secp256k1#864 | secp256k1#864]] The files have been adapted to match our codebase: - The build_autotools.sh and build_cmake.sh scripts are reused from the Travis setup. - The nixos base file is removed since it would build openjdk from source for i686 and time out. This is not a good move but these files will be removed in a follow-up diff anyway, so not a big deal. - Skip JNI tests on macOS, since Java is not installed on Cirrus macOS VM. - Work around valgrind install path not being searched by CMake. This is a known CMake issue (/usr/local/include should be part of the system search path for headers): https://gitlab.kitware.com/cmake/cmake/-/issues/19120 - Increase the timeouts, since we are running each test twice the valgrind task will not complete in 1h on macOS. Test Plan: See result from my own repo: https://cirrus-ci.com/build/6029790437179392 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9397

Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls). This is just a quick fix. We should still look into other methods, e.g., asm and bitcoin-core#457. We should also consider not caring about constant-time in scalar_low_impl.h We should also consider testing on very new compilers in nightly CI, see bitcoin-core#864 (comment)

…ations 4a496a3 ct: Use volatile "trick" in all fe/scalar cmov implementations (Tim Ruffing) Pull request description: Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls). This is just a quick fix. We should still look into other methods, e.g., asm and #457. We should also consider not caring about constant-time in scalar_low_impl.h We should also consider testing on very new compilers in nightly CI, see #864 (comment) ACKs for top commit: jonasnick: ACK 4a496a3 Tree-SHA512: a6010f9d752e45f01f88b804a9b27e77caf5ddf133ddcbc4235b94698bda41c9276bf588c93710e538250d1a96844bcec198ec5459e675f166ceaaa42da921d5

Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls). This is just a quick fix. We should still look into other methods, e.g., asm and #457. We should also consider not caring about constant-time in scalar_low_impl.h We should also consider testing on very new compilers in nightly CI, see bitcoin-core/secp256k1#864 (comment)

Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls). This is just a quick fix. We should still look into other methods, e.g., asm and bitcoin-core#457. We should also consider not caring about constant-time in scalar_low_impl.h We should also consider testing on very new compilers in nightly CI, see bitcoin-core#864 (comment)

real-or-random mentioned this pull request Jan 8, 2021

Try to use qemu for big endian test on travis #843

Closed

real-or-random mentioned this pull request Jan 11, 2021

Autoconf improvements #862

Merged

real-or-random force-pushed the 202012-cirrusci branch 3 times, most recently from 95035d3 to 776e30a Compare January 13, 2021 18:59

jonasnick reviewed Jan 13, 2021

View reviewed changes

ci/cirrus.sh Outdated Show resolved Hide resolved

jonasnick reviewed Jan 14, 2021

View reviewed changes

real-or-random force-pushed the 202012-cirrusci branch 11 times, most recently from e5c0317 to ac702da Compare January 14, 2021 15:52

real-or-random marked this pull request as ready for review January 28, 2021 16:20

real-or-random force-pushed the 202012-cirrusci branch from ac702da to eb16650 Compare January 28, 2021 16:36

jonasnick reviewed Jan 28, 2021

View reviewed changes

ci/cirrus.sh Outdated Show resolved Hide resolved

ci/shell.nix Outdated Show resolved Hide resolved

ci/cirrus.sh Outdated Show resolved Hide resolved

sipa reviewed Jan 28, 2021

View reviewed changes

.cirrus.yml Outdated Show resolved Hide resolved

real-or-random force-pushed the 202012-cirrusci branch 2 times, most recently from 5bb5be5 to fc5c739 Compare January 29, 2021 19:15

real-or-random force-pushed the 202012-cirrusci branch from fc5c739 to e8d5d52 Compare January 29, 2021 20:46

real-or-random added 3 commits January 29, 2021 21:54

ci: Add support for Cirrus CI

8c02e46

ci: Enable simple cache for brewing valgrind on macOS

2b359f1

ci: Remove support for Travis CI

2480e55

So long, and thanks for all fish!

real-or-random force-pushed the 202012-cirrusci branch from e8d5d52 to 2480e55 Compare January 29, 2021 20:54

ci: Refactor Nix shell files

cc2a545

real-or-random force-pushed the 202012-cirrusci branch from 80fb7a5 to cc2a545 Compare January 29, 2021 21:26

This was referenced Jan 30, 2021

Safegcd inverses, drop Jacobi symbols, remove libgmp #831

Merged

Signed-digit multi-comb for ecmult_gen (by peterdettman) #693

Closed

jonasnick approved these changes Jan 30, 2021

View reviewed changes

jonasnick merged commit f8c0b57 into bitcoin-core:master Jan 30, 2021

real-or-random mentioned this pull request Apr 1, 2023

ct: Use volatile "trick" in all fe/scalar cmov implementations #1257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Cirrus CI #864

Add support for Cirrus CI #864

real-or-random commented Jan 6, 2021 •

edited

Loading

real-or-random commented Jan 7, 2021

sipa commented Jan 7, 2021

real-or-random commented Jan 8, 2021

jonasnick left a comment

theuni commented Jan 14, 2021

jonasnick left a comment

sipa commented Jan 27, 2021

real-or-random commented Jan 28, 2021 •

edited

Loading

jonasnick left a comment

real-or-random commented Jan 29, 2021

sipa commented Jan 30, 2021

jonasnick left a comment

real-or-random commented Feb 4, 2021

real-or-random commented Feb 4, 2021

Add support for Cirrus CI #864

Add support for Cirrus CI #864

Conversation

real-or-random commented Jan 6, 2021 • edited Loading

real-or-random commented Jan 7, 2021

sipa commented Jan 7, 2021

real-or-random commented Jan 8, 2021

jonasnick left a comment

Choose a reason for hiding this comment

theuni commented Jan 14, 2021

jonasnick left a comment

Choose a reason for hiding this comment

sipa commented Jan 27, 2021

real-or-random commented Jan 28, 2021 • edited Loading

jonasnick left a comment

Choose a reason for hiding this comment

real-or-random commented Jan 29, 2021

sipa commented Jan 30, 2021

jonasnick left a comment

Choose a reason for hiding this comment

real-or-random commented Feb 4, 2021

real-or-random commented Feb 4, 2021

real-or-random commented Jan 6, 2021 •

edited

Loading

real-or-random commented Jan 28, 2021 •

edited

Loading