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

@@ -9,6 +9,8 @@ jobs:
    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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CI script #420

Improve CI script #420

tcharding commented Mar 17, 2022 •

edited

Loading

tcharding commented Mar 17, 2022

thomaseizinger commented Mar 17, 2022

tcharding commented Mar 17, 2022

thomaseizinger commented Mar 17, 2022

tcharding commented Mar 17, 2022

apoelstra commented Mar 17, 2022

TheBlueMatt commented Mar 17, 2022

apoelstra commented Mar 17, 2022

tcharding commented Mar 18, 2022

tcharding commented Mar 19, 2022

apoelstra left a comment

tcharding commented Mar 25, 2022

thomaseizinger commented Mar 25, 2022

tcharding commented Mar 25, 2022

apoelstra commented Mar 26, 2022

tcharding commented Mar 27, 2022

tcharding commented Mar 29, 2022

thomaseizinger left a comment

thomaseizinger Mar 29, 2022

tcharding Mar 29, 2022

thomaseizinger Mar 29, 2022

tcharding Mar 29, 2022

tcharding commented Mar 29, 2022 •

edited

Loading

apoelstra left a comment

apoelstra commented Mar 30, 2022

thomaseizinger left a comment

thomaseizinger commented Mar 31, 2022

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

Improve CI script #420

Improve CI script #420

Conversation

tcharding commented Mar 17, 2022 • edited Loading

tcharding commented Mar 17, 2022

thomaseizinger commented Mar 17, 2022

tcharding commented Mar 17, 2022

thomaseizinger commented Mar 17, 2022

tcharding commented Mar 17, 2022

apoelstra commented Mar 17, 2022

TheBlueMatt commented Mar 17, 2022

apoelstra commented Mar 17, 2022

tcharding commented Mar 18, 2022

tcharding commented Mar 19, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 25, 2022

thomaseizinger commented Mar 25, 2022

tcharding commented Mar 25, 2022

apoelstra commented Mar 26, 2022

tcharding commented Mar 27, 2022

tcharding commented Mar 29, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Mar 29, 2022

Choose a reason for hiding this comment

tcharding Mar 29, 2022

Choose a reason for hiding this comment

thomaseizinger Mar 29, 2022

Choose a reason for hiding this comment

tcharding Mar 29, 2022

Choose a reason for hiding this comment

tcharding commented Mar 29, 2022 • edited Loading

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 30, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Mar 31, 2022

tcharding commented Mar 17, 2022 •

edited

Loading

tcharding commented Mar 29, 2022 •

edited

Loading