Repository navigation
Conversation
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
benderl
force-pushed
the
feature-python-compatibility
branch
from
November 12, 2026 06:22
ca4b3f6 to
3449924
Compare
benderl
marked this pull request as ready for review
November 12, 2026 10:45
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/github-actions-python.yml" line_range="13-14" />
<code_context>
+ strategy:
+ fail-fast: false
+ matrix:
+ raspberry-pi-os-release: ["Bullseye", "Bookworm", "Trixie"]
+ include:
+ - raspberry-pi-os-release: "Bullseye"
+ glibc: "2_31"
</code_context>
<issue_to_address>
**Bookworm compatibility jobs fail**
When the workflow runs its Bookworm matrix row, `matrix.include` has no Bookworm entry, so `actions/setup-python` receives an empty `matrix.python-version` and the ARM-wheel step gets empty `matrix.glibc` and `matrix.arm-wheel-python-version`; the job fails before compatibility checks.
Add a Bookworm entry to `matrix.include` with its `glibc`, `python-version`, and `arm-wheel-python-version` values.
Also at `.github/workflows/github-actions-python.yml:15-22`.
</issue_to_address>
### Comment 2
<location path="runs/run_python.sh" line_range="17" />
<code_context>
+ exec 8<"$venv/.openwb-lock"
+ flock -s 8
+ exec 9<&-
+ exec "$venv/bin/python" "$@"
+ ;;
+ esac
</code_context>
<issue_to_address>
**Integration helpers miss Python requirements**
When a smart-home integration invokes a child script on a fresh installation, `run_python.sh` leaves `PATH` unchanged, so `Sbase.callpro` resolves `python3` to the system interpreter instead of the VEnv interpreter; that interpreter lacks the installed requirements, and integrations fail or return missing or stale results.
Prepend the VEnv's `bin` directory to `PATH` when launching the service.
Also at `runs/setup_python_venv.sh:112`, `openwb-install.sh:122`, `runs/atreboot.sh:320`.
</issue_to_address>
### Comment 3
<location path="runs/install_packages.sh" line_range="57-58" />
<code_context>
-sudo -u "$OPENWB_USER" pip install -r "${OPENWBBASEDIR}/requirements.txt"
+python_build_packages=()
+python_binary_packages=()
+case $(dpkg --print-architecture) in
+ arm64) python_binary_packages=(python3-evdev) ;;
+ arm*) ;;
+ *) python_build_packages=(gcc python3-dev) ;;
</code_context>
<issue_to_address>
**Older ARM64 installs cannot bootstrap**
When a fresh ARM64 installation uses Bullseye or Bookworm repositories that provide an evdev version older than 1.9.1, the package installers select the distribution's unversioned `python3-evdev` on every ARM64 system, but `setup_python_venv.sh` requires its installed metadata to be exactly `evdev-1.9.1.dist-info`. On ARM64 Bullseye or Bookworm systems whose repository package is older, setup exits and a fresh installation fails.
Restrict the Debian-package copy path to distributions that provide evdev 1.9.1, and use a compatible installation path for other ARM64 releases.
Also at `openwb-install.sh:124-125`, `runs/setup_python_venv.sh:100-105`.
</issue_to_address>
### Comment 4
<location path="runs/setup_python_venv.sh" line_range="103-107" />
<code_context>
+ arm64)
+ evdev_source=${OPENWB_EVDEV_SOURCE:-/usr/lib/python3/dist-packages}
+ site_packages=$("$new_env/bin/python" -c 'import sysconfig; print(sysconfig.get_path("purelib"))')
+ if [[ ! -d "$evdev_source/evdev" || ! -d "$evdev_source/evdev-1.9.1.dist-info" ]]; then
+ echo "Debian python3-evdev 1.9.1 is required on ARM64" >&2
+ exit 1
+ fi
+ cp -a "$evdev_source/evdev" "$evdev_source/evdev-1.9.1.dist-info" "$site_packages/"
+ ;;
+ arm*) ;;
</code_context>
<issue_to_address>
**ARM64 evdev bootstrap rejects Debian metadata**
When the installed Debian `python3-evdev` package uses `.egg-info` metadata rather than `.dist-info`, `setup_python_venv.sh` requires and copies only `evdev-1.9.1.dist-info`; when Debian’s `python3-evdev` package provides its metadata as `evdev-1.9.1.egg-info`, the check exits before the venv is activated. A fresh ARM64 installation then has no usable Python environment.
Recognize and copy the metadata format actually supplied by Debian, or install evdev into the venv through a mechanism that preserves valid package metadata.
</issue_to_address>
### Comment 5
<location path="runs/run_python.sh" line_range="24" />
<code_context>
+ exec 9<&-
+fi
+if [[ -f "$base_dir/.venvs/.legacy" && ! -e "$base_dir/.venvs/.migrated" ]]; then
+ exec /usr/bin/python3 "$@"
+fi
+
</code_context>
<issue_to_address>
**Legacy fallback lacks new dependencies**
When an existing installation cannot build its new VEnv and has a satellite or dimm-kit integration configured, `run_python.sh` launches the old system interpreter without checking its packages; because the old global requirements did not include `telnetlib3`, importing the satellite or dimm-kit modules then raises `ModuleNotFoundError` and those configured integrations fail during a failed environment migration.
Ensure the legacy interpreter has the newly required dependencies before relying on it as a fallback, or make the fallback use a validated environment that includes them.
</issue_to_address>
### Comment 6
<location path="runs/setup_python_venv.sh" line_range="119-120" />
<code_context>
+printf '%s\n' "$fingerprint" > "$new_env/.openwb-ready"
+temporary_link="$venv_dir/.venv-link.$$"
+ln -s "$new_env" "$temporary_link"
+mv -Tf "$temporary_link" "$venv_link"
+new_env=
+touch "$venv_dir/.migrated"
+rm -f "$venv_dir/.legacy"
</code_context>
<issue_to_address>
**Activation can delete new environment**
When the setup process exits after `mv -Tf` activates the new environment but before `new_env=` runs, the activation rename succeeds, but `new_env` remains set until the next command; if the script exits in between, the EXIT trap removes that now-active directory and leaves `.venv` dangling, so the launcher cannot start Python.
Clear the cleanup ownership before activation or make the EXIT trap preserve the environment whenever `.venv` points to it.
</issue_to_address>
### Comment 7
<location path="runs/tests/test_python_venv.sh" line_range="43" />
<code_context>
+if [[ "${FAIL_INSTALL:-0}" == 1 && "$*" == '-m pip install '* ]]; then
+ exit 1
+fi
+exit 0
+PYTHON
+ chmod +x "$3/bin/python"
</code_context>
<issue_to_address>
**Broken dependencies pass validation**
When the environment's dependency check or import probe would fail, `test_python_venv.sh`'s fake interpreter reaches an unconditional `exit 0` at line 43 for `pip check` and import-probe calls, so bootstrap validation appears successful and the ready-environment assertions pass even when those checks would fail.
Make the fake interpreter record and independently control the results of `pip check` and the import probe, then assert failed validation prevents activation.
</issue_to_address>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="runs/setup_python_venv.sh" line_range="123-131" />
<code_context>
+ arm*) ;;
+ *) binary_options+=(--no-binary=evdev) ;;
+esac
+"$new_env/bin/python" -m pip install --no-user "${binary_options[@]}" --extra-index-url https://www.piwheels.org/simple -r "$base_dir/requirements.txt"
+"$new_env/bin/python" -m pip check
+"$new_env/bin/python" -c 'import aiohttp, evdev, grpc, jq, lxml, paho.mqtt.client, pymodbus, telnetlib3'
+touch "$new_env/.openwb-lock"
+printf '%s\n' "$fingerprint" > "$new_env/.openwb-ready"
+temporary_link="$venv_dir/.venv-link.$$"
+ln -s "$new_env" "$temporary_link"
+mv -Tf "$temporary_link" "$venv_link"
+new_env=
+touch "$venv_dir/.migrated"
+rm -f "$venv_dir/.legacy"
</code_context>
<issue_to_address>
**Stale dependencies remain active**
When an application update changes its dependency requirements and the new environment build fails or cannot reach package indexes, the failed install leaves the previous `.venv` symlink in place, and `run_python.sh` launches that environment without checking it against the current requirements. After an update that needs a changed or newly added dependency, the updated services run with stale packages and can fail at startup or when that dependency is used.
Before launching updated code with an old environment, verify that it satisfies the current requirements or prevent startup until a compatible environment is available.
</issue_to_address>
### Comment 2
<location path="runs/setup_python_venv.sh" line_range="103-115" />
<code_context>
+"$python" -m venv "$new_env"
+binary_options=(--only-binary=:all:)
+case $(dpkg --print-architecture) in
+ arm64)
+ evdev_source=${OPENWB_EVDEV_SOURCE:-/usr/lib/python3/dist-packages}
+ site_packages=$("$new_env/bin/python" -c 'import sysconfig; print(sysconfig.get_path("purelib"))')
+ evdev_metadata=
+ for metadata_suffix in dist-info egg-info; do
+ candidate="$evdev_source/evdev-1.9.1.$metadata_suffix"
+ if [[ -d "$candidate" ]]; then
+ evdev_metadata=$candidate
+ break
+ fi
+ done
+ if [[ ! -d "$evdev_source/evdev" || -z "$evdev_metadata" ]]; then
+ echo "Debian python3-evdev 1.9.1 is required on ARM64" >&2
+ exit 1
+ fi
</code_context>
<issue_to_address>
**Older ARM64 installs fail**
When an ARM64 installation uses a Debian release whose `python3-evdev` package is older than 1.9.1, the architecture-only package selection installs the OS-provided `python3-evdev`, then the bootstrap requires its metadata to report exactly 1.9.1. On older ARM64 Debian releases where that package is older, setup exits before activating the environment, so Python services cannot start after migration.
Select a compatible evdev source by OS release, or use a matching ARM64 wheel when the system package does not meet the pin.
Also at `runs/install_packages.sh:58`.
</issue_to_address>
### Comment 3
<location path="runs/install_packages.sh" line_range="34" />
<code_context>
+ exit 1
+ fi
+ if ! cmp -s "$source_file" "$temporary_source"; then
+ if ! sudo cp -p -- "$source_file" "$source_file.openwb-backup" || ! sudo cp -- "$temporary_source" "$source_file"; then
+ rm -f "$temporary_source"
+ exit 1
</code_context>
<issue_to_address>
**Interrupted source rewrite breaks APT**
When the process or system stops while the Bullseye security source file is being overwritten, `sudo cp` overwrites the active source file in place, so an interruption can leave it truncated despite the backup copy. Later runs treat the truncated file as the source of truth, and Bullseye security updates can remain disabled.
Write the replacement to a temporary file in the source file's directory and atomically rename it over the active file after the copy succeeds.
</issue_to_address>
### Comment 4
<location path=".github/workflows/github-actions-python.yml" line_range="13" />
<code_context>
+ strategy:
+ fail-fast: false
+ matrix:
+ raspberry-pi-os-release: ["Bullseye", "Trixie"]
+ include:
+ - raspberry-pi-os-release: "Bullseye"
</code_context>
<issue_to_address>
**Bookworm CI coverage is missing**
When the documentation lists Bookworm/Python 3.11 as configured, the workflow matrix contains only Bullseye and Trixie, so no job checks Bookworm/Python 3.11 and readers see a CI coverage claim for a job that never runs.
Add Bookworm to the workflow matrix or remove the Bookworm row from the documentation.
Also at `docs/Python-Kompatibilitaet.md:22-23`.
</issue_to_address>
### Comment 5
<location path="runs/setup_python_venv.sh" line_range="38" />
<code_context>
+ for candidate in "$venv_dir"/env.*; do
+ [[ -d "$candidate" && ! -L "$candidate" && "$candidate" != "$active_env" ]] || continue
+ [[ ${candidate##*/} =~ ^env\.[a-zA-Z0-9]{8}$ ]] || continue
+ [[ -f "$candidate/.openwb-ready" && -f "$candidate/.openwb-lock" && ! -e "$candidate/.openwb-unprotected" ]] || continue
+ if ! exec 8<"$candidate/.openwb-lock"; then
+ echo "Cannot lock Python environment $candidate" >&2
</code_context>
<issue_to_address>
**Interrupted builds leak disk space**
When power is lost or the setup process receives `SIGKILL` after creating an environment directory but before writing its readiness and lock files, `remove_unused_envs` skips environments without both readiness and lock files, so a power loss or `SIGKILL` during environment creation leaves the incomplete directory behind. Repeated interrupted builds accumulate abandoned files under `.venvs` and consume disk space.
During boot cleanup, remove inactive, name-matching environment directories that lack readiness metadata, while holding the setup lock so no build is in progress.
</issue_to_address>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Establish managed Python virtual environments and broaden compatibility validation for supported openWB platforms and Python versions.
New Features:
Bug Fixes:
Enhancements:
CI:
Deployment:
Documentation:
Tests: