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

v0.1.x: Don't deny warnings #1368

Merged
merged 3 commits into from
Sep 30, 2019
Merged

Conversation

dekellum
Copy link
Contributor

This is minimally, an alternative solution to #1351, and for maximum future-proofing, could be applied in addition to #1351. Note that #1351 greatly reduces build warning noise for the common missing dyn issue, while this PR just preserves the warnings.

Motivation

While there are tradeoffs and some open questions regarding using #![deny(warnings)] on tokio's master branch, its harder to justify keeping this setting on the v0.1.x stable maintenance branch where new features aren't being added, and as in #1351, subsequent rust stable releases can introduce new warnings that deny makes fatal errors (at least with a local dev tree or CI).

Solution

Remove deny(warnings) everywhere on v0.1.x branch (crate root modules, examples, tests, benches, etc.)! Also drop the explicit allow(rust_2018_idioms) (in d6dcaea) which was only selectively applied and caused a build failure on 1.27.2 (see #1351 (comment)).

@dekellum
Copy link
Contributor Author

This CI failure, is due to pkg-config new 0.3.15 release increasing its MSRV (yes, in a PATCH release) to 1.28. I'll need to look into that further in the release staging proposal (#1348).

@dekellum
Copy link
Contributor Author

CI for this isn't fixed by just raising CI MSRV test to 1.28.0, given:

error: failed to parse manifest at `/home/vsts/work/1/s/tokio-macros/Cargo.toml`

Caused by:
  editions are unstable

Caused by:
  feature `edition` is required

@dekellum
Copy link
Contributor Author

This can also be viewed as a partial backport (from the future) of #1416.

@LucioFranco LucioFranco added the T-v0.1.x Topic: tokio 0.1.x label Aug 15, 2019
This is just too aggressive for a stable maintenance branch of tokio,
in that new rust release warnings are prooving too hard to fix.
@dekellum dekellum force-pushed the dont-deny-warnings branch from da8fecd to a88793c Compare August 19, 2019 18:25
@dekellum
Copy link
Contributor Author

Just to preserve this in case there are additional future problems with deny(warnings) and to demonstrate it hopefully now passing CI, I rebased, given some redundant changes with current v0.1.x branch.

@LucioFranco
Copy link
Member

@dekellum do we want to deny warnings on CI test runs?

@dekellum
Copy link
Contributor Author

That would make this a complete backport of #1416. My intuition is to not do so however, given that v0.1.x is a maintenance branch and new warning denials are just as likely to come from new stable or nightly compilers (based on our experience). That said, IMO, either way is an improvement. Alternatively, this could wait until once the next denial becomes problematic.

@LucioFranco
Copy link
Member

@carllerche ^^ what do you think?

@carllerche
Copy link
Member

I'm fine w/ this. 0.1 is a maintenance branch.

@dekellum
Copy link
Contributor Author

Thanks for attempt to resolve the merge conflicts, @LucioFranco. Let me know if you need anything.

@LucioFranco
Copy link
Member

@dekellum I think we need to resolve the conflicts but need to wait for #1616 but I would like to hold off on releasing all the crates again as I need to get a 0.2 release out and I dont wanna release 50 crates today 😄

@dekellum
Copy link
Contributor Author

Thanks. FWIW, given cap-lints behavior, I don't think this PR warrants any releases itself. Its just a nice to have for in-tree builds and avoiding future (spurious) CI failures!

@LucioFranco
Copy link
Member

@dekellum sounds good, once I get the other PR merged and releases are out, would you be able to bring this PR inline and we can merge?

@dekellum
Copy link
Contributor Author

Yep, thanks.

@dekellum
Copy link
Contributor Author

@LucioFranco this can now be merged, no conflicts, with CI passing. Thanks in advance!

@LucioFranco
Copy link
Member

@dekellum thank you for all this!

@LucioFranco LucioFranco merged commit 39f369f into tokio-rs:v0.1.x Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-v0.1.x Topic: tokio 0.1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants