Skip to content

Read the Kolibri version from one shared helper across platforms - #15106

Open
rtibblesbot wants to merge 3 commits into
learningequality:developfrom
rtibblesbot:issue-15077-92a158
Open

Read the Kolibri version from one shared helper across platforms#15106
rtibblesbot wants to merge 3 commits into
learningequality:developfrom
rtibblesbot:issue-15077-92a158

Conversation

@rtibblesbot

@rtibblesbot rtibblesbot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Each platform build read the Kolibri version its own way: pkginfo against an unpacked wheel, the plaintext kolibri/VERSION, or a strip of the sdist tar's filename. The three that have no importable Kolibri at their read point — desktop-app, android, the root pex target — now go through one stdlib-only helper keyed on kolibri/_version.py, the file setuptools-scm writes and the one kolibri.__version__ itself reads. kolibri-server's read is deleted rather than routed: its .deb build already runs inside the workspace uv env, so generate_changelog.py imports the version itself. Per-platform conversions (PEP 440 → Debian, Android versionCode) stay with their platform. The desktop app also gains stage-workspace-whl, mirroring android's stage-workspace-tar, so it can bundle the Kolibri you have checked out instead of a wheel downloaded from a URL.

Excluding the helper's test, the diff is +96/-121. The helper is 33 lines and replaces three bespoke reads; the fourth goes away. pkginfo leaves the desktop app's build extra with it.

References

Fixes #15077. Related: #15072 (release-path migration — where the external installer consumers move).

Acceptance criteria

  • One shared helper for the version read; per-platform conversions stay separate. build_tools/read_kolibri_version.py is the single reader of kolibri/_version.py for the platforms that cannot import it: android (app/build.gradle:113), desktop-app (Makefile:23) and the root pex target (Makefile:229). kolibri-server can import it, so it does — generate_changelog.py:52, the same read platform-debian-server-release.yml:35 already used — and its tar-filename strip is gone.
  • desktop-app has stage-workspace-whl mirroring android's stage-workspace-tar. platforms/desktop-app/Makefile:80.
  • Stale clone/venv/get-tar instructions gone from platforms/android/README.md. grep -n "get-tar\|kolibri-installer-android\|uv venv\|--extra build\|0\.17\.0" platforms/android/README.md returns nothing.

(Ticked here and in this issue comment rather than in the issue body — this account lacks UpdateIssue permission on learningequality/kolibri.)

Deviations from the issue spec

  • kolibri/VERSION is not retired, though the issue thread asked for it once android stopped reading it. It has no reader in this repo, but kolibri-installer-debian untars the sdist we publish and reads it out of that tar (Makefile:18-21 at v0.16.1), and this repo calls that installer from pr_build_kolibri.yml:24-30 and release_kolibri.yml:78. Deleting the file turns the PR deb job red, taking zip, pi_smoke_test and server_smoke_test with it. Makefile:204 records this; please sanity-check the pins. (The external kolibri-installer-android reads it too, but no workflow here calls that repo any more — platforms/android/ builds both the PR and the release APK — so the retention rests on the one Debian consumer.)
  • raspberry-pi keeps its .deb-filename parse (platforms/raspberry-pi/Makefile:62). The .deb it packages is the correct artifact there — the version is already Debian-converted and no _version.py exists at Pi-image build — so the helper would mean reading a version it does not ship.

Open question

  • No released sdist contains _version.py, so a workflow_dispatch of platform-android-build_apk.yml with a pre-monorepo release tar-url now fails at Gradle configure. The reusable-call path from pr_build_kolibri.yml is unaffected (same-commit tar). Acceptable, or should the read fall back?

platforms/desktop-app/README.md:44 still says git clone kolibri-app.git. Its build section is rewritten here for stage-workspace-whl, but that pre-monorepo clone step is left — the same staleness AC3 fixed for android, and AC3 names only the android file. Happy to fold it in.

AI usage

Used Claude Code to implement the plan agreed in the issue thread, writing the version-read helper test-first and scoping the kolibri/VERSION question against the external installer repos before deciding not to delete it. Verified with the test/ suite, prek run --all-files, a real Gradle configure against a workspace-built sdist, an end-to-end make stage-workspace-whl, and a round-trip of the read through generate_changelog.py's Debian conversion.


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?
  • Ran pre-flight CI checks (lint, format, tests) and verified all pass
  • Rebased onto the target branch and resolved any conflicts
  • Reorganized commit history into clean, logical commits
  • Audited the diff to ensure only issue-relevant files are changed
  • Built PR body from the repository's PR template with evidence blocks
@rtibblesbot

🟡 Waiting for feedback

Last updated: 2026-07-30 06:01 UTC

@github-actions github-actions Bot added DEV: dev-ops Continuous integration & deployment DEV: backend Python, databases, networking, filesystem... SIZE: medium labels Jul 30, 2026
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

@rtibblesbot
rtibblesbot force-pushed the issue-15077-92a158 branch 3 times, most recently from 049574b to 45afba7 Compare July 30, 2026 02:02
@rtibblesbot
rtibblesbot marked this pull request as ready for review July 30, 2026 02:03

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first impression of this from a skim is that this is incredibly verbose for what it achieves and rather than consolidating things and reducing code duplication it seems to add rather a lot of code.

@rtibblesbot

Copy link
Copy Markdown
Contributor Author

Fair, and mostly deserved. Pushed a trim in a78fd26 and cut the PR body down too.

The helper was carrying two readers nobody calls. No caller passes an sdist tar — android extracts it and hands over the tree — so the tarfile branch and its two tests went, along with argparse (one positional arg), the three private _read_* wrappers, and the error-path tests for entities that no longer exist. It is now one 45-line file: read kolibri/_version.py out of a directory or a .whl, ast-parse the __version__ assignment. 92 → 45 lines, tests 129 → 66. I also trimmed the comments at every call site and dropped the eight-line directive GradleException in app/build.gradle for a three-line comment.

On the volume, with the helper's test set aside the diff is now +112/-98 — so the consolidation is close to line-neutral, and the test file is most of what is left. The _version.py read itself replaces four bespoke ones (pkginfo.Installed, a VERSION-file read, a tar-filename strip, pkginfo.Wheel), and pkginfo drops out of desktop-app's build deps.

Two adds are not consolidation and I should flag them as such: stage-workspace-whl (11 lines) is AC2, not deduplication; and the one-line guard in desktop-app's needs-version exists because $(shell) swallows the helper's exit status, so a failed read would otherwise pass an empty version into the build.

Two places I could go further if you want it — say the word rather than my guessing:

  1. Drop the debian-server CI version step entirely. That job runs uv sync --all-packages, so generate_changelog.py's existing import kolibri fallback already resolves the same _version.py. The step and its env: block are ~9 lines that only make the version explicit in the log. Removing them means debian-server stops calling the shared helper — which reads either as the right per-platform mechanism or as a hole in AC1, and that is your call, not mine.
  2. Drop the .whl branch (4 lines) and have pex read the repo tree instead of each wheel. Same answer in practice, since pex runs right after dist in the same tree; I kept it so the pex filename comes from the artifact it names.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more concise, but I'm confused by the changes to kolibri-server, unless I'm just missing something?

Comment thread .github/workflows/platform-debian-server-build_deb.yml
Comment thread build_tools/read_kolibri_version.py Outdated
with open(os.path.join(path, VERSION_MODULE)) as version_module:
source = version_module.read()
# ast, not exec: handles either quoting style without running build output.
for node in ast.parse(source).body:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why _version.py can't just be imported? It's a standalone file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — it can, and now it is executed rather than parsed. The ast walk is gone:

    if path.endswith(".whl"):
        with zipfile.ZipFile(path) as wheel:
            source = wheel.read(VERSION_MODULE)
    else:
        with open(os.path.join(path, VERSION_MODULE), "rb") as version_module:
            source = version_module.read()
    # _version.py imports nothing and only assigns, so running it is the same as
    # importing it -- which is not an option for a member of a wheel.
    namespace = {}
    exec(source, namespace)
    return namespace["__version__"]

The one reason it is exec on the source rather than an import of the path: the pex target hands over a .whl, and a member of a zip cannot be imported without unpacking it — so reading the text and running it is the single form that covers both a tree and a wheel. The helper is 36 lines now, and the two tests that only pinned the parse (quoting style, version-alias-only) went with it, leaving the two that assert the actual interface: read from a tree, read from a wheel.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's tidy up the commit history ready for merge here, this doesn't need to be more than a few commits.

@rtibblesbot

Copy link
Copy Markdown
Contributor Author

Squashed to three commits, one per acceptance criterion — the tree is byte-identical to the previous head:

  • 53b5198 Read the Kolibri version from _version.py with one shared helper (helper + test, pex, android, kolibri-server, the kolibri/VERSION comments)
  • a80a4b8 Stage and read the desktop app's Kolibri whl from the workspace (needs-version + stage-workspace-whl, pkginfo dropped)
  • 4fff366 Drop pre-migration setup from the Android README

git diff <old-head> HEAD is empty; pytest test/ 26 passed.

@rtibblesbot
rtibblesbot force-pushed the issue-15077-92a158 branch 2 times, most recently from f0c1c0d to 31122b8 Compare July 30, 2026 05:37
rtibblesbot and others added 3 commits July 29, 2026 22:59
Every platform had its own way of finding the version of the Kolibri it
was packaging: android read the plaintext kolibri/VERSION out of the
extracted tar, kolibri-server's CI stripped it off the whl job's tar
artifact filename, and the pex target parsed wheel METADATA with
pkginfo. build_tools/read_whl_version.py becomes read_kolibri_version.py
and covers all of them, reading kolibri/_version.py -- the setuptools-scm
source of truth -- out of a directory or a wheel.

kolibri-server needs no helper call: make deb runs under uv run, so the
changelog generator's import kolibri fallback already resolves the same
_version.py. So the tar-file-name input, the version step and the
cross-job needs: whl all go, which lets the job start in parallel with
whl instead of waiting on it.

kolibri/VERSION keeps shipping in the sdist despite losing its last
in-repo reader: kolibri-installer-debian reads it out of our tar, so
deleting it would redden the PR deb job. Both sites now say so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
needs-version dropped pkginfo.Installed for the shared helper, reading
the same kolibrisrc/ tree PyInstaller bundles. $(shell) swallows a
nonzero exit, so an empty read gets its own guard rather than passing an
empty version into the build. pkginfo was the last consumer of that
dependency, so it leaves the build extra.

The only way to get a whl in place was make get-whl whl=<URL>, and no
released whl ships _version.py, so add stage-workspace-whl to build and
stage the checked-out Kolibri instead -- the counterpart to android's
stage-workspace-tar.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quick Start and Initial Setup still told you to clone
kolibri-installer-android, make a standalone uv venv and download a
pinned v0.17.0 tar, all of which contradicted the monorepo section
directly above them. Quick Start now uses the workspace sync and
stage-workspace-tar, Initial Setup goes (it duplicated Quick Start
otherwise), and make help's stage-workspace-tar line joins the target
list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DEV: backend Python, databases, networking, filesystem... DEV: dev-ops Continuous integration & deployment SIZE: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean up duplication left by the platform migration

2 participants