Add cross-process cache for Python dependency checks.#438
Conversation
📝 WalkthroughWalkthroughAdded cross-process dependency caching and file-lock coordination to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Proc as Process (requesting)
participant LockFS as File Lock (.cache/.lock)
participant CacheFS as Cache State (.cache/state.json)
participant Network as Network/Index
participant Penv as Python virtualenv
Proc->>LockFS: try acquire lock (timeout)
alt lock acquired
LockFS-->>Proc: lock granted
Proc->>CacheFS: read fingerprint
alt fingerprint valid
CacheFS-->>Proc: valid -> skip install
else fingerprint missing/invalid
Proc->>Network: check has_internet_connection
alt network available
Network-->>Proc: reachable
Proc->>Penv: install python_deps (uv etc.)
Penv-->>Proc: install results
Proc->>CacheFS: write new fingerprint/state
else network unavailable
Network-->>Proc: unreachable
Proc->>CacheFS: if valid fallback else error and exit
end
end
Proc->>LockFS: release lock
else lock timeout
LockFS-->>Proc: cannot acquire
Proc->>CacheFS: if valid use cache else error and exit
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/penv_setup.py`:
- Around line 122-124: The lock wait loop uses lock_file and timeout_sec and
currently allows code later to proceed to the dependency-installation block even
after a timeout; change this to fail-fast: ensure the TimeoutError raised on
lock acquisition is not caught and ignored or followed by an unsynchronized
fallback, and instead propagate the TimeoutError out of the penv setup function
so no installation code runs; remove or alter any try/except or conditional that
swallows TimeoutError and prevents early exit, and ensure the code path that
performs dependency installation (the block that installs packages into the
penv) only runs when the lock was successfully acquired.
- Around line 83-90: The current _deps_fingerprint uses only python_deps,
sys.version_info and penv_python path so a recreated virtualenv at the same path
can produce the same cache key; update _deps_fingerprint to also include a penv
instance marker (readable unique id) from the virtualenv itself (e.g., a marker
file or a value from pyvenv.cfg inside the venv directory) or, if missing, the
venv directory mtime/UUID created at venv creation; include that marker in the
payload alongside python_deps and sys.version_info so cache misses on venv
recreation, and ensure the same change is applied to the call site referenced
around the logic at the lines that check/cache (the branch around lines 633-635)
so the new fingerprint is used when deciding whether to skip installs.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
builder/penv_setup.py (2)
662-667: Minor TOCTOU: fingerprint computed before lock acquisition.The fingerprint is computed at line 663 before the lock is acquired at line 666. If another process recreates the penv directory between these two points, the fingerprint (which includes
pyvenv.cfgstats) would be stale.This is a narrow race window and the secondary validation at line 679 provides a safety net, so this is unlikely to cause practical issues. However, for maximum correctness, consider recomputing the fingerprint after acquiring the lock.
♻️ Suggested fix
state_file, lock_file = _deps_state_paths(platformio_dir) - fingerprint = _deps_fingerprint(penv_python) try: lock_wait_started_at = time.monotonic() lock_fd = _acquire_file_lock(lock_file) print(f"[penv] Dependency lock acquired in {time.monotonic() - lock_wait_started_at:.2f}s") + fingerprint = _deps_fingerprint(penv_python) except TimeoutError as e: lock_fd = None + fingerprint = _deps_fingerprint(penv_python) if _is_penv_dependency_cache_valid(state_file, fingerprint, penv_python):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 662 - 667, The fingerprint is computed by _deps_fingerprint(penv_python) before acquiring the dependency lock via _acquire_file_lock(lock_file), creating a TOCTOU window; after obtaining the lock (after lock_fd is set) recompute the fingerprint (e.g., call _deps_fingerprint(penv_python) again and overwrite the earlier value) and then proceed with the existing validation logic that uses fingerprint and state_file so the fingerprint reflects the locked state of the penv directory.
133-136: Consider chaining theTimeoutErrorfor better diagnostics.Per Python best practices, exceptions raised within an
exceptclause should be chained to preserve the original context. This helps with debugging.♻️ Suggested fix
except FileExistsError: if time.monotonic() - started >= timeout_sec: - raise TimeoutError(f"Timeout waiting lock: {lock_file}") + raise TimeoutError(f"Timeout waiting lock: {lock_file}") from None time.sleep(0.1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 133 - 136, The except block currently swallows the original FileExistsError; change the handler to capture the original exception (e.g., "except FileExistsError as e:") and when raising the TimeoutError include the original as the cause using exception chaining (raise TimeoutError(f"Timeout waiting lock: {lock_file}") from e) so the original FileExistsError context (including where it occurred) is preserved; this touches the except FileExistsError block that references started, timeout_sec, lock_file and time.monotonic().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/penv_setup.py`:
- Around line 163-178: The import check currently only validates a small
hardcoded set of packages in the subprocess.run block and can miss missing deps;
update the cache validation to programmatically import all packages from the
python_deps list (or at least include the missing critical packages like
"cryptography", "intelhex", "pyelftools") instead of the fixed "certifi,
platformio, rich, yaml" string so the subprocess invocation using penv_python
verifies every dependency; modify the code that builds the "-c" import string
(or loop over python_deps) and keep the same exception handling around
subprocess.run to return False on failure.
---
Nitpick comments:
In `@builder/penv_setup.py`:
- Around line 662-667: The fingerprint is computed by
_deps_fingerprint(penv_python) before acquiring the dependency lock via
_acquire_file_lock(lock_file), creating a TOCTOU window; after obtaining the
lock (after lock_fd is set) recompute the fingerprint (e.g., call
_deps_fingerprint(penv_python) again and overwrite the earlier value) and then
proceed with the existing validation logic that uses fingerprint and state_file
so the fingerprint reflects the locked state of the penv directory.
- Around line 133-136: The except block currently swallows the original
FileExistsError; change the handler to capture the original exception (e.g.,
"except FileExistsError as e:") and when raising the TimeoutError include the
original as the cause using exception chaining (raise TimeoutError(f"Timeout
waiting lock: {lock_file}") from e) so the original FileExistsError context
(including where it occurred) is preserved; this touches the except
FileExistsError block that references started, timeout_sec, lock_file and
time.monotonic().
| try: | ||
| subprocess.run( | ||
| [ | ||
| penv_python, | ||
| "-c", | ||
| "import certifi, platformio, rich, yaml", | ||
| ], | ||
| check=True, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| timeout=10, | ||
| ) | ||
| except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError): | ||
| print("[penv] Dependency cache invalidated: required Python packages are missing from penv") | ||
| return False | ||
|
|
There was a problem hiding this comment.
Cache validation only verifies a subset of dependencies.
The import check validates certifi, platformio, rich, yaml, but python_deps includes many more critical packages (cryptography, intelhex, pyelftools, etc.). If one of the unchecked packages is missing or corrupted, the cache would still be considered valid, potentially causing build failures later.
Consider either expanding the import check to include more critical packages, or documenting why this subset is sufficient.
💡 Suggested improvement
subprocess.run(
[
penv_python,
"-c",
- "import certifi, platformio, rich, yaml",
+ "import certifi, platformio, rich, yaml, cryptography, intelhex",
],
check=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
timeout=10,
)🧰 Tools
🪛 Ruff (0.15.6)
[error] 164-164: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builder/penv_setup.py` around lines 163 - 178, The import check currently
only validates a small hardcoded set of packages in the subprocess.run block and
can miss missing deps; update the cache validation to programmatically import
all packages from the python_deps list (or at least include the missing critical
packages like "cryptography", "intelhex", "pyelftools") instead of the fixed
"certifi, platformio, rich, yaml" string so the subprocess invocation using
penv_python verifies every dependency; modify the code that builds the "-c"
import string (or loop over python_deps) and keep the same exception handling
around subprocess.run to return False on failure.
|
No, will not add this complex logic which will introduce for sure problems. |
|
Have a better way to solve this problem? |
|
The wait time before compilation is really frustrating, especially when conducting hardware testing. |
|
Setting the offline flag does solve. pioarduino is Open Source. Fork and implement anything you want. |
All right, then. What kind of computer are you using? I don’t think it has anything to do with the specs, though. That’s really strange. |
|
pioarduino is tested and working with MacOS, Linux (different ones) and Windows. Developing on MacOS. |
I'm also on macOS. Could you tell me which version of UV you're using? |
|
|
2 participants
Description:
Related issue (if applicable): fixes #423
Checklist:
Summary by CodeRabbit
Performance
Improvements