Linux Update scripts - #27
Conversation
bhavsarpratik
left a comment
There was a problem hiding this comment.
I'm Pratik's agent, reviewing on his behalf.
Linux update already works on main. scripts/install dispatches to install-linux (scripts/install:4-11), and scripts/update:140 calls scripts/install, so Linux already got the right installer. I tested the base script with a faked uname — it fast-forwards and writes phase: complete.
diff scripts/update scripts/update-linux is 9 lines out of 149. If there's a real Linux failure behind this, tell me what it is, because I can't reproduce one.
Two bugs in the split:
The safety guards are now untested. tests/test_meter.py:10465 checks --ff-only, the dirty-tree guard, no reset --hard, no sudo — on scripts/update only. The copy has none of that. update-windows.ps1:39 has already drifted this way.
A missing helper wedges the app. scripts/update:7 execs before write_status exists, and app.py:1662 only checks that scripts/update is executable. Phase starting is written first, stderr is dropped, nothing times out. If the exec fails you get "Updating…" forever. I reproduced it: exit 1, no status file.
I'd put the 3 real lines into scripts/update and drop the new file. Per-OS scripts are fine when they differ — install vs install-linux differ by 82 lines. These differ by 3.
Also: tests/test_linux_update.py is Linux-gated and there's no CI, so nothing runs it. Needs a Linux run log before merge, not after. And it calls update-linux directly, so the dispatcher is never tested. Minor: AGENTS.md:52 didn't get the new script that specs/AGENTS.md:50 did.
|
Can you provide a set of steps I can use to test update on linux? |
|
I'm Pratik's agent, reviewing on his behalf. I don't think we should merge this as-is. The existing Linux path already works because If we keep this design, please first:
After the PR is merged and a newer commit lands on git clone https://github.com/splunk/token-meter.git
cd token-meter
./scripts/installThen run the installed dispatcher: RUNTIME="${XDG_DATA_HOME:-$HOME/.local/share}/token-meter/runtime"
SOURCE="$(cat "$RUNTIME/SOURCE_CHECKOUT")"
STATUS="$HOME/.token-meter/update-status.json"
"$RUNTIME/scripts/update" "$SOURCE" "$STATUS"
python3 -m json.tool "$STATUS"The final status should contain test "$(git -C "$SOURCE" branch --show-current)" = main
test -z "$(git -C "$SOURCE" status --porcelain)"
test "$(git -C "$SOURCE" rev-parse HEAD)" = \
"$(git -C "$SOURCE" rev-parse origin/main)"
systemctl --user is-active token-meter-server.service
systemctl --user is-active token-meter-tray.service
curl -fsS http://127.0.0.1:8722/health
curl -fsS http://127.0.0.1:8722/menubarThe Linux integration test can be run with: python3 -m unittest discover -s tests -p 'test_linux_update.py' -vIf anything fails, please share the status JSON and the relevant systemd logs. Once this is addressed, I can re-review. |
Once this PR is merged I would like to test it with the next change to make it sure it works smoothly and also doesn't impact any other operating systems.