Skip to content

Commit 7bd91f4

Browse files
committed
cargo-rbmt: add API break whitelist feature
Add allow_breaking_changes configuration option to filter out intentional breaking changes from API check failures.
1 parent ccd187c commit 7bd91f4

File tree

2 files changed

+116
-22
lines changed

2 files changed

+116
-22
lines changed

cargo-rbmt/README.md

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@ Maintainer tools for Rust-based projects in the Bitcoin domain. Built with [xshe
99
- [Lint](#lint)
1010
- [Test](#test)
1111
- [Integration](#integration)
12+
- [Fuzz](#fuzz)
1213
- [Prerelease](#prerelease)
1314
- [Lock Files](#lock-files)
15+
- [API Checking](#api-checking)
1416
- [Workspace Integration](#workspace-integration)
1517
- [1. Install on system](#1-install-on-system)
1618
- [2. Add as a dev-dependency](#2-add-as-a-dev-dependency)
@@ -91,6 +93,10 @@ package = "bitcoind-tests"
9193
versions = ["29_0", "28_2", "27_2"]
9294
```
9395

96+
## Fuzz
97+
98+
The `fuzz` command assumes there is a package in a workspace which defines fuzz targets.
99+
94100
## Prerelease
95101

96102
The `prerelease` command performs readiness checks before releasing a package. By default, all packages are checked unless they explicitly opt-out.
@@ -135,7 +141,7 @@ When you specify `--lock-file`, the tool copies that lock file to `Cargo.lock` b
135141

136142
## API Checking
137143

138-
The `api` command helps maintain API stability by generating public API snapshots and checking for breaking changes. It uses the [public-api](https://github.com/Enselic/cargo-public-api) crate to analyze a crate's public interface.
144+
The `api` command helps maintain API stability by generating public API snapshots and checking for breaking changes. The generated API files are stored in `api/<package-name>/`.
139145

140146
```bash
141147
cargo rbmt api
@@ -145,14 +151,30 @@ cargo rbmt api
145151
2. Validates that features are additive (enabling features only adds to the API, never removes).
146152
3. Checks for uncommitted changes to API files.
147153

148-
The generated API files are stored in `api/<package-name>/`.
149-
150154
```bash
151155
cargo rbmt api --baseline v0.1.0
152156
```
153157

154158
Compares the current API against a baseline git reference (tag, branch, or commit) to detect breaking changes.
155159

160+
### Whitelisting Breaking Changes
161+
162+
The `api` command checks for two types of breaking changes.
163+
164+
* **Additivity** compares a package's `no-features` API vs. its `all-features` API to ensure its features are additive.
165+
* **Semver** compares some baseline version of a package's API vs the current API to ensure no breaking changes.
166+
167+
Sometimes breaking changes are intentional and safe (e.g., replacing a specific impl with a more general blanket impl for sealed traits). Configure the `[api]` section a your package's `rbmt.toml` to whitelist specific changes:
168+
169+
```toml
170+
[api]
171+
# Items that can be removed or changed without triggering errors.
172+
allow_breaking_changes = [
173+
"impl MyTrait for SpecificType",
174+
"pub fn deprecated_function()",
175+
]
176+
```
177+
156178
## Workspace Integration
157179

158180
`cargo-rbmt` can simply be installed globally on a system or added as a dev-dependency to a package.

cargo-rbmt/src/api.rs

Lines changed: 91 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::collections::HashMap;
22
use std::fs;
33
use std::path::{Path, PathBuf};
44

5+
use serde::Deserialize;
56
use xshell::Shell;
67

78
use crate::{environment, quiet_cmd, toolchain};
@@ -59,6 +60,41 @@ impl FeatureConfig {
5960
}
6061
}
6162

63+
/// API configuration loaded from rbmt.toml.
64+
#[derive(Debug, Deserialize, Default)]
65+
#[serde(default)]
66+
struct Config {
67+
api: ApiConfig,
68+
}
69+
70+
/// API-specific configuration.
71+
#[derive(Debug, Deserialize, Default)]
72+
#[serde(default)]
73+
struct ApiConfig {
74+
/// List of API items that are allowed to be removed or changed without causing
75+
/// semver or additivity check failures.
76+
///
77+
/// This is useful for intentional breaking changes like replacing a specific impl
78+
/// with a more general blanket impl for sealed traits.
79+
allow_breaking_changes: Vec<String>,
80+
}
81+
82+
impl ApiConfig {
83+
/// Load API configuration from the package directory.
84+
fn load(_sh: &Shell, package_dir: &Path) -> Result<Self, Box<dyn std::error::Error>> {
85+
let config_path = package_dir.join(environment::CONFIG_FILE_PATH);
86+
87+
if !config_path.exists() {
88+
// Return empty config if file doesn't exist.
89+
return Ok(Self { allow_breaking_changes: Vec::new() });
90+
}
91+
92+
let contents = fs::read_to_string(&config_path)?;
93+
let config: Config = toml::from_str(&contents)?;
94+
Ok(config.api)
95+
}
96+
}
97+
6298
/// Run the API check task.
6399
///
64100
/// This command checks for changes to the public API of workspace packages by generating
@@ -131,6 +167,44 @@ fn get_package_apis(
131167
Ok(apis)
132168
}
133169

170+
/// Check for breaking changes in a diff, respecting the whitelist.
171+
///
172+
/// Returns a list of breaking change descriptions (empty if no breaks).
173+
fn check_for_breaking_changes(
174+
diff: &public_api::diff::PublicApiDiff,
175+
allow_breaking: &[String],
176+
) -> Vec<String> {
177+
let mut breaking = Vec::new();
178+
179+
for item in &diff.removed {
180+
let item_str = item.to_string();
181+
if allow_breaking.contains(&item_str) {
182+
environment::quiet_println(&format!(
183+
"Warning: Allowing removal of '{}' (whitelisted)",
184+
item_str
185+
));
186+
} else {
187+
breaking.push(format!("Removed: {}", item_str));
188+
}
189+
}
190+
191+
for change in &diff.changed {
192+
let old_str = change.old.to_string();
193+
let new_str = change.new.to_string();
194+
195+
if allow_breaking.contains(&old_str) || allow_breaking.contains(&new_str) {
196+
environment::quiet_println(&format!(
197+
"Warning: Allowing change to '{}' (whitelisted)",
198+
old_str
199+
));
200+
} else {
201+
breaking.push(format!("Changed:\n old: {}\n new: {}", old_str, new_str));
202+
}
203+
}
204+
205+
breaking
206+
}
207+
134208
/// Check API files for all packages.
135209
///
136210
/// For each package, generates public API files for different feature configurations,
@@ -160,24 +234,14 @@ fn check_apis(
160234

161235
let diff = public_api::diff::PublicApiDiff::between(no_features, all_features);
162236

163-
if !diff.removed.is_empty() || !diff.changed.is_empty() {
164-
eprintln!("Non-additive features detected in {}:", package_name);
237+
let config = ApiConfig::load(sh, package_dir)?;
238+
let breaking_changes = check_for_breaking_changes(&diff, &config.allow_breaking_changes);
165239

166-
if !diff.removed.is_empty() {
167-
eprintln!(" Items removed when enabling features:");
168-
for item in &diff.removed {
169-
eprintln!(" - {}", item);
170-
}
171-
}
172-
173-
if !diff.changed.is_empty() {
174-
eprintln!(" Items changed when enabling features:");
175-
for item in &diff.changed {
176-
eprintln!(" - old: {}", item.old);
177-
eprintln!(" new: {}", item.new);
178-
}
240+
if !breaking_changes.is_empty() {
241+
eprintln!("Non-additive features detected in {}:", package_name);
242+
for change in breaking_changes {
243+
eprintln!(" {}", change);
179244
}
180-
181245
return Err("Non-additive features detected".into());
182246
}
183247
}
@@ -240,7 +304,7 @@ fn check_semver(
240304
quiet_cmd!(sh, "git switch {current_ref}").run()?;
241305

242306
// Check for breaking changes in each package.
243-
for package_name in package_info.iter().map(|(name, _)| name) {
307+
for (package_name, package_dir) in package_info {
244308
let Some(mut baseline) = baseline_apis.remove(package_name) else {
245309
environment::quiet_println(&format!(
246310
"Warning: Package '{}' not found in baseline - skipping comparison",
@@ -257,14 +321,22 @@ fn check_semver(
257321
continue;
258322
};
259323

324+
let api_config = ApiConfig::load(sh, package_dir)?;
325+
260326
for config in [FeatureConfig::None, FeatureConfig::Alloc, FeatureConfig::All] {
261327
let baseline_api = baseline.remove(&config).ok_or("Config not found in baseline")?;
262328
let current_api = current.remove(&config).ok_or("Config not found in current")?;
263329

264330
let diff = public_api::diff::PublicApiDiff::between(baseline_api, current_api);
265331

266-
if !diff.removed.is_empty() || !diff.changed.is_empty() {
267-
eprintln!("API changes detected in {} ({})", package_name, config.display_name());
332+
let breaking_changes =
333+
check_for_breaking_changes(&diff, &api_config.allow_breaking_changes);
334+
335+
if !breaking_changes.is_empty() {
336+
eprintln!("Breaking changes in {} ({}):", package_name, config.display_name());
337+
for change in breaking_changes {
338+
eprintln!(" {}", change);
339+
}
268340
return Err("Semver compatibility check failed: breaking changes detected".into());
269341
}
270342
}

0 commit comments

Comments
 (0)