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

Can you please document the minimum required Rust version and treat a bump as breaking change? #936

Closed
d-e-s-o opened this issue Aug 17, 2020 · 8 comments

Comments

@d-e-s-o
Copy link
Contributor

d-e-s-o commented Aug 17, 2020

Perhaps I missed it, but I couldn't find the minimum Rust version that tracing requires anywhere. That's conceptually fine, I can find it out. But you should almost certainly "pin" it and treat any update of it as a breaking change, otherwise the next tracageddon is just around the corner. So you will need to have some control over it yourself.

Pretty please? :-)

@hawkw
Copy link
Member

hawkw commented Aug 17, 2020

The minimum Rust version should be documented in the lib.rs docs and READMEs for most of the crates — at least, I know if it's present for the core crates (for example, tracing lists it at the end of the paragraph here: https://github.com/tokio-rs/tracing/tree/master/tracing#overview).

I think the MSRV note in the lib.rs docs somehow ended up getting misplaced, as it's in the "Feature flags" section (where it definitely doesn't belong) — I'll make sure that gets fixed. If it's missing anywhere else, I'll make sure it's documented there as well.

@hawkw
Copy link
Member

hawkw commented Aug 17, 2020

As for whether MSRV bumps are considered a breaking change, we have thus far followed the same policy as the rest of the Tokio project: we will always support at least the last three Rust versions, but don't consider a minimum Rust version bump breaking, as long as it is at least three versions old.

I'd be willing to consider a different MSRV policy if there's a compelling argument, though.

@davidbarsky
Copy link
Member

Like Eliza, I'm willing to consider a different MSRV policy in a good-faith manner, but I would be strongly opposed to making an MSRV bump a breaking change. That's beyond what most Rust projects provide and even the Rust team itself provides.

I also think that adopting a policy of "an MSRV bump is a breaking change" could be catastrophic to tracing-core (and to a lesser extent, tracing) because we want to keep the number of breaking changes to tracing-core to an absolute minimum. Multiple tracing-cores in a single dependency closure shouldn't ever happen, because it does, some libraries or crates will just stop emitting tracing spans and events.

@hawkw
Copy link
Member

hawkw commented Aug 17, 2020

FWIW, I don't think I'm aware of any major crate that treats any MSRV bump as a breaking change. Typically, the policy tends to be of the form "the last n versions from the latest Rust", where n depends on the project's policies. Then, MSRV increases are not considered breaking changes as long as the new MSRV is at least n versions behind the current compiler version. See, for example, crossbeam-rs/crossbeam#503 (comment) for a discussion of this policy in crossbeam.

@d-e-s-o
Copy link
Contributor Author

d-e-s-o commented Aug 18, 2020

Thanks for the responses and the pointers!

Let me provide a bit of background for my question: I provide a minimum supported Rust version for most of my crates. Now in order to provide and have trust in that claim, I have to test with that very version. So that's what I do in CI. So, if tracing decides to bump its minimum Rust version and accepts a change that requires that new version, my pipeline is red.

Now, I want to be clear here: it's not the end of the world. It's not nice, but assuming everything builds fine with a newer version I can bump my minimum Rust version and life goes on. It's annoying, yes. And it's forced on me from some external entity that I have no control over at a point in time that I can't control (and which may and will be bad), but it's not the end of the world. I also do have the option of pinning dependencies in the CI specifically. It's not going to be great to maintain that and it also would arguably no longer be 100% in line with the claim I want to make, which is that the project builds with a certain version of Rust, without users having to manually fiddle with a lock file or adjust their Cargo.toml or any such magic. So I would consider that last resort (and in all likelihood will not do it because it's simply impractical from my point of view).

Reading between the lines in some of the linked threads that's what many projects do and it is (or was) also a source of breakage in perhaps as many projects. And it does not seem as if a one-size-fits-all solution to this problem has been identified yet.

Anyway, that's ultimately where my request is coming from. A policy like "the last three Rust versions" doesn't really help me because it's not directly actionable. I am not going to start a timer to remind me every six weeks to check a subset of my crates for dependencies that may have bumped and could have broken my setup (same argument about impracticality) or automatically bump my own guarantees in that time frame.

I am admittedly not super aware of what the wider Rust ecosystem is doing in this respect (I did read through the links you provided, mostly). If I am understanding correctly, though, Rayon seems to have adopted such a policy (or at least a policy close enough to what I suggested that would still help with my use case): rayon-rs/rfcs#3 (I didn't find anything indicating that they dropped it). Not sure if it is considered "major" enough.

But I guess my point is: I would argue it's the right thing to consider a MSRV bump a breaking change but I can see reasons (perhaps specifically with your crate) that speak against doing that. So I won't give you a hard time ;-) I would also hope that over time with Rust maturing (which I would consider has happened already to the degree relevant for this discussion) and tracing doing the same, your policy can be adapted (although personally I'd not get much out of a change a la "we now support four versions of Rust" for the reasons explained :-))

[...] making an MSRV bump a breaking change. That's beyond what [...] even the Rust team itself provides.

Can you please elaborate and provide an example? I don't think I am following.

Just FYI: tracing-attributes mentions: "Compiler support: requires rustc 1.39+". I got a CI build breakage with 1.39.0 (seems to be Option::flatten, which is labelled as available in 1.40). Is your statement meant to be read as >1.39? I am also not consuming that crate directly, but only through tracing, which suggests that (if 1.39.0 is meant to be supported as I read it), its 1.39 statement may also be wrong.

@hawkw
Copy link
Member

hawkw commented Aug 18, 2020

Thanks for the responses and the pointers!

[...]

But I guess my point is: I would argue it's the right thing to consider a MSRV bump a breaking change but I can see reasons (perhaps specifically with your crate) that speak against doing that. So I won't give you a hard time ;-) I would also hope that over time with Rust maturing (which I would consider has happened already to the degree relevant for this discussion) and tracing doing the same, your policy can be adapted (although personally I'd not get much out of a change a la "we now support four versions of Rust" for the reasons explained :-))

I totally understand where you're coming from. I agree that it's difficult to provide MSRV guarantees while also using the latest versions of dependencies, especially when those dependencies may not support the same minimum Rust version as you do, and may change MSRV in patch versions that Cargo will pick up automatically. The current state of minimum-Rust support is not great, and I'd rather not make your life worse.

Unfortunately, the problem is that tracing also has dependencies. In many cases, those dependencies are required to provide compatibility layers with various other crates, such as log and opentelemetry. For some dependencies that are only used internally, it's possible to remove the dependency at the cost of writing more code ourselves, or to pin it to a fixed version to avoid MSRV breakage in patches.

However, for crates that we depend on specifically to provide compatibility, those crates are part of our public API. Obviously, we can't remove them. Furthermore, we need to be able to have permissive version requirements for them: if a downstream project requires, say foo = "1", and tracing-foo pins the dependency to something like foo = "=1.2.34", to avoid a MSRV bump in foo 1.2.35, there will be two different versions of foo in the downstream project's dependency tree. foo:1.2.34 and foo:1.2.35's types will not be compatible, so the compatibility layer will no longer work. Therefore, in most cases, we can't easily pin dependencies to specific versions.

I'd love to be able to consider a MSRV bump breaking, but as long as we have dependencies that don't, we are stuck with the lowest common denominator across all our dependencies' MSRV policies. I think the only real solution for this is if Cargo were to (hypothetically) make MSRV a first-class concept in its data model, so that dependency version selection can be constrained based on the current toolchain version. If we didn't have to worry about dependencies' MSRVs, it would be much easier for us to consider MSRV as part of our stability guarantees. But, I'm not currently aware of any Cargo work in this area, unfortunately.

We could also consider having separate MSRVs for each crate in tracing, so that the MSRV for tracing-foo can be bumped when foo bumps its MSRV without changing the minimum supported crate in the repo. But, automated MSRV checks become significantly more complex if we do this (we can't just have a step that builds everything at a global MSRV) and I'm concerned that the complexity could be potentially error-prone. I'd be willing to consider it, though, if there's a lot of demand for more stable MSRVs for the core crates, especially if someone else wants to help set up CI automation for this... :)

Just FYI: tracing-attributes mentions: "Compiler support: requires rustc 1.39+". I got a CI build breakage with 1.39.0 (seems to be Option::flatten, which is labelled as available in 1.40). Is your statement meant to be read as >1.39? I am also not consuming that crate directly, but only through tracing, which suggests that (if 1.39.0 is meant to be supported as I read it), its 1.39 statement may also be wrong.

Yeah, this one's on us: due to an error in the CI configuration, our MSRV checks were not actually setting the correct compiler version, so we missed some MSRV issues. This has been fixed in #934, which also bumps the MSRV to 1.40 due to dependencies no longer supporting 1.39. Sorry about that! PR #941 updates the MSRV notes to 1.40, and adds new documentation explaining the current policy, which should, at least, make this a bit clearer in the future.

Once #941 merges, I'll publish a new tracing-attributes version and yank the current one, since its documentation claims to support a compiler that it doesn't compile on. Again, sorry about that.

hawkw added a commit that referenced this issue Aug 18, 2020
## Motivation

PR #934 fixed a bug in the CI configuration where MSRV checks were not
being run correctly. After this was fixed, it was necessary to bump the
MSRV to 1.40.0, as the tests were no longer actually passing on 1.39,
because some dependencies no longer support it.

While updating the documentation to indicate that the new MSRV is 1.40,
I noticed that the note on the MSRV was located inconsistently in the
READMEs and `lib.rs` documentation of various crates, and missing
entirely in some cases. Additionally, there have been some questions on
what our MSRV _policies_ are, and whether MSRV bumps are considered
breaking changes (see e.g. #936). 

## Solution

I've updated all the MSRV notes in the documentation and READMEs to
indicate that the MSRV is 1.40. I've also ensured that the MSRV note is
in the same place for every crate (at the end of the "Overview" section
in the docs), and that it's formatted consistently.

Furthermore, I added a new section to the READMEs and `lib.rs` docs
explaining the current MSRV policy in some detail. Hopefully, this
should answer questions like #936 in the future. The MSRV note in the
overview section includes a link to the section with further details.

Finally, while doing this, I noticed a couple of crates
(`tracing-journald` and `tracing-serde`) were missing top-level `lib.rs`
docs. Rather than just adding an MSRV note and nothing else, I went
ahead and fixed this using documentation from those crate's READMEs.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@d-e-s-o d-e-s-o changed the title Can you please document the minimum required Rust version and treat a bump is breaking change? Can you please document the minimum required Rust version and treat a bump as breaking change? Aug 19, 2020
@d-e-s-o
Copy link
Contributor Author

d-e-s-o commented Aug 19, 2020

Thanks for your response! Agreed, there are dependencies on other crates and they would in most cases have to play along. The same would also apply to most of my crates (definitely the ones using tracing), so even if you were to comply I'd strictly speaking need to petition all my other dependencies as well.

So let me close this issue. My questions have been answered and I understand that ultimately what we would need is a more or less eco system wide push for a change. I am happy to see agreement on the underlying point, though.

[...] making an MSRV bump a breaking change. That's beyond what [...] even the Rust team itself provides.

Can you please elaborate and provide an example? I don't think I am following.

(I'd still be interested in getting an answer to this question though, @davidbarsky)

@d-e-s-o d-e-s-o closed this as completed Aug 19, 2020
@hawkw
Copy link
Member

hawkw commented Aug 19, 2020

For the record, I was also not quite sure what David was referring to exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants