Bundle Python 3.10 for older Ubuntu support - #157
Conversation
7f2be83 to
c8faa92
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Well-structured approach to bundling Python 3.10 for older Ubuntu. The layered resolution (user config → bundled → system), SHA256 verification at both build and install time, and the kolibri-wrapper.sh replacing the setuptools entry point are all solid design choices.
CI passing across Ubuntu 16.04–24.04. No UI files changed — visual inspection skipped.
- suggestion:
install-python.sharchitecture handling could be more graceful on unsupported arches - suggestion:
python3-pkg-resourcesremoval — confirm no runtime dependency - praise: Checksum verification, Python resolution chain, and
add_statereordering are all well done
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| ;; | ||
| *) | ||
| echo "install-python.sh: Unsupported architecture: $DPKG_ARCH" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: On architectures other than amd64/arm64 where system Python is below the minimum, this exit 1 will fail the postinst and break package installation. Since the package is Architecture: all, it could be installed on armhf or i386 systems.
Consider exit 0 with a warning instead, letting the system fall back to whatever Python is available — Kolibri may degrade gracefully rather than refusing to install entirely. Alternatively, if the intent is that Kolibri truly cannot run without Python 3.10, this is fine, but worth documenting in the package description.
| Recommends: python3-cryptography (>= 1.2.3) | ||
| Depends: adduser | ||
| Recommends: python3 (>= 3.10), | ||
| python3-cryptography (>= 1.2.3) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The previous Depends included python3-pkg-resources. With it now fully removed (not moved to Recommends), could Kolibri fail at runtime if pkg_resources is imported? The wrapper avoids the setuptools console_scripts entry point, but Kolibri internals or its vendored dependencies may still use pkg_resources. Worth confirming this isn't needed.
There was a problem hiding this comment.
Kolibri already bundles this internally when needed.
|
|
||
| # Resolve the Python interpreter to use for Kolibri. | ||
| # Priority: 1) KOLIBRI_PYTHON from daemon.conf/conf.d, 2) /opt/kolibri/python, 3) system python3 | ||
| if [ -z "$KOLIBRI_PYTHON" ]; then |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Clean resolution chain with well-documented priority (user config → bundled → system). The comment on line 69 makes the intent immediately clear, and the fallback to python3 ensures the package degrades gracefully when bundled Python isn't needed.
| if [ -f "$dest" ]; then | ||
| echo "Already downloaded: $dest" | ||
| local actual_sha256 | ||
| actual_sha256=$(sha256sum "$dest" | cut -d' ' -f1) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Good defensive pattern — verifying checksums on both cache hits (lines 57–64) and fresh downloads (lines 73–79), with automatic re-download on mismatch. This prevents stale or corrupted tarballs from propagating into builds.
| echo -n "$KOLIBRI_USER" > /etc/kolibri/username | ||
| fi | ||
|
|
||
| add_state "after_postinst" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Moving add_state "after_postinst" before service registration is a good robustness improvement. If systemctl enable or update-rc.d fails (e.g., in containers without systemd), the package state is still correctly recorded.
|
|
||
| # Source the kolibri environment if available (sets KOLIBRI_PYTHON, PYTHONPATH, etc.) | ||
| if [ -f /etc/default/kolibri ]; then | ||
| set -o allexport |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The wrapper sources /etc/default/kolibri to resolve KOLIBRI_PYTHON. When invoked via the init script, /etc/default/kolibri has already been sourced and the environment exported. The wrapper then re-sources it, which is harmless but redundant for the init path. For the direct CLI path (user runs kolibri from terminal), this sourcing is essential.
No change needed — just noting that this dual-sourcing is intentional and safe, since the file is idempotent.
bjester
left a comment
bjester
left a comment
There was a problem hiding this comment.
Overall, looks complete. Some areas for improvement:
- have the download script ensure it has checksums to check the download with before downloading
- use
sourceinstead of.in scripts - consider the tradeoffs of using checksum files and
sha256sum -cinstead-- may not be of benefit because of the rich error logging
| ARCH="" | ||
|
|
||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --arch) | ||
| ARCH="$2" | ||
| shift 2 | ||
| ;; | ||
| *) | ||
| echo "Unknown option: $1" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Since there's only one argument, there minimal benefit of the named argument versus a positional argument.
| download_and_verify "x86_64" "$PYTHON_SHA256_X86_64" | ||
| fi | ||
|
|
||
| if [ -z "$ARCH" ] || [ "$ARCH" = "aarch64" ]; then | ||
| download_and_verify "aarch64" "$PYTHON_SHA256_AARCH64" |
There was a problem hiding this comment.
Considering these env vars are sourced, it'd be helpful if there was a comment, and perhaps even more helpful if these checked whether the checksum is even set. Why go through with the download when you know it'll fail without a checksum to check it with!
There was a problem hiding this comment.
Added a check for the env vars before doing the download and verify.
| SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
|
|
||
| . "$SCRIPT_DIR/python_versions.conf" |
There was a problem hiding this comment.
It isn't totally uncommon for .conf to be used for this use case, but it does feel odd to me. Since this file is intended to just hold environment variables, I think .env is clearer.
There was a problem hiding this comment.
Updated to an .env file.
| python3 -c " | ||
| import json, sys | ||
| target = '${TARGET}' | ||
| releases = json.load(sys.stdin) | ||
| for r in releases: | ||
| urls = {} | ||
| python_version = None | ||
| full_version = None | ||
| for a in r.get('assets', []): | ||
| name = a['name'] | ||
| if f'cpython-{target}' not in name: | ||
| continue | ||
| if 'install_only_stripped' not in name: | ||
| continue | ||
| if 'unknown-linux-gnu' not in name: | ||
| continue | ||
| if python_version is None: | ||
| # Extract e.g. '3.10.20' and '3.10.20+20260325' | ||
| full_version = name.split('-')[1] | ||
| python_version = full_version.split('+')[0] | ||
| # Match plain x86_64 (not v2/v3/v4) and aarch64 | ||
| if 'x86_64-unknown' in name: | ||
| urls['x86_64'] = a['browser_download_url'] | ||
| elif 'aarch64-unknown' in name: | ||
| urls['aarch64'] = a['browser_download_url'] | ||
| if python_version and 'x86_64' in urls and 'aarch64' in urls: | ||
| print(f\"{r['tag_name']}|{python_version}|{full_version}|{urls['x86_64']}|{urls['aarch64']}\") | ||
| sys.exit(0) | ||
| print('NOT_FOUND', file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Have updated all the new build time scripting to Python to avoid these shenanigans.
| exit 0 | ||
| fi | ||
|
|
||
| . "$CONF_FILE" |
There was a problem hiding this comment.
. is quick and dirty, for when you're in the terminal. In scripts, source is better IMO
| fi | ||
| sleep 2 | ||
| done | ||
| echo "FAIL: Kolibri did not respond on port 8080 within 120 seconds" |
There was a problem hiding this comment.
It's localhost so it shouldn't but what if the curl call hangs? this comment shows an assumption, but what if it waits for some 'timeout' period?
There was a problem hiding this comment.
Added a 5 second timeout on every loop above.
23d3a63 to
33fea47
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Resolved:
- Exit 1 on unknown arch in
debian/install-python.sh(suggestion) — code now exits 0 with "Falling back to system Python."
Acknowledged (not re-raised):
python3-pkg-resourcesremoval from Depends (suggestion) — author replied: "Kolibri already bundles this internally when needed."
2/2 prior findings resolved or acknowledged. 0 re-raised.
CI passing. No UI files changed.
- praise: see inline on
build_tools/manage_python.py - suggestion: see inline on
build_tools/manage_python.py
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| for arch in arches: | ||
| sha_key = f"PYTHON_SHA256_{arch.upper()}" | ||
| expected_sha = config.get(sha_key) | ||
| if not expected_sha: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Good defensive check — failing fast with a clear error before making a network request is much better than downloading 28 MB only to discover the checksum is missing. Addresses bjester's prior concern cleanly.
|
|
||
| # Download and compute checksums | ||
| checksums = {} | ||
| with tempfile.TemporaryDirectory() as tmpdir: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: cmd_update downloads both tarballs (~56 MB) to a temp directory just to compute checksums, then discards them. After running make update-python, the developer must run make get-python to download the same 56 MB again for the actual build.
Consider writing directly to build_src/ during update — that directory is where cmd_download already puts them. You could reuse the same download-and-verify loop from cmd_download, just seeded with the freshly-fetched URLs and newly-computed checksums.
There was a problem hiding this comment.
Good catch — cmd_update now writes directly to build_src/ using the same tarball_filename(full_version, arch) naming cmd_download expects, so a subsequent make get-python hits the cache and verifies instead of re-downloading.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
All 1 prior finding resolved: cmd_update double-download suggestion — code now writes directly to build_src/ using tarball_filename(full_version, arch), matching cmd_download's naming. 0 re-raised.
Clean additions — smoke tests and KOLIBRI_PYTHON resolution are both well-structured. CI passing. No UI files changed.
- praise: see inline on
install_test.ymlanddebian/startup/kolibri.default - nitpick: see inline on
install_test.yml
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| name: Install test (${{ matrix.image }}) | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: fail-fast: false is the right choice for a smoke test — ensures all five Ubuntu versions are tested independently so a failure on 16.04 doesn't hide a separate failure on 22.04.
| fi | ||
| sleep 2 | ||
| done | ||
| echo "FAIL: Kolibri did not respond on port 8080 within 120 seconds" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: The failure message says "within 120 seconds" but the actual max wait is ~420 seconds (60 iterations × up to 5s curl timeout + 2s sleep). Could say "within 2 minutes of sleep time" or just drop the time estimate.
| # When using a non-system Python, the bundled Python needs access to | ||
| # Kolibri installed in system dist-packages | ||
| if [ "$KOLIBRI_PYTHON" != "python3" ] && [ -n "$KOLIBRI_PYTHON" ]; then | ||
| PYTHONPATH="/usr/lib/python3/dist-packages${PYTHONPATH:+:$PYTHONPATH}" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: ${PYTHONPATH:+:$PYTHONPATH} correctly handles both the set and unset cases — prepends a colon only when PYTHONPATH is already non-empty, avoiding a spurious leading colon that would add an implicit . to the search path.
🔵 Review postedLast updated: 2026-07-22 00:03 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #157 — All prior findings resolved or acknowledged; the runtime path (init → wrapper → bundled/system Python) is coherent and covered by the new 16.04–24.04 smoke test. CI passing. Two new findings on the new build_tools/manage_python.py, both inline.
- important —
manage_python.pyships with no tests despite the siblinggenerate_changelog.py/tests/convention. Its pure functions (tarball_filename,tarball_url,read_config/write_config,find_release) are prime unit-test candidates. - suggestion — unanchored version-prefix match in
find_releasecould resolve3.1to a 3.10/3.11 tarball.
No UI files changed — Phase 3 (UI verification / manual QA) not applicable.
Prior-finding status
RESOLVED — debian/install-python.sh — exit 1 on unknown arch (now falls back to system Python)
ACKNOWLEDGED — debian/control:22 — python3-pkg-resources removal (Kolibri bundles internally)
RESOLVED — debian/startup/kolibri.default:70 — resolution-chain documentation
RESOLVED — build_tools/download_python.sh — checksum/source/.env concerns (rewritten to manage_python.py)
RESOLVED — debian/kolibri.scripts-common:285 — add_state ordering
RESOLVED — debian/kolibri-wrapper.sh:9 — wrapper re-sources /etc/default/kolibri under su
RESOLVED — build_tools/manage_python.py:143 — fail-fast before network request
RESOLVED — build_tools/manage_python.py — cmd_update double-download (now writes to build_src/)
RESOLVED — .github/workflows/install_test.yml:16 — fail-fast: false
RESOLVED — .github/workflows/install_test.yml — retry-wait wording (now "after 60 attempts")
RESOLVED — debian/startup/kolibri.default:81 — PYTHONPATH prepend handling
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| @@ -0,0 +1,295 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
important: This new 295-line module ships with no tests. The sibling build_tools/generate_changelog.py already establishes the convention — tests/test_generate_changelog.py unit-tests its pure functions and uses a VCR cassette for the GitHub-API call. manage_python.py has the same shape: tarball_filename, tarball_url (esp. the +→%2B encoding branch), read_config/write_config round-trip, and find_release (fixture-driven from a list of release dicts — the existing tests/cassettes/releases_full.yaml shows the team already captures /releases responses) are all directly testable with no network. Please add a test file following that pattern.
| urls = {} | ||
| for arch in SUPPORTED_ARCHES: | ||
| for name, url in assets.items(): | ||
| if name.startswith(f"cpython-{target}") and name.endswith(f"-{arch}{suffix}"): |
There was a problem hiding this comment.
suggestion: The version match name.startswith(f"cpython-{target}") (and the matched_name selection on line 191) isn't anchored on the version boundary. target is validated only as ^3\.\d+$, so cpython-3.1 is a prefix of cpython-3.10.20+… — requesting 3.1 would silently resolve to a 3.10/3.11 tarball (same class of collision for a future 3.10 vs 3.100). Since the filename always has a patch component, anchoring on the trailing dot fixes it cheaply:
name.startswith(f"cpython-{target}.")Low severity — current Kolibri targets (≥3.10) don't collide — but the guard is nearly free and removes a silent-wrong-version footgun.
Add scripts and configuration to download and manage Python 3.10 from python-build-standalone for bundling in the .deb: - python_versions.conf: pins version, release tag, and SHA256 checksums - download_python.sh: downloads and verifies tarballs for x86_64/aarch64 - update_python_version.sh: developer tool to bump Python version - Makefile: get-python and update-python targets, get-python as prerequisite for deb and changes builds, fix globs to not match cpython tarballs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Package now includes python-build-standalone tarballs for x86_64 and aarch64. At install time, if system Python is below 3.10, the matching tarball is extracted to /opt/kolibri/python/. - install-python.sh: install-time script to verify and extract Python - debian/rules: installs bundled artifacts to /opt/kolibri/bundled/ - build.sh: copies tarballs and config into source tree during build - postinst: calls install-python.sh during configure - postrm: cleans up /opt/kolibri/ on purge - control: move python3 to Recommends (>= 3.10) since we bundle our own Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
At service start, resolve the Python interpreter: 1. KOLIBRI_PYTHON from daemon.conf (user override) 2. /opt/kolibri/python/bin/python3 (package-installed) 3. system python3 (fallback) When using a non-system Python, the init script overrides KOLIBRI_COMMAND to use '$KOLIBRI_PYTHON -m kolibri' instead of the system 'kolibri' entry point. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test that the .deb installs correctly and Kolibri starts on all supported Ubuntu versions. Verifies Python >= 3.10 is available and Kolibri responds on port 8080. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary
Bundles Python 3.10 from python-build-standalone in the .deb so Kolibri can run on Ubuntu 16.04–20.04 without depending on the system Python version.
Both x86_64 and aarch64 tarballs are included (~56 MB total). At install time, if system Python is already >= 3.10, the bundled tarballs are ignored.
Tested installation on Ubuntu 16.04 (Python 3.5) via distrobox — bundled Python 3.10.20 extracted and verified working, Kolibri service resolves to the correct interpreter, purge cleans up completely.
References
In support of #141 (Python availability only; kolibri-server merge is a separate effort)
Reviewer guidance
make get-pythonwhich downloads ~56 MB of Python tarballs. This runs automatically as a prerequisite ofmake kolibri.debandmake kolibri.changes.python3moved from Depends to Recommends (>= 3.10). The package no longer requires system Python to install.debian/startup/kolibri.init:44-47— when bundled Python is active,KOLIBRI_COMMANDis overridden to$KOLIBRI_PYTHON -m kolibri. Worth verifying this works correctly on a real system with systemd.debian/install-python.sh— the install-time extraction script. Check the arch detection and SHA256 verification logic.Open questions:
AI usage
Built collaboratively with Claude Code (Opus 4.6). Design brainstormed interactively, then implemented task-by-task. All code reviewed, tested via distrobox on Ubuntu 16.04, and commit history cleaned up manually.