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.

Sign up for GitHub

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

Jump to bottom

Run cargo fmt in CI #1111

Merged
merged 6 commits into from
Mar 7, 2023
Merged

Run cargo fmt in CI #1111

merged 6 commits into from
Mar 7, 2023

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jul 20, 2022 •
edited
Loading

Run the formatter in CI so we stop continually introducing formatting problems in the code that is currently supposed to be formatted.

@tcharding tcharding force-pushed the 07-21-run-fmt-in-ci branch from 81313b8 to b00e275 Compare July 20, 2022 23:40
apoelstra
apoelstra previously approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK b00e275de0bbe2ebb12ab59e4fb99ac3ce5b1fe4

tcharding
tcharding commented Jul 21, 2022
View reviewed changes
apoelstra
apoelstra previously approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 7d603fac2df9d8699168db8164e8fcfebc31693a

Kixunil
Kixunil reviewed Jul 21, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Looks buggy.

Kixunil
Kixunil previously approved these changes Jul 21, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 7d603fac2df9d8699168db8164e8fcfebc31693a

dpc
dpc previously approved these changes Jul 21, 2022
View reviewed changes
dunxen
dunxen previously approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

ACK 7d603fa

@tcharding tcharding dismissed stale reviews from dunxen, dpc, Kixunil, and apoelstra via 83cd37c July 21, 2022 22:08
apoelstra
apoelstra previously approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 83cd37c61d11c6dc91f6ba3deaad71b7cbb10c39

@tcharding tcharding mentioned this pull request Jul 22, 2022
Merged
@tcharding
Copy link
Member Author

I ran shellcheck on this branch and found a single bug, fixed and pushed as a separate patch.

apoelstra
apoelstra previously approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK ac62fa7cc6fd32d4b417fda9159cad06431e735d

@tcharding tcharding added this to the 0.30.0 milestone Aug 5, 2022
@tcharding tcharding added ci one ack PRs that have one ACK, so one more can progress them no release notes mention labels Aug 5, 2022
@tcharding
Copy link
Member Author

You know it might actually be better to hold off on merging this until we have stopped ignoring submodules, that way when we are moving things around we can move them in a separate commit to formatting them and still have every commit pass CI.

@apoelstra
Copy link
Member

I think, no matter what we'll have to adjust the ignore rules while moving stuff. Enforcing it in CI, vs on people's local machines (where I believe some contributors have configured their editors to auto-mangle code according to rustfmt) doesn't seem to make a lot of difference.

@tcharding
Copy link
Member Author

to auto-mangle code according to rustfmt

I had to laugh at this one :)

@tcharding tcharding mentioned this pull request Aug 31, 2022
Closed
@tcharding
Copy link
Member Author

Changes in force push:

  • Rebase (remove changes to the ci script that have been merged in other PRs)

@tcharding tcharding mentioned this pull request Sep 8, 2022
Merged
@tcharding
Copy link
Member Author

Draft till #1533 goes in

@tcharding tcharding marked this pull request as draft March 2, 2023 03:24
@apoelstra
Copy link
Member

Needs rebase, then let's gooooo.

@apoelstra apoelstra mentioned this pull request Mar 5, 2023
Merged
tcharding added 3 commits March 6, 2023 10:21
@tcharding
Use new fn_params_layout option
21d716b 
The `fn_args_layout` rustfmt option was recently changed to
`fn_params_layout`, use the new name.
@tcharding
Run the formatter
a11cf07 
Various formatting issues have crept into the codebase because we do not
run the formatter in CI.

In preparation for enabling formatting checks in CI run `cargo +nightly
fmt` to fix current formatting issues. No changes other than those
create by the formatter.
@tcharding
Improve the DO_BENCH error message
61c560b 
Benchmarking requires a non-stable toolchain not a nightly toolchain
i.e., includes beta.

Improve the error output to indicate as such.
@tcharding tcharding force-pushed the 07-21-run-fmt-in-ci branch from cc687d4 to 65eee20 Compare March 5, 2023 23:22
@tcharding
Copy link
Member Author

Rebased and re-did a11cf075 Run the formatter

@tcharding tcharding marked this pull request as ready for review March 5, 2023 23:22
@apoelstra
Copy link
Member

This only enables the formatter in CI for bitcoin, not the other crates. Is that intentional?

apoelstra
apoelstra previously approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 65eee20c53593763b2b8953a49a7ae752c571322

tcharding added 3 commits March 7, 2023 08:57
@tcharding
Enable formatting in CI
c136006 
Enable formatting in CI by doing:

- Add a section to the `test.sh` scripts to run the formatter (guarded by
  the env variable `DO_FMT`) for all crates (bitcoin, hashes, internals).
- Add `DO_FMT` to the nightly `Tests` CI job.
@tcharding
Update contributing docs re cargo fmt
55312f2 
Now that we use `cargo fmt`, update the section in the contributing
documentation.
@tcharding
Add cargo fmt to pre-commit githook
2d6467f 
To save devs getting frustrated by CI; add a call to `cargo +nightly
fmt` to our git pre-commit hook.
@tcharding tcharding force-pushed the 07-21-run-fmt-in-ci branch from 65eee20 to 2d6467f Compare March 6, 2023 21:58
@tcharding
Copy link
Member Author

tcharding commented Mar 6, 2023 •
edited
Loading

This only enables the formatter in CI for bitcoin, not the other crates. Is that intentional?

Not intentional, my mistake. Fixed and force pushed. FTR we still ignore hashes in rustfmt.toml - that is intentionally not changed here.

Thanks

@apoelstra
Copy link
Member

Looks good -- though we're still not covering internals. I'd suggest adding the formatting code to the top-level contrib/test.sh rather than to the individual crates' contrib/test.shs.

@sanket1729
Copy link
Member

Is this the only 0.30.0 blockcing PR?

@tcharding tcharding mentioned this pull request Mar 7, 2023
Merged
8 tasks
@tcharding
Copy link
Member Author

Please see #1699 for release blocking PRs

@tcharding
Copy link
Member Author

tcharding commented Mar 7, 2023 •
edited
Loading

Looks good -- though we're still not covering internals.

Sure we are, its in c1360067 Enable formatting in CI

I'd suggest adding the formatting code to the top-level contrib/test.sh rather than to the individual crates' contrib/test.shs.

I'm not comfortable doing that for this PR because currently the rust-bitcoin/contrib/test.sh is just a thin wrapper. If we want to put othe logic into it we should put all the logic that makes sense into it.

apoelstra
apoelstra approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 2d6467f

sanket1729
sanket1729 approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 2d6467f

@apoelstra
Copy link
Member

We have an old ACK from Kix and we've merged everything we wanted to merge before this. I'm gonna do it.

@apoelstra apoelstra merged commit e074ee4 into rust-bitcoin:master Mar 7, 2023
apoelstra added a commit that referenced this pull request Mar 8, 2023
@apoelstra
Merge #1694: release bitcoin_hashes 0.12
9ffbbb8 
bfd401c bitcoin_hashes: add CHANgELOG (Andrew Poelstra)
d1b7b54 bump bitcoin-hashes version to 0.12 (Andrew Poelstra)

Pull request description:

  It was a little tricky to bump the version number because of #1553. There are a couple other things I considered trying, which maybe we'll do for future releases, but I believe this works for now.

  Maybe should wait for #1111.

ACKs for top commit:
  tcharding:
    ACK bfd401c
  sanket1729:
    utACK bfd401c. ChangeLog looks good to me, did not review whether all noteworthy changes were included.

Tree-SHA512: d2104fc93e364415ae955e8123e6087c1eaa4c955aeaf4647ead051a223563326f66c0e278d68f64335e22c9d0af9b296dc3b744cd9d82d206844461fe7bf9c9
apoelstra added a commit that referenced this pull request Mar 22, 2023
@apoelstra
Merge #1699: Release tracking PR - v0.30.0
d65646c 
ffee8ad Bump version to v0.30.0 (Tobin C. Harding)

Pull request description:

  Add changelog notes and bump the version number to v0.30.0.

  ## TODO - pre-merge

  - [x] Release `bitcoin_hashes` 0.12: #1694
  - [x] Release secp 0.27: rust-bitcoin/rust-secp256k1#588
    - rust-bitcoin/rust-secp256k1#590
  - [x] Update `secp256k1` dependency to use newly released v0.27: #1714
  - [x] Merge
    - ~#1696
    - #1695
    -  #1111
  - [x] If time permits merge these:
    - #1710
    - #1705
    - #1713
  - [x] Set the release date in changelog header
  - [x] And merge these:
    - #1721
    - #1720
    - #1719
    - #1717

  ## TODO  - post release
  - [ ] Release the blogpost: rust-bitcoin/www.rust-bitcoin.org#2
     - ~Set the date in the blog post to match the date 0.30 is released~

ACKs for top commit:
  sanket1729:
    reACK ffee8ad
  Kixunil:
    ACK ffee8ad
  apoelstra:
    ACK ffee8ad

Tree-SHA512: b0ea113ee1726fd9b263d0e01fe14bd544c007c05a9ac43b6c2d4edbeef3bb3ad456b061ef086626e1e1b27a0cda49cb6bc28aac3ad1691d72ffe00400ed5b45
@tcharding tcharding deleted the 07-21-run-fmt-in-ci branch May 22, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@dpc dpc dpc left review comments

@dunxen dunxen dunxen left review comments

Assignees
No one assigned
Labels
ci no release notes mention one ack PRs that have one ACK, so one more can progress them
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tcharding @apoelstra @sanket1729 @Kixunil @dpc @dunxen