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

chore(ci): check rustfmt with nightly toolchain #5743

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented Dec 14, 2024

Description

Some of our rustfmt rules aren't stable yet. Thus our rustfmt CI check should run on nightly.

Notes & open questions

Depends on #5742.

I am not super familiar with CI workflows. Can/ should this be solved differently?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@elenaf9 elenaf9 requested a review from jxs December 14, 2024 12:03
@elenaf9 elenaf9 mentioned this pull request Dec 14, 2024
4 tasks
@kamuik16
Copy link
Contributor

kamuik16 commented Dec 14, 2024

run: cargo fmt -- --check -> run: cargo +nightly fmt -- --check I think.

@elenaf9
Copy link
Contributor Author

elenaf9 commented Dec 14, 2024

run: cargo fmt -- --check -> run: cargo +nightly fmt -- --check I think.

Isn't this implicitly done because we directly install the nightly toolchain? The rustfmt job on this PR fails as expected on master with the current change: https://github.com/libp2p/rust-libp2p/actions/runs/12329671999/job/34414253861?pr=5743.

@kamuik16
Copy link
Contributor

run: cargo fmt -- --check -> run: cargo +nightly fmt -- --check I think.

Isn't this implicitly done because we directly install the nightly toolchain? The rustfmt job on this PR fails as expected on master with the current change: https://github.com/libp2p/rust-libp2p/actions/runs/12329671999/job/34414253861?pr=5743.

not that I know of, but I could be wrong.

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks for going ahead with this Elena, I noticed when I ran locally tat there were files needing fmt.
We just need to run cargo fmt now so for the CI to pass

@elenaf9
Copy link
Contributor Author

elenaf9 commented Dec 16, 2024

Thanks for going ahead with this Elena, I noticed when I ran locally tat there were files needing fmt.
We just need to run cargo fmt now so for the CI to pass

Should be fixed now with #5742 merged?

@jxs jxs added the send-it label Dec 16, 2024
@mergify mergify bot merged commit 54d7f21 into libp2p:master Dec 16, 2024
71 checks passed
@elenaf9 elenaf9 deleted the ci/rustfmt-nightly branch December 17, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants