Skip to content

Commit e76a6bb

Browse files
fix(api): read public-crates, so the gate covers only the external libraries (#142)
* fix(api): read public-crates, so the gate covers only the external libraries When public-crates of the visibility table is set, api reads only the libraries that public-crates and exclude-crates name: the same libraries that visibility treats as an external API. The list of HEAD selects the libraries of both trees. A narrowed item of an internal crate is then no removed finding. Signed-off-by: Bart Waardenburg <bart@waardenburg.dev> * fix(api): add --public-crate and --all-published, and read the names of the base One selection reads both trees with the names of HEAD and of the base, so a library that a change removes with its name still gives crate-removed. exclude-crates adds a library also without public-crates, as for visibility. The table is read before the checkout of the base tree. Signed-off-by: Bart Waardenburg <bart@waardenburg.dev> * style(api): format the api tests Signed-off-by: Bart Waardenburg <bart@waardenburg.dev> --------- Signed-off-by: Bart Waardenburg <bart@waardenburg.dev>
1 parent 4bdc30a commit e76a6bb

7 files changed

Lines changed: 244 additions & 8 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
### Fixed
2+
3+
- `cargo fallow api --base <REF>` now reads the same libraries that
4+
`visibility` treats as an external API. When `public-crates` of
5+
`[workspace.metadata.fallow.visibility]` is set, the command reads only the
6+
libraries that `public-crates` and `exclude-crates` name. A narrowed item of
7+
an internal crate is then no `removed` finding.
8+
9+
### Added
10+
11+
- `cargo fallow api --public-crate <CRATE>` replaces `public-crates` for one
12+
run. `--all-published` reads each published library and ignores the table.

‎docs/api.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,26 @@ published libraries of the workspace. A published library is a library whose
55
`publish` is not `false`. The command compares HEAD with the merge base of HEAD
66
and the ref, the same as `size --base`. `cargo fallow check` does not run it.
77

8+
The command reads the same libraries that `visibility` treats as an external
9+
API (see [Visibility](visibility.md)):
10+
11+
- When `public-crates` of `[workspace.metadata.fallow.visibility]` is set, the
12+
command reads only the libraries that `public-crates` and `exclude-crates`
13+
name. A library with `publish = false` in the list counts too. Use this
14+
when a workspace publishes internal crates only because a published crate
15+
depends on them: a narrowed item of an internal crate is then no `removed`
16+
finding.
17+
- Without `public-crates`, the command reads the published libraries and the
18+
libraries of `exclude-crates`.
19+
- One selection reads both trees: the names of HEAD and the names of the
20+
base. A library that a change removes together with its name still gives
21+
`crate-removed`.
22+
- `--public-crate <CRATE>` replaces `public-crates`. The flag can repeat.
23+
- `--all-published` reads each published library and ignores both keys.
24+
25+
A pattern of HEAD that matches no library is an error with the code
26+
`CARGO_FALLOW_INVALID_CONFIG`.
27+
828
```bash
929
cargo fallow api --base origin/main
1030
cargo fallow api --base origin/main -p app-core --format json

‎docs/visibility.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ crate name or a package name, and `*` matches any text, for example
1717
`"my-api-*"`. The match is exact, so `my_api` and `my-api` differ: use the
1818
form of the crate name or of the package name. A pattern that matches no
1919
library is an error with the code `CARGO_FALLOW_INVALID_CONFIG`. The flag
20-
replaces the list of the key. `check`, `fix` and the `unused-feature` rule of
21-
`deps` read the key. fallow for TypeScript has the same option as
20+
replaces the list of the key. `check`, `fix`, `api` and the `unused-feature`
21+
rule of `deps` read the key. fallow for TypeScript has the same option as
2222
`publicPackages`.
2323

2424
The exported items of these libraries are an external API. They are entry

‎src/api/mod.rs‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
//!
44
//! The command reads the full tree of the merge base, runs `cargo metadata
55
//! --no-deps` on it, and builds the module graph of both versions. A
6-
//! published library is a library whose `publish` is not `false`. The
6+
//! published library is a library whose `publish` is not `false`. When
7+
//! `public-crates` of the `visibility` table is set, the published libraries
8+
//! are the libraries that it and `exclude-crates` name. The
79
//! command compares the keys of [`surface`]: each public path of each item.
810
//!
911
//! - A key that only HEAD has is `added`. A key that only the base has is
@@ -64,6 +66,34 @@ pub struct ApiArgs {
6466
/// with no breaking version bump.
6567
#[arg(long)]
6668
fail_on_issues: bool,
69+
/// A library whose API the command reads. The flag replaces
70+
/// `public-crates` of the visibility table. A crate or package name, with
71+
/// `*` as a glob. The flag can repeat.
72+
#[arg(long, value_name = "CRATE", conflicts_with = "all_published")]
73+
public_crate: Vec<String>,
74+
/// Read each library that a registry can publish, and ignore
75+
/// `public-crates` and `exclude-crates`.
76+
#[arg(long)]
77+
all_published: bool,
78+
}
79+
80+
impl ApiArgs {
81+
/// The libraries of HEAD that the run reads, before the base tree loads:
82+
/// a bad table or pattern stops the run early.
83+
fn head_selection(&self, head: &workspace::Workspace) -> Result<surface::Selection, CliError> {
84+
if self.all_published {
85+
return Ok(surface::Selection::published());
86+
}
87+
let (exclude, public) = crate::visibility::config::external_api(head)?;
88+
if self.public_crate.is_empty() {
89+
return Ok(surface::Selection { exclude, public });
90+
}
91+
crate::visibility::config::check_public(head, &self.public_crate)?;
92+
Ok(surface::Selection {
93+
exclude,
94+
public: Some(self.public_crate.clone()),
95+
})
96+
}
6797
}
6898

6999
/// The kind of one change.
@@ -181,12 +211,19 @@ pub fn run(args: &ApiArgs) -> Result<Section, CliError> {
181211
let root = head.root.as_std_path();
182212
let merge_base = crate::structure::git::merge_base(root, &args.base)?;
183213
let scope = Scope::load(&head, &args.common.scope)?;
214+
let mut selection = args.head_selection(&head)?;
184215
progress.step("Reading the tree of the merge base");
185216
let tree = tree::checkout(root, &merge_base)?;
186217
let base = workspace::load(Some(&tree.root.join("Cargo.toml")), &cargo_flags)?;
218+
// One selection reads both trees, so a change of the lists gives no
219+
// crate finding. The table of the base adds its names: a library that a
220+
// change removes together with its name still gives `crate-removed`.
221+
if !args.all_published && args.public_crate.is_empty() {
222+
selection.add_base(&base);
223+
}
187224
progress.step("Reading the API of the published libraries");
188-
let before = surface::surface(&base);
189-
let after = surface::surface(&head);
225+
let before = surface::surface(&base, &selection);
226+
let after = surface::surface(&head, &selection);
190227
// `-p` keeps the findings of the packages that it names: a finding
191228
// belongs to its package, also when the item comes from another crate.
192229
let manifests: BTreeMap<&String, &String> = before

‎src/api/surface.rs‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,56 @@ pub struct Surface {
6262
pub versions: BTreeMap<String, String>,
6363
}
6464

65+
/// The libraries that `api` reads.
66+
#[derive(Debug, Clone, Default)]
67+
pub struct Selection {
68+
/// The `exclude-crates` list of the `visibility` table.
69+
pub exclude: Vec<String>,
70+
/// The `public-crates` patterns of the `visibility` table, or `None`.
71+
pub public: Option<Vec<String>>,
72+
}
73+
74+
impl Selection {
75+
/// The libraries that a registry can publish, with no table.
76+
pub const fn published() -> Self {
77+
Self {
78+
exclude: Vec::new(),
79+
public: None,
80+
}
81+
}
82+
83+
/// Adds the names of the table of the base tree. A base table that is not
84+
/// valid adds nothing. Without `public-crates` at HEAD, the base list
85+
/// adds nothing either.
86+
pub fn add_base(&mut self, base: &Workspace) {
87+
let Ok((exclude, public)) = crate::visibility::config::external_api(base) else {
88+
return;
89+
};
90+
for name in exclude {
91+
if !self.exclude.contains(&name) {
92+
self.exclude.push(name);
93+
}
94+
}
95+
if let (Some(list), Some(names)) = (&mut self.public, public) {
96+
for name in names {
97+
if !list.contains(&name) {
98+
list.push(name);
99+
}
100+
}
101+
}
102+
}
103+
104+
/// True when `api` reads the library: a library that `visibility` treats
105+
/// as an external API. Without `public-crates`, these are the libraries
106+
/// that a registry can publish and the excluded libraries.
107+
fn reads(&self, krate: &graph::Crate) -> bool {
108+
krate.kind.is_library()
109+
&& crate::visibility::is_api(krate, &self.exclude, self.public.as_deref())
110+
}
111+
}
112+
65113
/// Reads the API of each published library of `workspace`.
66-
pub fn surface(workspace: &Workspace) -> Surface {
114+
pub fn surface(workspace: &Workspace, selection: &Selection) -> Surface {
67115
let module_graph = graph::build(workspace, &graph::Production::default());
68116
let resolver = Resolver::new(&module_graph);
69117
let walker = Walker::new(workspace, &module_graph, &resolver);
@@ -74,7 +122,7 @@ pub fn surface(workspace: &Workspace) -> Surface {
74122
.insert(member.name.clone(), member.version.clone());
75123
}
76124
for (id, krate) in module_graph.crates.iter().enumerate() {
77-
if !krate.published || !krate.kind.is_library() {
125+
if !selection.reads(krate) {
78126
continue;
79127
}
80128
let Some(member) = workspace

‎src/visibility/config.rs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,20 @@ pub fn api_crates(workspace: &Workspace) -> Option<(Vec<String>, Vec<String>)> {
7676
})
7777
}
7878

79+
/// The `exclude-crates` list and the `public-crates` patterns of the table,
80+
/// for `api`. The patterns are `None` when the table has no `public-crates`.
81+
/// A pattern that matches no library is an error.
82+
pub fn external_api(workspace: &Workspace) -> Result<(Vec<String>, Option<Vec<String>>), CliError> {
83+
let config: Config = crate::structure::config::section(workspace, KEY)?;
84+
if let Some(patterns) = &config.public_crates {
85+
check_public(workspace, patterns)?;
86+
}
87+
Ok((config.exclude_crates, config.public_crates))
88+
}
89+
7990
/// Fails when a `public-crates` pattern matches no library of the workspace.
8091
/// A typo would make each library internal without notice.
81-
fn check_public(workspace: &Workspace, patterns: &[String]) -> Result<(), CliError> {
92+
pub fn check_public(workspace: &Workspace, patterns: &[String]) -> Result<(), CliError> {
8293
for pattern in patterns {
8394
let matched = workspace.members.iter().any(|member| {
8495
member

‎tests/api.rs‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,3 +430,111 @@ fn a_rule_set_to_off_removes_its_findings() {
430430
);
431431
assert!(kinds.contains("removed"), "{report:#}");
432432
}
433+
434+
/// With `public-crates`, `api` reads only the libraries that the list names,
435+
/// so the removals of the unlisted `demo` give no finding. These are
436+
/// the libraries that `visibility` treats as an external API. A library
437+
/// with `publish = false` in the list counts too.
438+
#[test]
439+
fn public_crates_selects_the_libraries_of_api() {
440+
let repo = Repo::new("0.3.2");
441+
let manifest = repo.root().join("Cargo.toml");
442+
let text = fs::read_to_string(&manifest).expect("read the manifest");
443+
let with = |list: &str| {
444+
fs::write(
445+
&manifest,
446+
format!("{text}\n[workspace.metadata.fallow.visibility]\npublic-crates = [{list}]\n"),
447+
)
448+
.expect("write the manifest");
449+
};
450+
with("\"fresh\", \"internal\"");
451+
let (code, report) = repo.json(&["api", "--base", "main", "--fail-on-issues"]);
452+
assert_eq!(code, 0, "{report:#}");
453+
assert_eq!(
454+
changes(&report),
455+
[entry("fresh", "crate-added", "fresh")],
456+
"{report:#}"
457+
);
458+
assert_eq!(report["summary"]["crates"], 2, "{report:#}");
459+
with("\"nothing-*\"");
460+
let (code, report) = repo.json(&["api", "--base", "main"]);
461+
assert_eq!(code, 2, "{report:#}");
462+
assert_eq!(report["code"], "CARGO_FALLOW_INVALID_CONFIG", "{report:#}");
463+
}
464+
465+
/// A library that a change deletes together with its name in
466+
/// `public-crates` still gives `crate-removed`: the list of the base adds its
467+
/// names.
468+
#[test]
469+
fn a_library_removed_with_its_public_crates_name_is_crate_removed() {
470+
let dir = tempfile::tempdir().expect("create a temporary directory");
471+
let repo = Repo { dir };
472+
let root = repo.root().to_path_buf();
473+
let write_root = |members: &str, list: &str| {
474+
fs::write(
475+
root.join("Cargo.toml"),
476+
format!(
477+
"[workspace]\nmembers = [{members}]\nresolver = \"3\"\n\n[workspace.metadata.fallow.visibility]\npublic-crates = [{list}]\n"
478+
),
479+
)
480+
.expect("write the root manifest");
481+
};
482+
write_root("\"a\", \"b\"", "\"a\", \"b\"");
483+
package(&root, "a", "0.1.0", "", "", "pub fn a() {}\n");
484+
package(&root, "b", "0.1.0", "", "", "pub fn b() {}\n");
485+
repo.git(&["init", "-q"]);
486+
repo.git(&["add", "-A"]);
487+
repo.git(&["commit", "-q", "-m", "base"]);
488+
repo.git(&["checkout", "-q", "-b", "feature"]);
489+
fs::remove_dir_all(root.join("b")).expect("remove b");
490+
write_root("\"a\"", "\"a\"");
491+
repo.git(&["add", "-A"]);
492+
repo.git(&["commit", "-q", "-m", "head"]);
493+
let (code, report) = repo.json(&["api", "--base", "main"]);
494+
assert_eq!(code, 0, "{report:#}");
495+
assert_eq!(
496+
changes(&report),
497+
[entry("b", "crate-removed", "b")],
498+
"{report:#}"
499+
);
500+
}
501+
502+
/// `exclude-crates` adds a library without `public-crates`, as for
503+
/// `visibility`. `--public-crate` replaces the list, and `--all-published`
504+
/// reads each library that a registry can publish.
505+
#[test]
506+
fn the_flags_and_exclude_crates_select_the_libraries() {
507+
let repo = Repo::new("0.3.2");
508+
let manifest = repo.root().join("Cargo.toml");
509+
let text = fs::read_to_string(&manifest).expect("read the manifest");
510+
fs::write(
511+
&manifest,
512+
format!(
513+
"{text}\n[workspace.metadata.fallow.visibility]\nexclude-crates = [\"internal\"]\n"
514+
),
515+
)
516+
.expect("write the manifest");
517+
let crates = |args: &[&str]| -> Value {
518+
let mut all = vec!["api", "--base", "main"];
519+
all.extend_from_slice(args);
520+
let (code, report) = repo.json(&all);
521+
assert_eq!(code, 0, "{report:#}");
522+
report["summary"]["crates"].clone()
523+
};
524+
assert_eq!(crates(&[]), 4);
525+
assert_eq!(crates(&["--all-published"]), 3);
526+
assert_eq!(
527+
crates(&["--public-crate", "fresh"]),
528+
2,
529+
"fresh and the excluded internal"
530+
);
531+
let (code, _) = repo.json(&[
532+
"api",
533+
"--base",
534+
"main",
535+
"--public-crate",
536+
"fresh",
537+
"--all-published",
538+
]);
539+
assert_eq!(code, 2);
540+
}

0 commit comments

Comments
 (0)