simics-python-utils: fix Simics 7 support for Windows#93
Conversation
4de23d0 to
d603f2e
Compare
d603f2e to
4fb712e
Compare
There was a problem hiding this comment.
Pull request overview
Updates the Simics Python discovery/linking flow (especially on Windows and newer Simics layouts) so builds can reliably locate libpython and its import library, and expands CI to exercise both Simics 6 and 7 package sets.
Changes:
- Adjust Windows libpython discovery to use
win64/lib/python3.X/and add animport_lib_dirto supportpython3.lib. - Update macro/build utilities to consume the unified
simics-python-utilsdiscovery results. - Expand CI to run Linux/Windows builds against Simics 6 and Simics 7 package bundles via a matrix.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| simics-python-utils/src/lib.rs | Documentation wording update (“traditional” → “bundled”). |
| simics-python-utils/src/environment.rs | Adds import_lib_dir, renames package source, and extends validation/display. |
| simics-python-utils/src/discovery.rs | Updates platform config and directory resolution for Windows; adds import-lib dir discovery; test updates. |
| simics-macro/src/interface/mod.rs | Uses discovered import-lib location for Windows linking and adds -L for it. |
| simics-build-utils/src/lib.rs | Replaces ad-hoc Windows python DLL discovery with unified discovery. |
| .github/workflows/ci.yml | Adds Simics 6/7 matrix for Linux and Windows builds; updates cache keys and artifact names. |
Comments suppressed due to low confidence (1)
simics-python-utils/src/environment.rs:36
- Adding a new
pubfield (import_lib_dir) to the publicPythonEnvironmentstruct is a breaking change for downstream users constructing/destructuring via struct literals or pattern matching. If this crate must remain semver-compatible, consider making the struct non-exhaustive from the start (in a major release) and/or exposing this via a getter while keeping fields private in a future major bump; otherwise, ensure the version/changelog reflects the break.
/// Complete Python environment information for Simics
#[derive(Debug, Clone)]
pub struct PythonEnvironment {
/// Path to mini-python executable
pub mini_python: PathBuf,
/// Path to Python include directory (contains python3.X subdirectory)
pub include_dir: PathBuf,
/// Include flag for C compilation (e.g., "-I/path/to/include/python3.9")
pub include_flag: String,
/// Directory containing libpython*.so files
pub lib_dir: PathBuf,
/// Full path to the specific libpython*.so file
pub lib_path: PathBuf,
/// Directory containing python3.lib import library (Windows).
/// On Unix, this mirrors `lib_dir` and is unused.
pub import_lib_dir: PathBuf,
/// Parsed Python version information
pub version: PythonVersion,
/// Source package where Python was found
pub package_source: PackageSource,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try bundled paths first (Simics base package 1000) | ||
| let bundled_err = match try_bundled_paths(simics_base) { | ||
| Ok(env) => return Ok(env.with_source(PackageSource::Bundled)), | ||
| Err(err) => err.context("Bundled discovery failed"), | ||
| }; | ||
|
|
||
| // Try dynamic discovery of separate Python package (Simics 1033) | ||
| let dynamic_err = match try_dynamic_python_package_discovery(simics_base) { | ||
| // Try separate Python package discovery (Simics 1033) | ||
| let separate_err = match try_separate_python_package_discovery(simics_base) { | ||
| Ok(env) => return Ok(env.with_source(PackageSource::SeparatePackage)), | ||
| Err(err) => err.context("Dynamic discovery failed"), | ||
| Err(err) => err.context("Separate package discovery failed"), |
There was a problem hiding this comment.
discover_python_environment_from_base calls .context(...), but this file does not import the anyhow::Context trait, so this will not compile. Add use anyhow::Context; (or import it in the existing use anyhow::{...} list).
| /// Found in traditional Simics base package (1000) | ||
| Traditional, | ||
| /// Found in bundled Simics base package (1000) | ||
| Bundled, |
There was a problem hiding this comment.
Renaming the public enum variant PackageSource::Traditional to Bundled is a breaking change for downstream crates (this crate is publish = true). Consider keeping Traditional as a deprecated alias/variant (mapping it to the same meaning), or bumping the crate’s semver major version and documenting the break in the changelog/release notes.
| Bundled, | |
| Bundled, | |
| /// Legacy name for [`PackageSource::Bundled`], kept for backward compatibility. | |
| /// | |
| /// This variant is semantically equivalent to [`PackageSource::Bundled`] | |
| /// and may be removed in a future major release. | |
| #[deprecated(note = "Use PackageSource::Bundled instead")] | |
| Traditional, |
| "Python import library path is not a directory: {}", | ||
| self.import_lib_dir.display() | ||
| )); | ||
| } |
There was a problem hiding this comment.
On Windows, validate() checks that import_lib_dir exists/is a directory, but it never checks that the import library file (python3.lib) actually exists. Since callers use import_lib_path() for linking, add an explicit python3.lib existence (and is_file) check here so discovery fails with a clear error when the import lib is missing.
| } | |
| } | |
| let import_lib_path = self.import_lib_dir.join("python3.lib"); | |
| if !import_lib_path.exists() { | |
| return Err(anyhow!( | |
| "Python import library file not found: {}", | |
| import_lib_path.display() | |
| )); | |
| } | |
| if !import_lib_path.is_file() { | |
| return Err(anyhow!( | |
| "Python import library path is not a file: {}", | |
| import_lib_path.display() | |
| )); | |
| } |
| @@ -299,15 +309,18 @@ fn discover_from_base_path(base_path: PathBuf) -> Result<PythonEnvironment> { | |||
| // Unix: SIMICS_BASE/HOST_DIRNAME/sys/lib/libpython3.X.so.Y.Z | |||
| // Windows: SIMICS_BASE/HOST_DIRNAME/bin/python3.X.dll | |||
There was a problem hiding this comment.
The comment describing the Windows libpython location is now out of date. This code discovers the Windows DLL under win64/lib/python3.X/ (per PLATFORM_CONFIG), but the comment still says win64/bin/python3.X.dll; please update the comment to match the new layout to avoid confusion.
| // Windows: SIMICS_BASE/HOST_DIRNAME/bin/python3.X.dll | |
| // Windows: SIMICS_BASE/HOST_DIRNAME/lib/python3.X/python3.X.dll |
| /// Find the directory containing the python3.lib import library (Windows). | ||
| /// Tries `bin/py3/` first (separate package layout in Simics 7.70.0+), | ||
| /// then falls back to `bin/` (bundled layout). | ||
| /// On Unix this returns `lib_dir` equivalent (unused in practice). |
There was a problem hiding this comment.
find_import_lib_dir’s doc comment says that on Unix it returns a lib_dir equivalent, but the implementation always returns a bin/... path. Please adjust the comment (or the implementation) so they match; otherwise it’s misleading when debugging on non-Windows platforms.
| /// On Unix this returns `lib_dir` equivalent (unused in practice). | |
| /// On non-Windows platforms this still returns a `bin/...` directory but is effectively unused. |
51b0d25 to
b5108ce
Compare
fixes #94