Improve CI script #420

tcharding · 2022-03-17T00:46:41Z

Improve the CI pipeline. Done while investigating #419. This PR now only includes the GitHub actions changes, I'm moving the test.sh changes to a #422 because they cannot be merged yet.

(Please note, this PR has been heavily re-worked so discussion below may be confusing to reviewers new to the PR.)

tcharding · 2022-03-17T00:51:27Z

Ha! This reverts changes I made in commit 0fd07ad059f2995b1ced5313ae514eaaee88f0ad (putting WASM stuff inside the feature matrix job).

.github/workflows/rust.yml

thomaseizinger · 2022-03-17T00:56:01Z

Ha! This reverts changes I made in commit 0fd07ad059f2995b1ced5313ae514eaaee88f0ad (putting WASM stuff inside the feature matrix job).

Do we have to revert that? Why shouldn't the wasm also run on stable, beta and nightly? (1.29.0 is probably too old).

I think we may want to contemplate moving 1.29.0 as a completely different MSRV job. In general, compiling should be enough because that is what users need. Or do we enforce 1.29.0 also for local development?

tcharding · 2022-03-17T01:12:10Z

Ha! This reverts changes I made in commit 0fd07ad059f2995b1ced5313ae514eaaee88f0ad (putting WASM stuff inside the feature matrix job).

Do we have to revert that? Why shouldn't the wasm also run on stable, beta and nightly? (1.29.0 is probably too old).

The bit that was reverted was having WASM as a separate job, not that it was only run with the stable toolchain - so the stated aim of this PR (to make it clearer what runs when) was a success! I know basically zero about WASM, including whether or not we want to test with stable and nightly. If you think there is benefit in that then I'll add it to this PR.

I think we may want to contemplate moving 1.29.0 as a completely different MSRV job. In general, compiling should be enough because that is what users need. Or do we enforce 1.29.0 also for local development?

I think we want to test as well, especially considering we (I) have been trying to add rustdoc tests that give good coverage of the public API.

thomaseizinger · 2022-03-17T05:06:03Z

I think we may want to contemplate moving 1.29.0 as a completely different MSRV job. In general, compiling should be enough because that is what users need. Or do we enforce 1.29.0 also for local development?

I think we want to test as well, especially considering we (I) have been trying to add rustdoc tests that give good coverage of the public API.

I am not sure I see the benefit of testing with 1.29.0. It just forces that all dev-dependencies have to compile with 1.29.0 as well. The public API doesn't change depending on which version of rustc you compile it with so if you run the tests with stable, then you know the behaviour and API are what you assert in the tests. If the concerns is differences in behaviour of rustc then we'd have to test all versions from stable to 1.29.0.

Ha! This reverts changes I made in commit 0fd07ad059f2995b1ced5313ae514eaaee88f0ad (putting WASM stuff inside the feature matrix job).

Do we have to revert that? Why shouldn't the wasm also run on stable, beta and nightly? (1.29.0 is probably too old).

The bit that was reverted was having WASM as a separate job, not that it was only run with the stable toolchain - so the stated aim of this PR (to make it clearer what runs when) was a success! I know basically zero about WASM, including whether or not we want to test with stable and nightly. If you think there is benefit in that then I'll add it to this PR.

The point of compiling and testing a library crate against stable, beta and nightly is that you get a 6 (beta) - 12 (nightly) week notice when something merges into rustc that breaks what you are doing in your library. That is relevant regardless of the target architecture you compile for, be it x86, aarch64 or wasm. Typically, I would only mark CI runs against stable as required to be passing for merging because that is immediately relevant for releases etc and you don't want to block development on that. Stuff breaks on beta and nightly all the time so a user using that opts out of most stability guarantees.

tcharding · 2022-03-17T05:35:52Z

I am not sure I see the benefit of testing with 1.29.0. It just forces that all dev-dependencies have to compile with 1.29.0 as well. The public API doesn't change depending on which version of rustc you compile it with so if you run the tests with stable, then you know the behaviour and API are what you assert in the tests.

I'm liking this point, from now on I'm only going to run cargo +1.29 build instead of all the tests before pushing PRs. If nobody disagrees I'll push changes to this PR to only build with 1.29 and not run the full feature matrix tests. This has the added benefit that we can use 'modern' features in the tests and not worry about sticking to the old Rust 1.29 constructs.

If the concerns is differences in behaviour of rustc then we'd have to test all versions from stable to 1.29.0.

Ouch, lets not do that :)

The point of compiling and testing a library crate against stable, beta and nightly is that you get a 6 (beta) - 12 (nightly) week notice when something merges into rustc that breaks what you are doing in your library. That is relevant regardless of the target architecture you compile for, be it x86, aarch64 or wasm. Typically, I would only mark CI runs against stable as required to be passing for merging because that is immediately relevant for releases etc and you don't want to block development on that. Stuff breaks on beta and nightly all the time so a user using that opts out of most stability guarantees.

hmm, interesting, I like the sound of this also. Its above my pay grade to do such admin changes, @apoelstra are you interested in slackening off the merge requirements? (Require stable and MSRV builds to pass, stable tests to pass, and allow nightly/beta builds/tests to be red.)

apoelstra · 2022-03-17T17:44:39Z

I would like to continue running tests on 1.29.0. This hasn't caused us much trouble in the past and anyway we're increasing 1.29 to 1.41 soon so the trouble it has caused us should go away soon.

I would worry otherwise that the library might build ok on 1.29.0, but not build with cfg(test) on; or not build when certain macros are in use.

TheBlueMatt · 2022-03-17T17:58:51Z

we're increasing 1.29 to 1.41 soon so the trouble it has caused us should go away soon.

Isn't the timeline for this, like, now? Do we need to care about it anymore?

apoelstra · 2022-03-17T20:30:59Z

@TheBlueMatt yep, as soon as we get the new rust-bitcoin release out (I believe this is blocked only on rust-bitcoin/rust-bitcoin#796)

tcharding · 2022-03-18T23:47:49Z

Since I've not got a great track record on CI pipeline changes I've split everything up into individual patches to make review more likely to catch any mistakes (incl. the justification of each patch given in the commit messages).

tcharding · 2022-03-19T00:07:11Z

Totally re-worked the PR, re-written the PR message.

apoelstra

ACK 6808416

This all seems good/uncontroversial to me.

tcharding · 2022-03-25T01:12:23Z

I don't know why I closed this, bit savage yesterday cleaning up stale branches I guess. Could you please re-open it @apoelstra if there is a button to do so (\me puts money in tip jar).

thomaseizinger · 2022-03-25T01:27:59Z

I don't know why I closed this, bit savage yesterday cleaning up stale branches I guess. Could you please re-open it @apoelstra if there is a button to do so (\me puts money in tip jar).

I don't think GitHub allows this but you can always open a new PR :)

tcharding · 2022-03-25T02:29:16Z

Cool, thanks man.

apoelstra · 2022-03-26T01:03:30Z

I have a reopen button. Kinda weird that you don't, when it's your PR and you closed it..

tcharding · 2022-03-27T22:45:27Z

Last time I did this the explanation was I couldn't re-open because I'd force pushed but for this one that did not apply. My current guess is only maintainers can re-open. Not to worry.

Remove single character of trailing whitespace.

tcharding · 2022-03-29T00:18:07Z

Rebase includes:

Fix spelling mistake in commit log
Install package clang instead of clang-9 for ASan job
rebase on master

thomaseizinger

Nice re-write!

thomaseizinger · 2022-03-29T05:28:11Z

.github/workflows/rust.yml

    steps:
      - name: Checkout Crate
        uses: actions/checkout@v2
+      - name: Install clang     # Needed for ASan.


Suggested change

- name: Install clang # Needed for ASan.

- name: Install clang for ASan

Ha! That's nicer. Will use.

thomaseizinger · 2022-03-29T05:29:44Z

.github/workflows/rust.yml

+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: nightly


Suggested change

toolchain: nightly

toolchain: ${{ matrix.rust }}

Need to actually use the matrix variable :)

Nice reviewing, thanks man. Will re-spin tomorrow.

In line with the `Tests` job and for the fact that this job does stuff with the nightly toolchain other than bench. Re-name nightly CI job from `bench_nightly`to `Nightly`.

We use a matrix with a single element, this is unnecessary.

The address sanitizer job is silently failing at the moment because we do not install clang. Install clang so the address sanitizer job can run. Do not fix the silent failure, that will be done later on.

We have a separate CI job for things that require a nightly toolchain. Building the docs requires a nightly toolchain (because of `--cfg docsrc` flag). It makes more sense to run the docs build in the `Nightly` job instead of hidden in the `Tests` job. Do the docs build in the `Nightly` job instead of in the `Tests` job.

WASM is supported by Rust 1.30. We can therefore run the WASM tests on any all the toolchains except MSRV (1.29.0). This has benefit of catching nightly/beta issues before they get to stable. Done as a separate CI job since it is conceptually different to the `Tests` job. Run WASM for nightly, beta, and stable toolchains.

tcharding · 2022-03-29T21:52:15Z

Changes in force push:

Implement both changes suggested by @thomaseizinger
- Use the rust matrix
- Remove comment in favour of more descriptive name
Be more descriptive with WASM comment
Remove the patch that changed test.sh, it was out of place for this PR, raised a separate PR for it.

d2e1f8c Move panic test to top of script (Tobin Harding) Pull request description: In `test.sh` we have a test that checks for a panic by greping the output of `cargo test --exact 'tests::test_panic_raw_ctx_should_terminate_abnormally'`. If we put this test at the top of the script right after we run `cargo test` we are guaranteed to not trigger a re-build. ### Note to reviewers I just noticed this patch somehow snuck into #420, all other changes in that PR are to `.github/workflows/rust.yml` so this change does not fit in there. Hence raising it as a separate PR. ACKs for top commit: apoelstra: ACK d2e1f8c Tree-SHA512: 90ad7a8762a6fd977345f347f0aa8b0979a7576585000b6d80624c0672b7de457dec471dc63b2e7fa4c3f52143d0f6fd1f4031a70f85c9fab4b7c22a787c438b

apoelstra

ACK 58db1b6

apoelstra · 2022-03-30T16:31:17Z

@thomaseizinger looks like your nits were addressed -- ok if I merge this?

thomaseizinger

ACK 58db1b6

thomaseizinger · 2022-03-31T03:00:05Z

@thomaseizinger looks like your nits were addressed -- ok if I merge this?

Merge away! :)

97dc0ea Run correct clang --version (Tobin Harding) a3582ff test.sh: Use set -e to exit on failure (Tobin Harding) 7bec31c test.sh: explicitly return 0 (Tobin Harding) Pull request description: Change the test script to exit with non-zero status code if any command fails. The `test.sh` script is silently failing, that means changes causing failures are slipping through our CI pipeline and being merged. Resolves: #419 ## Note Just the last 3 patches, the first 6 are from #420. re-base just shows it works on top of 420, it is going to have to be rebased again when 420 merges. ACKs for top commit: apoelstra: ACK 97dc0ea Tree-SHA512: b86a6876d8c45a2b90b7b3c8adbc08ad6f49b430b1cfaec31cd2de8441cb96af39c63da02b98d6ed71dfab045d466d71d3757297886b5e44ebb6cbaeb4ed32dd

58db1b67536794b24cdc0bf8149460509a247b31 Run WASM for multiple toolchains (Tobin Harding) 946ac3b51ebb8191b051c8d80bd27bee1656993e Do docs build in Nightly job (Tobin Harding) f7bc7d3728326e6de0d2462dfba9705b93a1f55e Install clang to run adress sanitizer (Tobin Harding) 96685c571da4f3c64d178e0f0d3a02ce99de9136 Remove unnecessary matrix (Tobin Harding) a8a679ed7dbfa4871270de779aee78b3cdaeebd6 Re-name nightly CI job to Nightly (Tobin Harding) 9c9d622b0ee26a3de2d43de298ded822245520a4 Remove trailing whitespace (Tobin Harding) Pull request description: Improve the CI pipeline. Done while investigating #419. This PR now only includes the GitHub actions changes, I'm moving the `test.sh` changes to a rust-bitcoin/rust-secp256k1#422 because they cannot be merged yet. (Please note, this PR has been heavily re-worked so discussion below may be confusing to reviewers new to the PR.) ACKs for top commit: apoelstra: ACK 58db1b67536794b24cdc0bf8149460509a247b31 thomaseizinger: ACK 58db1b67536794b24cdc0bf8149460509a247b31 Tree-SHA512: 5520cf7a7ea0ba701aeaf6b97150416192c0629df8b65545a20d8937a4d76bd323a0d7a875deccb7ce9adc4f3a423e6cd27b300682f206f79407f5ab4eaa5ddb

thomaseizinger reviewed Mar 17, 2022

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

tcharding force-pushed the make-ci-fail branch from a349619 to 9bc27ed Compare March 18, 2022 23:45

tcharding force-pushed the make-ci-fail branch 2 times, most recently from f3126b4 to 6808416 Compare March 18, 2022 23:53

tcharding changed the title ~~Improve CI script and pipeline~~ Improve CI script Mar 19, 2022

This was referenced Mar 19, 2022

Combine PRs for ease of merging #423

Closed

Latest release (0.5.0) does not work on WASM & test.shis broken #419

Closed

apoelstra previously approved these changes Mar 21, 2022

View reviewed changes

tcharding closed this Mar 24, 2022

tcharding deleted the make-ci-fail branch March 24, 2022 01:34

tcharding restored the make-ci-fail branch March 25, 2022 01:08

apoelstra reopened this Mar 26, 2022

Remove trailing whitespace

9c9d622

Remove single character of trailing whitespace.

tcharding dismissed apoelstra’s stale review via 6d08fae March 29, 2022 00:16

tcharding force-pushed the make-ci-fail branch from 6808416 to 6d08fae Compare March 29, 2022 00:16

thomaseizinger reviewed Mar 29, 2022

View reviewed changes

tcharding mentioned this pull request Mar 29, 2022

Move panic test to top of script #432

Merged

tcharding added 5 commits March 30, 2022 08:44

Re-name nightly CI job to Nightly

a8a679e

In line with the `Tests` job and for the fact that this job does stuff with the nightly toolchain other than bench. Re-name nightly CI job from `bench_nightly`to `Nightly`.

Remove unnecessary matrix

96685c5

We use a matrix with a single element, this is unnecessary.

Install clang to run adress sanitizer

f7bc7d3

The address sanitizer job is silently failing at the moment because we do not install clang. Install clang so the address sanitizer job can run. Do not fix the silent failure, that will be done later on.

tcharding force-pushed the make-ci-fail branch from 6d08fae to 58db1b6 Compare March 29, 2022 21:50

apoelstra approved these changes Mar 30, 2022

View reviewed changes

thomaseizinger approved these changes Mar 31, 2022

View reviewed changes

tcharding mentioned this pull request Mar 31, 2022

Fix test script silent failure #422

Merged

apoelstra merged commit 8caf41d into rust-bitcoin:master Apr 1, 2022

tcharding deleted the make-ci-fail branch April 4, 2022 02:38

	- name: Install clang # Needed for ASan.
	- name: Install clang for ASan

Improve CI script #420

Improve CI script #420

Uh oh!

Conversation

tcharding commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 17, 2022

Uh oh!

Uh oh!

thomaseizinger commented Mar 17, 2022

Uh oh!

tcharding commented Mar 17, 2022

Uh oh!

thomaseizinger commented Mar 17, 2022

Uh oh!

tcharding commented Mar 17, 2022

Uh oh!

apoelstra commented Mar 17, 2022

Uh oh!

TheBlueMatt commented Mar 17, 2022

Uh oh!

apoelstra commented Mar 17, 2022

Uh oh!

tcharding commented Mar 18, 2022

Uh oh!

tcharding commented Mar 19, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 25, 2022

Uh oh!

thomaseizinger commented Mar 25, 2022

Uh oh!

tcharding commented Mar 25, 2022

Uh oh!

apoelstra commented Mar 26, 2022

Uh oh!

tcharding commented Mar 27, 2022

Uh oh!

tcharding commented Mar 29, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 30, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

thomaseizinger commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Mar 17, 2022 •

edited

Loading

tcharding commented Mar 29, 2022 •

edited

Loading