CI: Re-write using maintainer tools #699

tcharding · 2024-05-19T23:51:02Z

Patch 1 is now on its own in #728

Re-write CI using the new maintainer tools script. A few things to note:

Currently we have Cross job in rust.yaml as well as cross.yaml, remove the one in rust.yaml.
Put sanitizer and wasm jobs in their own scripts
Utilize extra_tests.sh for additional feature combos
We are exceeding the 20 job limit, see the README

Unless I'm made a mistake this shouldn't reduce the test coverage in any way (except sanitizer mentioned below).

I commented out the MSAN stuff same as we did in hashes. I'm not sure what is the status of that but it seems to be failing still - did not look into it.

Please note, I do not know why the xargo stuff is run from in the ASAN job currently, but this PR keep it that way - adding it to the sanitizer.sh script.

tcharding · 2024-05-20T00:00:48Z

I don't know what the windows cross problem is, it exists on master ATM - I've just disabled the job in this PR and included the error message. Debugging windows builds is not my jam.

Kixunil · 2024-06-29T05:14:22Z

Can you explain what i the point of this PR? I don't understand it, I just see a bunch of seemingly unrelated and unimportant changes.

apoelstra · 2024-06-29T13:08:44Z

@Kixunil I haven't read it yet because it's in a draft state with red Xs, but the point is to unify the CI scripts across all the repos in this ecosystem because they do a lot of things are were constantly getting out of sync.

Kixunil

I'm really not in the mood to review the last commit rn, sorry.

Kixunil · 2024-08-07T03:33:06Z

src/key.rs

@@ -996,6 +996,7 @@ impl str::FromStr for Keypair {
    type Err = Error;

    #[allow(unused_variables, unreachable_code)] // When built with no default features.
+    #[allow(clippy::diverging_sub_expression)] // Because of panic.


I disagree, we should just disable FromStr impl if neither global-context nor alloc is available.

I can do that, will do it as a separate PR because its unrelated.

Can we put it in front of this one?

Sure, will do.

Yeah, oops, we should've filed an issue for this. The panic thing was there to avoid a major version bump at some point in the past but we should've fixed it properly as soon as we were ready to do a major one. (Maybe we need a new label for this? "Must be addressed by next major rev" or something?)

I don't see how that was helpful since the code was broken anyway under that configuration, it just blew up during runtime rather than compile time.

Only if the user actually called the function (and didn't catch the panic).

If we'd changed things to fail at compile time then previously-compiling code would have stopped compiling.

As for "why change the code at all then" it was to fix #491, in #492 (the previous code always failed, and with a C abort even, not a panic).

Kixunil · 2024-08-07T03:37:03Z

.github/workflows/cross.yml

@@ -37,20 +37,16 @@ jobs:
          - x86_64-unknown-linux-musl
          # - x86_64-unknown-netbsd  # error in tests "error: test failed, to rerun pass '--lib'", disabled for now
    steps:
-      - name: Checkout Crate
-        uses: actions/checkout@v2
+      - name: "Checkout repo"


IME quotes are unusual in GH actions ant thus I think they're harder to read.

Do you want me to remove all quotes from all of our GitHub actions files?

I think that every string on YAML should be quoted.
@Kixunil don't know if you already seen this but here are a bunch of good arguments: https://ruudvanasseldonk.com/2023/01/11/the-yaml-document-from-hell

(PS: I wish we could do GH Actions using TOML, I honestly think YAML is very very easy to shoot yourself in the foot...)

I knew YAML is bad but I didn't know it's that bad. I'd make an exception for the name: field since it just contains a human-readable name but if you want to use some kind of linter and ban it entirely, it's fine.

tcharding · 2024-08-07T21:22:28Z

Note to self, check that CRATES is set, and commented, correctly.

tcharding · 2024-08-08T05:57:14Z

Force push includes intalling xargo and running it in sanitizer.sh (I had missed the install previously). Please see PR description for mention of how I do not know why it is run from there.

apoelstra · 2024-08-08T16:28:00Z

In f12fd75:

We should reenable the disabled msan test. It's a year old and probably whatever incompatibility was causing it has gone away.

Also I don't see any confusion about xargo in the commit message or content.

tcharding · 2024-08-09T20:09:52Z

Kix is away till Monday he told me, there is no rush on this one anyways.

Kixunil

ACK bdb8bc3

Will try to review the last commit tomorrow.

tcharding · 2024-08-22T23:25:18Z

Gentle bump

Kixunil · 2024-08-23T05:39:55Z

Before I review it, did you fix the SSOT stuff?

tcharding · 2024-08-23T06:04:58Z

Let me check another day (its Friday arvo now for me), I did not realize there was an SSOT issue with this PR.

Kixunil · 2024-08-23T07:12:38Z

It's already Friday?! Shit!

apoelstra · 2024-08-23T14:51:19Z

I'm also not aware of any SSOT thing, but:

I think the first commit (change panic to compile-time failure) should be in its own PR.
In ee458d8 you need to change rev: b2ac11 to ref: <full sha1 hash>
While you're at it, we should update to the latest rev of maintainer-tools
And while we're updating stuff we could bump up nightly-version.

Kixunil · 2024-08-23T18:42:12Z

I mean crate list. I haven't looked into it yet.

tcharding · 2024-08-23T21:44:21Z

It's already Friday?! Shit!

I come from the future, didn't you know?

tcharding · 2024-08-23T21:45:01Z

Will re-spin, thanks.

We have a `Cross` job in `rust.yml` and also a `cross` workflow. The workflow is a superset of the job, remove the redundant job.

As we do in other places stop using the `actions` runner and use the `dtolnay` one to checkout toolchain. While we are at it, use double quotes for `name` fields (this is a small stylistic thing I have been introducing in an effort to make the yaml files a bit easier to read).

tcharding · 2024-08-29T02:13:06Z

I mean crate list. I haven't looked into it yet, I don't

This uses

# Dot for single crate in workspace to test.
CRATES=(".")

I'm not sure how you'd like me to improve on that, but it does introduce the same maintenance burden that we are trying to eliminate with rust-bitcoin/rust-bitcoin#3201

storopoli

ACK 8101442

storopoli · 2024-08-29T21:03:35Z

justfile

-  cargo clippy --all-targets --all-features -- --deny warnings
+  cargo +$(cat ./nightly-version) clippy --workspace --all-targets --all-features -- --deny warnings
+
+# Run cargo fmt


All of the others have . at the end

Suggested change

# Run cargo fmt

# Run cargo fmt.

Fixed, thanks.

storopoli · 2024-08-29T21:03:43Z

justfile

+
+# Run cargo fmt
+fmt:
+  cargo +$(cat ./nightly-version) fmt --all

 # Check the formatting


Suggested change

# Check the formatting

# Check the formatting.

storopoli

ACK 7402490

d600a6c Feature gate the Keypair::FromStr impl (Tobin C. Harding) Pull request description: Currently we are panicing if neither `global-context` or `alloc` features are enabled. We do not need to do so, we can just disable the whole impl of `FromStr`. This was pulled out of #699. ACKs for top commit: apoelstra: ACK d600a6c successfully ran local tests Kixunil: ACK d600a6c Tree-SHA512: 940bec95ce732b4bc482e23da114cb03b767780f93777621c9d0985d1288e36756bdf6f050172eac00f89b6f39aa0efdb30cc77425b6f87505659c8c012981ca

apoelstra · 2024-08-30T13:32:49Z

In 7402490

The rev/ref thing is still not fixed.

Re-write CI using the new maintainer tools script. A few things to note: - Put sanitizer and wasm jobs in their own scripts - Utilize `extra_tests.sh` for additional feature combos - We are exceeding the 20 job limit, see the README Unless I'm made a mistake this shouldn't reduce the test coverage in any way.

tcharding · 2024-09-01T22:04:54Z

Ah yes, thanks. I used the just merged rust-bitcoin/rust-bitcoin-maintainer-tools#13

tcharding · 2024-09-01T22:39:12Z

And it appears to work - WIN!

storopoli

ACK aee0cfc

apoelstra

ACK aee0cfc successfully ran local tests

tcharding mentioned this pull request May 19, 2024

ci: shellcheck checks rust-bitcoin/rust-miniscript#689

Merged

tcharding force-pushed the 05-02-ci-maintainer-tools branch 2 times, most recently from 6d5723c to a70ff50 Compare May 19, 2024 23:59

tcharding force-pushed the 05-02-ci-maintainer-tools branch from a70ff50 to 5280f73 Compare May 20, 2024 00:11

tcharding marked this pull request as draft May 20, 2024 00:37

tcharding mentioned this pull request May 20, 2024

ci: shellcheck checks #697

Merged

tcharding force-pushed the 05-02-ci-maintainer-tools branch 9 times, most recently from c6ce901 to f7a6d64 Compare August 6, 2024 23:37

tcharding marked this pull request as ready for review August 6, 2024 23:38

Kixunil reviewed Aug 7, 2024

View reviewed changes

tcharding marked this pull request as draft August 7, 2024 21:22

tcharding force-pushed the 05-02-ci-maintainer-tools branch 2 times, most recently from 30535f1 to e9bb412 Compare August 8, 2024 05:54

tcharding marked this pull request as ready for review August 8, 2024 05:55

tcharding force-pushed the 05-02-ci-maintainer-tools branch 3 times, most recently from ddf1209 to f12fd75 Compare August 8, 2024 06:32

Kixunil reviewed Aug 11, 2024

View reviewed changes

tcharding mentioned this pull request Aug 22, 2024

Truncate secret hash #722

Merged

tcharding added 2 commits August 29, 2024 12:06

CI: Remove cross job

0668943

We have a `Cross` job in `rust.yml` and also a `cross` workflow. The workflow is a superset of the job, remove the redundant job.

tcharding dismissed apoelstra’s stale review via 8101442 August 29, 2024 02:11

tcharding force-pushed the 05-02-ci-maintainer-tools branch from ee458d8 to 8101442 Compare August 29, 2024 02:11

tcharding mentioned this pull request Aug 29, 2024

Feature gate the Keypair::FromStr impl #728

Merged

storopoli approved these changes Aug 29, 2024

View reviewed changes

tcharding force-pushed the 05-02-ci-maintainer-tools branch 2 times, most recently from 520b1dc to 7402490 Compare August 29, 2024 23:01

storopoli approved these changes Aug 30, 2024

View reviewed changes

tcharding force-pushed the 05-02-ci-maintainer-tools branch from 7402490 to aee0cfc Compare September 1, 2024 22:04

storopoli approved these changes Sep 2, 2024

View reviewed changes

apoelstra approved these changes Sep 2, 2024

View reviewed changes

apoelstra merged commit 41a6d43 into rust-bitcoin:master Sep 2, 2024
29 checks passed

CI: Re-write using maintainer tools #699

CI: Re-write using maintainer tools #699

Conversation

tcharding commented May 19, 2024 • edited Loading

tcharding commented May 20, 2024

Kixunil commented Jun 29, 2024

apoelstra commented Jun 29, 2024

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Aug 7, 2024

tcharding commented Aug 8, 2024

apoelstra commented Aug 8, 2024

tcharding commented Aug 9, 2024

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Aug 22, 2024

Kixunil commented Aug 23, 2024

tcharding commented Aug 23, 2024

Kixunil commented Aug 23, 2024

apoelstra commented Aug 23, 2024

Kixunil commented Aug 23, 2024

tcharding commented Aug 23, 2024 • edited Loading

tcharding commented Aug 23, 2024

tcharding commented Aug 29, 2024 • edited Loading

storopoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storopoli left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 30, 2024

tcharding commented Sep 1, 2024

tcharding commented Sep 1, 2024

storopoli left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented May 19, 2024 •

edited

Loading

apoelstra Aug 11, 2024 •

edited

Loading

tcharding commented Aug 23, 2024 •

edited

Loading

tcharding commented Aug 29, 2024 •

edited

Loading