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.

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.

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.

Its a win already! I love this tool.

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.

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.

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.

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

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.

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).

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.

Epic, I'm on it.

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

Changes in force push:

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

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

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.

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

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

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

@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.

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

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

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.

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

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.

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?

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
Change from base Build 7742657266:	0.0%
Covered Lines:	19328
Relevant Lines:	22969

💛 - Coveralls

Looks like the script needs to be rerun.

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

Ouch - damn sort order is still different!

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

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.

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.

Rebase only, no other changes.

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.

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

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.

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

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.