Skip to content

Fix test script silent failure #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Apr 1, 2022
Merged

Fix test script silent failure #422

merged 9 commits into from
Apr 1, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 19, 2022 •
edited
Loading

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.

This was referenced Mar 19, 2022
Merged
Merged
Closed
Closed
apoelstra
apoelstra previously approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e080754

Big improvement! Though maybe we shouldn't merge it while WASM is broken.

@tcharding
Copy link
Member Author

I noticed the clang sanity check is missing in one case and a little wrong in the other. I pushed an additional patch to fix that.

Though maybe we shouldn't merge it while WASM is broken.

It shouldn't get through CI till then ;)

@apoelstra
Copy link
Member

utACK 51a79fb

@tcharding
Remove trailing whitespace
9c9d622 
Remove single character of trailing whitespace.
@tcharding
Copy link
Member Author

Bringing to your attention @apoelstra that this PR removes the pipes instead of using the option you proposed someplace else to make any command in a pipe fail (set -o pipefail I think it was).

@tcharding
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`.
@tcharding
Remove unnecessary matrix
96685c5 
We use a matrix with a single element, this is unnecessary.
@tcharding
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
Do docs build in Nightly job
946ac3b 
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.
@tcharding
Run WASM for multiple toolchains
58db1b6 
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.
@apoelstra
Copy link
Member

Yep, I am aware -- I suggested pipefail after this PR was open, actually :). (I'm also not sure if pipefail is actually what we want, since these are && connectors rather than pipes..)

In any case, I think it's clearer to have -e on and to not use &&. Thanks.

apoelstra added a commit that referenced this pull request Mar 30, 2022
@apoelstra
Merge #421: Fix wasm build
f7cae46 
bfd88db Move WASM const definitions to a source file (Tobin Harding)

Pull request description:

  Total re-write ... again :)

  Currently we are defining the WASM integer size and alignments in the `stdio.h` header file, this is wrong because this file is included in the build by way of `build.rs` as well as by upstream `libsecp256k1`.

  Move WASM integer definitions to a `C` source file and build the file  into the binary if target is WASM.

  Fixes the first part of #419 (#422 does the second part).

  ### Note to reviewers

  I'm not exactly sure why the directory `was-sysroot` is named as it is or if the name is significant to `cargo` , please review carefully the directory tree changes.

  ```
  cd secp256k1-sys
  tree wasm
  wasm
  ├── wasm.c
  └── wasm-sysroot
             ├── stdio.h
             ├── stdlib.h
             └── string.h
  ```

ACKs for top commit:
  thomaseizinger:
    ACK bfd88db
  apoelstra:
    ACK bfd88db

Tree-SHA512: ba822b764fb5f74dfd22cc797f7e3f70440dbaabfe34e0475c796e0e5d88f2086bedb00a1ec765cce91bde6bb45130b9abe5d9289317d6c20f692c6ed711969e
thomaseizinger
thomaseizinger previously approved these changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 51a79fb

I think this needs a rebase onto #420 so we have clang-9 available for the WASM stuff.

contrib/test.sh
@@ -94,3 +94,4 @@ if [ "$DO_BENCH" = true ]; then
cargo bench --all --features="unstable"
fi

exit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of explicitly saying exit 0? Isn't that the default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default exit status is the exit status of the last command, because we use set -e to exit the script early if any command fails then it is going to be 0. I like to do it explicitly though.

@tcharding
Copy link
Member Author

I think this needs a rebase onto #420 so we have clang-9 available for the WASM stuff.

Yes, now that #421 merged #420 is the only one left :)

@tcharding
Copy link
Member Author

Rebased on #420, might be able to merge without me rebasing again, let's see :)

@tcharding
test.sh: explicitly return 0
7bec31c 
As per UNIX convention a Bash script should exit with status code 0 if
it completes successfully.
@tcharding
test.sh: Use set -e to exit on failure
a3582ff 
Currently the `test.sh` script is silently failing because we do not
exit if a command fails. We can achieve this by using the Bash builtin
`set -e`.

For some reason I cannot explain a chain of commands that fails does not
fail the script. Instead of working out _why_ just remove the chain and
run each command on its own. This is functionally the same and, I hazard
a guess, is what the original author hoped to achieve with the chaining.
@tcharding
Run correct clang --version
97dc0ea 
For the test that uses `clang-9` do the sanity call using `clang-9`
instead of `clang`.

For the test that uses `clang` add a sanity call to `clang --version`.
apoelstra added a commit that referenced this pull request Apr 1, 2022
@apoelstra
Merge #420: Improve CI script
8caf41d 
58db1b6 Run WASM for multiple toolchains (Tobin Harding)
946ac3b Do docs build in Nightly job (Tobin Harding)
f7bc7d3 Install clang to run adress sanitizer (Tobin Harding)
96685c5 Remove unnecessary matrix (Tobin Harding)
a8a679e Re-name nightly CI job to Nightly (Tobin Harding)
9c9d622 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 #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 58db1b6
  thomaseizinger:
    ACK 58db1b6

Tree-SHA512: 5520cf7a7ea0ba701aeaf6b97150416192c0629df8b65545a20d8937a4d76bd323a0d7a875deccb7ce9adc4f3a423e6cd27b300682f206f79407f5ab4eaa5ddb
@apoelstra
Copy link
Member

Looks like both GIthub and my local commit-labelling tool are smart enough to do the right thing, since #420 is merged. (Well, Github still shows "9 commits" and doesn't indicate that they're shared, but it doesn't demand a rebase, so all good.)

apoelstra
apoelstra approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 97dc0ea

Thank you for doing this!

@apoelstra apoelstra merged commit e4fb575 into rust-bitcoin:master Apr 1, 2022
@tcharding tcharding deleted the fix-test-script branch April 4, 2022 02:38
@apoelstra apoelstra mentioned this pull request Jul 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@thomaseizinger thomaseizinger thomaseizinger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Latest release (0.5.0) does not work on WASM & test.shis broken
3 participants
@tcharding @apoelstra @thomaseizinger