Consider using `cargo fmt`

[funbringer]

Hi everyone!

Please consider formatting the code with cargo fmt before the code style problem gets out of control. When a project is too big, losing the ability to git blame could become an instant nonstarter.

[kelvich]

Fair point =)

[hlinnaka]

+1

I tried, but couldn't get rustmt to run on my laptop:

~/git-sandbox/zenith (main)$ PATH=~/.cargo/bin/:$PATH cargo fmt --all 
This version of rustfmt is deprecated. Use rustfmt-nightly. Use --force to run deprecated rustfmt.
...

and with the -- --force flag, it fails with a bunch of errors like this (this is just one example):

error: expected one of `,` or `as`, found `::`
  --> /home/heikki/git-sandbox/zenith/integration_tests/tests/control_plane/mod.rs:15:7
   |
15 |     io::Write,
   |       ^^ expected one of `,` or `as` here

I think that's because the version of rustfmt that I have installed doesn't support the more recent Rust version we're using. I also tried installing the nightly version with cargo install --force rustfmt-nightly, which also failed :

error[E0554]: `#![feature]` may not be used on the stable release channel
  --> /home/heikki/.cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-1.6.1/src/lib.rs:76:37
   |
76 | #![cfg_attr(feature = "may_dangle", feature(dropck_eyepatch))]
   |                                     ^^^^^^^^^^^^^^^^^^^^^^^^
...

In summary, I can't get rustfmt to run easily. Anyone else have better luck?

[kelvich]

Worked without any issues for me. Maybe different cargo version?

> cargo --version
cargo 1.50.0 (f04e7fab7 2021-02-04)

Right now it produces following diff:

> git diff --stat=80
 integration_tests/tests/control_plane/mod.rs  |  217 ++--
 integration_tests/tests/test_control_plane.rs |    7 +-
 integration_tests/tests/test_pageserver.rs    |   31 +-
 integration_tests/tests/test_wal_acceptor.rs  |  196 +--
 pageserver/src/bin/cli/main.rs                |    9 +-
 pageserver/src/bin/cli/pg.rs                  |  162 +--
 pageserver/src/bin/cli/snapshot.rs            |   32 +-
 pageserver/src/bin/cli/storage.rs             |   23 +-
 pageserver/src/bin/pageserver.rs              |   24 +-
 pageserver/src/lib.rs                         |    6 +-
 pageserver/src/page_cache.rs                  |  751 ++++++------
 pageserver/src/page_service.rs                |  198 +--
 pageserver/src/restore_s3.rs                  |   98 +-
 pageserver/src/tui.rs                         |  196 +--
 pageserver/src/tui_logger.rs                  |   24 +-
 pageserver/src/waldecoder.rs                  |  391 +++---
 pageserver/src/walreceiver.rs                 |   63 +-
 pageserver/src/walredo.rs                     |  123 +-
 walkeeper/src/bin/wal_acceptor.rs             |   96 +-
 walkeeper/src/lib.rs                          |    2 +-
 walkeeper/src/pq_protocol.rs                  |  117 +-
 walkeeper/src/wal_service.rs                  | 1592 +++++++++++++------------
 walkeeper/src/xlog_utils.rs                   |  371 +++---
 23 files changed, 2485 insertions(+), 2244 deletions(-)

I can push that and add cargo fmt -- --check as a CI step.

[hlinnaka]

I can push that ...

Great, thanks!

... and add cargo fmt -- --check as a CI step.

I'm not so excited about that, for as long as I can't easily run rustfmt myself. Can you run it in CI, but have it not fail the build?

[funbringer]

I see. @hlinnaka do you use rustup or distro packages? I strongly suggest using rustup, as it does a good job of managing multiple toolchains at the same time.

By the way, pinning a version of the rust toolchain itself might be a good idea too.

First, new rustc/libstd/whatever might cause minor breakages.

Second, a fellow developer could accidentally use a new feature or a library method and break smb else's workflow.

[kelvich]

Great, thanks!

Pushed that part

I'm not so excited about that, for as long as I can't easily run rustfmt myself. Can you run it in CI, but have it not fail the build?

Hm, one option is to ignore check result, but this way you can only consult logs to check if fmt was successful. Other option is to have integration tests and fmt check as different pipeline jobs that can fail independently. Whole pipeline would be failed though.