rust-bitcoin / rust-secp256k1

Rust language bindings for Bitcoin secp256k1 library.
Creative Commons Zero v1.0 Universal
341 stars 262 forks source link

CI: Re-write using maintainer tools #699

Closed tcharding closed 1 month ago

tcharding commented 4 months ago

Patch 1 is now on its own in #728

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

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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.

tcharding commented 1 month ago

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

tcharding commented 1 month ago

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 commented 1 month ago

In f12fd75c6b1acb647d74fa17391576294c81a38a:

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 commented 1 month ago

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

I've moved the xargo (no-std test) to a separate script and CI job. Also, put the MSAN stuff back in.

apoelstra commented 1 month ago

Will give @Kixunil a couple days to review since he engaged with this PR.

tcharding commented 1 month ago

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

tcharding commented 1 month ago

Gentle bump

Kixunil commented 1 month ago

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

tcharding commented 1 month ago

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

Kixunil commented 1 month ago

It's already Friday?! Shit!

apoelstra commented 1 month ago

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

Kixunil commented 1 month ago

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

tcharding commented 1 month ago

It's already Friday?! Shit!

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

tcharding commented 1 month ago

Will re-spin, thanks.

tcharding commented 1 month ago

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 https://github.com/rust-bitcoin/rust-bitcoin/pull/3201

apoelstra commented 1 month ago

In 7402490b96e64991334f707479f3f864a3924a33

The rev/ref thing is still not fixed.

tcharding commented 1 month ago

Ah yes, thanks. I used the just merged https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/pull/13

tcharding commented 1 month ago

And it appears to work - WIN!