8000 Check for changes to the public API by tcharding · Pull Request #1880 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Check for changes to the public API #1880

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
Closed
wants to merge 3 commits into from
Closed

Check for changes to the public API #1880

wants to merge 3 commits into from

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented May 26, 2023 •
edited
Loading

Add a script that checks the public API of hashes and bitcoin. Document how to use it during development. Call it in CI. Do not add it to githooks because the githooks because its expected to be run per PR not per commit.

Now includes a just command to run the script: just check-api

Implied workflow change

This PR imposes workflow changes, it may trigger your anti-authoritarian side.

Explicitly: all PRs that change the public API of bitcoin, hashes, io, or units must contain changes to the api text files.

Suggestion: We add the patch updating api text files as a separate patch at the end of each PR so we can haggle over it separately from the actual code changes.

Fix: #1875

@tcharding tcharding mentioned this pull request May 26, 2023
Merged
@apoelstra
Copy link
Member

This is amazing. It even shows shit like Send and Sync that can easily be dropped without anybody noticing. Several comments:

  1. It will also be very easy to grep for e.g. structs called Error that don't impl std::error::Error. Or empty enums that aren't sealed, or are missing #derives, or something (I don't know exactly what rules we want here, but the point is they're easy to check). For a followup PR, of course.
  2. It also highlights that with default features we export some serde macros -- I'll bet if these are used they'll error out, though I'm not sure. They should be gated behind serde. Followup.
  3. Your is_dirty check I suspect is not correct ... I think you need to use git status ... but I don't think it's too important. Using git-diff will detect if anyone has edited existing files, which they would need to do to cause any API changes. It also prevents running the script after editing the script itself, which is a little annoying. Maybe we should tighten it to only check whether .rs files have been modified, or something.
  4. cargo-public-api requires a nightly cargo. We should check for this and gracefully error out, and maybe remind the user that they can set RUSTUP_TOOLCHAIN=nightly to override it.
  5. More than needing a nightly ... it currently needs a nightly from 2 days ago! I expect this will be a common pattern. I don't think there's anything we can do about it, but we should be aware that having a fresh-baked nightly puts a fairly onerous burden on contributors who want to make API changes.
  6. Finally, cargo public-api by default uses default features. I think we should run it thrice: with no default features (or just no-std in the case of bitcoin), with default features, and with all features. It's pretty damn fast so maybe we could run it on a larger matrix...but at the very least we should check those three cases.

Only the last point needs to be addressed in this PR.

@tcharding
Copy link
Member Author

This is amazing. It even shows shit like Send and Sync that can easily be dropped without anybody noticing. Several comments:

  1. It will also be very easy to grep for e.g. structs called Error that don't impl std::error::Error. Or empty enums that aren't sealed, or are missing #derives, or something (I don't know exactly what rules we want here, but the point is they're easy to check). For a followup PR, of course.

Yeah, sick huh. I foresee a world of scripts that automagically tells us impls, derives, and just what code in general we need to write - chatGPT eat your heart out.

  1. It also highlights that with default features we export some serde macros -- I'll bet if these are used they'll error out, though I'm not sure. They should be gated behind serde. Followup.

Its a win already! I love this tool.

  1. Your is_dirty check I suspect is not correct ... I think you need to use git status ... but I don't think it's too important. Using git-diff will detect if anyone has edited existing files, which they would need to do to cause any API changes. It also prevents running the script after editing the script itself, which is a little annoying. Maybe we should tighten it to only check whether .rs files have been modified, or something.

ha! That is why I added the script as an initial patch, it kinda made testing during dev easy actually but I didn't think about future changes - I'll mess around with it, like you say, in a followup.

  1. cargo-public-api requires a nightly cargo. We should check for this and gracefully error out, and maybe remind the user that they can set RUSTUP_TOOLCHAIN=nightly to override it.

Yep nice. I'll throw that in this PR.

  1. More than needing a nightly ... it currently needs a nightly from 2 days ago! I expect this will be a common pattern. I don't think there's anything we can do about it, but we should be aware that having a fresh-baked nightly puts a fairly onerous burden on contributors who want to make API changes.

Ha! I didn't even read the date thinking this was to run in CI and nightly would be the latest one, @stevenroose is going to have a heart attack (\me chuckles to himself).

  1. Finally, cargo public-api by default uses default features. I think we should run it thrice: with no default features (or just no-std in the case of bitcoin), with default features, and with all features. It's pretty damn fast so maybe we could run it on a larger matrix...but at the very least we should check those three cases.

Only the last point needs to be addressed in this PR.

Epic, I'm on it.

@tcharding tcharding force-pushed the 05-26-check-public-api branch from ae108a9 to 67837a7 Compare May 27, 2023 05:34
@tcharding
Copy link
Member Author

Currently a bunch of warnings are generated when running the script, this is annoying. See #1884

@tcharding tcharding force-pushed the 05-26-check-public-api branch from 67837a7 to 2a95237 Compare May 27, 2023 05:41
@tcharding
Copy link
Member Author

Changes in force push:

  • Added check for nightly
  • Created various api text files for different feature sets (put them in rust-bitcoin/api)

@apoelstra
Copy link
Member

In #1884 I suggested silencing the warnings with cfg_attr to just allow them when std isn't set.

@tcharding
Copy link
Member Author

Might be polite to leave this one open for a week or two before merging so other folk can see it since it effects everyone's workflow.

@tcharding tcharding force-pushed the 05-26-check-public-api branch from 513d91f to ac81dde Compare May 28, 2023 03:14
@tcharding
Copy link
Member Author

Force pushed to remove the attribute and allow broken intra doc links on the command line by setting RUSTDOCFLAGS in the check script.

@tcharding tcharding force-pushed the 05-26-check-public-api branch from ac81dde to 848c8ef Compare May 28, 2023 03:17
@tcharding
Copy link
Member Author

Noticed the commit brief log was stale (still mentioned api.txt), reworded and force pushed.

@RCasatta
Copy link
Collaborator

Nice!

But wouldn't be better to perform the API-break-check in a standard rust test instead, as suggested here? https://github.com/Enselic/cargo-public-api#-as-a-ci-check

@apoelstra
Copy link
Member

@RCasatta honestly I think for what we're doing (comparing two text files to each other) shell is way better. It doesn't require an extra expect_test dep, it doesn't require you specify the specific unit test name to run the test in isolation, and it's easy to see at a glance what's going on.

apoelstra
apoelstra reviewed May 28, 2023
View reviewed changes
apoelstra
apoelstra reviewed May 28, 2023
View reviewed changes
junderw
junderw reviewed May 28, 2023
View reviewed changes
@tcharding tcharding force-pushed the 05-26-check-public-api branch from 848c8ef to 4c26073 Compare May 29, 2023 03:23
@tcharding
Copy link
Member Author

Changes in force push:

  • Fixed Option typo
  • shellchecked the script cleanly
  • Removed CRATES
  • Added REPO_DIR
  • Added API_DIR and used it in directory strings
  • Ran git diff against API docs directory and also checked for cached changes as well

apoelstra
apoelstra reviewed May 29, 2023
View reviewed changes
apoelstra
apoelstra reviewed May 29, 2023
View reviewed changes
@tcharding
Copy link
Member Author
tcharding commented May 30, 2023 •
edited
Loading

Dang, this script might not be able to be run in CI without giving limiting the output using --simplified. Running it today I get the following changes

-impl<V, T> ppv_lite86::types::VZip<V> for bitcoin::bip152::PrefilledTransaction where V: ppv_lite86::types::MultiLane<T>
+impl<V, T> ppv_lite86::types::types::VZip<V> for bitcoin::bip152::PrefilledTransaction where V: ppv_lite86::types::types::MultiLane<T>
 pub fn bitcoin::bip152::PrefilledTransaction::vzip(self) -> V
 pub struct bitcoin::bip152::ShortId(_)
 impl bitcoin::bip152::ShortId
@@ -1796,6 +1796,9 @@ impl core::marker::Sync for bitcoin::bip152::ShortId
 impl core::marker::Unpin for bitcoin::bip152::ShortId
 impl core::panic::unwind_safe::RefUnwindSafe for bitcoin::bip152::ShortId
 impl core::panic::unwind_safe::UnwindSafe for bitcoin::bip152::ShortId
+impl<'f, T> bech32::CheckBase32<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>> for bitcoin::bip152::ShortId where T: core::convert::AsRef<[>
+pub type bitcoin::bip152::ShortId::Err = bech32::Error
+pub fn bitcoin::bip152::ShortId::check_base32(self) -> core::result::Result<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>, <T as bech32::Ch>
 impl<T, U> core::convert::Into<U> for bitcoin::bip152::ShortId where U: core::convert::From<T>
 pub fn bitcoin::bip152::ShortId::into(self) -> U
 impl<T, U> core::convert::TryFrom<U> for bitcoin::bip152::ShortId where U: core::convert::Into<T>
@@ -1812,9 +1815,6 @@ impl<T> alloc::string::ToString for bitcoin::bip152::ShortId where T: core::fmt:
 pub fn bitcoin::bip152::ShortId::to_string(&self) -> alloc::string::String
 impl<T> bech32::Base32Len for bitcoin::bip152::ShortId where T: core::convert::AsRef<[u8]>
 pub fn bitcoin::bip152::ShortId::base32_len(&self) -> usize
-impl<T> bech32::CheckBase32<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>> for bitcoin::bip152::ShortId where T: core::convert::AsRef<[u8]>
-pub type bitcoin::bip152::ShortId::Err = bech32::Error
-pub fn bitcoin::bip152::ShortId::check_base32(self) -> core::result::Result<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>, <T as bech32::Ch>
 impl<T> bech32::ToBase32 for bitcoin::bip152::ShortId where T: core::convert::AsRef<[u8]>
 pub fn bitcoin::bip152::ShortId::write_base32<W>(&self, writer: &mut W) -> core::result::Result<(), <W as bech32::WriteBase32>::Err> where W: b>
 impl<T> core::any::Any for bitcoin::bip152::ShortId where T: 'static + core::marker::Sized

@tcharding tcharding force-pushed the 05-26-check-public-api branch from 4c26073 to 43cca34 Compare May 30, 2023 05:44
@tcharding
Copy link
Member Author
tcharding commented May 30, 2023 •
edited
Loading

Changes in force push:

  • Used cargo public-api --simplified
  • Added global CMD to save writing the command above many times
  • Used if statement as suggested
  • Used push/pop suggestion

We might have a few teething problems with this one but I rekon we will be able to work them out within a few PRs after this merges - its worth it.

@apoelstra
Copy link
Member

Looks good! We may want to revisit --simplified in the future when the tool settles down a bit.

apoelstra
apoelstra previously approved these changes May 30, 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 43cca341afc089a389dd93e713623af03375ca1e

@tcharding
Copy link
Member Author

Looks good! We may want to revisit --simplified in the future when the tool settles down a bit.

Yep, agree. Did you see that one can pass this flag multiple times also? If we get too much grief from CI we could play with hard fails vs soft warnings for different combinations of features (i.e., I'm guessing --all-features will give us most grief just because it has the biggest dependency tree.

yancyribbens
yancyribbens reviewed Jun 5, 2023
6D40 View reviewed changes
Copy link
Contributor
@yancyribbens yancyribbens left a comment

Choose a reason for hiding this comment

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

Interesting idea, but the txt files are huge. Do the .txt files that document the API actually need to be checked into git?

@yancyribbens
Copy link
Contributor

Dang, this script might not be able to be run in CI without giving limiting the output using --simplified. Running it today I get the following changes

-impl<V, T> ppv_lite86::types::VZip<V> for bitcoin::bip152::PrefilledTransaction where V: ppv_lite86::types::MultiLane<T>
+impl<V, T> ppv_lite86::types::types::VZip<V> for bitcoin::bip152::PrefilledTransaction where V: ppv_lite86::types::types::MultiLane<T>
 pub fn bitcoin::bip152::PrefilledTransaction::vzip(self) -> V
 pub struct bitcoin::bip152::ShortId(_)
 impl bitcoin::bip152::ShortId
@@ -1796,6 +1796,9 @@ impl core::marker::Sync for bitcoin::bip152::ShortId
 impl core::marker::Unpin for bitcoin::bip152::ShortId
 impl core::panic::unwind_safe::RefUnwindSafe for bitcoin::bip152::ShortId
 impl core::panic::unwind_safe::UnwindSafe for bitcoin::bip152::ShortId
+impl<'f, T> bech32::CheckBase32<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>> for bitcoin::bip152::ShortId where T: core::convert::AsRef<[>
+pub type bitcoin::bip152::ShortId::Err = bech32::Error
+pub fn bitcoin::bip152::ShortId::check_base32(self) -> core::result::Result<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>, <T as bech32::Ch>
 impl<T, U> core::convert::Into<U> for bitcoin::bip152::ShortId where U: core::convert::From<T>
 pub fn bitcoin::bip152::ShortId::into(self) -> U
 impl<T, U> core::convert::TryFrom<U> for bitcoin::bip152::ShortId where U: core::convert::Into<T>
@@ -1812,9 +1815,6 @@ impl<T> alloc::string::ToString for bitcoin::bip152::ShortId where T: core::fmt:
 pub fn bitcoin::bip152::ShortId::to_string(&self) -> alloc::string::String
 impl<T> bech32::Base32Len for bitcoin::bip152::ShortId where T: core::convert::AsRef<[u8]>
 pub fn bitcoin::bip152::ShortId::base32_len(&self) -> usize
-impl<T> bech32::CheckBase32<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>> for bitcoin::bip152::ShortId where T: core::convert::AsRef<[u8]>
-pub type bitcoin::bip152::ShortId::Err = bech32::Error
-pub fn bitcoin::bip152::ShortId::check_base32(self) -> core::result::Result<alloc::vec::Vec<bech32::u5, alloc::alloc::Global>, <T as bech32::Ch>
 impl<T> bech32::ToBase32 for bitcoin::bip152::ShortId where T: core::convert::AsRef<[u8]>
 pub fn bitcoin::bip152::ShortId::write_base32<W>(&self, writer: &mut W) -> core::result::Result<(), <W as bech32::WriteBase32>::Err> where W: b>
 impl<T> core::any::Any for bitcoin::bip152::ShortId where T: 'static + core::marker::Sized

I think we only care about what changed, not where in the txt file items are listed (what order). It looks like some of the lines show as changed when in reality they are now on a different line of the txt file?

@tcharding tcharding marked this 9E88 pull request as ready for review January 25, 2024 00:49
@coveralls
Copy link
coveralls commented Jan 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 7749810170

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.148%
Totals Coverage Status
Change from base Build 7742657266: 0.0%
Covered Lines: 19328
Relevant Lines: 22969
💛 - Coveralls

@tcharding tcharding force-pushed the 05-26-check-public-api branch 2 times, most recently from 2775fc0 to a9a3a0d Compare January 25, 2024 01:03
@apoelstra
Copy link
Member

Looks like the script needs to be rerun.

@tcharding
Copy link
Member Author
tcharding commented Jan 25, 2024 •
edited
Loading

That is what it looks like but I tried again and when I run this on my machine there are no changes. Something is different between the script output on the CI vms and the script output on my machine, specifically the sort order.

Ah you solved it here: rust-bitcoin/rust-bech32#141 (comment)

Legend

@tcharding tcharding force-pushed the 05-26-check-public-api branch from cf4ae5e to 6c84a5e Compare January 25, 2024 22:54
@tcharding
Copy link
Member Author

Ouch - damn sort order is still different!

@tcharding tcharding force-pushed the 05-26-check-public-api branch 2 times, most recently from 4e6580a to 884cfd2 Compare February 1, 2024 05:05
@tcharding
Copy link
Member Author

Updated to mirror those done in bech32 and hex-conservative. Also added coverage of units and io - BOOM!

@Kixunil
Copy link
Collaborator
Kixunil commented Feb 1, 2024

I guess this conflicts with #2353 let's get that one in first. Also I'm not very happy about having to run another foreign binary. Note that binary uses nightly which @stevenroose doesn't want.

@apoelstra
Copy link
Member

Yes, this PR will prevent @stevenroose from changing the API (without copy/pasting stuff out of the diff in the CI failure).

But even moreso than clippy, I want/expect issues from this job to be rare, so I think the nightly dependence (and the dependence on a mystery-meat cargo-install crate) is okay.

@tcharding
Add a script to check the public API
3695a11 
We would like to check for API changes during development and in CI so
that such changes can be discussed separately from the code. We have API
listings already.

Add tooling and documentation for creating API listings for the public
crates within the workspace (i.e., not `internals`).

Add API text files from an initial run of the script.
@tcharding
CI: Add job to check for API changes
413caa2 
Add a job that runs the new script to check for changes to the public
APIs of `hashes` and `bitcoin`.
@tcharding
Add a just cmd to check for API changes
944583d 
Add a `just` command to run the API checking script. Makes it more
discoverable.
@tcharding tcharding force-pushed the 05-26-check-public-api branch from 884cfd2 to 944583d Compare February 2, 2024 00:57
@tcharding
Copy link
Member Author

Rebase only, no other changes.

apoelstra
apoelstra approved these changes Feb 2, 2024
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 944583d

@Kixunil
Copy link
Collaborator
Kixunil commented Feb 2, 2024

I want/expect issues from this job to be rare

I don't. This includes minor changes and ignores the fact that we do a lot of breaking changes lately anyway. I definitely do.

@tcharding
Copy link
Member Author

This one effects devs heavily, if Kix is not into it lets leave for a later release.

@tcharding tcharding marked this pull request as draft February 5, 2024 01:02
@tcharding tcharding closed this Feb 27, 2024
@apoelstra apoelstra mentioned this pull request Mar 20, 2024
Merged
@tcharding tcharding mentioned this pull request Apr 23, 2024
Closed
@apoelstra
Copy link
Member

I think we should just do this (though we should open a new PR) and tag Kix, saying that we'll revert it (or at least, remove it from CI and/or move it to a weekly "update the API files" job) when he returns.

@tcharding
Copy link
Member Author

Lets do it. I've tickled the script a little in the first patch and updated all the api files.

  • Added base58
  • Used set -euo pipefail
  • Used the pinned nightly

@tcharding
Copy link
Member Author

The UI doesn't seem to want me to re-open, perhaps i should have re-opended before force pushing. Anyway, I'll open a new PR.

@tcharding tcharding mentioned this pull request Apr 24, 2024
Merged
apoelstra added a commit that referenced this pull request May 18, 2024
@apoelstra
Merge #2713: Check for changes to the public API
13569b8 
76331ae Add a just cmd to check for API changes (Tobin C. Harding)
b222f40 CI: Add job to check for API changes (Tobin C. Harding)
9e7cd97 Add a script to check the public API (Tobin C. Harding)

Pull request description:

  This PR is #1880 re-opened.

  Add a script that checks the public API of `hashes` and `bitcoin`. Document how to use it during development. Call it in CI. Do not add it to githooks because the githooks because its expected to be run per PR not per commit.

  Includes a `just` command to run the script: `just check-api`

  ### Implied workflow change

  This PR imposes workflow changes.

  Explicitly: all PRs that change the public API of `bitcoin`, `base58`, `hashes`, `io`, or `units` must contain changes to the api text files.

  Suggestion: We add the patch updating api text files as a separate patch at the end of each PR so we can haggle over it separately from the actual code changes.

  Fix: #1875

ACKs for top commit:
  apoelstra:
    ACK 76331ae normally would complain about the whitespace but I would like to ACK/merge this quickly since most other PR that gets merged will force it to be rebased. also will one-ACK merge as "no NACKs or comments from maintainers in 2 weeks"

Tree-SHA512: 67b20e2ce0c22aa67be931c4da0b591bc351ccb1aa620003c60bfb4b10d5c292edceca929bf6318993f2d16f9f58374aac336adf0f8234c5e2f16e3857b7901b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yancyribbens yancyribbens yancyribbens left review comments

@junderw junderw junderw left review comments

@apoelstra apoelstra apoelstra approved these changes

@RCasatta RCasatta RCasatta left review comments

@sanket1729 sanket1729 sanket1729 left review comments

Assignees
No one assigned
Labels
ci doc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check for API breaking changes in CI
8 participants
@tcharding @apoelstra @RCasatta @yancyribbens @sanket1729 @Kixunil @coveralls @junderw
0