Skip to content

Epic CI overhaul#2353

Merged
apoelstra merged 4 commits intorust-bitcoin:masterfrom
tcharding:01-18-ci
Feb 2, 2024
Merged

Epic CI overhaul#2353
apoelstra merged 4 commits intorust-bitcoin:masterfrom
tcharding:01-18-ci

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jan 18, 2024 •
edited
Loading

The combination of some work by myself [0] and Kix [1].

Draft so I can use github's infrastructure to test it all out.

Includes some patches at the front to fix real issues that the new test infrastructure found - WIN.

[0] #2328
[1] #2343

Coincidentally this closes 1124

Resolve: #1124

@Kixunil Kixunil mentioned this pull request Jan 18, 2024
Closed
@pool2win pool2win mentioned this pull request Jan 19, 2024
Merged
apoelstra added a commit that referenced this pull request Jan 20, 2024
@apoelstra
Merge #2361: Add github action for llvm-cov coverage
782c2d1 
47b476e Ignore fuzz dir from coverage reports (pool2win)
50ff685 Add github action for llvm-cov coverage (pool2win)

Pull request description:

  There has been discussion around generating coverage reports for tests. See #1853  and #2353.

  This PR adds an action using llvm-cov and coveralls.

  The action generates lcov coverage report using llvm-cov and uploads the generated report to coveralls. You can see sample reports for my fork here: https://coveralls.io/github/pool2win/rust-bitcoin

  I am using the [cargo-llvm-cov](https://github.com/taiki-e/cargo-llvm-cov) wrapper around llvm-cov.

  I also use the coveralls official github action to push the coverage report to coveralls. It removes the need to deal with repository secrets etc, making the action easy to run for all contributors on their forks.

  We can move this action later to rust.yml if we need to.

ACKs for top commit:
  apoelstra:
    ACK 47b476e
  Kixunil:
    ACK 47b476e
  tcharding:
    ACK 47b476e

Tree-SHA512: 7a7b8caf94d4b4828416bc1e6df49bfe47fede7b8fa4386d905a6f40439a95df1d3fedecdd6d64675f2f6858851f18d2f8863994aa20ac1ab224f97ca48b4af2
This was referenced Jan 23, 2024
Closed
Closed
@github-actions github-actions bot added ci C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-io PRs modifying the io crate test labels Jan 23, 2024
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 23, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 7749605098

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.148%
Totals Coverage Status
Change from base Build 7742657266: 0.0%
Covered Lines: 19328
Relevant Lines: 22969
💛 - Coveralls

@tcharding tcharding mentioned this pull request Jan 23, 2024
Merged
@github-actions github-actions bot added the C-internals PRs modifying the internals crate label Jan 23, 2024
@tcharding tcharding force-pushed the 01-18-ci branch 2 times, most recently from 12bc1b0 to b8889d2 Compare January 23, 2024 04:30
@tcharding tcharding mentioned this pull request Jan 23, 2024
Open
@tcharding tcharding force-pushed the 01-18-ci branch 3 times, most recently from a77cea5 to c82fb32 Compare January 23, 2024 05:15
@github-actions github-actions bot added the doc label Jan 23, 2024
@tcharding tcharding force-pushed the 01-18-ci branch 3 times, most recently from 8689eee to 6ac3b03 Compare January 23, 2024 06:10
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jan 23, 2024

There is a problem with the sed statement, can't work it out right now.

for featuretwo in `echo "${features}" | sed 's/^.*'\(^| \)$feature'\($| \)//'`

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jan 23, 2024

' should go right before $feature, not \(^. Also I suggest enclosing $feature in double quotes just in case.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jan 24, 2024

Legend, cheers.

Also I suggest enclosing $feature in double quotes just in case.

shellcheck told me that too, will do.

tcharding and others added 3 commits February 2, 2024 05:55
@tcharding
Use bash to run shell scripts
422d301 
Use `bash` instead of `sh` to run shell scripts.

We would like to support Nix users who do not typically have any shell
other than `sh` at a known path, therefore use `/usr/bin/env bash`.
@tcharding
Use env bash instead of /bin/bash
242aa67 
As we do in all the other shell scripts use `env` to call `bash`.
@Kixunil @tcharding
CI: Epic overhaul
5c15ed5 
Re-write the whole CI pipeline.

Co-developed-by: Martin Habovstiak <martin.habovstiak@gmail.com>
@tcharding tcharding force-pushed the 01-18-ci branch 3 times, most recently from 5df660e to f9844db Compare February 1, 2024 19:17
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Feb 1, 2024 •
edited
Loading

Changes in force push:

  • Rebased on master
  • Added an err function
  • Called locale
  • Called env

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Feb 1, 2024

err calls say ... but what's say?

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Feb 1, 2024

Bother, sometimes I wonder why I got out of bed. (I thought shellcheck would catch this, I actually ran the script this time.)

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Feb 1, 2024 •
edited
Loading

FYI I took these functions from https://github.com/rust-lang-deprecated/rustup.sh/blob/master/rustup.sh and I back-and-forth'ed on how many of those helper functions to use. If you haven't read that file before then read the comments at the top, my favourite one is "# Don't make typos. You just have to be better than that."

@Kixunil you might enjoy

# # Error handling
#
# Oh, my goodness, error handling. It's terrifying.

apoelstra
apoelstra previously approved these changes Feb 1, 2024
View reviewed changes
Copy link
Copy Markdown
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 a740657fe2c0baef5d954003e5e98d51ddb980dc

@tcharding
CI: Check for required commands
01a66a7 
Add a function to check for commands used by the `run_task` script. Move
the version printing to after the check. Also print the bash version in
use.
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Feb 2, 2024

Another mistake, force push updates the last patch to correctly print the bash version.

@tcharding tcharding added the P-high High priority label Feb 2, 2024
apoelstra
apoelstra approved these changes Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
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 01a66a7

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Feb 2, 2024

I can't do a proper review now but if you decide to merge this anyway because it's not super-important to get 100% right I'm fine with it.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Feb 2, 2024

Let's just merge it. I think we both have a bunch of things we'd like to try but we can do it in followup PRs, and none of this affects users of the library.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Feb 2, 2024

Lets go!

@apoelstra apoelstra merged commit 585af9d into rust-bitcoin:master Feb 2, 2024
@tcharding tcharding deleted the 01-18-ci branch February 5, 2024 00:58
apoelstra added a commit to apoelstra/rust-bitcoin that referenced this pull request Feb 28, 2024
@apoelstra
ci: delete *test.sh files
e386cbf 
These are not run in CI since rust-bitcoin#2353 and are likely to go out of date. If
we want a script that users can run locally then we should create a new
script that wraps our current CI.
@nyonson nyonson mentioned this pull request Mar 16, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees

No one assigned

Labels

C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-internals PRs modifying the internals crate C-io PRs modifying the io crate C-units PRs modifying the units crate ci doc P-high High priority test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Print tool versions in CI

4 participants

@tcharding @coveralls @Kixunil @apoelstra