build(xtask): dynamic discovery for MSVC, LLVM, and Android SDK paths#3065
Open
brbrainerd wants to merge 1 commit into
Open
build(xtask): dynamic discovery for MSVC, LLVM, and Android SDK paths#3065brbrainerd wants to merge 1 commit into
brbrainerd wants to merge 1 commit into
Conversation
WalkthroughAdds dynamic, OS-aware native toolchain discovery in xtask: detects Windows SDK/MSVC include paths and libclang, discovers Android SDK/NDK with latest-version selection, normalizes paths to forward slashes, exposes bindgen-related config fields, and updates cargo template and tests; also appends Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build System
participant Config as xtask::config.rs
participant WinDiscover as Windows SDK/MSVC Discovery
participant AndroidDiscover as Android SDK/NDK Discovery
participant Bindgen as Bindgen Config/Libclang
Build->>Config: generate_cargo_config()
alt Windows
Config->>WinDiscover: find_windows_include_paths()
WinDiscover-->>Config: include paths
Config->>WinDiscover: find_libclang_path()
WinDiscover-->>Config: libclang path
Config->>Bindgen: set bindgen_extra_clang_args + libclang_path
else macOS
Config->>Config: read macOS SDK path
Config->>Bindgen: set -isysroot in extra args
else Linux
Config->>Bindgen: leave bindgen args empty
end
Config->>AndroidDiscover: find_android_ndk()
alt NDK found
AndroidDiscover->>AndroidDiscover: select latest NDK version
AndroidDiscover-->>Config: NDK path
Config->>Bindgen: configure Android cross-compile args
else NDK not found
AndroidDiscover-->>Config: None (log warning) -> skip Android targets
end
Bindgen-->>Build: write normalized cargo config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtask/src/config.rs`:
- Around line 423-445: The test test_windows_dynamic_discovery currently relies
on real machine toolchains; change it to avoid that by refactoring into two
parts: (1) create a new unit test that injects temporary/mock directory roots
and files and verifies find_windows_include_paths and find_libclang_path against
those injected roots (use tempdir or a test fixture and pass those paths into
the discovery helpers), and (2) mark the existing real-machine probe
(test_windows_dynamic_discovery) as #[ignore] or a #[cfg(feature = "smoke")] so
it only runs intentionally on machines with VS/SDK/libclang; update
calls/reference points to find_windows_include_paths and find_libclang_path so
they can accept an optional base/path override for testing.
- Around line 100-109: The code currently falls back to an empty
android_ndk_home when find_android_ndk() fails, prints messages with println!,
and still leaves has_android true causing Android linker/template output with
empty paths; change the fallback to set android_ndk_home = None (or an Option)
and also set has_android = false when NDK discovery fails so Android config is
actually disabled, replace the println! calls with tracing::warn! (or
info!/debug! as appropriate), and update any uses of android_ndk_home (e.g.,
template rendering or linker path emission) to check the Option (or has_android)
before emitting Android-specific config; refer to find_android_ndk,
android_ndk_home and has_android to locate and update the logic (also apply the
same fix to the similar code at the later occurrence around lines 142-143).
- Around line 36-39: The generated fields bindgen_extra_clang_args (serde rename
"bindgenExtraClangArgs") and libclang_path (serde rename "libclangPath") are
serialized but not injected into the .cargo/config.toml.mustache template;
update the template to consume these two keys so Cargo sees the
Windows/MSVC/libclang discovery. Edit the mustache template to reference
{{bindgenExtraClangArgs}} and {{libclangPath}} where appropriate (e.g., in clang
args and libclang path entries), and ensure any other computed fields from the
same struct (referenced around the same block in xtask/src/config.rs) are
likewise wired into the template so the serialized values are propagated to the
generated cargo config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f73350b1-8db8-44e9-99ec-be5d7e8cdfbc
📒 Files selected for processing (3)
.gitignore.tasks/core/DEV-002-dynamic-native-build-discovery.mdxtask/src/config.rs
brbrainerd
force-pushed
the
build/xtask-dynamic-discovery
branch
4 times, most recently
from
46b6b6e to
94b4f37
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
xtask/src/config.rs (3)
227-262: Inconsistent version-selection strategy between MSVC/SDK and NDK.You added a dedicated
get_latest_ndk_versionhelper that splits on.and compares numerically, which is the right thing to do for semver-like directory names. Here the MSVC and Windows SDK roots fall back toversions.sort()onOsString, i.e. lexicographic byte order. For well-formed zero-padded MSVC versions this usually works, but any non-version directory in those roots (backup folders,v141, etc.) can lex-sort above a real version and be selected. Factor the digit-first, numeric-component comparator out and reuse it for all three discovery paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/src/config.rs` around lines 227 - 262, The MSVC and Windows SDK discovery currently use lexicographic versions.sort() on OsString which can pick non-numeric names; reuse the numeric-component comparator logic from get_latest_ndk_version by extracting it (or calling it) to compare version directory names numerically (split on '.', parse components as numbers, compare component-wise, treat missing parts as 0) and replace the versions.sort()/versions.last() usage in the MSVC ("VC\\Tools\\MSVC") and SDK ("C:\\Program Files (x86)\\Windows Kits\\10\\Include") blocks to use versions.sort_by(...) or a helper like get_latest_ndk_version to select the true latest numeric version.
447-460:test_android_discoveryasserts nothing.As written, the test only guards against a panic in
find_android_sdk/find_android_ndk; on any machine without an SDK it prints nothing and passes trivially, and on machines with an SDK it still does not verify the returned path has the expected shape (exists, containsndksubdir, is forward-slash normalized, etc.).Either add positive assertions on the path shape when discovery succeeds, or restructure around injected roots (a
tempdirpopulated withndk/25.2.9519653/) so you can deterministically assert the selection.♻️ Example restructure
- #[test] - fn test_android_discovery() { - // This test will only pass if an Android SDK is actually present - // but we can at least ensure it doesn't crash. - let sdk = find_android_sdk(); - let ndk = find_android_ndk(); - - if let Some(s) = sdk { - println!("Found Android SDK at: {:?}", s); - } - if let Some(n) = ndk { - println!("Found Android NDK at: {}", n); - } - } + #[test] + fn test_android_discovery_smoke() { + // Smoke check: helpers must not panic regardless of host state, and any + // returned NDK path must be forward-slash normalized per the PR contract. + let _ = find_android_sdk(); + if let Some(n) = find_android_ndk() { + assert!(!n.contains('\\'), "NDK path must be forward-slash normalized: {n}"); + assert!(Path::new(&n.replace('/', std::path::MAIN_SEPARATOR_STR)).exists()); + } + }Also, per repo guidelines, prefer
tracing::info!(or simply drop the prints) instead ofprintln!inside tests when the goal is diagnostic output.As per coding guidelines: "Use
tracingmacros (info!,debug!,warn!,error!) instead ofprintln!for logging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/src/config.rs` around lines 447 - 460, test_android_discovery currently only checks for panics and uses println!, so add deterministic assertions and replace prints with tracing: when discovery returns Some, assert the path exists and has expected structure (e.g., contains an `ndk` subdirectory or a versioned entry like `ndk/25.2.9519653`); to make the test deterministic, create a temporary directory (tempdir), populate it with a fake `ndk/<version>/` layout and inject it into the discovery functions (or refactor find_android_sdk/find_android_ndk to accept a root path) and assert the selected path equals the created layout; remove println! and use tracing::info! for any diagnostic messages.
359-373: Smallget_latest_ndk_versionsimplifications.Two nitpicks:
s.chars().next().map_or(false, |c| c.is_ascii_digit())is equivalent tos.starts_with(|c: char| c.is_ascii_digit())ors.as_bytes().first().is_some_and(|b| b.is_ascii_digit()), both of which read more clearly and skip the UTF-8 decode of the full prefix.- Sorting the whole vector and taking
.last()isO(n log n);Iterator::max_bydoes the same job inO(n)without the allocation's reordering cost. For < 100 NDK versions it's immaterial, but it also removes the intermediate mutable binding.- let mut versions: Vec<_> = versions - .map(|s| s.as_ref().to_string()) - .filter(|s| s.chars().next().map_or(false, |c| c.is_ascii_digit())) - .collect(); - - versions.sort_by(|a, b| { - let a_parts: Vec<_> = a.split('.').filter_map(|s| s.parse::<u32>().ok()).collect(); - let b_parts: Vec<_> = b.split('.').filter_map(|s| s.parse::<u32>().ok()).collect(); - a_parts.cmp(&b_parts) - }); - - versions.last().cloned() + versions + .map(|s| s.as_ref().to_string()) + .filter(|s| s.as_bytes().first().is_some_and(|b| b.is_ascii_digit())) + .max_by_key(|s| { + s.split('.') + .filter_map(|p| p.parse::<u32>().ok()) + .collect::<Vec<_>>() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/src/config.rs` around lines 359 - 373, Simplify get_latest_ndk_version by replacing the filter that uses s.chars().next().map_or(...) with a clearer ASCII-digit check (e.g., s.starts_with(|c: char| c.is_ascii_digit()) or s.as_bytes().first().is_some_and(|b| b.is_ascii_digit())) and avoid sorting the whole versions Vec: use Iterator::max_by on the mapped semver-component vectors (or compare parsed component Vec<u32> directly) to find the maximum in O(n) instead of versions.sort_by(...) followed by versions.last(); update the code paths referencing the versions iterator and the final returned value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtask/src/config.rs`:
- Around line 204-265: The function find_windows_include_paths currently
hardcodes "C:\\Program Files (x86)\\..." for vswhere_installer and sdk_root
which fails on non-default installs; update find_windows_include_paths to build
those paths from the ProgramFiles(x86) environment variable
(std::env::var("ProgramFiles(x86)") with a fallback to env::var("ProgramFiles")
or a sensible default) and use that root to construct the vswhere path and the
Windows Kits include root instead of hardcoded strings, and also either change
the function signature from Result<Vec<String>> to Vec<String> or make the
IO/Command calls actually return errors by using anyhow::Result and adding
.context(...) to Command::new(...).output() and fs::read_dir(...) so failures
are surfaced; reference symbols: find_windows_include_paths, vswhere_installer,
sdk_root, Command::new(...).output(), fs::read_dir(...).
- Line 158: The current args.push_str(&format!(" -isystem\\\"{}\\\"",
path.replace('\\', "/"))) produces unescaped quotes that break TOML; replace it
so you append a single shell-quoted token (e.g. a space then
"\"-isystem<path>\"") with path normalized via path.replace('\\', "/") and
ensure the string is TOML-escaped before template substitution; in practice
change the args.push_str call to format the argument as a shell token like "
\"-isystem{}\"" (using the path.replace result) so the resulting TOML value
contains an escaped quote-delimited token that shlex/bindgen can parse
correctly.
---
Nitpick comments:
In `@xtask/src/config.rs`:
- Around line 227-262: The MSVC and Windows SDK discovery currently use
lexicographic versions.sort() on OsString which can pick non-numeric names;
reuse the numeric-component comparator logic from get_latest_ndk_version by
extracting it (or calling it) to compare version directory names numerically
(split on '.', parse components as numbers, compare component-wise, treat
missing parts as 0) and replace the versions.sort()/versions.last() usage in the
MSVC ("VC\\Tools\\MSVC") and SDK ("C:\\Program Files (x86)\\Windows
Kits\\10\\Include") blocks to use versions.sort_by(...) or a helper like
get_latest_ndk_version to select the true latest numeric version.
- Around line 447-460: test_android_discovery currently only checks for panics
and uses println!, so add deterministic assertions and replace prints with
tracing: when discovery returns Some, assert the path exists and has expected
structure (e.g., contains an `ndk` subdirectory or a versioned entry like
`ndk/25.2.9519653`); to make the test deterministic, create a temporary
directory (tempdir), populate it with a fake `ndk/<version>/` layout and inject
it into the discovery functions (or refactor find_android_sdk/find_android_ndk
to accept a root path) and assert the selected path equals the created layout;
remove println! and use tracing::info! for any diagnostic messages.
- Around line 359-373: Simplify get_latest_ndk_version by replacing the filter
that uses s.chars().next().map_or(...) with a clearer ASCII-digit check (e.g.,
s.starts_with(|c: char| c.is_ascii_digit()) or
s.as_bytes().first().is_some_and(|b| b.is_ascii_digit())) and avoid sorting the
whole versions Vec: use Iterator::max_by on the mapped semver-component vectors
(or compare parsed component Vec<u32> directly) to find the maximum in O(n)
instead of versions.sort_by(...) followed by versions.last(); update the code
paths referencing the versions iterator and the final returned value
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8d53631-1239-43af-89c1-e83bc793545b
📒 Files selected for processing (3)
.gitignore.tasks/core/DEV-002-dynamic-native-build-discovery.mdxtask/src/config.rs
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- .tasks/core/DEV-002-dynamic-native-build-discovery.md
Comment on lines
+204
to
+265
| fn find_windows_include_paths() -> Result<Vec<String>> { | ||
| let mut paths = Vec::new(); | ||
|
|
||
| // 1. Find MSVC path via vswhere | ||
| let vswhere_installer = "C:\\Program Files (x86)\\Microsoft Visual Studio\\Installer\\vswhere.exe"; | ||
| let vs_path = Command::new(vswhere_installer) | ||
| .args([ | ||
| "-latest", | ||
| "-products", | ||
| "*", | ||
| "-requires", | ||
| "Microsoft.VisualStudio.Component.VC.Tools.x86.x64", | ||
| "-property", | ||
| "installationPath", | ||
| ]) | ||
| .output(); | ||
|
|
||
| if let Ok(output) = vs_path { | ||
| let install_path = String::from_utf8_lossy(&output.stdout).trim().to_string(); | ||
| if !install_path.is_empty() { | ||
| // Find latest MSVC version | ||
| let tools_path = Path::new(&install_path).join("VC\\Tools\\MSVC"); | ||
| if let Ok(entries) = fs::read_dir(tools_path) { | ||
| let mut versions: Vec<_> = entries | ||
| .filter_map(|e| e.ok()) | ||
| .filter(|e| e.path().is_dir()) | ||
| .map(|e| e.file_name()) | ||
| .collect(); | ||
| versions.sort(); | ||
| if let Some(latest) = versions.last() { | ||
| paths.push( | ||
| Path::new(&install_path) | ||
| .join("VC\\Tools\\MSVC") | ||
| .join(latest) | ||
| .join("include") | ||
| .to_string_lossy() | ||
| .to_string(), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // 2. Find Windows SDK path | ||
| let sdk_root = "C:\\Program Files (x86)\\Windows Kits\\10\\Include"; | ||
| if let Ok(entries) = fs::read_dir(sdk_root) { | ||
| let mut versions: Vec<_> = entries | ||
| .filter_map(|e| e.ok()) | ||
| .filter(|e| e.path().is_dir()) | ||
| .map(|e| e.file_name()) | ||
| .collect(); | ||
| versions.sort(); | ||
| if let Some(latest) = versions.last() { | ||
| let latest_path = Path::new(sdk_root).join(latest); | ||
| paths.push(latest_path.join("ucrt").to_string_lossy().to_string()); | ||
| paths.push(latest_path.join("um").to_string_lossy().to_string()); | ||
| paths.push(latest_path.join("shared").to_string_lossy().to_string()); | ||
| } | ||
| } | ||
|
|
||
| Ok(paths) | ||
| } |
There was a problem hiding this comment.
Hardcoded C:\Program Files (x86) paths break on non-default installs.
Both the vswhere.exe path (line 208) and the Windows Kits include root (line 248) assume the system drive is C: and that the 32-bit Program Files directory is at its default location. On machines where Windows is installed on another drive, or where ProgramFiles(x86) has been redirected, discovery silently returns an empty Vec and the Windows include paths are never emitted. Use the canonical environment variable instead.
🐛 Suggested fix
- // 1. Find MSVC path via vswhere
- let vswhere_installer = "C:\\Program Files (x86)\\Microsoft Visual Studio\\Installer\\vswhere.exe";
+ // 1. Find MSVC path via vswhere
+ let program_files_x86 = std::env::var("ProgramFiles(x86)")
+ .unwrap_or_else(|_| "C:\\Program Files (x86)".to_string());
+ let vswhere_installer = Path::new(&program_files_x86)
+ .join("Microsoft Visual Studio")
+ .join("Installer")
+ .join("vswhere.exe");
let vs_path = Command::new(vswhere_installer)
.args([
"-latest",
...
])
.output();
...
// 2. Find Windows SDK path
- let sdk_root = "C:\\Program Files (x86)\\Windows Kits\\10\\Include";
+ let sdk_root = Path::new(&program_files_x86).join("Windows Kits\\10\\Include");
if let Ok(entries) = fs::read_dir(sdk_root) {Also, find_windows_include_paths returns Result<Vec<String>> but never produces an Err; consider simplifying the signature to Vec<String> (or actually surface errors via anyhow::Context on the Command/read_dir calls).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtask/src/config.rs` around lines 204 - 265, The function
find_windows_include_paths currently hardcodes "C:\\Program Files (x86)\\..."
for vswhere_installer and sdk_root which fails on non-default installs; update
find_windows_include_paths to build those paths from the ProgramFiles(x86)
environment variable (std::env::var("ProgramFiles(x86)") with a fallback to
env::var("ProgramFiles") or a sensible default) and use that root to construct
the vswhere path and the Windows Kits include root instead of hardcoded strings,
and also either change the function signature from Result<Vec<String>> to
Vec<String> or make the IO/Command calls actually return errors by using
anyhow::Result and adding .context(...) to Command::new(...).output() and
fs::read_dir(...) so failures are surfaced; reference symbols:
find_windows_include_paths, vswhere_installer, sdk_root,
Command::new(...).output(), fs::read_dir(...).
Author
|
Addressed automated review feedback: wired up template bindings, replaced environment-dependent tests with mocked fixtures, and properly handled Android NDK discovery fallbacks. |
brbrainerd
force-pushed
the
build/xtask-dynamic-discovery
branch
from
94b4f37 to
eeb9cb0
Compare
Author
|
Addressed additional CodeRabbit nitpicks: unified the version sorting logic to use numeric-component comparators for MSVC and Windows SDK discovery, added tracing to xtask to resolve compilation errors from logging, and replaced the Android environment-probe test with a deterministic mocked test. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
xtask/src/config.rs (1)
206-273:⚠️ Potential issue | 🟠 MajorHardcoded
C:\Program Files (x86)fallbacks still break non-default installs.This was raised in a prior review and doesn't appear to have been addressed on lines 216 and 258. Machines with Windows on a non-
C:drive, or with a redirectedProgramFiles(x86), will silently get an empty MSVCinstall_pathand aread_dirfailure onsdk_root, so no-isystempaths get emitted and bindgen falls back to failing onerrno.has before. Use the canonical environment variable with a reasonable default.Additionally,
find_windows_include_pathsdeclares-> Result<Vec<String>>but never actually returnsErr— theif let Ok(...)at the call site (line 158) is misleading. Either drop theResultwrapper or wire real errors throughanyhow::Contexton theCommand/read_dircalls.🐛 Suggested fix
-fn find_windows_include_paths( - vs_install_path_override: Option<&str>, - sdk_root_override: Option<&str>, -) -> Result<Vec<String>> { +fn find_windows_include_paths( + vs_install_path_override: Option<&str>, + sdk_root_override: Option<&str>, +) -> Vec<String> { let mut paths = Vec::new(); + let program_files_x86 = std::env::var("ProgramFiles(x86)") + .unwrap_or_else(|_| "C:\\Program Files (x86)".to_string()); // 1. Find MSVC path let install_path = if let Some(p) = vs_install_path_override { p.to_string() } else { - let vswhere_installer = "C:\\Program Files (x86)\\Microsoft Visual Studio\\Installer\\vswhere.exe"; + let vswhere_installer = Path::new(&program_files_x86) + .join("Microsoft Visual Studio") + .join("Installer") + .join("vswhere.exe"); let vs_path = Command::new(vswhere_installer) ... }; ... - let sdk_root = sdk_root_override.unwrap_or("C:\\Program Files (x86)\\Windows Kits\\10\\Include"); + let default_sdk_root = Path::new(&program_files_x86) + .join("Windows Kits").join("10").join("Include") + .to_string_lossy().to_string(); + let sdk_root: &str = sdk_root_override.unwrap_or(&default_sdk_root); ... - Ok(paths) + paths }Then update the call site at line 158 from
if let Ok(paths) = ...to a direct binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/src/config.rs` around lines 206 - 273, The function find_windows_include_paths currently hardcodes "C:\\Program Files (x86)" and returns Result<Vec<String>> that never errors; change it to read ProgramFiles(x86) from the environment (e.g. env::var("ProgramFiles(x86)").unwrap_or_else(|_| "C:\\Program Files (x86)".to_string())) and build the sdk_root default from that env var instead of the hardcoded path, update any Command/read_dir calls to use anyhow::Context if you prefer to keep Result (or simply change the signature to -> Vec<String> and remove Result wrapping), and then update the caller that used if let Ok(paths) = find_windows_include_paths(...) to a direct binding (e.g. let paths = find_windows_include_paths(...);) to match the new signature; reference symbols: function find_windows_include_paths and its call site where paths were matched with if let Ok(...).
🧹 Nitpick comments (2)
xtask/src/config.rs (2)
440-441: Tests share fixed paths understd::env::temp_dir()— racy undercargo test.
test_windows_dynamic_discovery_mockedandtest_android_discovery_mockedboth derive their working directory from a constant suffix (sd_test_dynamic_discovery/sd_test_android_discovery). If these tests (or any future test reusing the same names) run in parallel across workers,remove_dir_all+create_dir_allrace and the assertions become flaky. Prefer thetempfilecrate (already idiomatic in the Rust ecosystem) so each invocation gets a process-unique directory that's cleaned up on drop.let temp = tempfile::tempdir()?; let root = temp.path();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/src/config.rs` around lines 440 - 441, Replace the shared fixed-temp-dir usage (the let temp = std::env::temp_dir().join("sd_test_dynamic_discovery") + fs::remove_dir_all) used by tests like test_windows_dynamic_discovery_mocked and test_android_discovery_mocked with a per-test temporary directory from the tempfile crate (e.g. tempfile::tempdir() -> TempDir and use temp.path() as the root), so each test gets a unique directory that is cleaned up on drop and avoids remove_dir_all races; update the test functions to accept the TempDir lifetime (or convert to Path via temp.path()) and add tempfile to dev-dependencies if not already present.
100-111: Consider surfacing the NDK guidance once, not as six separate warn lines.Six consecutive
tracing::warn!lines per process startup is noisy and fragments structured log sinks. Consolidate into a single multi-line message (or awarn!+info!pair) so log aggregators keep the context together:♻️ Suggested refactor
- let android_ndk_home = find_android_ndk(None).or_else(|| { - tracing::warn!("Android NDK not found. Android targets will be skipped."); - tracing::warn!("You can provide the location manually by setting the ANDROID_NDK_HOME environment variable."); - tracing::warn!("Use forward slashes (/) as they are OS-independent and work on Windows, Linux, and macOS."); - tracing::warn!("Examples:"); - tracing::warn!(" Windows: ANDROID_NDK_HOME=\"C:/Users/Username/AppData/Local/Android/Sdk/ndk/25.1.8937393\""); - tracing::warn!(" macOS: ANDROID_NDK_HOME=\"/Users/Username/Library/Android/sdk/ndk/25.1.8937393\""); - tracing::warn!(" Linux: ANDROID_NDK_HOME=\"/home/username/Android/Sdk/ndk/25.1.8937393\""); + let android_ndk_home = find_android_ndk(None).or_else(|| { + tracing::warn!( + "Android NDK not found; Android targets will be skipped. \ + Set ANDROID_NDK_HOME (forward slashes, e.g. \ + C:/Users/<you>/AppData/Local/Android/Sdk/ndk/<ver>, \ + ~/Library/Android/sdk/ndk/<ver>, or ~/Android/Sdk/ndk/<ver>) to enable them." + ); has_android = false; None });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtask/src/config.rs` around lines 100 - 111, The repeated tracing::warn! calls in the android NDK lookup block (around variables has_android and android_ndk_home and the call to find_android_ndk) should be consolidated into a single warning so the guidance stays together; replace the six consecutive tracing::warn! invocations with one tracing::warn! that contains a single multi-line message (or a single warn! and a follow-up info! if you prefer separation) that includes the brief notice, how to set ANDROID_NDK_HOME, the forward-slash recommendation, and the three platform examples, and ensure you still set has_android = false and return None when the NDK is not found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtask/src/config.rs`:
- Around line 438-477: The test test_windows_dynamic_discovery_mocked fails on
non-Windows because find_windows_include_paths constructs MSVC subpaths using a
hardcoded Windows backslash string ("VC\\Tools\\MSVC") which Path::join treats
literally on Unix; update find_windows_include_paths to build the MSVC path
portably (e.g., replace join("VC\\Tools\\MSVC") with chained
.join("VC").join("Tools").join("MSVC") or otherwise use Path components or
std::path::MAIN_SEPARATOR) so the mocked directories created in the test are
discovered on all platforms; modify the codepaths that create
msbuild/install_path joins (the code in find_windows_include_paths) accordingly
so the test test_windows_dynamic_discovery_mocked passes cross-platform.
- Around line 148-167: The Windows branch in bindgen_extra_clang_args always
emits "-I{}/include" because native_deps.as_deref().unwrap_or("") yields an
empty string, producing "-I/include" when native_deps is None; update the
Os::Windows arm to only prepend the "-I.../include" token when
native_deps.is_some() (or use if let Some(native) = native_deps.as_deref() { ...
} to build the args string) and otherwise omit the "-I" flag entirely (or
short-circuit the Windows branch if the prep step hasn't run). Adjust
construction of args in the Os::Windows branch (reference
bindgen_extra_clang_args, native_deps, find_windows_include_paths, and
Os::Windows) so the invalid "-I/include" is never produced.
- Around line 479-498: The test test_android_discovery_mocked is brittle because
find_android_ndk reads ANDROID_NDK_HOME/ANDROID_NDK before sdk_root_override;
modify the test or function so the mocked SDK wins: either (A) in the test call
std::env::remove_var for "ANDROID_NDK_HOME" and "ANDROID_NDK" (and restore them
after) before invoking find_android_ndk, or (B) change find_android_ndk to
accept an explicit ndk_override parameter (or treat sdk_root_override as highest
precedence) so the supplied temp SDK root is used regardless of environment
variables; update test_android_discovery_mocked to use the new override path if
you choose the refactor.
---
Duplicate comments:
In `@xtask/src/config.rs`:
- Around line 206-273: The function find_windows_include_paths currently
hardcodes "C:\\Program Files (x86)" and returns Result<Vec<String>> that never
errors; change it to read ProgramFiles(x86) from the environment (e.g.
env::var("ProgramFiles(x86)").unwrap_or_else(|_| "C:\\Program Files
(x86)".to_string())) and build the sdk_root default from that env var instead of
the hardcoded path, update any Command/read_dir calls to use anyhow::Context if
you prefer to keep Result (or simply change the signature to -> Vec<String> and
remove Result wrapping), and then update the caller that used if let Ok(paths) =
find_windows_include_paths(...) to a direct binding (e.g. let paths =
find_windows_include_paths(...);) to match the new signature; reference symbols:
function find_windows_include_paths and its call site where paths were matched
with if let Ok(...).
---
Nitpick comments:
In `@xtask/src/config.rs`:
- Around line 440-441: Replace the shared fixed-temp-dir usage (the let temp =
std::env::temp_dir().join("sd_test_dynamic_discovery") + fs::remove_dir_all)
used by tests like test_windows_dynamic_discovery_mocked and
test_android_discovery_mocked with a per-test temporary directory from the
tempfile crate (e.g. tempfile::tempdir() -> TempDir and use temp.path() as the
root), so each test gets a unique directory that is cleaned up on drop and
avoids remove_dir_all races; update the test functions to accept the TempDir
lifetime (or convert to Path via temp.path()) and add tempfile to
dev-dependencies if not already present.
- Around line 100-111: The repeated tracing::warn! calls in the android NDK
lookup block (around variables has_android and android_ndk_home and the call to
find_android_ndk) should be consolidated into a single warning so the guidance
stays together; replace the six consecutive tracing::warn! invocations with one
tracing::warn! that contains a single multi-line message (or a single warn! and
a follow-up info! if you prefer separation) that includes the brief notice, how
to set ANDROID_NDK_HOME, the forward-slash recommendation, and the three
platform examples, and ensure you still set has_android = false and return None
when the NDK is not found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc6b5b50-8e36-4f33-9a39-aa3e52806a4b
⛔ Files ignored due to path filters (1)
xtask/Cargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (4)
.cargo/config.toml.mustache.gitignore.tasks/core/DEV-002-dynamic-native-build-discovery.mdxtask/src/config.rs
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- .tasks/core/DEV-002-dynamic-native-build-discovery.md
Comment on lines
+148
to
+167
| bindgen_extra_clang_args: match system.os { | ||
| Os::MacOS => format!("-isysroot {}", sdk_path), | ||
| Os::Windows => { | ||
| let mut args = format!( | ||
| "-I{}/include -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_CRT_USE_BUILTIN_OFFSETOF -D_WIN32 -D_WIN64", | ||
| native_deps.as_deref().unwrap_or("") | ||
| ); | ||
|
|
||
| // Try to find Windows SDK and MSVC include paths | ||
| // This helps bindgen find errno.h and other system headers | ||
| if let Ok(paths) = find_windows_include_paths(None, None) { | ||
| for path in paths { | ||
| args.push_str(&format!(" -isystem\\\"{}\\\"", path.replace('\\', "/"))); | ||
| } | ||
| } | ||
|
|
||
| args | ||
| } | ||
| Os::Linux => String::new(), | ||
| }, |
There was a problem hiding this comment.
Native-deps include flag is emitted even when native_deps is unset.
native_deps.as_deref().unwrap_or("") silently produces -I/include when no native-deps directory was prepared, which is invalid and will confuse bindgen diagnostics. Either skip the -I flag entirely when native_deps is None, or short-circuit the whole Windows branch when the prep step hasn't run.
♻️ Suggested fix
- Os::Windows => {
- let mut args = format!(
- "-I{}/include -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_CRT_USE_BUILTIN_OFFSETOF -D_WIN32 -D_WIN64",
- native_deps.as_deref().unwrap_or("")
- );
+ Os::Windows => {
+ let mut args = String::from(
+ "-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_CRT_USE_BUILTIN_OFFSETOF -D_WIN32 -D_WIN64",
+ );
+ if let Some(nd) = native_deps.as_deref() {
+ args = format!("-I{}/include {}", nd, args);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtask/src/config.rs` around lines 148 - 167, The Windows branch in
bindgen_extra_clang_args always emits "-I{}/include" because
native_deps.as_deref().unwrap_or("") yields an empty string, producing
"-I/include" when native_deps is None; update the Os::Windows arm to only
prepend the "-I.../include" token when native_deps.is_some() (or use if let
Some(native) = native_deps.as_deref() { ... } to build the args string) and
otherwise omit the "-I" flag entirely (or short-circuit the Windows branch if
the prep step hasn't run). Adjust construction of args in the Os::Windows branch
(reference bindgen_extra_clang_args, native_deps, find_windows_include_paths,
and Os::Windows) so the invalid "-I/include" is never produced.
Comment on lines
+438
to
+477
| #[test] | ||
| fn test_windows_dynamic_discovery_mocked() { | ||
| let temp = std::env::temp_dir().join("sd_test_dynamic_discovery"); | ||
| let _ = fs::remove_dir_all(&temp); | ||
| fs::create_dir_all(&temp).unwrap(); | ||
|
|
||
| // Mock MSVC | ||
| let msvc_root = temp.join("VS"); | ||
| let msvc_tools = msvc_root.join("VC").join("Tools").join("MSVC").join("14.38.33130").join("include"); | ||
| fs::create_dir_all(&msvc_tools).unwrap(); | ||
|
|
||
| // Mock SDK | ||
| let sdk_root = temp.join("Windows Kits").join("10").join("Include"); | ||
| let sdk_version = sdk_root.join("10.0.22621.0"); | ||
| fs::create_dir_all(sdk_version.join("ucrt")).unwrap(); | ||
| fs::create_dir_all(sdk_version.join("um")).unwrap(); | ||
| fs::create_dir_all(sdk_version.join("shared")).unwrap(); | ||
|
|
||
| let paths = find_windows_include_paths( | ||
| Some(msvc_root.to_str().unwrap()), | ||
| Some(sdk_root.to_str().unwrap()), | ||
| ).unwrap(); | ||
|
|
||
| assert_eq!(paths.len(), 4); | ||
| assert!(paths.iter().any(|p| p.contains("14.38.33130"))); | ||
| assert!(paths.iter().any(|p| p.contains("ucrt"))); | ||
|
|
||
| // Mock Libclang | ||
| let llvm_root = temp.join("LLVM").join("bin"); | ||
| fs::create_dir_all(&llvm_root).unwrap(); | ||
| fs::write(llvm_root.join("libclang.dll"), "").unwrap(); | ||
|
|
||
| let libclang = find_libclang_path( | ||
| None, | ||
| Some(vec![llvm_root.to_str().unwrap()]), | ||
| ); | ||
| assert!(libclang.is_some()); | ||
|
|
||
| let _ = fs::remove_dir_all(&temp); | ||
| } |
There was a problem hiding this comment.
Windows MSVC discovery test will fail on non-Windows CI runners.
find_windows_include_paths builds the MSVC subdirectory with Path::new(&install_path).join("VC\\Tools\\MSVC") (lines 238, 247). On Linux/macOS, Path::join treats \ as a literal character in the filename, so fs::read_dir looks for a single directory literally named VC\Tools\MSVC under the override root — it will never match the VC/Tools/MSVC/14.38.33130/include directory created by the test (line 446). The MSVC branch silently returns nothing and only the three SDK paths get pushed, so assert_eq!(paths.len(), 4) fails on every non-Windows runner.
Either gate the test with #[cfg(windows)] (and add a parallel Unix-safe variant if desired), or — better — fix the helper to use portable separators so it works uniformly:
🐛 Proposed fix in find_windows_include_paths
- let tools_path = Path::new(&install_path).join("VC\\Tools\\MSVC");
+ let tools_path = Path::new(&install_path).join("VC").join("Tools").join("MSVC");
if let Ok(entries) = fs::read_dir(tools_path) {
...
if let Some(latest) = get_latest_numeric_version(versions) {
paths.push(
Path::new(&install_path)
- .join("VC\\Tools\\MSVC")
+ .join("VC").join("Tools").join("MSVC")
.join(latest)
.join("include")
.to_string_lossy()
.to_string(),
);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtask/src/config.rs` around lines 438 - 477, The test
test_windows_dynamic_discovery_mocked fails on non-Windows because
find_windows_include_paths constructs MSVC subpaths using a hardcoded Windows
backslash string ("VC\\Tools\\MSVC") which Path::join treats literally on Unix;
update find_windows_include_paths to build the MSVC path portably (e.g., replace
join("VC\\Tools\\MSVC") with chained .join("VC").join("Tools").join("MSVC") or
otherwise use Path components or std::path::MAIN_SEPARATOR) so the mocked
directories created in the test are discovered on all platforms; modify the
codepaths that create msbuild/install_path joins (the code in
find_windows_include_paths) accordingly so the test
test_windows_dynamic_discovery_mocked passes cross-platform.
Comment on lines
+479
to
+498
| #[test] | ||
| fn test_android_discovery_mocked() { | ||
| let temp = std::env::temp_dir().join("sd_test_android_discovery"); | ||
| let _ = fs::remove_dir_all(&temp); | ||
| fs::create_dir_all(&temp).unwrap(); | ||
|
|
||
| // Mock NDK directory structure | ||
| let ndk_root = temp.join("ndk"); | ||
| fs::create_dir_all(ndk_root.join("25.1.8937393")).unwrap(); | ||
| fs::create_dir_all(ndk_root.join("25.2.9519653")).unwrap(); | ||
| fs::create_dir_all(ndk_root.join(".DS_Store")).unwrap(); | ||
|
|
||
| let ndk = find_android_ndk(Some(temp.to_str().unwrap())); | ||
| assert!(ndk.is_some(), "Should find NDK in mocked SDK root"); | ||
|
|
||
| let ndk_path = ndk.unwrap(); | ||
| assert!(ndk_path.contains("25.2.9519653"), "Should select the latest version: {}", ndk_path); | ||
|
|
||
| let _ = fs::remove_dir_all(&temp); | ||
| } |
There was a problem hiding this comment.
Android NDK test remains environment-dependent despite the mocked fixture.
find_android_ndk checks ANDROID_NDK_HOME and ANDROID_NDK before consulting sdk_root_override (lines 392–398). If the developer (or CI runner) has either variable exported, this test silently resolves to the host NDK and the assertion ndk_path.contains("25.2.9519653") will fail nondeterministically. This is the same class of issue flagged in the previous review round.
Either unset these variables for the duration of the test, or refactor the helper to accept an override that takes precedence:
🐛 Suggested fix
#[test]
fn test_android_discovery_mocked() {
+ // Ensure host env doesn't mask the mocked SDK root
+ std::env::remove_var("ANDROID_NDK_HOME");
+ std::env::remove_var("ANDROID_NDK");
let temp = std::env::temp_dir().join("sd_test_android_discovery");Note: std::env::remove_var affects the whole process, so if you keep this approach make sure no parallel tests read those vars. Alternatively, plumb an explicit ndk_override parameter through find_android_ndk and skip the env lookup when it's provided.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtask/src/config.rs` around lines 479 - 498, The test
test_android_discovery_mocked is brittle because find_android_ndk reads
ANDROID_NDK_HOME/ANDROID_NDK before sdk_root_override; modify the test or
function so the mocked SDK wins: either (A) in the test call
std::env::remove_var for "ANDROID_NDK_HOME" and "ANDROID_NDK" (and restore them
after) before invoking find_android_ndk, or (B) change find_android_ndk to
accept an explicit ndk_override parameter (or treat sdk_root_override as highest
precedence) so the supplied temp SDK root is used regardless of environment
variables; update test_android_discovery_mocked to use the new override path if
you choose the refactor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements dynamic path discovery for native build dependencies on Windows and Android.
Key changes:
Task File: .tasks/core/DEV-002-dynamic-native-build-discovery.md
Closes DEV-002