Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a generalized placeholder-replacement mechanism and wires it into Tool so that command arguments/env values and runner config files (QEMU/U-Boot) can use consistent ${...} variables (including ${env:VAR}).
Changes:
- Added
replace_placeholdersutility and refactoredreplace_env_placeholdersto use it. - Added
Tool::replace_string/replace_path_variablesand updatedTool::commandto use the new substitution logic. - Updated QEMU/U-Boot config loading to parse TOML first, then substitute placeholders across string fields, with new unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ostool/src/utils.rs |
Adds generic placeholder engine and refactors env-only substitution to build on it; adds tests. |
ostool/src/tool.rs |
Centralizes variable substitution in Tool and applies it to command arg/env handling; adds tests for supported variables. |
ostool/src/run/uboot.rs |
Moves placeholder substitution to post-TOML-parse and applies it across U-Boot config string fields; adds tests. |
ostool/src/run/qemu.rs |
Applies placeholder substitution across QEMU config string fields and supports variable expansion in explicit config paths; adds tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let package_dir = self.package_root_for_variables()?; | ||
| let workspace_dir = self.workspace_dir.display().to_string(); | ||
| let package_dir = package_dir.display().to_string(); | ||
| let tmp_dir = std::env::temp_dir().display().to_string(); | ||
|
|
||
| replace_placeholders(input, |placeholder| { | ||
| let value = match placeholder { | ||
| "workspace" | "workspaceFolder" => Some(workspace_dir.clone()), | ||
| "package" => Some(package_dir.clone()), | ||
| "tmpDir" => Some(tmp_dir.clone()), | ||
| p if p.starts_with("env:") => Some(std::env::var(&p[4..]).unwrap_or_default()), |
There was a problem hiding this comment.
replace_string always calls package_root_for_variables() (which may run cargo metadata) even when the input has no ${package} placeholder. Since this is used by command() argument/env substitution, it can introduce a large and repeated performance cost. Consider making package-dir resolution lazy (only when resolving the package placeholder) and caching it within the closure for the duration of a single call.
| let package_dir = self.package_root_for_variables()?; | |
| let workspace_dir = self.workspace_dir.display().to_string(); | |
| let package_dir = package_dir.display().to_string(); | |
| let tmp_dir = std::env::temp_dir().display().to_string(); | |
| replace_placeholders(input, |placeholder| { | |
| let value = match placeholder { | |
| "workspace" | "workspaceFolder" => Some(workspace_dir.clone()), | |
| "package" => Some(package_dir.clone()), | |
| "tmpDir" => Some(tmp_dir.clone()), | |
| p if p.starts_with("env:") => Some(std::env::var(&p[4..]).unwrap_or_default()), | |
| let workspace_dir = self.workspace_dir.display().to_string(); | |
| let tmp_dir = std::env::temp_dir().display().to_string(); | |
| let mut package_dir: Option<anyhow::Result<String>> = None; | |
| replace_placeholders(input, |placeholder| { | |
| let value = match placeholder { | |
| "workspace" | "workspaceFolder" => Some(workspace_dir.clone()), | |
| "package" => { | |
| if package_dir.is_none() { | |
| package_dir = Some( | |
| self.package_root_for_variables() | |
| .map(|p| p.display().to_string()), | |
| ); | |
| } | |
| Some(package_dir.as_ref().unwrap().as_ref()?.clone()) | |
| } | |
| "tmpDir" => Some(tmp_dir.clone()), | |
| p if p.starts_with("env:") => { | |
| Some(std::env::var(&p[4..]).unwrap_or_default()) | |
| } |
| pub fn replace_value<S>(&self, value: S) -> String | ||
| where | ||
| S: AsRef<OsStr>, | ||
| { | ||
| self.replace_string(&value.as_ref().to_string_lossy()) | ||
| .unwrap_or_else(|_| value.as_ref().to_string_lossy().into_owned()) | ||
| } | ||
|
|
There was a problem hiding this comment.
replace_value silently falls back to the original string on any error from replace_string. This can hide real configuration problems (e.g., failure to resolve ${package} due to metadata errors) and lead to commands running with unreplaced placeholders. Consider at least logging a warning/debug message on failure, or providing a try_replace_value API returning Result for call sites that can/should fail fast.
| pub fn replace_value<S>(&self, value: S) -> String | |
| where | |
| S: AsRef<OsStr>, | |
| { | |
| self.replace_string(&value.as_ref().to_string_lossy()) | |
| .unwrap_or_else(|_| value.as_ref().to_string_lossy().into_owned()) | |
| } | |
| /// Try to replace placeholders in a value, returning an error on failure. | |
| pub fn try_replace_value<S>(&self, value: S) -> anyhow::Result<String> | |
| where | |
| S: AsRef<OsStr>, | |
| { | |
| self.replace_string(&value.as_ref().to_string_lossy()) | |
| } | |
| /// Replace placeholders in a value, logging a warning on failure and | |
| /// falling back to the original string. | |
| pub fn replace_value<S>(&self, value: S) -> String | |
| where | |
| S: AsRef<OsStr>, | |
| { | |
| let original = value.as_ref().to_string_lossy(); | |
| match self.replace_string(&original) { | |
| Ok(replaced) => replaced, | |
| Err(err) => { | |
| eprintln!( | |
| "Warning: failed to replace placeholders in value {:?}: {:#}. Falling back to original.", | |
| original, | |
| err | |
| ); | |
| original.into_owned() | |
| } | |
| } | |
| } |
| /// Creates a new command builder for the given program. | ||
| pub fn command(&self, program: &str) -> crate::utils::Command { | ||
| let workspace_dir = self.workspace_dir.clone(); | ||
| let mut command = crate::utils::Command::new(program, &self.manifest_dir, move |s| { | ||
| let raw = s.to_string_lossy(); | ||
| raw.replace( | ||
| "${workspaceFolder}", | ||
| format!("{}", workspace_dir.display()).as_ref(), | ||
| ) | ||
| }); | ||
| let tool = self.clone(); | ||
| let mut command = | ||
| crate::utils::Command::new(program, &self.manifest_dir, move |s| tool.replace_value(s)); | ||
| command.env("WORKSPACE_FOLDER", self.workspace_dir.display().to_string()); |
There was a problem hiding this comment.
Tool::command now performs placeholder expansion via replace_value, which supports ${env:VAR}. Since Command::run() prints the full expanded command line, this can inadvertently expose secrets if users pass sensitive env vars via arguments. Consider either documenting this clearly, masking env-derived values in command printing, or making env substitution opt-in for arguments.
| let mut config: QemuConfig = toml::from_str(&content).with_context(|| { | ||
| format!("failed to parse QEMU config: {}", config_path.display()) | ||
| })?; | ||
| config.replace_strings(tool)?; | ||
| config.normalize(&format!("QEMU config {}", config_path.display()))?; |
There was a problem hiding this comment.
config.replace_strings(tool)? is only called when a config file is successfully loaded. If the config file is missing and we build a default config from overrides (including success_regex/fail_regex), any ${...} placeholders in those override strings will remain unresolved and can break output matching. Consider also applying replace_strings in the NotFound/default-config branch before normalize()/writing.
No description provided.