fix: recreate penv on Python version mismatch, resolve site-packages dynamically#459
Conversation
…dynamically When the host Python interpreter is upgraded (e.g. Homebrew 3.13→3.14) the existing penv still contains packages compiled for the old ABI. setup_python_paths() constructed the site-packages path from sys.version_info of the *running* process, which pointed to a directory that did not exist inside the penv, causing ModuleNotFoundError for littlefs, fatfs, and other C-extension packages. This commit: 1. Adds _get_penv_python_version() to detect the actual Python version inside an existing penv by inspecting lib/pythonX.Y/ directories (POSIX) or querying the penv Python executable (Windows). 2. Adds _penv_version_matches() to compare penv vs running Python. 3. Modifies setup_pipenv_in_package() and _setup_pipenv_minimal() to detect version mismatches and automatically recreate the penv with the correct Python version. 4. Replaces the hardcoded sys.version_info path construction in setup_python_paths() with _get_penv_site_packages() that dynamically discovers the actual site-packages directory. 5. Filters conflicting system site-packages from sys.path and inserts the penv's site-packages at index 0 to ensure penv packages always take precedence. Fixes: esphome/esphome#15619 Fixes: esphome/esphome#15519 Amp-Thread-ID: https://ampcode.com/threads/T-019d7790-e061-71cf-9dba-d9da022b203b Co-authored-by: Amp <amp@ampcode.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 full review and analyze if the PR solves the esphome issues: esphome/esphome#15619 |
🧩 Analysis chainNow let me look at the actual PR code to do a thorough review. 🏁 Script executed: Length of output: 10975 🏁 Script executed: Length of output: 826 🏁 Script executed: Length of output: 8644 Now let me look at the esphome issues referenced.
Analysis: Does This PR Fix the ESPHome Issues?ESPHome
|
| Issue | Status |
|---|---|
esphome #15619 ModuleNotFoundError: littlefs |
✅ Fixed by Layer 1 (penv recreate) + Layer 2 (dynamic path) |
esphome #15519 ImportError: lfs partially initialized |
✅ Fixed by Layer 1 (ABI-correct penv recreate) |
| Lexicographic vs version sort | |
import shutil inside function |
|
site.addsitedir duplicate path |
|
Windows: subprocess vs pyvenv.cfg |
|
Double call to _get_penv_python_version |
The PR correctly addresses both reported ESPHome issues. The core logic is sound. The issues above are minor quality/robustness improvements.
🧠 Learnings used
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T14:15:00.476Z
Learning: In the pioarduino/platform-espressif32 project, espidf.py creates an extra/separate virtual environment from the penv setup, so cryptography version differences between penv_setup.py and espidf.py do not cause conflicts since they operate in isolated environments.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 448
File: builder/relinker/configuration.py:145-150
Timestamp: 2026-03-29T17:40:06.024Z
Learning: In pioarduino/platform-espressif32, when the relinker (builder/relinker/configuration.py, builder/relinker/relinker.py) runs inside the PlatformIO/SCons build system, both BUILD_DIR and IDF_PATH are always set in the process environment by PlatformIO. There is no need to defensively guard against missing BUILD_DIR or IDF_PATH in the relinker scripts when used in the PlatformIO context. The existing `build_dir or os.environ.get('BUILD_DIR')` fallback in paths_c.__init__ is sufficient.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/penv_setup.py:612-647
Timestamp: 2025-09-23T16:55:39.788Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers no fallback mechanisms in penv setup functions. The penv virtual environment setup must work properly and should crash immediately rather than falling back to host Python environments, to avoid using "unknown env" configurations.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T14:15:00.476Z
Learning: In the pioarduino/platform-espressif32 project, espidf.py creates and manages its own separate IDF virtual environment (accessed via get_python_exe() from get_idf_venv_dir()). This IDF venv is completely isolated from the penv setup, so different cryptography versions between penv_setup.py and espidf.py don't cause conflicts as they exist in separate virtual environments.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-22T10:59:14.425Z
Learning: In the pioarduino/platform-espressif32 project, the penv setup ensures that uv gets installed in the venv (penv), so additional safeguards for uv availability are not needed.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
✅ Actions performed
Full review triggered.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
builder/penv_setup.py (3)
230-241: Redundant_get_penv_python_version()calls on Windows.
_penv_version_matches()(line 232) already calls_get_penv_python_version()internally. The subsequent call at line 233 duplicates this work—on Windows, this spawns a second subprocess. Since this only runs on version mismatch (uncommon), the impact is low.Potential optimization to avoid redundant calls
# Recreate penv when Python version changed (e.g. Homebrew upgraded 3.13→3.14) penv_python_path = get_executable_path(penv_dir, "python") - if os.path.isfile(penv_python_path) and not _penv_version_matches(penv_dir): - penv_ver = _get_penv_python_version(penv_dir) + penv_ver = _get_penv_python_version(penv_dir) if os.path.isfile(penv_python_path) else None + current_ver = (sys.version_info.major, sys.version_info.minor) + if penv_ver is not None and penv_ver != current_ver: - current_ver = (sys.version_info.major, sys.version_info.minor) print( f"Python version mismatch: penv has {penv_ver[0]}.{penv_ver[1]}, " f"current interpreter is {current_ver[0]}.{current_ver[1]}. " f"Recreating penv..." ) import shutil shutil.rmtree(penv_dir, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 230 - 241, Avoid the duplicate subprocess call to _get_penv_python_version by calling it once and using its result for the mismatch check and print: fetch penv_ver = _get_penv_python_version(penv_dir) (using get_executable_path/penv_python_path as needed), compute current_ver from sys.version_info, compare penv_ver to current_ver instead of calling _penv_version_matches again, and only then call shutil.rmtree(penv_dir, ignore_errors=True) when they differ; alternatively, change _penv_version_matches to return the detected penv version so the caller can reuse it instead of invoking _get_penv_python_version twice.
319-323:site.addsitedir()may alter path ordering.
site.addsitedir()processes.pthfiles and can append additional paths tosys.path, potentially placing them after the penvsite-packagesthat was inserted at index 0. If any.pthfiles in the penv add paths that should take precedence, they might end up in the wrong order.In practice, this should be fine since conflicting system
site-packagesare already filtered out (lines 313-317). Consider re-ensuring penv remains first if issues arise:Optional: Re-ensure penv is first after addsitedir
site.addsitedir(site_packages) + + # Ensure penv site-packages remains first after addsitedir processing + if site_packages in sys.path: + sys.path.remove(site_packages) + sys.path.insert(0, site_packages)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 319 - 323, The current sequence (inserting site_packages at sys.path[0] then calling site.addsitedir(site_packages)) can let .pth processing append new paths after your penv; to fix, ensure the penv stays first by either calling site.addsitedir(site_packages) first and then inserting site_packages at sys.path[0], or by re-checking and moving site_packages back to index 0 after site.addsitedir(site_packages) so that sys.path ordering guarantees penv site-packages precedence (referencing site.addsitedir, sys.path, and site_packages).
162-169: Lexicographic sorting may mis-order version directories.
sorted(..., reverse=True)orders strings lexicographically, causingpython3.9to sort beforepython3.10(since'1' < '9'). If a penv ever contains multiplepythonX.Ydirectories, this would select the wrong one.In practice, a typical penv has only one
pythonX.Ydirectory, so this is unlikely to cause issues. Consider using numeric sorting if robustness is desired:Suggested numeric sort
- for entry in sorted(lib_dir.iterdir(), reverse=True): + def _version_key(e): + if e.name.startswith("python"): + try: + parts = e.name[len("python"):].split(".") + return (int(parts[0]), int(parts[1])) + except (ValueError, IndexError): + return (0, 0) + return (0, 0) + + for entry in sorted(lib_dir.iterdir(), key=_version_key, reverse=True):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 162 - 169, The loop relies on lexicographic sorting of lib_dir.iterdir(), which can mis-order names like "python3.9" vs "python3.10"; instead, collect candidate entries that match the "pythonX.Y" pattern (use entry.name and ver_str), parse major and minor into ints (as you already do into major, minor), sort candidates numerically by (major, minor) in descending order, then pick the first candidate and return (int(major), int(minor)); update the code around entry.name, ver_str, major, minor to build and sort a list of (major, minor, entry) tuples rather than relying on sorted(..., reverse=True).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builder/penv_setup.py`:
- Around line 230-241: Avoid the duplicate subprocess call to
_get_penv_python_version by calling it once and using its result for the
mismatch check and print: fetch penv_ver = _get_penv_python_version(penv_dir)
(using get_executable_path/penv_python_path as needed), compute current_ver from
sys.version_info, compare penv_ver to current_ver instead of calling
_penv_version_matches again, and only then call shutil.rmtree(penv_dir,
ignore_errors=True) when they differ; alternatively, change
_penv_version_matches to return the detected penv version so the caller can
reuse it instead of invoking _get_penv_python_version twice.
- Around line 319-323: The current sequence (inserting site_packages at
sys.path[0] then calling site.addsitedir(site_packages)) can let .pth processing
append new paths after your penv; to fix, ensure the penv stays first by either
calling site.addsitedir(site_packages) first and then inserting site_packages at
sys.path[0], or by re-checking and moving site_packages back to index 0 after
site.addsitedir(site_packages) so that sys.path ordering guarantees penv
site-packages precedence (referencing site.addsitedir, sys.path, and
site_packages).
- Around line 162-169: The loop relies on lexicographic sorting of
lib_dir.iterdir(), which can mis-order names like "python3.9" vs "python3.10";
instead, collect candidate entries that match the "pythonX.Y" pattern (use
entry.name and ver_str), parse major and minor into ints (as you already do into
major, minor), sort candidates numerically by (major, minor) in descending
order, then pick the first candidate and return (int(major), int(minor)); update
the code around entry.name, ver_str, major, minor to build and sort a list of
(major, minor, entry) tuples rather than relying on sorted(..., reverse=True).
…process Parse the 'version' key from pyvenv.cfg which is always written by both 'python -m venv' and 'uv venv'. This is faster, more reliable, and works even when the penv Python executable is corrupted or unresponsive. Falls back to inspecting lib/pythonX.Y/ directories on POSIX if pyvenv.cfg is missing. Amp-Thread-ID: https://ampcode.com/threads/T-019d7790-e061-71cf-9dba-d9da022b203b Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai analyze the changes in commit ef3fb76 if the fix the minor issues you listed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5784
Detailed breakdownIssue 1 — Lexicographic sort (partially fixed)
Issue 2 — Issue 3 — if sys.path[0] != site_packages:
sys.path.remove(site_packages)
sys.path.insert(0, site_packages)This guarantees penv stays first even if Issue 4 — Windows subprocess (fixed) Issue 5 — Double call to Summary: 4 out of 5 issues are fully resolved. The one remaining gap (lexicographic sort in the POSIX fallback of 🧠 Learnings used |
1 participant
Problem
When the host Python interpreter is upgraded (e.g. Homebrew
3.13→3.14, or ESPHome ships with a different Python than the one that created the penv), the existing~/.platformio/penvstill contains packages compiled for the old ABI.setup_python_paths()constructed the site-packages path fromsys.version_infoof the running process:This pointed to a directory that did not exist inside the penv, causing:
ModuleNotFoundError: No module named 'littlefs'(esphome 2026.2.4+ "ModuleNotFoundError: No module named 'littlefs':" esphome/esphome#15619)ImportError: cannot import name 'lfs' from partially initialized module 'littlefs'(ImportError: cannot import name 'lfs' from partially initialized module 'littlefs' esphome/esphome#15519)Solution
This PR implements a two-layer defense:
1. Auto-recreate penv on Python version mismatch
Both
setup_pipenv_in_package()and_setup_pipenv_minimal()now detect when the existing penv was created with a different Python major.minor version and automatically recreate it:2. Dynamically discover site-packages (defense in depth)
setup_python_paths()no longer hardcodes the path fromsys.version_info. Instead,_get_penv_site_packages()inspects the actual penv directory structure to find the real site-packages path. Additionally, conflicting system site-packages are filtered fromsys.pathand the penv site-packages is inserted at index 0.New helper functions
_get_penv_python_version()lib/pythonX.Y/, Windows: query penv Python executable)_penv_version_matches()_get_penv_site_packages()Testing
python3 -c "import py_compile; py_compile.compile('builder/penv_setup.py', doraise=True)"lib/pythonX.Y/) and Windows (Lib/) layoutsFixes: esphome/esphome#15619
Fixes: esphome/esphome#15519
Supersedes: #458
Summary by CodeRabbit