Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop pinning everything in CI #1398

Open
LLFourn opened this issue Apr 5, 2024 · 3 comments
Open

Stop pinning everything in CI #1398

LLFourn opened this issue Apr 5, 2024 · 3 comments
Labels
ci discussion There's still a discussion ongoing
Milestone

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Apr 5, 2024

          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 #1397 (comment)

@tnull
Copy link
Contributor

tnull commented Apr 8, 2024

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.

@notmandatory notmandatory added the ci label Apr 8, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Apr 8, 2024
@notmandatory notmandatory added the discussion There's still a discussion ongoing label Apr 8, 2024
@LLFourn
Copy link
Contributor Author

LLFourn commented Apr 9, 2024

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 😅. 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
Copy link
Contributor

tnull commented Apr 9, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci discussion There's still a discussion ongoing
Projects
Status: Discussion
Development

No branches or pull requests

3 participants