balena-os / wifi-connect

Easy WiFi setup for Linux devices from your mobile phone or laptop
Apache License 2.0
1.28k stars 355 forks source link

Git hooks for checking code validity with `rustfmt` and `clippy` #184

Open dhoney opened 6 years ago

dhoney commented 6 years ago

I'm tracking the master branch remotely and building with circleci, I seem to be getting format errors like so

#!/bin/bash -eo pipefail
cargo fmt -- --write-mode=diff
Diff in /root/project/src/server.rs at line 226:
     let uuid_display = uuid_path.display();⏎
 ⏎
     let mut uuid_file = match File::create(&uuid_path) {⏎
-        Err(why) => panic!("couldn't create {}: {}",⏎
-                           uuid_display,⏎
-                           why.description()),⏎
+        Err(why) => panic!("couldn't create {}: {}", uuid_display, why.description()),⏎
         Ok(uuid_file) => uuid_file,⏎
     };⏎
 ⏎
Diff in /root/project/src/server.rs at line 233:
     match uuid_file.write_all(uuid.as_bytes()) {⏎
-        Err(why) => {⏎
-            panic!("couldn't write to {}: {}", uuid_display,⏎
-                                               why.description())⏎
-        },⏎
+        Err(why) => panic!("couldn't write to {}: {}", uuid_display, why.description()),⏎
         Ok(_) => println!("successfully wrote to {}", uuid_display),⏎
     }⏎
 ⏎
Diff in /root/project/src/server.rs at line 240:
     let vpncreds_display = vpncreds_path.display();⏎
 ⏎
     let mut vpncreds_file = match File::create(&vpncreds_path) {⏎
-        Err(why) => panic!("couldn't create {}: {}",⏎
-                           vpncreds_display,⏎
-                           why.description()),⏎
+        Err(why) => panic!(⏎
+            "couldn't create {}: {}",⏎
+            vpncreds_display,⏎
+            why.description()⏎
+        ),⏎
         Ok(vpncreds_file) => vpncreds_file,⏎
     };⏎
 ⏎
Diff in /root/project/src/server.rs at line 251:
     match vpncreds_file.write_all(vpncreds.as_bytes()) {⏎
-        Err(why) => {⏎
-            panic!("couldn't write to {}: {}", vpncreds_display,⏎
-                                               why.description())⏎
-        },⏎
+        Err(why) => panic!(⏎
+            "couldn't write to {}: {}",⏎
+            vpncreds_display,⏎
+            why.description()⏎
+        ),⏎
         Ok(_) => println!("successfully wrote to {}", vpncreds_display),⏎
     }⏎
 ⏎
Exited with code 4

I'm unsure what to make of these and so far have gotten by by using

- run: cargo fmt -- --write-mode=overwrite  
- run: cargo clippy

This feels kinda hacky and I'm not sure if there is a more correct way to address what the build error is trying to tell me.

What's most perplexing is that i'm confident these are files I have not changed and should have parity with your master branch.

Thoughts?

majorz commented 6 years ago

On the test job in the CircleCI configuration there is a check whether cargo fmt (rustfmt) was executed before commits. Here is the output for the current 4.0.6 version: https://circleci.com/gh/resin-io/resin-wifi-connect/862.

On your side you are most probably running a bit older version of rustfmt, which leads to the warnings. You are using probably our older build toolchains. One way is to upgrade to the newer toolchains. Or you may just remove the cargo fmt -- --write-mode=overwrite check from your CircleCI configuration, since this is just a formatting check.

Please let me know if my assumptions are incorrect.

dhoney commented 6 years ago

Suppressing clippy warnings seems to have worked.

Comparing our runs side by side I think they are the same, least I can't see where the version of rustfmt is different.

I'm using the same nightly container and it's near identical as far as I can tell.

majorz commented 6 years ago

@dhoney oh, now looking at the exact diff again, the difference is in the modifications on your side. I don't know where I was looking at... :) All makes sense now, since that part was not processed with the same rustfmt version.

I will reopen the ticket and change its name, since it is a problem for people doing forks when they try to use the same build environment. Probably I need to provide some git pre-commit hooks for other people wanting to maintain forks that will not allow pushing code that does not pass the clippy or rustfmt checks on the local machine.

BTW I spent some time today working on a new cross-compilation workflow based on crosstool-NG, since our current images are tricky to upgrade as some of them are based on the Emdebian toolchains which are getting outdated. The Rust compiler itself is cross-compiled for the different platforms with the help of toolchains generated with crosstool-NG. The machine code itself is generated by the Rust compiler, but the cross-compilation toolchains are needed for linking and stripping the final binaries. In the case with WiFi Connect, those are used for linking to libdbus-1 more specifically. Anyway, just mentioning it, since somewhere in the near future we need to have a more reliable way for cross-compiling Rust binaries.

majorz commented 6 years ago

Something like that as a title..

dhoney commented 6 years ago

@majorz thanks that makes sense, I did have some scoping issues on my side in server.rs because password changed to passphrase but that's resolved now.

Great info on the cross compiling, I'm happy that I can use your circle ci setup with little to no config/effort on my side :)

Can't wait to see new crosstool-NG stuff when it's ready.

Name change makes sense given the added context.