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

Update Discv5 version #212

Merged
merged 3 commits into from
Jan 4, 2022
Merged

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Jan 3, 2022

Fixes #182
Maybe fixes # 186

@njgheorghita njgheorghita force-pushed the discv5-version branch 8 times, most recently from 5ba3d48 to a59349c Compare January 3, 2022 11:20
@njgheorghita
Copy link
Collaborator Author

njgheorghita commented Jan 3, 2022

@carver Ping for thoughts on #186 - I'm a bit baffled as to what's going on with this one. IMO it comes down to the kind of error that's being emitted (ignoring invalid dependency trin-cli which is missing a lib target).

From what I can tell, we're well set up to raise this warning as an error. We have RUSTFLAGS="-D warnings" set in in our ci environment. We even specifically run cargo clippy with --deny warnings. So it seems to me like this warning, which appears to be a rustc warning and not a regular linting warning (which are all raised as errors in our workflow via clippy) should be flagged as an error by cargo.

I've tried a couple other approaches to catch this warning

  • Adding a .cargo/config.toml to pass in RUSTFLAGS
  • Using #![deny(warnings)] inside crate roots
  • Using "-Dwarnings" syntax for RUSTFLAGS which is syntax I've seen used fairly frequently
  • Explicitly passing rustflags into cargo RUSTFLAGS="-Dwarnings" cargo build ...

IMO it comes down to us using multiple crates in a workspace. It seems to be a fairly problematic setup for various config / passing rustc settings to cargo (here and here)

Any thoughts as to whether you want me to fix the warning in this pr (by adding a lib.rs) and leave the issue open/closed?

@ogenev
Copy link
Member

ogenev commented Jan 3, 2022

Any thoughts as to whether you want me to fix the warning in this pr (by adding a lib.rs) and leave the issue open/closed?

I addressed this issue in #209 (sorry for not including it in the PR description). I think removing trin-cli from trin dependencies makes sense because the crate is not a library (no lib.rs file) and trin can't depend on a binary crate.

@njgheorghita
Copy link
Collaborator Author

@ogenev Oh nice, yeah that makes sense to me. I've removed the redundant lib.rs from this pr. Still waiting final confirmation re: open/closing #186 from @carver since the motivation behind the issue was more about generally catching warnings rather than specifically fixing that warning (re discussion here)

@@ -10,7 +10,7 @@ jobs:
name: rust/default
tag: 1.56.1
environment:
RUSTFLAGS: '-D warnings'
RUSTFLAGS: '-Dwarnings'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo, shouldn't be -D warnings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe so. In many of the various blogs/forums I read about this I saw both -D warnings and -Dwarnings used interchangeably as the RUSTFLAGS env variable. But, I've changed it back to its original -D warnings since yeah I'd agree it's more obviously not a typo

@njgheorghita njgheorghita merged commit 5ea2055 into ethereum:master Jan 4, 2022
@njgheorghita njgheorghita deleted the discv5-version branch January 4, 2022 08:47
@carver
Copy link
Collaborator

carver commented Jan 6, 2022

the motivation behind the issue was more about generally catching warnings rather than specifically fixing that warning (re discussion here)

Yeah, this is definitely the reason for #186, to prevent new warnings to show up. The existing warning was just a convenient way to test that we can configure CI to catch that special type of warning.

So in summary:

  1. Don't close Fix build warning #186, since we can't confirm that we can catch warnings in CI
  2. 👍🏻 for removing any attempts at flagging warnings, since we can't confirm that they work

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.

Update discv5 to v0.1.0-beta.12
3 participants