search

bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
838 stars 300 forks source link

Stop pinning everything in CI #1398

Open LLFourn opened 5 months ago

LLFourn commented 5 months ago 
          Why do we need all these pins?

We don't have to test on MSRV we just have to check that it builds on MSRV. dev-dependencies shouldn't have to be pinned. Or at least that's the policy I'd like to adopt.

See sophisticated technology on how to do this: https://github.com/bitcoindevkit/coin-select/blob/990226a982bd6036bed5a7674fe57d6f9d48e49e/.github/workflows/test.yml#L40

cc @notmandatory

_Originally posted by @LLFourn in https://github.com/bitcoindevkit/bdk/pull/1397#discussion_r1553437195_

tnull commented 5 months ago

We don't have to test on MSRV we just have to check that it builds on MSRV. dev-dependencies shouldn't have to be pinned.

I disagree: MSRV means "This project supports/works with this rust version", and, everything untested may be considered broken. I regularly run into MSRV breakages for depedencies that 'check' but don't test their MSRV on all supported platforms.

LLFourn commented 5 months ago

We don't have to test on MSRV we just have to check that it builds on MSRV. dev-dependencies shouldn't have to be pinned.

I disagree: MSRV means "This project supports/works with this rust version", and, everything untested may be considered broken. I regularly run into MSRV breakages for depedencies that 'check' but don't test their MSRV on all supported platforms.

This "regularly" is quite concerning tbh :sweat_smile:. It's a good point that if our tests don't have the same MSRV as build then there's no way to verify that the tests actually pass on the MSRV. I want to add that this situation would imply a bug in the rust compiler, either at the version in CI or at the MSRV. Either that or some weird rust version based conditional compilation in a dependency.

For me the monotonically growing list of dependency pins and using old versions of dev dependencies is a worse problem than this. I definitely don't take the attitude that "everything untested may be considered broken" when developing in rust. Logic that is not tested can be considered broken but not everything :)

Not a hill I'll die on though and if the maintainers who are really affected by this want to take the stricter interpretation of MSRV that's fine with me!

tnull commented 5 months ago

This "regularly" is quite concerning tbh 😅. It's a good point that if our tests don't have the same MSRV as build then there's no way to verify that the tests actually pass on the MSRV. I want to add that this situation would imply a bug in the rust compiler, either at the version in CI or at the MSRV. Either that or some weird rust version based conditional compilation in a dependency.

Yeah, at the very least building with MSRV on every supported platform should be covered by CI. I previously suggested that to other projects which deemed it unnecessary, only to introduce MSRV unnoticed violations on some of the platforms (looking at you reqwest).

For me the monotonically growing list of dependency pins and using old versions of dev dependencies is a worse problem than this.

Hopefully it's not monotonically growing. Many projects now support an MSRV compatible with BDK's, but they tend to not check it and violate it at some point in time. In my experience they are often willing to fix it, at which point the pins can be dropped again (also looking an reqwest, for example).

I definitely don't take the attitude that "everything untested may be considered broken" when developing in rust. Logic that is not tested can be considered broken but not everything :)

Yeah, fair, that was was of course hyperbolic :)