[codex] add flmexec runtime parameter#473
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a runtime parameter to script execution, allowing users to specify a particular Python version or shell type (e.g., zsh, bash). While this adds valuable flexibility, the changes in the Python script runner introduce a regression by removing the fallback chain that auto-detects the latest installed Python version or reads from the FLAME_PYTHON_VERSION_ENV environment variable. Feedback focuses on restoring this fallback behavior to preserve backward compatibility, updating tests accordingly, and optimizing the shell path validation to avoid unnecessary heap allocations.
| .or_insert_with(|| flame_home.to_string_lossy().to_string()); | ||
|
|
||
| let python_version = python_version(envs, &flame_home); | ||
| let python_version = python_version(runtime); |
There was a problem hiding this comment.
Pass envs and &flame_home to python_version to allow falling back to the propagated FLAME_PYTHON_VERSION_ENV environment variable and auto-detecting the latest installed Python version when runtime is not specified.
| let python_version = python_version(runtime); | |
| let python_version = python_version(envs, &flame_home, runtime); |
| fn python_version(runtime: Option<&str>) -> String { | ||
| runtime | ||
| .map(str::trim) | ||
| .filter(|version| !version.is_empty()) | ||
| .map(version_number) | ||
| .map(ToOwned::to_owned) | ||
| .or_else(|| latest_installed_python_version(flame_home)) | ||
| .unwrap_or_else(|| DEFAULT_PYTHON_VERSION.to_string()) | ||
| } | ||
|
|
||
| fn version_number(version: &str) -> &str { | ||
| version.strip_prefix("python").unwrap_or(version) | ||
| } |
There was a problem hiding this comment.
By only checking runtime and defaulting to DEFAULT_PYTHON_VERSION, the new implementation completely ignores the propagated FLAME_PYTHON_VERSION_ENV environment variable and removes the latest_installed_python_version auto-detection fallback. This is a regression for existing environments that rely on these fallbacks.
Restoring the full fallback chain (checking runtime first, then FLAME_PYTHON_VERSION_ENV, then auto-detecting the latest installed version, and finally defaulting to DEFAULT_PYTHON_VERSION) preserves backward compatibility and robust environment-based configuration.
fn python_version(envs: &HashMap<String, String>, flame_home: &Path, runtime: Option<&str>) -> String {
runtime
.map(str::trim)
.filter(|version| !version.is_empty())
.map(version_number)
.map(ToOwned::to_owned)
.or_else(|| {
envs.get(FLAME_PYTHON_VERSION_ENV)
.map(|version| version_number(version))
.filter(|version| !version.is_empty())
.map(ToOwned::to_owned)
})
.or_else(|| latest_installed_python_version(flame_home))
.unwrap_or_else(|| DEFAULT_PYTHON_VERSION.to_string())
}
fn version_number(version: &str) -> &str {
version.strip_prefix("python").unwrap_or(version)
}
fn latest_installed_python_version(flame_home: &Path) -> Option<String> {
let lib_path = flame_home.join("lib");
let mut versions = fs::read_dir(lib_path)
.ok()?
.flatten()
.filter_map(|entry| {
let path = entry.path();
if !path.is_dir() || !path.join("site-packages").is_dir() {
return None;
}
let name = entry.file_name();
let name = name.to_string_lossy();
name.strip_prefix("python")
.filter(|version| !version.is_empty())
.map(|version| version.to_string())
})
.collect::<Vec<_>>();
versions.sort_by_key(|version| minor_version(version));
versions.pop()
}
fn minor_version(version: &str) -> Vec<u32> {
version
.split('.')
.map(|part| part.parse::<u32>().unwrap_or(0))
.collect()
}| ( | ||
| FLAME_PYTHON_VERSION_ENV.to_string(), | ||
| "python3.11".to_string(), | ||
| ), | ||
| ]); |
| let supported_path = SUPPORTED_SHELLS | ||
| .iter() | ||
| .any(|shell| runtime == format!("/bin/{shell}") || runtime == format!("/usr/bin/{shell}")); |
There was a problem hiding this comment.
Summary
Verification