Conversation
mplemay
commented
Mar 31, 2026
mplemay
commented
- build: updated the python and rust dependencies
- build: set the python abi version
- build: updated the rust edition to 2024
- build: pinned the local python version to the minimum supported version
- build: improved the python version range
- fix: update pyo3 bindings and python harness
- refactor: moved python source into the src directory
piercefreeman
left a comment
piercefreeman
left a comment
There was a problem hiding this comment.
I'm noticing a lot more robustness was added here to the python path handling / generation... was this motivated by some wild cases that weren't covered in the previous code? Would want more test coverage that helps justify the additional complexity by reproducing the underlying bugs.
src/firehot/embedded/types.py
Outdated
| """ | ||
|
|
||
| from typing import TypedDict | ||
| from typing import Optional, TypedDict |
There was a problem hiding this comment.
nit: prefer | None for Optional types, unless targeting python versions that didn't yet have the pipe operator.
There was a problem hiding this comment.
Changed this. We now target Python 3.10+ for the abi.
| // If PYTHONPATH doesn't exist, create it with just the temp dir | ||
| env::set_var("PYTHONPATH", &container_path); | ||
| } | ||
| crate::python::add_python_path(PathBuf::from(&container_path)); |
There was a problem hiding this comment.
What was the motivation for this refactor? Easier testability?
There was a problem hiding this comment.
Main reason was to stop relying on a process-wide PYTHONPATH change in tests. The temp module path now stays scoped to the test, but child Python processes still get that path when they start.
| Ok(child) | ||
| } | ||
|
|
||
| fn build_pythonpath(project_name: &str, project_path: &str) -> Result<OsString> { |
There was a problem hiding this comment.
What class of issues does this address that weren't covered by the previous implementation? Can we add test cases for them?
There was a problem hiding this comment.
This covers two real cases. First, the test harness creates temp modules outside the environment project root, and child imports fail without that extra path. Second, Python gives Rust the package directory, so child imports also need the package parent path. I added direct Rust and Python tests for both cases.
2 participants
