Skip to content

CI related fixes#381

Closed
dekellum wants to merge 4 commits intohyperium:masterfrom
dekellum:ci-warn-fixes
Closed

CI related fixes#381
dekellum wants to merge 4 commits intohyperium:masterfrom
dekellum:ci-warn-fixes

Conversation

@dekellum
Copy link
Copy Markdown
Contributor

@dekellum dekellum commented Jan 3, 2020

I'll start by demonstrating that CI is broken (on latest master, due to a new deprecation) and go on to fix those things with edits here.

Denying _all_ warnings causes local tree development trouble. The
intent can be better captured by RUSTFLAGS=-Dwarnings in CI.
@dekellum
Copy link
Copy Markdown
Contributor Author

dekellum commented Jan 3, 2020

So 066a3ee shows that CI was broken. This can be nominally fixed by not denying all warnings (cae30b0). Next I'll cleanup the deprecations…

This crate uses static strings for error messages throughout and in
many cases Display or outer error are implemented by calling
`description()`.  We silence these kinds of warnings for now:

   warning: use of deprecated item 'std::error::Error::description':
       use the Display impl or to_string()
       --> src/header/name.rs:2006:14
	|
   2006 |         self.description().fmt(f)
	|              ^^^^^^^^^^^
	|
	= note: `#[warn(deprecated)]` on by default

by adding `#[allow(deprecated)]` where necessary.  Once the ecosystem
has had more time to transition away from `StdError::description`,
this can be revisited, likely by pulling out description
implementations here completely, possibly moving these static messages
to const values.
@seanmonstar
Copy link
Copy Markdown
Member

We should probably just remove the fn description usage (and keep the deny). WDYT?

@dekellum
Copy link
Copy Markdown
Contributor Author

dekellum commented Jan 4, 2020

I've restored deny warnings in CI (ccd3701), using what I believe is a better way (also seen in use in tokio, etc.) Or do you not like that approach?

Removing the description impls could be considered a breaking change if anyone is still relying on those descriptions, which the API was still promoting as of 0.2.0 release. To remove those impls means default std description of "description() is deprecated; use Display" is returned, which would be unfortunate if seen by an end-end user?

Thus I've taken the more conservative approach of silencing the warnings for now. I suggest revisiting a few rust releases after 1.41 is stable.

@dekellum dekellum marked this pull request as ready for review January 4, 2020 00:05
@dekellum dekellum requested a review from carllerche January 17, 2020 16:07
@dekellum
Copy link
Copy Markdown
Contributor Author

Always a pleasure.

@dekellum dekellum closed this Jan 24, 2020
@seanmonstar
Copy link
Copy Markdown
Member

Ouch, I'm sorry I forgot about this :(

@dekellum
Copy link
Copy Markdown
Contributor Author

dekellum commented Jan 24, 2020

Your memory aside, I suspect you have also decided not to worry about the possibility of end-users seeing (e.g. on stderr in console) runtime error messages of the form "description() is deprecated; use Display".

Yes of course that implies they are using the deprecated description API, but they may also be using a rustc before it was really deprecated in 1.41.

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

Successfully merging this pull request may close these issues.

2 participants