#26972 closed enhancement (fixed)

Create make target to ensure that all Rust files have been formatted with rustfmt

Reported by: chelseakomlo Owned by: teor
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust
Cc: teor Actual Points:
Parent ID: #24629 Points:
Reviewer: teor Sponsor:

Description

We should have a CI task that ensures Rust files have been properly formatted- this will be helpful when reviewing PRs.

Other linting tooling can be added here in the future (for example, any clippy warnings we want to explicitly check) but starting with rustfmt seems like a good first step.

It looks like running rustfmt with --check will be helpful here: https://github.com/rust-lang-nursery/rustfmt#running

Child Tickets

TicketTypeStatusOwnerSummary
#27071defectclosedStop using max_width=80 for rustfmt

Attachments (1)

0001-rust-add-cargo-fmt-step-to-travis-CI.patch (1.4 KB) - added by cypherpunks3 13 months ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 13 months ago by teor

Owner: set to teor
Status: newassigned

Changed 13 months ago by cypherpunks3

comment:2 Changed 13 months ago by cypherpunks3

Can someone check out the branch 'rustfmt-travis' at https://gitgud.io/onionk/tor.git ?

But there's a bug upstream in rustfmt, which happens when it's configured with max_width=80 (like it is in src/rust/.rustfmt.toml).

Reduced testcase:

fn main() {
    loop {
        loop {
            loop {
                let supported_vers: &mut HashMap<Version, usize> =
                    all_count.entry(protocol.clone()).or_insert(HashMap::new());
            }
        }
    }
}

It wrongly transforms that into:

                let supported_vers: &mut HashMap<
                    Version,
                    usize,
                > = all_count.entry(protocol.clone()).or_insert(HashMap::new());
Last edited 13 months ago by cypherpunks3 (previous) (diff)

comment:3 in reply to:  2 ; Changed 13 months ago by teor

Reviewer: teor
Status: assignedneeds_revision
Type: taskenhancement

Replying to cypherpunks3:

Can someone check out the branch 'rustfmt-travis' at https://gitgud.io/onionk/tor.git ?

Thanks for this patch!

The changes look like they should work, but we could tweak a few things to follow Tor's standards.

Here are the changes I'd like to see:

  • open a separate ticket for deny(missing_docs), so we don't forget to fix the docs
  • make cargo fmt --all -- --check into make check-rustfmt. When tor is configured with Rust, and rustfmt-preview is installed, run make check-rustfmt during make check-spaces.
  • add a make rustfmt target, and tell developers about it if make check-rustfmt fails.

(If we make these changes, we won't have to add a separate check to travis.)

But there's a bug upstream in rustfmt, which happens when it's configured with max_width=80 (like it is in src/rust/.rustfmt.toml).

I'm not sure about the impact of the bug.
Does it produce ugly formatting, or is the syntax incorrect?

Do you think we should give up on max_width=80?
We can reconsider wrapping to 80 characters when stable supports wrapping comments.

If we want to match Tor's C files, we should be wrapping to 79 characters:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n177

comment:4 in reply to:  3 Changed 13 months ago by cypherpunks3

Replying to teor:

Rebased the branch and filed #27052.

comment:5 in reply to:  3 ; Changed 13 months ago by teor

Hi,

The travis build failed, due to old autoconf macros:
https://travis-ci.org/teor2345/tor/builds/412925525

Please fix the build, and check that it works on Travis.

comment:6 Changed 13 months ago by teor

If you have GitHub, you can open a pull request at https://github.com/torproject/tor and Travis will run automatically.

Or you can set up travis on your own GitHub using these instructions:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/HelpfulTools.md#n7

If you're using Travis integration with another git repository, please let us know where it is.

comment:7 in reply to:  5 ; Changed 13 months ago by cypherpunks3

Replying to teor:

The travis build failed, due to old autoconf macros:
https://travis-ci.org/teor2345/tor/builds/412925525
Please fix

Huh? According to the log, the source of the warning is a use of AC_WARN in configure.ac, and none of these patches added that, or even touched that file.

It looks like a pre-existing condition. It seems to have already gotten fixed in f90c0533777f2220cd6fb5ed07a5b63cd9c3e881. (Although not sure what #23878 has to do with it.)

Last edited 13 months ago by cypherpunks3 (previous) (diff)

comment:8 in reply to:  7 Changed 13 months ago by teor

Replying to cypherpunks3:

Replying to teor:

The travis build failed, due to old autoconf macros:
https://travis-ci.org/teor2345/tor/builds/412925525
Please fix

Huh? According to the log, the source of the warning is a use of AC_WARN in configure.ac, and none of these patches added that, or even touched that file.

Sorry, I didn't realise that CI was failing due to a bug in master.

It looks like a pre-existing condition. It seems to have already gotten fixed in f90c0533777f2220cd6fb5ed07a5b63cd9c3e881. (Although not sure what #23878 has to do with it.)

I've pushed your rebased branch to my github, and I'll see if CI works this time.

comment:9 in reply to:  3 Changed 13 months ago by teor

Replying to teor:

Replying to cypherpunks3:

But there's a bug upstream in rustfmt, which happens when it's configured with max_width=80 (like it is in src/rust/.rustfmt.toml).

I'm not sure about the impact of the bug.
Does it produce ugly formatting, or is the syntax incorrect?

Do you think we should give up on max_width=80?
We can reconsider wrapping to 80 characters when stable supports wrapping comments.

If we want to match Tor's C files, we should be wrapping to 79 characters:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n177

I opened #27071 to deal with the max_width issue.

comment:10 Changed 13 months ago by teor

Hi, the CI passed after merging the fix from master:
https://travis-ci.org/teor2345/tor/builds/413363895

I opened a GitHub pull request with my review on this branch:
https://github.com/torproject/tor/pull/265#pullrequestreview-144228176

Please see my comments on that pull request.

comment:11 Changed 13 months ago by cypherpunks3

Rebased and reran with max_width=100. Also, echo prints a newline at the end, while printf doesn't.

comment:13 Changed 13 months ago by cypherpunks3

Sorry, fixed.

comment:14 Changed 13 months ago by teor

Milestone: Tor: 0.3.5.x-final
Status: needs_revisionmerge_ready

Thanks, I reviewed the changes and made sure that "make rustfmt" produces commit 7fb342a716.

Please merge the branch 'rustfmt-travis' from ​https://gitgud.io/onionk/tor.git , or the rustfmt-travis branch in my repository.

comment:15 Changed 13 months ago by teor

Status: merge_readyneeds_revision

I'd like to merge this branch after #24629 merges.

I'm happy to do the merge, it only affects 5d749e4bde:

  • install rustfmt in .travis.yml after the rewrite

We should also:

comment:16 Changed 13 months ago by teor

Parent ID: #24629

I'm making #24629 the parent task, so I remember to do the merge.

comment:17 Changed 13 months ago by teor

Keywords: 036-proposed added

Rust stable and beta disagree about some formatting. For example, see privcount_shamir's #26973: https://travis-ci.org/teor2345/privcount_shamir/jobs/414426254#L1612

We could merge this patch with the travis check disabled, because the rust code passes on rustfmt 0.99.1.

Either way, we should defer the remaining tasks until Tor 0.3.6:

  • wait until rustfmt 0.99.1 hits stable
  • do a final format with rustfmt
  • activate the travis rustfmt check

comment:18 Changed 13 months ago by cypherpunks3

Since we won't be including the Travis check commit after all (for now), and that was the sole conflict with the branch in #24629, can this be merged immediately instead? All other Rust work has to be rebased on top of this one.

comment:19 in reply to:  14 Changed 13 months ago by cypherpunks3

Replying to teor:

I created a new branch 'rustfmt-notravis' at https://gitgud.io/onionk/tor.git with the only new changes being the removal of the Travis commit, and fixing a typo in CodingStandardsRust.md.

comment:20 Changed 13 months ago by teor

Please open a new ticket in 0.3.6 with a new branch containing the travis commit, so we remember to merge it when rustfmt 0.99.1 becomes stable.

comment:21 Changed 13 months ago by teor

Status: needs_revisionneeds_review

comment:22 in reply to:  20 ; Changed 12 months ago by teor

Keywords: 036-proposed removed
Status: needs_reviewmerge_ready
Summary: Create CI task to ensure that all Rust files have been formatted with rustfmtCreate make target to ensure that all Rust files have been formatted with rustfmt

Replying to cypherpunks3:

Replying to teor:

I created a new branch 'rustfmt-notravis' at https://gitgud.io/onionk/tor.git with the only new changes being the removal of the Travis commit, and fixing a typo in CodingStandardsRust.md.

Thanks for making these changes.

There's a typo in CodingStandardsRust.md in your 132c7950dc. I did a fixup in da6054fe97 in my rustfmt-notravis branch.

Next time, please add reverts and fixups to the existing commits. If you rewrite the whole branch, reviewers have to review it all again. (Or diff the diffs of each commit.)

Replying to teor:

Please open a new ticket in 0.3.6 with a new branch containing the travis commit, so we remember to merge it when rustfmt 0.99.1 becomes stable.

I opened #27161 for follow-up.

Please merge rustfmt-notravis in my https://github.com/teor2345/tor.git
The pull request is https://github.com/torproject/tor/pull/275

comment:23 Changed 12 months ago by nickm

Teor, before I merge this, can you confirm that you re-ran rustfmt on the rust code as it stood before this branch, and got the same (or equivalent) results as cypherpunks3 did? (I'd rather not hand-review the entire rustfmt diff.)

comment:24 in reply to:  22 ; Changed 12 months ago by cypherpunks3

Replying to teor:

There's a typo in CodingStandardsRust.md in your 132c7950dc. I did a fixup in da6054fe97

Sorry about the rebase, thanks for fixing that. Of course trying to fix a typo had a typo...

Replying to nickm:

Can't you just run make check-rustfmt to confirm that it recommends no further formatting changes?

Edit: Oh wait, this is already merged.

Last edited 12 months ago by cypherpunks3 (previous) (diff)

comment:25 in reply to:  24 Changed 12 months ago by nickm

Replying to cypherpunks3:

Replying to teor:

There's a typo in CodingStandardsRust.md in your 132c7950dc. I did a fixup in da6054fe97

Sorry about the rebase, thanks for fixing that. Of course trying to fix a typo had a typo...

Replying to nickm:

Can't you just run make check-rustfmt to confirm that it recommends no further formatting changes?

That's not the issue: the issue is to make sure that the commit contains _only_ formatting changes, to make sure that you didn't accidentally (or on purpose) mess with the code and commit any extra hidden surprises along with the formatting changesx.

(That's not something I would expect you to do, but it's a good idea to be careful!)

Edit: Oh wait, this is already merged.

Yeah -- after I wrote that, I went and checked it myself, but I'm hoping teor can doublecheck before we close.

comment:26 Changed 12 months ago by teor

Resolution: fixed
Status: merge_readyclosed

I have checked that "cargo fmt" produces onionk/rustfmt-notravis (c9d792d50f) from onionk/rustfmt-notravis^.

Note: See TracTickets for help on using tickets.