TeamAI/zai-tray-checker
4
0
Fork 0
Code Issues Pull requests Projects Releases 3 Packages Wiki Activity Actions

fix: release verify step fails on headless CI #9

Merged
bjoern merged 2 commits from fix/release-verify-headless into main 2026-06-28 21:48:46 +02:00
Conversation 3 Commits 2 Files changed 2 +41 -3
matikane commented 2026-06-28 20:55:33 +02:00
Member
Copy link

Fix: release verify step fails on headless CI

The release pipeline's verify step ran zai-tray-checker --version, which imports main.py → from PySide6.QtCore import ... at module level. On the headless Debian runner (no libGL), this crashes with ModuleNotFoundError: No module named 'PySide6' because --no-deps skipped installing PySide6.

Fix

Changed the verify step to read version from wheel metadata via importlib.metadata.version() instead of running the CLI entry point. This doesn't import main.py at all, so no PySide6 or system GL libs are needed.

Also added @pyside6_required skipif guard to the two subprocess-based version tests in test_version.py so CI doesn't fail there either.

70 tests passing.

## Fix: release verify step fails on headless CI The release pipeline's verify step ran `zai-tray-checker --version`, which imports `main.py` → `from PySide6.QtCore import ...` at module level. On the headless Debian runner (no `libGL`), this crashes with `ModuleNotFoundError: No module named 'PySide6'` because `--no-deps` skipped installing PySide6. ### Fix Changed the verify step to read version from **wheel metadata** via `importlib.metadata.version()` instead of running the CLI entry point. This doesn't import `main.py` at all, so no PySide6 or system GL libs are needed. Also added `@pyside6_required` skipif guard to the two subprocess-based version tests in `test_version.py` so CI doesn't fail there either. 70 tests passing.
fix: release verify step uses importlib.metadata instead of CLI
All checks were successful
CI / test (pull_request) Successful in 20s
Details
f0267ecbf1 
The old verify step ran 'zai-tray-checker --version' which imports
main.py → PySide6 at module level, failing on headless CI without
system GL libs. Now reads version from wheel metadata via
importlib.metadata — no PySide6 needed.

Also guard test_version.py subprocess tests with skipif for headless CI.
matikane commented 2026-06-28 21:34:40 +02:00
Author
Member
Copy link

🤖 Hermes automated review: changes requested

Reviewed the full diff (39 additions, 3 deletions, 2 files) for the headless-CI release verify fix. The direction (read version from wheel metadata via importlib.metadata.version() instead of importing main.py) is correct, and the @pyside6_required skipif guards are a good addition. However, the verify step as written will fail on every release run due to a cross-step variable scoping bug.

⚠️ Note: this is a PR conversation comment, not a formal Forgejo review approval. The current MCP integration cannot create inline diff comments or approval states.

🔴 Major (blocking)

1. $TAG is step-scoped and will be empty in the verify step — assertion always fails — .forgejo/workflows/release.yml:60

      - name: Extract version from tag
        env:
          TAG: ${{ github.event.release.tag_name }}   # ← step-scoped
        run: |
          VERSION="${TAG#v}"
          ...

      - name: Build wheel          # ← $TAG not available here
        run: ...

      - name: Verify version in built package
        run: |
          pip install dist/*.whl --no-deps
          python3 -c "
          ...
          expected = '${TAG#v}'                      # ← $TAG is empty here
          assert v == expected, f'Version mismatch: wheel={v}, expected={expected}'
          "

env: declared under a step makes $TAG available only within that step's shell. It is not persisted to subsequent steps (the run: blocks run in separate shell processes, and nothing is written to $GITHUB_ENV). So in the verify step ${TAG#v} expands to the empty string '', and the assertion becomes assert v == '' — which fails for any non-empty version, e.g. AssertionError: Version mismatch: wheel=1.0.0, expected=. This swaps the original PySide6 import crash for a guaranteed assertion failure on every release.

Suggested fix — either persist VERSION to $GITHUB_ENV in the extract step, or pass the tag down via the verify step's own env:. The cleanest is to repeat the source-of-truth env: on the verify step (same pattern the extract step already uses):

      - name: Verify version in built package
        env:
          TAG: ${{ github.event.release.tag_name }}
        run: |
          . .venv/bin/activate
          pip install dist/*.whl --no-deps
          python3 -c "
          from importlib.metadata import version
          v = version('zai-tray-checker')
          print(f'Wheel version: {v}')
          expected = '${TAG#v}'
          assert v == expected, f'Version mismatch: wheel={v}, expected={expected}'
          print('Version verified OK')
          "

(Alternatively, add echo "TAG=$TAG" >> "$GITHUB_ENV" in the extract step so it's available to all later steps as $TAG.)

⚠️ Note: the release.yml workflow only triggers on release: published, so CI (the ci.yml workflow that runs pytest on PRs) will not exercise this path. The bug won't be caught before the next release unless the release workflow is run manually or the line is fixed directly.

What looks good

  • Using importlib.metadata.version() avoids importing main.py → no PySide6 / libGL dependency for the verify step. Correct and minimal fix for the original problem.
  • The --no-deps install is appropriate here since you only need the metadata, not the runtime deps.
  • _can_import_pyside6() helper in test_version.py correctly probes the C++ extensions (QtCore + QtGui), not just the Python package presence.
  • test_version_in_wheel_metadata is a good new test that mirrors the CI verify logic and runs without PySide6.

Verification

  • CI/CD: ci.yml runs pytest on PRs to main, but no CI result comment exists yet for head f0267ecb (PR just opened, 0 comments). Per CI-absent policy, ran tests locally.
  • Local: python3 -m pytest tests/ -q → 70 passed in 1.93s (including the 2 PySide6-guarded subprocess version tests, which ran here since PySide6 is importable in this sandbox).

Automated daily review. I never merge PRs.

## 🤖 Hermes automated review: changes requested Reviewed the full diff (39 additions, 3 deletions, 2 files) for the headless-CI release verify fix. The direction (read version from wheel metadata via `importlib.metadata.version()` instead of importing `main.py`) is correct, and the `@pyside6_required` skipif guards are a good addition. However, the verify step as written will **fail on every release run** due to a cross-step variable scoping bug. > ⚠️ Note: this is a PR conversation comment, not a formal Forgejo review approval. The current MCP integration cannot create inline diff comments or approval states. --- ### 🔴 Major (blocking) **1. `$TAG` is step-scoped and will be empty in the verify step — assertion always fails** — `.forgejo/workflows/release.yml:60` ```yaml - name: Extract version from tag env: TAG: ${{ github.event.release.tag_name }} # ← step-scoped run: | VERSION="${TAG#v}" ... - name: Build wheel # ← $TAG not available here run: ... - name: Verify version in built package run: | pip install dist/*.whl --no-deps python3 -c " ... expected = '${TAG#v}' # ← $TAG is empty here assert v == expected, f'Version mismatch: wheel={v}, expected={expected}' " ``` `env:` declared under a step makes `$TAG` available **only within that step's shell**. It is not persisted to subsequent steps (the `run:` blocks run in separate shell processes, and nothing is written to `$GITHUB_ENV`). So in the verify step `${TAG#v}` expands to the empty string `''`, and the assertion becomes `assert v == ''` — which fails for any non-empty version, e.g. `AssertionError: Version mismatch: wheel=1.0.0, expected=`. This swaps the original PySide6 import crash for a guaranteed assertion failure on every release. **Suggested fix** — either persist `VERSION` to `$GITHUB_ENV` in the extract step, or pass the tag down via the verify step's own `env:`. The cleanest is to repeat the source-of-truth `env:` on the verify step (same pattern the extract step already uses): ```yaml - name: Verify version in built package env: TAG: ${{ github.event.release.tag_name }} run: | . .venv/bin/activate pip install dist/*.whl --no-deps python3 -c " from importlib.metadata import version v = version('zai-tray-checker') print(f'Wheel version: {v}') expected = '${TAG#v}' assert v == expected, f'Version mismatch: wheel={v}, expected={expected}' print('Version verified OK') " ``` (Alternatively, add `echo "TAG=$TAG" >> "$GITHUB_ENV"` in the extract step so it's available to all later steps as `$TAG`.) ⚠️ Note: the `release.yml` workflow only triggers on `release: published`, so CI (the `ci.yml` workflow that runs `pytest` on PRs) will **not** exercise this path. The bug won't be caught before the next release unless the release workflow is run manually or the line is fixed directly. --- ### What looks good - Using `importlib.metadata.version()` avoids importing `main.py` → no PySide6 / libGL dependency for the verify step. Correct and minimal fix for the original problem. - The `--no-deps` install is appropriate here since you only need the metadata, not the runtime deps. - `_can_import_pyside6()` helper in `test_version.py` correctly probes the C++ extensions (`QtCore` + `QtGui`), not just the Python package presence. - `test_version_in_wheel_metadata` is a good new test that mirrors the CI verify logic and runs without PySide6. ### Verification - **CI/CD:** `ci.yml` runs `pytest` on PRs to main, but no CI result comment exists yet for head `f0267ecb` (PR just opened, 0 comments). Per CI-absent policy, ran tests locally. - **Local:** `python3 -m pytest tests/ -q` → **70 passed in 1.93s** (including the 2 PySide6-guarded subprocess version tests, which ran here since PySide6 is importable in this sandbox). *Automated daily review. I never merge PRs.*
fix: add TAG env to verify step (was step-scoped, assertion would fail)
All checks were successful
CI / test (pull_request) Successful in 22s
Details
5289cb15ca 
The $TAG variable was only available in the 'Extract version from tag'
step. The verify step referenced ${TAG#v} but TAG was empty there,
making the assertion always fail with 'expected='. Now the verify step
has its own env: TAG declaration.
matikane commented 2026-06-28 21:39:31 +02:00
Author
Member
Copy link

Good catch — that's a real blocking bug. $TAG was step-scoped to the "Extract version from tag" step, so ${TAG#v} in the verify step would expand to an empty string, making the assertion always fail.

Fixed in 5289cb1: added env: TAG: ${{ github.event.release.tag_name }} to the verify step directly, so it has its own copy of the tag name.

70 tests still passing.

Good catch — that's a real blocking bug. `$TAG` was step-scoped to the "Extract version from tag" step, so `${TAG#v}` in the verify step would expand to an empty string, making the assertion always fail. Fixed in `5289cb1`: added `env: TAG: ${{ github.event.release.tag_name }}` to the verify step directly, so it has its own copy of the tag name. 70 tests still passing.
matikane commented 2026-06-28 21:46:54 +02:00
Author
Member
Copy link

🤖 Hermes automated review: no blocking issues found

Follow-up review of fix commit 5289cb1 (head changed f0267ecb → 5289cb1, force-push after prior changes_requested).

The one blocking finding from the previous review is resolved, and no new issues were introduced.

⚠️ Note: this is a PR conversation comment, not a formal Forgejo review approval. The current MCP integration cannot create inline diff comments or approval states.

✅ Prior major finding — resolved

$TAG step-scoped → ${TAG#v} empty in verify step — .forgejo/workflows/release.yml

Previously the verify step had no env: of its own, so ${TAG#v} expanded to '' and the assertion assert v == '' failed on every release. The fix adds the same source-of-truth env directly to the verify step:

      - name: Verify version in built package
        env:
          TAG: ${{ github.event.release.tag_name }}   # ← now present
        run: |
          ...
          expected = '${TAG#v}'   # ← now expands correctly

Verified at head: TAG now appears on both the extract step (line 24) and the verify step (line 53). ${TAG#v} in the verify step will now correctly strip the leading v. ✅

What still looks good (unchanged from prior review)

  • importlib.metadata.version() avoids importing main.py → no PySide6 / libGL dependency for the verify step. Correct minimal fix for the original headless-CI crash.
  • --no-deps install is appropriate (only metadata needed).
  • _can_import_pyside6() helper probes the C++ extensions (QtCore + QtGui), not just package presence.
  • test_version_in_wheel_metadata mirrors the CI verify logic and runs without PySide6.

Verification

  • CI/CD: ci.yml runs pytest on PRs to main, but no CI result comment exists yet for head 5289cb1. The release.yml workflow (where this fix lives) only triggers on release: published, so CI does not exercise this path. Per CI-absent policy, ran tests locally.
  • Local: python3 -m pytest tests/ -q at head 5289cb1 → 70 passed in 0.77s (all 4 version tests, including the 2 PySide6-guarded ones which ran since PySide6 is importable in this sandbox).

Automated daily review. I never merge PRs.

## 🤖 Hermes automated review: no blocking issues found Follow-up review of fix commit `5289cb1` (head changed `f0267ecb` → `5289cb1`, force-push after prior `changes_requested`). The one blocking finding from the previous review is **resolved**, and no new issues were introduced. > ⚠️ Note: this is a PR conversation comment, not a formal Forgejo review approval. The current MCP integration cannot create inline diff comments or approval states. --- ### ✅ Prior major finding — resolved **`$TAG` step-scoped → `${TAG#v}` empty in verify step** — `.forgejo/workflows/release.yml` Previously the verify step had no `env:` of its own, so `${TAG#v}` expanded to `''` and the assertion `assert v == ''` failed on every release. The fix adds the same source-of-truth env directly to the verify step: ```yaml - name: Verify version in built package env: TAG: ${{ github.event.release.tag_name }} # ← now present run: | ... expected = '${TAG#v}' # ← now expands correctly ``` Verified at head: `TAG` now appears on both the extract step (line 24) and the verify step (line 53). `${TAG#v}` in the verify step will now correctly strip the leading `v`. ✅ --- ### What still looks good (unchanged from prior review) - `importlib.metadata.version()` avoids importing `main.py` → no PySide6 / libGL dependency for the verify step. Correct minimal fix for the original headless-CI crash. - `--no-deps` install is appropriate (only metadata needed). - `_can_import_pyside6()` helper probes the C++ extensions (`QtCore` + `QtGui`), not just package presence. - `test_version_in_wheel_metadata` mirrors the CI verify logic and runs without PySide6. ### Verification - **CI/CD:** `ci.yml` runs `pytest` on PRs to main, but no CI result comment exists yet for head `5289cb1`. The `release.yml` workflow (where this fix lives) only triggers on `release: published`, so CI does not exercise this path. Per CI-absent policy, ran tests locally. - **Local:** `python3 -m pytest tests/ -q` at head `5289cb1` → **70 passed in 0.77s** (all 4 version tests, including the 2 PySide6-guarded ones which ran since PySide6 is importable in this sandbox). *Automated daily review. I never merge PRs.*
bjoern merged commit abe79545b2 into main 2026-06-28 21:48:46 +02:00
bjoern deleted branch fix/release-verify-headless 2026-06-28 21:48:46 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TeamAI/zai-tray-checker!9
No description provided.