refactor(scripts): improve version handling in build scripts#1280
refactor(scripts): improve version handling in build scripts#1280TimeToBuildBob wants to merge 2 commits intoActivityWatch:masterfrom
Conversation
- Replace hardcoded VERSION="0.1.0" in build_app_tauri.sh with dynamic getversion.sh call - Refactor getversion.sh to support multiple CI environments (GitHub Actions, Travis, AppVeyor) and add --strip-v flag - Use SCRIPT_DIR for robust relative path resolution in package scripts - Replace manual sed-based v-prefix stripping with --strip-v flag - Add verbose version logging in CI workflows for easier debugging Salvaged from ActivityWatch#1271 by @caoweiping.
Greptile SummaryThis PR refactors version-detection across the build scripts: Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/cleanup suggestions that do not affect correctness. The PR fixes a concrete bug (hardcoded version), improves path robustness, and adds CI visibility. All open comments are P2: unused GITHUB_ENV exports, double getversion.sh invocation, and an edge-case v* branch filter. None affect the primary use cases defined by the workflow triggers. .github/workflows/build.yml and .github/workflows/build-tauri.yml — the new GITHUB_ENV exports are dead code and could be either removed or wired to replace the older VERSION_TAG fallback pattern in the Package dmg step. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI / Local shell] --> B{Caller}
B --> C[build_app_tauri.sh]
B --> D[package-all.sh]
B --> E[package-deb.sh]
B --> F[GitHub Actions workflow step]
C -->|SCRIPT_DIR/getversion.sh --strip-v| G[getversion.sh]
D -->|get_version / get_version_no_prefix| G
E -->|getversion.sh / getversion.sh --strip-v| G
F -->|bash getversion.sh / getversion.sh --strip-v| G
G --> H{CI env var?}
H -->|GITHUB_REF_NAME starts with v| I[Use GITHUB_REF_NAME]
H -->|TRAVIS_TAG set| J[Use TRAVIS_TAG]
H -->|APPVEYOR_REPO_TAG_NAME set| K[Use APPVEYOR tag]
H -->|None| L[git describe --exact-match]
L -->|found| M[Release tag version]
L -->|not found| N[latest-tag.dev-shortsha]
I & J & K & M & N --> O{--strip-v?}
O -->|yes| P[sed strip leading v]
O -->|no| Q[Output as-is]
P & Q --> R[echo version]
Reviews (1): Last reviewed commit: "refactor(scripts): improve version handl..." | Re-trigger Greptile |
| echo "VERSION_WITH_V=${VERSION_WITH_V}" >> $GITHUB_ENV | ||
| echo "VERSION_NO_V=${VERSION_NO_V}" >> $GITHUB_ENV |
There was a problem hiding this comment.
GITHUB_ENV exports appear unused downstream
VERSION_WITH_V and VERSION_NO_V are written to $GITHUB_ENV here, but no subsequent step in build.yml references ${{ env.VERSION_WITH_V }} or ${{ env.VERSION_NO_V }}. The existing Package dmg step (line 196) still falls back to its own VERSION_TAG:-$(scripts/package/getversion.sh) pattern. The echo logging is useful, but the env-var exports are dead code as written.
Consider either removing the >> $GITHUB_ENV lines or updating downstream steps to consume these variables (e.g., replacing ${VERSION_TAG:-$(scripts/package/getversion.sh)} with ${VERSION_WITH_V}).
There was a problem hiding this comment.
Fixed in 89f8080: the Package dmg step now uses ${VERSION_WITH_V} (already exported to GITHUB_ENV by the version-logging step), eliminating the fallback getversion.sh call there.
| echo "VERSION_WITH_V=${VERSION_WITH_V}" >> $GITHUB_ENV | ||
| echo "VERSION_NO_V=${VERSION_NO_V}" >> $GITHUB_ENV |
There was a problem hiding this comment.
GITHUB_ENV exports appear unused downstream
Same pattern as build.yml — VERSION_WITH_V and VERSION_NO_V are exported to $GITHUB_ENV but no later step in build-tauri.yml consumes them. The Package dmg step at line 182 still uses the VERSION_TAG:-$(scripts/package/getversion.sh) fallback. If the intent is only debug logging, the >> $GITHUB_ENV lines can be dropped; if the intent is to consolidate version resolution, downstream steps should be updated to reference these new variables.
There was a problem hiding this comment.
Fixed in 89f8080: same as build.yml — Package dmg now uses ${VERSION_WITH_V} from GITHUB_ENV.
| if [[ -n "$GITHUB_REF_NAME" && "$GITHUB_REF_NAME" == v* ]]; then | ||
| _version="$GITHUB_REF_NAME" |
There was a problem hiding this comment.
GITHUB_REF_NAME == v* can match non-release branch names
Any branch whose name starts with 'v' (e.g. v2-refactor, v-next) will satisfy the == v* glob and be used verbatim as the version string instead of falling through to git describe. In the workflows' defined triggers this is low-risk, but getversion.sh is a standalone utility callable outside those constraints.
A tighter guard using GITHUB_REF:
if [[ -n "$GITHUB_REF" && "$GITHUB_REF" == refs/tags/v* ]]; then
_version="${GITHUB_REF_NAME}"This matches only actual tag pushes, not branches that happen to start with 'v'.
There was a problem hiding this comment.
Fixed in 89f8080: now checks $GITHUB_REF == refs/tags/v* instead of $GITHUB_REF_NAME == v*, so only actual tag pushes match.
| VERSION="$("$SCRIPT_DIR/getversion.sh")" | ||
| VERSION_NUM="$("$SCRIPT_DIR/getversion.sh" --strip-v)" |
There was a problem hiding this comment.
Double
getversion.sh invocation runs git twice
getversion.sh is called twice back-to-back, each time running git describe (the hot path when no CI env var is set). The same pattern appears in package-all.sh via get_version / get_version_no_prefix. A single call and one sed strip would be cleaner:
VERSION="$("$SCRIPT_DIR/getversion.sh")"
VERSION_NUM="$(echo "$VERSION" | sed -e 's/^v//')"This also matches what --strip-v does internally, so no behavior change occurs.
There was a problem hiding this comment.
Fixed in 89f8080: package-deb.sh now calls getversion.sh once and derives VERSION_NUM with sed -e 's/^v//'.
|
@TimeToBuildBob Address greptile's nits |
|
@TimeToBuildBob why not responding? |
- getversion.sh: tighten GITHUB_REF guard to refs/tags/v* to avoid matching v-prefixed branch names - package-deb.sh: call getversion.sh once, derive VERSION_NUM with sed (avoids double git-describe) - build.yml, build-tauri.yml: use VERSION_WITH_V env var in Package dmg step (wires up existing export, removes redundant getversion.sh call)
|
Addressed all 4 Greptile nits in 89f8080:
|
Summary
VERSION="0.1.0"inscripts/package/build_app_tauri.shwith a dynamic call togetversion.shgetversion.shto support multiple CI environments (GitHub Actions, Travis, AppVeyor) with a clean--strip-vflagSCRIPT_DIRfor robust relative path resolution inpackage-all.shandpackage-deb.shsed-based v-prefix stripping with--strip-vflag throughoutBackground
Salvaged from #1271 by @caoweiping (closed due to PR title issues). The changes themselves are clean and passed CI. Greptile rated them 4/5 confidence.
Changes
scripts/package/getversion.sh— refactored with multi-CI support and--strip-vflagscripts/package/build_app_tauri.sh— dynamic version instead of hardcoded0.1.0scripts/package/package-all.sh— uses$SCRIPT_DIRand--strip-vscripts/package/package-deb.sh— uses$SCRIPT_DIRand--strip-v.github/workflows/build.yml— adds version logging step.github/workflows/build-tauri.yml— adds version logging step