chore: correct the MSRV check #934

JohnTitor · 2020-08-16T19:39:16Z

Motivation

The current MSRV check is lazybones, it's meaningless unless we override toolchain. Actually, we cannot compile tracing crates with 1.39.0, it should work with 1.44.0 or later because of the protobuf crate and others.

Solution

This enforces to override toolchain and updates MSRV to 1.44.0, this should compile all the crates fine.
And replaces actions/checkout@master with main as they've renamed the default branch name to main.

hawkw

Thanks for adding override: true, I hadn't realized that this was necessary.

The MSRV bump is not ideal. In general, we have tried to follow the same policy as the rest of the Tokio project, and always support at least the last three stable compiler versions. Therefore, I'd prefer to avoid bumping to 1.44.0 if there's anything else we can do instead.

In this case, it looks like the bump was necessary due to the dependency on the protobuf crate. I believe we depend on protobuf only as a transitive dependency of prometheus, which is itself a dependency of opentelemetry (which tracing-opentelemetry depends on). However, we don't currently use the opentelemetry crate's "metrics" feature.

The tracing-opentelemetry crate's dependency on opentelemetry disables the default features and only enables the trace feature, which shouldn't require protobuf:

tracing/tracing-opentelemetry/Cargo.toml

Line 25 in 8fd9e76

    
           opentelemetry = { version = "0.8", default-features = false, features = ["trace"] }

However, it looks like the "metrics" feature is still being enabled because the examples depend on opentelemetry without disabling its default features:

tracing/examples/Cargo.toml

Lines 54 to 56 in 8fd9e76

    
           # opentelemetry example 
        
           opentelemetry = "0.8" 
        
           opentelemetry-jaeger = "0.7"

Therefore, I think we should be able to build successfully on 1.39.0, if we just change the opentelemetry dependency in the examples to

opentelemetry = { version = "0.8", default-features = false, features = ["trace"] }

Then, our build should not include the protobuf crate.

Can we do that instead? Thanks!

I'm surprised that our builds are pulling

hawkw · 2020-08-17T16:51:12Z

.github/workflows/CI.yml

    steps:
-    - uses: actions/checkout@master
+    - uses: actions/checkout@main


I have a slight preference for making the change from master -> main in a separate branch so that the git history is clearer, but it's not a big deal.

+1. I believe that changing this now would close all open PRs on tracing.

Well, this branch is actions/checkout's, not tracing's. We use actions/checkout (https://github.com/actions/checkout) to checkout on GHA and we're currently using master. But on that repo, they, i.e., GitHub renamed their default branch to main. So this change is reasonable.

robjtede · 2020-08-17T17:15:15Z

Therefore, I think we should be able to build successfully on 1.39.0

The compile error we observed on our MSRV 1.39 seemed to be a 1.40 feature (option flattening) that came into use in the most recent tracing-attributes crate (0.1.10).

tracing/tracing-attributes/src/lib.rs

Line 933 in 35fee1c

.flatten();

Failed job: https://github.com/actix/actix-net/runs/990495589

hawkw · 2020-08-17T17:22:13Z

The compile error we observed on our MSRV 1.39 seemed to be a 1.40 feature (option flattening) that came into use in the most recent tracing-attributes crate (0.1.10).

Ah. In that case, we should just back out that change and release a new version of tracing-attributes. I'll go ahead and do that now.

PR #875 added code to `tracing-attributes` that uses `Option::flatten`, which was only added to the standard library in Rust 1.40. This broke our MSRV, but we missed it because the MSRV CI checks weren't working correctly (fixed in #934). This commit removes the use of `Option::flatten`, and replaces it with a manual implementation. It's a little less pretty, but it builds on our MSRV (1.39.0).

The `opentelemetry` crate depends on `prometheus`, which depends on `protobuf`, a crate which doesn't compile on our MSRV (Rust 1.39). This was missed due to issues with the MSRV CI checks, which will be fixed fixed in #934. Therefore, once the MSRV checks work properly, the `protobuf` dependency will break our builds. We don't _need_ the `opentelemetry/metrics` feature, which is what enables the `prometheus` (and thus `protobuf`) dependency. `tracing-opentelemetry` already has a `default-features = false` dependency on `opentelemetry`, but the examples don't. Therefore, I've changed the examples crate to disable `opentelemetry`'s default features as well.

PR #875 added code to `tracing-attributes` that uses `Option::flatten`, which was only added to the standard library in Rust 1.40. This broke our MSRV, but we missed it because the MSRV CI checks weren't working correctly (fixed in #934). This commit removes the use of `Option::flatten`, and replaces it with a manual implementation. It's a little less pretty, but it builds on our MSRV (1.39.0).

The `opentelemetry` crate depends on `prometheus`, which depends on `protobuf`, a crate which doesn't compile on our MSRV (Rust 1.39). This was missed due to issues with the MSRV CI checks, which will be fixed fixed in #934. Therefore, once the MSRV checks work properly, the `protobuf` dependency will break our builds. We don't _need_ the `opentelemetry/metrics` feature, which is what enables the `prometheus` (and thus `protobuf`) dependency. `tracing-opentelemetry` already has a `default-features = false` dependency on `opentelemetry`, but the examples don't. Therefore, I've changed the examples crate to disable `opentelemetry`'s default features as well.

hawkw · 2020-08-17T18:31:00Z

After the changes merged in #937, it looks like there are a couple of additional dependencies that don't support Rust 1.39.0: dashmap, which is a transitive dependency brought in by inferno, and remove_dir_all, a transitive dependency of tempdir.

The inferno dependency is only required by an example, while the tempdir dependency is only used in tests. This means that cargo +1.39.0 check --all will still pass, provided that we don't pass the --examples and --tests flags. So, it seems like we have two options here:

Bump MSRV. Everything, including tests & examples, builds fine on 1.40.0, but our policy (inherited from Tokio) of always guaranteeing at least the last three Rust versions would allow us to bump as far as 1.42.
Exclude tests and examples from the MSRV policy, and change the MSRV checks to cargo check --all without the --examples --tests flags. These should never break downstream builds on older Rust versions, since dependent crates don't pull our dev dependencies or any of the examples. However, the current CI configuration would also run the tests against our MSRV (if it was actually working), which is kind of nice to have --- although I think that running tests against multiple compiler version is much more important for beta and nightly. It's fairly unlikely for behavior to be different on an older compiler version in a way that breaks the tests, but it's much more likely that something on nightly accidentally breaks them.

@tokio-rs/tracing what do we think about this? Should we do a bump now, or change CI to exclude tests & examples from MSRV?

LucioFranco · 2020-08-17T21:03:56Z

I say just merge this if this implements the check, I don't think its critical that we check examples/tests for MSRV but its nice to have.

LucioFranco · 2020-08-17T21:04:19Z

but I would like to stick with the same MSRV as tokio if we can, its imo not important to enforce tests/examples if that means we need to bump to 1.44.

hawkw · 2020-08-17T21:19:37Z

@LucioFranco

but I would like to stick with the same MSRV as tokio if we can, its imo not important to enforce tests/examples if that means we need to bump to 1.44.

We only need to bump to 1.40 to build everything, now --- 1.44 was required by the protobuf crate, which we depended on because we didn't disable opentelemetry's default features. b358201 removes that.

Tokio's MSRV is 1.39 for Tokio 0.2 (0.3, as a breaking change, will bump to 1.45). However, we can't currently build tracing against 1.39, due to test- and example-only transitive dependencies. Thus, the question I asked in #934 (comment) is whether we should exclude the tests & examples from MSRV checks, so that we can continue to build against 1.39, or whether we should bump to 1.40 (or as far as 1.42, which a "current + three versions" MSRV policy would allow).

LucioFranco · 2020-08-17T21:20:43Z

Go for whatever tokio 0.3 is going for I think that is fine.

davidbarsky · 2020-08-17T21:21:23Z

Of the two options Eliza offered, I'd prefer to bump our MSRV to 1.42. I believe that we don't need 1.44 because Eliza removed usage of Option::flatten and the protobuf crate, where the latter required a bump to 1.44.

If this PR is updated to not change the the the master branch to the main branch (as I'm worried this will close any open PRs on this repo) and the MSRV is set to 1.42, I'm personally satisfied with this PR and will approve it.

hawkw · 2020-08-17T21:36:06Z

Go for whatever tokio 0.3 is going for I think that is fine.

We are not going to bump to the latest version except when we release a breaking change. This is for the release version of tracing. Tokio still supports 1.39 for v0.2.x, the bump to 1.45 is specifically for tokio 0.3.

I'm happy to bump to 1.45 or whatever when we release tracing 0.2, if there is an important reason to.

JohnTitor · 2020-08-18T01:03:51Z

Please ping me when you come to a conclusion and I'm happy to update. Not part of the team, but I agree that the MSRV should be as low as possible.

hawkw · 2020-08-18T01:18:39Z

Please ping me when you come to a conclusion and I'm happy to update. Not part of the team, but I agree that the MSRV should be as low as possible.

I think we should bump to 1.40.0 as part of this branch — per an offline conversation with @davidbarsky and @LucioFranco, I think they're both fine with that.

davidbarsky · 2020-08-18T01:18:49Z

@JohnTitor Sorry about that. Since Eliza removed the protobuf crate (which had a requirement of 1.44) and the the usage Option::flatten (which had a requirement of 1.40) in #937 and following Tokio's MSRV policy, I think we should set the MSRV to 1.42.

JohnTitor · 2020-08-18T01:23:19Z

No worries, but it seems there's still a conflict, 1.40.0 or 1.42.0?

davidbarsky · 2020-08-18T01:29:13Z

Yup, there is. Again, sorry about that! Let's do 1.40, it's a bit more conservative and it unblocks you. Final answer!

hawkw

Once we change the minimum Rust version in the configs, this will be good to merge! Thank you for the fix!

@JohnTitor, I hope you don't mind if I go ahead and apply the change to the MSRV in the configs and merge this? I'd really like to get the MSRV checks actually working again.

.github/workflows/CI.yml

JohnTitor · 2020-08-18T17:05:20Z

Sorry for the delay!

@JohnTitor, I hope you don't mind if I go ahead and apply the change to the MSRV in the configs and merge this? I'd really like to get the MSRV checks actually working again.

Yeah, feel free. Thanks for taking care of it instead :)

hawkw · 2020-08-18T17:14:56Z

No problem! Thanks so much for noticing the issue and fixing it!

## 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>

JohnTitor added 2 commits August 17, 2020 04:21

chore: override toolchain to correct MSRV

db1a813

chore fix actions/checkout's branch name

ab474c0

JohnTitor requested review from hawkw and a team as code owners August 16, 2020 19:39

JohnTitor mentioned this pull request Aug 16, 2020

solve framed integration with actix-http actix/actix-net#179

Merged

4 tasks

hawkw reviewed Aug 17, 2020

View reviewed changes

hawkw mentioned this pull request Aug 17, 2020

chore: fix MSRV breakage #937

Merged

hawkw approved these changes Aug 18, 2020

View reviewed changes

.github/workflows/CI.yml Outdated Show resolved Hide resolved

.github/workflows/CI.yml Outdated Show resolved Hide resolved

Change configured MSRV from 1.44 to 1.40

68aeb28

hawkw merged commit 1e086e8 into tokio-rs:master Aug 18, 2020

JohnTitor deleted the tweak-ci branch August 18, 2020 17:24

This was referenced Aug 18, 2020

docs: consistent MSRV docs & policy explanation #941

Merged

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: correct the MSRV check #934

chore: correct the MSRV check #934

JohnTitor commented Aug 16, 2020 •

edited

Loading

hawkw left a comment

hawkw Aug 17, 2020

davidbarsky Aug 17, 2020

JohnTitor Aug 18, 2020

robjtede commented Aug 17, 2020 •

edited

Loading

hawkw commented Aug 17, 2020

hawkw commented Aug 17, 2020

LucioFranco commented Aug 17, 2020

LucioFranco commented Aug 17, 2020 •

edited

Loading

hawkw commented Aug 17, 2020

LucioFranco commented Aug 17, 2020

davidbarsky commented Aug 17, 2020

hawkw commented Aug 17, 2020 •

edited

Loading

JohnTitor commented Aug 18, 2020 •

edited

Loading

hawkw commented Aug 18, 2020

davidbarsky commented Aug 18, 2020

JohnTitor commented Aug 18, 2020

davidbarsky commented Aug 18, 2020

hawkw left a comment

JohnTitor commented Aug 18, 2020

hawkw commented Aug 18, 2020

	# opentelemetry example
	opentelemetry = "0.8"
	opentelemetry-jaeger = "0.7"

chore: correct the MSRV check #934

chore: correct the MSRV check #934

Conversation

JohnTitor commented Aug 16, 2020 • edited Loading

Motivation

Solution

hawkw left a comment

Choose a reason for hiding this comment

hawkw Aug 17, 2020

Choose a reason for hiding this comment

davidbarsky Aug 17, 2020

Choose a reason for hiding this comment

JohnTitor Aug 18, 2020

Choose a reason for hiding this comment

robjtede commented Aug 17, 2020 • edited Loading

hawkw commented Aug 17, 2020

hawkw commented Aug 17, 2020

LucioFranco commented Aug 17, 2020

LucioFranco commented Aug 17, 2020 • edited Loading

hawkw commented Aug 17, 2020

LucioFranco commented Aug 17, 2020

davidbarsky commented Aug 17, 2020

hawkw commented Aug 17, 2020 • edited Loading

JohnTitor commented Aug 18, 2020 • edited Loading

hawkw commented Aug 18, 2020

davidbarsky commented Aug 18, 2020

JohnTitor commented Aug 18, 2020

davidbarsky commented Aug 18, 2020

hawkw left a comment

Choose a reason for hiding this comment

JohnTitor commented Aug 18, 2020

hawkw commented Aug 18, 2020

JohnTitor commented Aug 16, 2020 •

edited

Loading

robjtede commented Aug 17, 2020 •

edited

Loading

LucioFranco commented Aug 17, 2020 •

edited

Loading

hawkw commented Aug 17, 2020 •

edited

Loading

JohnTitor commented Aug 18, 2020 •

edited

Loading