Skip to content

Commit 2053429

Browse files
committed
tidy: skip triagebot.toml path checks for unchecked out submodules locally
I say "unchecked out", but really it's an extremely naive heuristic of "don't bother checking paths to or under a submodule if we can't even read the submodule dir".
1 parent 2da29db commit 2053429

File tree

1 file changed

+95
-17
lines changed

1 file changed

+95
-17
lines changed

src/tools/tidy/src/triagebot.rs

+95-17
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
//! Tidy check to ensure paths mentioned in triagebot.toml exist in the project.
22
3-
use std::path::Path;
3+
use std::collections::HashMap;
4+
use std::path::{Path, PathBuf};
45

6+
use build_helper::ci::CiEnv;
57
use toml::Value;
68

7-
pub fn check(path: &Path, bad: &mut bool) {
8-
let triagebot_path = path.join("triagebot.toml");
9+
pub fn check(checkout_root: &Path, bad: &mut bool) {
10+
let triagebot_path = checkout_root.join("triagebot.toml");
911
if !triagebot_path.exists() {
1012
tidy_error!(bad, "triagebot.toml file not found");
1113
return;
@@ -14,73 +16,91 @@ pub fn check(path: &Path, bad: &mut bool) {
1416
let contents = std::fs::read_to_string(&triagebot_path).unwrap();
1517
let config: Value = toml::from_str(&contents).unwrap();
1618

17-
// Check [mentions."*"] sections, i.e. [mentions."compiler/rustc_const_eval/src/"]
19+
// Cache mapping between submodule path <-> whether submodule is checked out. This cache is to
20+
// avoid excessive filesystem accesses.
21+
let submodule_checked_out_status = cache_submodule_checkout_status(checkout_root);
22+
23+
// Check `[mentions."*"]` sections, i.e.
24+
//
25+
// ```
26+
// [mentions."compiler/rustc_const_eval/src/"]
27+
// ```
1828
if let Some(Value::Table(mentions)) = config.get("mentions") {
1929
for path_str in mentions.keys() {
2030
// Remove quotes from the path
2131
let clean_path = path_str.trim_matches('"');
22-
let full_path = path.join(clean_path);
32+
let full_path = checkout_root.join(clean_path);
2333

24-
if !full_path.exists() {
34+
if !check_path_exists_if_required(&submodule_checked_out_status, &full_path) {
2535
tidy_error!(
2636
bad,
27-
"triagebot.toml [mentions.*] contains path '{}' which doesn't exist",
37+
"triagebot.toml `[mentions.*]` contains path `{}` which doesn't exist",
2838
clean_path
2939
);
3040
}
3141
}
3242
} else {
3343
tidy_error!(
3444
bad,
35-
"triagebot.toml missing [mentions.*] section, this wrong for rust-lang/rust repo."
45+
"`triagebot.toml` is missing the `[mentions.*]` section; this is wrong for the \
46+
`rust-lang/rust` repo."
3647
);
3748
}
3849

39-
// Check [assign.owners] sections, i.e.
50+
// Check `[assign.owners]` sections, i.e.
51+
//
52+
// ```
4053
// [assign.owners]
4154
// "/.github/workflows" = ["infra-ci"]
55+
// ```
4256
if let Some(Value::Table(assign)) = config.get("assign") {
4357
if let Some(Value::Table(owners)) = assign.get("owners") {
4458
for path_str in owners.keys() {
4559
// Remove quotes and leading slash from the path
4660
let clean_path = path_str.trim_matches('"').trim_start_matches('/');
47-
let full_path = path.join(clean_path);
61+
let full_path = checkout_root.join(clean_path);
4862

49-
if !full_path.exists() {
63+
if !check_path_exists_if_required(&submodule_checked_out_status, &full_path) {
5064
tidy_error!(
5165
bad,
52-
"triagebot.toml [assign.owners] contains path '{}' which doesn't exist",
66+
"`triagebot.toml` `[assign.owners]` contains path `{}` which doesn't exist",
5367
clean_path
5468
);
5569
}
5670
}
5771
} else {
5872
tidy_error!(
5973
bad,
60-
"triagebot.toml missing [assign.owners] section, this wrong for rust-lang/rust repo."
74+
"`triagebot.toml` is missing the `[assign.owners]` section; this is wrong for the \
75+
`rust-lang/rust` repo."
6176
);
6277
}
6378
}
6479

65-
// Verify that trigger_files in [autolabel."*"] exist in the project, i.e.
80+
// Verify that `trigger_files` paths in `[autolabel."*"]` exists, i.e.
81+
//
82+
// ```
6683
// [autolabel."A-rustdoc-search"]
6784
// trigger_files = [
6885
// "src/librustdoc/html/static/js/search.js",
6986
// "tests/rustdoc-js",
7087
// "tests/rustdoc-js-std",
7188
// ]
89+
// ```
7290
if let Some(Value::Table(autolabels)) = config.get("autolabel") {
7391
for (label, content) in autolabels {
7492
if let Some(trigger_files) = content.get("trigger_files").and_then(|v| v.as_array()) {
7593
for file in trigger_files {
7694
if let Some(file_str) = file.as_str() {
77-
let full_path = path.join(file_str);
95+
let full_path = checkout_root.join(file_str);
7896

7997
// Handle both file and directory paths
80-
if !full_path.exists() {
98+
if !check_path_exists_if_required(&submodule_checked_out_status, &full_path)
99+
{
81100
tidy_error!(
82101
bad,
83-
"triagebot.toml [autolabel.{}] contains trigger_files path '{}' which doesn't exist",
102+
"`triagebot.toml` `[autolabel.{}]` contains `trigger_files` path \
103+
`{}` which doesn't exist",
84104
label,
85105
file_str
86106
);
@@ -91,3 +111,61 @@ pub fn check(path: &Path, bad: &mut bool) {
91111
}
92112
}
93113
}
114+
115+
/// Very naive heuristics for whether a submodule is checked out.
116+
fn cache_submodule_checkout_status(checkout_root: &Path) -> HashMap<PathBuf, bool> {
117+
let mut cache = HashMap::default();
118+
119+
// NOTE: can't assume `git` exists.
120+
let submodule_paths = build_helper::util::parse_gitmodules(&checkout_root);
121+
122+
for submodule in submodule_paths {
123+
let full_submodule_path = checkout_root.join(submodule);
124+
125+
let is_checked_out = if CiEnv::is_ci() {
126+
// In CI, require all submodules to be checked out and thus don't skip checking any
127+
// paths.
128+
true
129+
} else {
130+
// NOTE: for our purposes, just skip checking paths to and under a submodule if we can't
131+
// read its dir locally (even if this can miss broken paths).
132+
std::fs::read_dir(&full_submodule_path).is_ok_and(|entry| {
133+
// NOTE: de-initializing a submodule can leave an empty folder behind
134+
entry.count() > 0
135+
})
136+
};
137+
138+
if let Some(_) = cache.insert(full_submodule_path.clone(), is_checked_out) {
139+
panic!(
140+
"unexpected duplicate submodule paths in `deps::WORKSPACES`: {} already in \
141+
submodule checkout cache",
142+
full_submodule_path.display()
143+
);
144+
}
145+
}
146+
147+
cache
148+
}
149+
150+
/// Check that a path exists. This is:
151+
///
152+
/// - Unconditionally checked under CI environment.
153+
/// - Only checked under local environment if submodule is checked out (if candidate path points
154+
/// under or to a submodule).
155+
fn check_path_exists_if_required(
156+
submodule_checkout_status: &HashMap<PathBuf, bool>,
157+
candidate: &Path,
158+
) -> bool {
159+
for (submodule_path, is_checked_out) in submodule_checkout_status {
160+
if candidate.starts_with(submodule_path) {
161+
if *is_checked_out {
162+
return candidate.exists();
163+
} else {
164+
// Not actually checked, but just skipped.
165+
return true;
166+
}
167+
}
168+
}
169+
170+
candidate.exists()
171+
}

0 commit comments

Comments
 (0)