warnings as errors #583

eauge · 2019-02-14T00:59:15Z

Address issue https://github.com/oasislabs/runtime-ethereum/issues/525

The best mechanism I've seen for now is to add #![deny(warnings)] in the top lib.rs file. I've seen that they are working on supporting a lint directive in the cargo.toml file but there's nothing yet rust-lang/cargo#5034

armaniferrante · 2019-02-14T01:54:14Z

common/src/confidential/key_manager.rs

@@ -41,56 +39,6 @@ impl KeyManagerClient {
    }
 }

-/// Wrapper around the Ekiden key manager client to provide a more convenient
-/// Ethereum address based interface along with runtime-specific utility methods.
-struct KeyManager;


What made you delete this? We still need it. :)

it was just not being used

This is definitely used. See KeyManagerClient. We do some conditional compilation for testing.

The e2e tests will fail if this is deleted.

armaniferrante · 2019-02-14T01:55:26Z

.gitignore

@@ -1,7 +1,7 @@
 # Mac things
 .DS_Store

-# Build.
+n# Build.


I think you added this by accident.

yes, let me fix it

armaniferrante · 2019-02-14T01:55:32Z

.gitignore

+
+# in case credentials are put in the repo for using
+# inside the docker container
+.git-credentials


armaniferrante · 2019-02-14T01:58:45Z

We should reconsider whether we want to use #![deny(warnings)] https://github.com/rust-unofficial/patterns/blob/master/anti_patterns/deny-warnings.md

At first glance, I think I'm ok with it.

eauge · 2019-02-14T05:47:45Z

@armaniferrante that looks like a good alternative. Is there a specific set of warnings that you'd rather deny, like the one suggested in that link that the author says it's safe to deny? Or still deny all warnings?

armaniferrante · 2019-02-15T06:04:10Z

api/build.rs

@@ -1,3 +1,5 @@
+//! build crate for the ekiden api


All comments should start with capitalization and end with periods.

armaniferrante · 2019-02-15T06:09:42Z

gateway/src/lib.rs

 extern crate rayon;
-extern crate regex;


Look at all of these removed crates. Glorious. Are they removed from Cargo.toml as well?

I haven't, but we could do that. I would prefer to do it in a separate PR if that's okay though :)

Why another PR? Might as well do it here.

let me try then

armaniferrante · 2019-02-15T06:14:03Z

.buildkite/rust/lint.txt

@@ -0,0 +1,32 @@
+#


Out of curiosity is this an exhaustive list? I assume not.

I got the list from here https://github.com/rust-unofficial/patterns/blob/master/anti_patterns/deny-warnings.md. It's not exhaustive since rust still adds/deprecates lints with quite some frequency. Based on that post, it looks like this is a reasonable set. Let me know if there's any other lint that you think should be added

I think this is great to start. We can add/subtract as necessary in the future.

armaniferrante · 2019-02-15T17:49:30Z

gateway/src/client.rs

@@ -86,9 +85,11 @@ pub trait ChainNotify: Send + Sync {
 pub struct Client {
    client: runtime_ethereum::Client,
    engine: Arc<EthEngine>,
+    #[allow(dead_code)]


If this is dead can we delete it?

armaniferrante

This all looks good to me. The only gripe I have is the usage of #[allow(dead_code)]. If possible, let's either delete such code or switch it to a conditional compilation. Let me know if neither are possible.

eauge · 2019-02-15T19:10:51Z

@armaniferrante this is the error that we get if we don't allow dead_code for the key manager when running the cargo tests.

error: struct is never constructed: `KeyManager`
--
  | --> common/src/confidential/key_manager.rs:47:1
  | \|
  | 47 \| struct KeyManager;
  | \| ^^^^^^^^^^^^^^^^^^
  | \|
  | = note: requested on the command line with `-D dead-code`

Let me know if there's a better way to fix this, otherwise we will have to live with allow dead code for now.

armaniferrante · 2019-02-15T19:26:50Z

Discussed offline. The conditional compilation for the KeyManager fixed the warnings. We don't need #[allow(dead_code)] there.

…auge/lint/warnings-as-errors

armaniferrante · 2019-02-15T23:49:00Z

build.rs

@@ -1,3 +1,5 @@
+//! build crate for the ekiden runtime based enclave


Suggested change

//! build crate for the ekiden runtime based enclave

//! Build crate for the ekiden runtime based enclave.

armaniferrante

LGTM.

armaniferrante · 2019-02-15T23:53:14Z

Filed https://github.com/oasislabs/runtime-ethereum/issues/590 as a followup to turn on "unused results".

eauge added 3 commits February 13, 2019 16:58

warnings as errors

90a034f

address fmt errors

79dea7d

fix fmt issues

7dad571

eauge force-pushed the eauge/lint/warnings-as-errors branch from 8065664 to 7dad571 Compare February 14, 2019 01:15

fix for tests

526fa24

armaniferrante reviewed Feb 14, 2019

View reviewed changes

eauge added 4 commits February 14, 2019 10:22

remove deny warning

34fa6c3

readd key manager

fb8e23f

fix unbound variable use

18a22a9

fix lint issues

edba5ee

eauge force-pushed the eauge/lint/warnings-as-errors branch 16 times, most recently from 068fb77 to 51becc5 Compare February 14, 2019 23:25

armaniferrante reviewed Feb 15, 2019

View reviewed changes

address PR comments

d453276

armaniferrante reviewed Feb 15, 2019

View reviewed changes

remove dependencies

b3d38a3

eauge force-pushed the eauge/lint/warnings-as-errors branch 2 times, most recently from fc8e426 to 50ff694 Compare February 15, 2019 18:59

eauge force-pushed the eauge/lint/warnings-as-errors branch from 50ff694 to 37e861f Compare February 15, 2019 19:25

eauge force-pushed the eauge/lint/warnings-as-errors branch from 37e861f to 51f0211 Compare February 15, 2019 19:37

key manager not undead code

2071cdc

eauge force-pushed the eauge/lint/warnings-as-errors branch 8 times, most recently from f50b47e to ca163f3 Compare February 15, 2019 23:13

eauge added 2 commits February 15, 2019 15:18

remove allow dead code

e876929

Merge branch 'master' of github.com:oasislabs/runtime-ethereum into e…

69aad5d

…auge/lint/warnings-as-errors

eauge force-pushed the eauge/lint/warnings-as-errors branch from ca163f3 to 69aad5d Compare February 15, 2019 23:18

armaniferrante reviewed Feb 15, 2019

View reviewed changes

armaniferrante approved these changes Feb 15, 2019

View reviewed changes

eauge merged commit 8062738 into master Feb 15, 2019

kostko deleted the eauge/lint/warnings-as-errors branch May 21, 2019 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warnings as errors #583

warnings as errors #583

eauge commented Feb 14, 2019 •

edited

Loading

armaniferrante Feb 14, 2019

eauge Feb 14, 2019

armaniferrante Feb 14, 2019

armaniferrante Feb 14, 2019

armaniferrante Feb 14, 2019

eauge Feb 14, 2019

armaniferrante Feb 14, 2019

armaniferrante commented Feb 14, 2019 •

edited

Loading

eauge commented Feb 14, 2019

armaniferrante Feb 15, 2019

armaniferrante Feb 15, 2019

eauge Feb 15, 2019

armaniferrante Feb 15, 2019

eauge Feb 15, 2019

armaniferrante Feb 15, 2019

eauge Feb 15, 2019

armaniferrante Feb 15, 2019

armaniferrante Feb 15, 2019

armaniferrante left a comment •

edited

Loading

eauge commented Feb 15, 2019

armaniferrante commented Feb 15, 2019

armaniferrante Feb 15, 2019

armaniferrante left a comment

armaniferrante commented Feb 15, 2019

		@@ -1,3 +1,5 @@
		//! build crate for the ekiden runtime based enclave

warnings as errors #583

warnings as errors #583

Conversation

eauge commented Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armaniferrante commented Feb 14, 2019 • edited Loading

eauge commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armaniferrante left a comment • edited Loading

Choose a reason for hiding this comment

eauge commented Feb 15, 2019

armaniferrante commented Feb 15, 2019

Choose a reason for hiding this comment

armaniferrante left a comment

Choose a reason for hiding this comment

armaniferrante commented Feb 15, 2019

eauge commented Feb 14, 2019 •

edited

Loading

armaniferrante commented Feb 14, 2019 •

edited

Loading

armaniferrante left a comment •

edited

Loading