Skip to content

Conversation

@nyonson
Copy link
Collaborator

@nyonson nyonson commented Dec 15, 2025

This is a little more complex than the other rbmt commands. Up to now, rbmt has just orchestrated cargo commands, no other tool dependencies. But this patch adds an api command which takes over the exposed API checks and uses an internal dependency.

The legacy scripts in rust-bitcoin use the cargo-public-api tool has a nice library crate, public-api. This patch uses the library crate under the hood for rbmt's api command.

The api command has two modes which I think models mostly the usage in rust-bitcoin. The standard cargo rbmt api generates exposed API files for all packages. It can use the global -p flag to filter which packages. It checks to make sure the feature flags are additive and if anything has changed. The second mode is cargo rbmt api --baseline $REF which takes a git ref and does a comparison. It fails if there are any breaking changes.

@nyonson nyonson force-pushed the api-command branch 5 times, most recently from a7565ff to 9eb4d82 Compare December 16, 2025 16:42
@nyonson nyonson changed the title [WIP] Add API subcommand Add API subcommand Dec 16, 2025
@nyonson nyonson force-pushed the api-command branch 3 times, most recently from ea7bc7e to dc254d6 Compare December 16, 2025 20:50
@nyonson nyonson marked this pull request as ready for review December 16, 2025 21:06
@apoelstra
Copy link
Member

utACK 88b818c. Needs rebase.

Overall this looks amazing. I'm super excited to try this. Unfortunately I suspect both the additivity checking and the semver checking are going to have too many false positives.

For example, imagine we have a sealed trait and we have impl Trait for SpecificType. Then when enabling the trait_for_everything feature, we drop this and instead have impl<T> Trait for T. This is not breaking (though it would be if the trait is unsealed, heh).

Maybe it's okay. Worth a shot. But I suspect we'll need to disable/weaken this, and fall back to using cargo-semver-checks for these sorts of checks.

Changes get_packages (previously get_crate_dirs) to return both package
names and directory paths as tuples.
@nyonson
Copy link
Collaborator Author

nyonson commented Dec 17, 2025

For example, imagine we have a sealed trait and we have impl Trait for SpecificType. Then when enabling the trait_for_everything feature, we drop this and instead have impl<T> Trait for T. This is not breaking (though it would be if the trait is unsealed, heh).

I'll admit this is making my head hurt a bit, so going to think on it, but the underlying public_api crate does have some options which sound promising for this issue: https://docs.rs/public-api/latest/public_api/struct.Builder.html#method.omit_blanket_impls

EDIT: looking at this omit_blanket_impls option, I don't think it "fixes" this issue since there will still be a dropped trait when the feature is enabled. These false positives might be hard to avoid given that the test is only looking at like the syntax, not the semantics, of a type? It looks like semver-checks publishes a library crate we could wrap in a similar fashion, but it has some heavy dependencies.

@nyonson
Copy link
Collaborator Author

nyonson commented Dec 17, 2025

d0abec0: rebased

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK d0abec0

@tcharding
Copy link
Member

This is sick. The --baseline flag is only meant to be used interactively, right? Before we do a release? If so it doesn't matter too much if there are false positives, right? Its still super useful.

@tcharding
Copy link
Member

I'm going to merge, we are running review, ack, merge, yolo policy ATM here.

@tcharding tcharding merged commit 32c0c8d into rust-bitcoin:master Dec 17, 2025
2 checks passed
@nyonson
Copy link
Collaborator Author

nyonson commented Dec 17, 2025

This is sick. The --baseline flag is only meant to be used interactively, right? Before we do a release? If so it doesn't matter too much if there are false positives, right? Its still super useful.

Ideally this command takes over these scripts in rust-bitcoin, so hopefully can be run on PRs? But probably needs a little work still.

@tcharding
Copy link
Member

I realize the command will be used, its just the --baseline flag I was wondering about?

@apoelstra
Copy link
Member

Yeah, my read of the code is also that these questionable checks only run on --baseline, and since we only intend to run --baseline interactively this is probably okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants