Pull Request Test Coverage Report for Build 10110951027

Details

• 2 of 7 (28.57%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.01%) to 83.228%

Totals
Change from base Build 10110491704:	-0.01%
Covered Lines:	19671
Relevant Lines:	23635

💛 - Coveralls

Apparently, this already caught a non-additive cargo feature in bitcoin-io:

FAIL [   0.001s]       major        auto_trait_impl_removed

<......>

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.32.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type Error is no longer UnwindSafe, in io/src/error.rs:7
  type Error is no longer RefUnwindSafe, in io/src/error.rs:7
     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   0.022s] <unknown>

We don't need nightly (and somehow it fails)

The rustdoc JSON format has frequent breaking changes: e.g. one last week, and very probably one more today or tomorrow. We don't always support the latest nightly's format because that would be a very heavy maintenance burden. I think my limited time and energy would benefit the community more if I dedicated it to other work instead.

Sounds like cargo-semver-checks should declare which version it supports and then we just pin that.

Sounds like cargo-semver-checks should declare which version it supports and then we just pin that.

stable, that's why I am running this on stable.

I think I should also update the original semver to run on stable and use the RUSTC_BOOTSTRAP env var as well. (another PR)

FYI to fix that error this new check found you need to add _not_unwind_safe: core::marker::PhantomData<(&'static mut (), core::cell::UnsafeCell<()>)>, field to the mentioned error struct. This is a breaking change for IO but it was in principle broken already.

FYI to fix that error this new check found you need to add _not_unwind_safe: core::marker::PhantomData<(&'static mut (), core::cell::UnsafeCell<()>)>, field to the mentioned error struct. This is a breaking change for IO but it was in principle broken already.

Done in 1164ed9e7b23262f391d566ceee6a8df677ab8db, since we use merge commits for the PR strategy, I took the liberty of adding to this PR.

Sounds like cargo-semver-checks should declare which version it supports and then we just pin that.

The latest version of cargo-semver-checks always supports current beta, current stable, and older stable versions as far back as noted in each set of release notes: https://github.com/obi1kenobi/cargo-semver-checks/releases/tag/v0.32.0

For the current version, that's Rust 1.74+. The upcoming release will likely raise that to 1.77+.

Note that this version requirement is about which version of Rust is used to generate the rustdoc JSON. It does not impose an MSRV bound on your crates, so you can support older Rust versions if you so choose.

@obi1kenobi I meant we need to sync them somehow. I suggest adding a --print-rust-json-version flag to print the current version so it can be synced in CI automatically.

@obi1kenobi I meant we need to sync them somehow. I suggest adding a --print-rust-json-version flag to print the current version so it can be synced in CI automatically.

Sorry, I'm confused. Could you elaborate on the problem and how the output of such a flag would be helpful? I'm not sure what needs to be synced and isn't currently.

My understanding of this PR is that if current-stable Rust is used, everything should just work.

Sorry, I'm confused. Could you elaborate on the problem and how the output of such a flag would be helpful? I'm not sure what needs to be synced and isn't currently.

Whenever we update one we need to update the other. The PR currently works but in a few months "latest stable" will be a different version than it is now and it may change the Json output breaking our CI. That' why we pin things. We can obviously pin both (and we should!) but we also want to update the pin periodically to not stay behind. A failed periodic update doesn't interfere with our work on merging PRs which is really helpful since we don't have to wait until a solution is found (in case it's not obvious) or a new compiler/tool version (if it's a their bug) and may avoid costly rebases of PRs.

To do the periodic update automatically int the CI we need an automated way to assign compiler version to the tool version. Having an option that just prints it is easiest. An alternative would be to publish a table at a stable URL that gets updated with each release. I think the switch is a simpler approach.

Whenever we update one we need to update the other. The PR currently works but in a few months "latest stable" will be a different version than it is now and it may change the Json output breaking our CI. That' why we pin things. We can obviously pin both (and we should!) but we also want to update the pin periodically to not stay behind.

The current code in the PR installs latest stable and latest cargo-semver-checks, which are guaranteed to work with each other and will not break:
https://github.com/rust-bitcoin/rust-bitcoin/pull/3003/files#diff-8c3de78a6f07b22f56457fe701ccc74e767b3e22be85276c500eb1a81c01281eR48-R53

I recommend against pinning to a specific cargo-semver-checks version in most cases, since it usually ends up being more work and more trouble than it's worth.

Is there a specific reason you wanted to pin it?

@obi1kenobi because if you add a new lint to cargo-semver-checks, which reveals breakage in our codebase which results in a red X on CI, then all of our release branches (which do not receive regular updates, especially with API-breaking things) will permanently have a red X on them. Furthermore, every one of our current PRs will have a red X on them forcing us to drop everything to address the new cargo-semver-checks issue. (And if the issue is a bug in cargo-semver-checks, then we will wind up needing to pin it anyway while the upstream issue is resolved.)

Furthermore, if we find an old issue that CI should have caught in the past, we will have no way of re-running (or even knowing) what "CI" was back then, which makes our forensics harder.

If we just pin everything and then use cronjobs to update the pins, that fixes all of the above issues. New cargo-semver-checks issues will appear as a red X on the "update pin" PR and we can address it at our leisure.

@Kixunil honestly I don't think we need upstream support to do this. If @obi1kenobi says it always supports "the latest stable" then our cronjob can just query "the latest stable" and use rustc -V to get its exact version.

Your call on pinning, of course! 👍

Just keep in mind that "the latest stable" policy means that the latest cargo-semver-checks supports the stable that was latest at the time of its release. It does not mean that an arbitrary past version of cargo-semver-checks will always support today's latest stable (i.e. a Rust released "in the future" relative to that cargo-semver-checks version). This is probably obvious, but I wanted to err on the side of caution and mention it just in case :)

@obi1kenobi what @apoelstra said plus I want to mention that problems with unpinned stuff happened already multiple times. Stability is very helpful and since we pin other things already anyway it's not that big deal.

@Kixunil honestly I don't think we need upstream support to do this. If @obi1kenobi says it always supports "the latest stable" then our cronjob can just query "the latest stable" and use rustc -V to get its exact version.

Good point, though it assumes cargo-semver-checks will be released sufficiently soon after rustc release. Otherwise we'll get transient failures on the update PR which would be a bit annoying. @obi1kenobi do you have any policy on this? E.g. "release the version no later than 12 hours after Rust is released" would allow us to set the cron to update at certain time (since Rust also updates at certain time) as an easy way to avoid the annoyance.

E.g. "release the version no later than 12 hours after Rust is released" would allow us to set the cron to update at certain time (since Rust also updates at certain time) as an easy way to avoid the annoyance.

This is why latest beta is supported — that's the version that graduates to become the next stable, so it's supported "in advance" essentially.

If I remember correctly, Rust releases on Thursdays. So if you have the cron job re-pin to latest stable Rust and latest cargo-semver-checks each Thursday, you should be good. I have a job like this in cargo-semver-checks, in case that's useful for inspiration: https://github.com/obi1kenobi/cargo-semver-checks/blob/main/.github/workflows/add_new_rust_to_matrix.yml

Oh that's great then! So does it mean both beta and stable are supported and it can detect the correct version and use it dynamically?

Oh that's great then! So does it mean both beta and stable are supported and it can detect the correct version and use it dynamically?

In your case, you're generating the rustdoc JSON with some version of Rust, and cargo-semver-checks will detect the format version and (if supported) adjust itself automatically, yes. The current-stable and current-beta versions at the time of the cargo-semver-checks' version release are both supported, always.

If you were having cargo-semver-checks itself generate the rustdoc JSON, it would use whatever Rust version is the default as configured in rustup. It won't switch to another version since I felt that was too magical and would be the sort of thing that causes a few hours of debugging for someone sooner or later. Better to do "the obvious thing" even if it results in a (very quick and obvious-to-fix) error.

Thanks @obi1kenobi that all sounds great!

So @storopoli I think we should have files storing latest stable version and semver checks version, then use those in the script and attempt to update them to the latest ones weekly on Thursday (not sure about the time, maybe try Friday?)

🚨 API BREAKING CHANGE DETECTED

Ok this is ready for another round!

🚨 API BREAKING CHANGE DETECTED

I'm kinda out of the loop on this but I might read it more later.

API BREAKING CHANGE DETECTED

I much prefer the alerts vs the CI breakage and the diff being stuffed into git. Although I wish there was a way to silence it after the first time or something because it gets pretty noisy after every commit.

FTR this specific PR shouldn't change any API so something wrong is going on.

We should have some marker to say "intended break" (probably a comment) which would silence the notification on pushes but still perform the check to allow us see its output. (Is it really the break we're expecting or a different one?)

FTR this specific PR shouldn't change any API so something wrong is going on.

We should have some marker to say "intended break" (probably a comment) which would silence the notification on pushes but still perform the check to allow us see its output. (Is it really the break we're expecting or a different one?)

Actually we are changing it in 7dbd44bacd42834e488093154e4e505a0ab53ce3

https://github.com/rust-bitcoin/rust-bitcoin/blob/7dbd44bacd42834e488093154e4e505a0ab53ce3/io/src/error.rs#L5-L18

Do you want me to move it into a new PR?

I much prefer the alerts vs the CI breakage and the diff being stuffed into git. Although I wish there was a way to silence it after the first time or something because it gets pretty noisy after every commit.

Good idea! I'll try to hack something with actions/github-script to suppress the warning if we already commented on the PR.

🚨 API BREAKING CHANGE DETECTED

LOL, I completely forgot, sorry about the noise.

🚨 API BREAKING CHANGE DETECTED

🚨 API BREAKING CHANGE DETECTED

🚨 API BREAKING CHANGE DETECTED