Skip to content

.2059806565729300:ea2bd6200ad210607021cd167237f62b_69e3499ab13256d09ece8088.69e3996bb13256d09ece81a4.69e3996a7afba9383018b935:Trae CN.T(2026/4/18 22:47:07)#1271

Closed
Wpc-121 wants to merge 1 commit intoActivityWatch:masterfrom
Wpc-121:mod_ver_01
Closed

.2059806565729300:ea2bd6200ad210607021cd167237f62b_69e3499ab13256d09ece8088.69e3996bb13256d09ece81a4.69e3996a7afba9383018b935:Trae CN.T(2026/4/18 22:47:07)#1271
Wpc-121 wants to merge 1 commit intoActivityWatch:masterfrom
Wpc-121:mod_ver_01

Conversation

@Wpc-121
Copy link
Copy Markdown
Contributor

@Wpc-121 Wpc-121 commented Apr 18, 2026

重构版本号获取脚本,统一处理不同CI环境和本地环境的版本号获取
添加版本信息输出功能,便于调试和验证构建版本
修改相关脚本以使用新的版本号获取方式

重构版本号获取脚本,统一处理不同CI环境和本地环境的版本号获取
添加版本信息输出功能,便于调试和验证构建版本
修改相关脚本以使用新的版本号获取方式
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 18, 2026

Greptile Summary

This PR refactors getversion.sh to support a --strip-v flag, adds GITHUB_REF_NAME as the first-priority CI env source, and wires the new helper into package-all.sh, package-deb.sh, build_app_tauri.sh, and both CI workflows. The hardcoded VERSION="0.1.0" placeholder in the Tauri build script is replaced with a live call, which is the most impactful fix.

Confidence Score: 5/5

Safe to merge; all findings are P2 style suggestions with no correctness or data-integrity impact.

The refactoring is logically sound, set -e edge cases are handled with proper fallbacks, and the GITHUB_REF_NAME guard (v* prefix check) is correct. Only minor style issues remain.

scripts/package/package-deb.sh and scripts/package/package-all.sh for the double getversion.sh invocation pattern.

Important Files Changed

Filename Overview
scripts/package/getversion.sh Comprehensive refactor: adds --strip-v flag, structured argument parsing, usage help, GITHUB_REF_NAME support as first priority, and safe fallbacks (
scripts/package/package-all.sh Adds SCRIPT_DIR for robust path resolution, exposes get_version_no_prefix() function, moves version_no_prefix computation to top level; removes the winget-pkgs context comment explaining the v-prefix restriction.
scripts/package/package-deb.sh Switches from bash string slicing to getversion.sh --strip-v; now calls getversion.sh twice instead of once, spawning redundant git describe invocations.
.github/workflows/build.yml Adds "Determine and output version" step that sets VERSION_WITH_V and VERSION_NO_V env vars for debugging/downstream use; calls getversion.sh twice unnecessarily.
scripts/package/build_app_tauri.sh Replaces the hardcoded VERSION="0.1.0" placeholder with a dynamic call to getversion.sh --strip-v using SCRIPT_DIR.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getversion.sh called] --> B{GITHUB_REF_NAME set\nand starts with v?}
    B -- Yes --> C[Use GITHUB_REF_NAME]
    B -- No --> D{TRAVIS_TAG set?}
    D -- Yes --> E[Use TRAVIS_TAG]
    D -- No --> F{APPVEYOR_REPO_TAG_NAME set?}
    F -- Yes --> G[Use APPVEYOR tag]
    F -- No --> H[git describe --exact-match]
    H -- Found --> I[Use exact tag]
    H -- Not found --> J[git describe latest + .dev-HASH]
    J --> K[version string]
    C --> K
    E --> K
    G --> K
    I --> K
    K --> L{--strip-v flag?}
    L -- Yes --> M[sed remove leading v]
    L -- No --> N[Output as-is]
    M --> O[echo version]
    N --> O
Loading

Reviews (1): Last reviewed commit: "refactor(scripts): 统一版本号获取逻辑并添加版本信息输出" | Re-trigger Greptile

Comment on lines +10 to +11
VERSION_NUM="$("$SCRIPT_DIR/getversion.sh" --strip-v)"
echo "Version (with v): $VERSION"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Double invocation of getversion.sh

getversion.sh is called twice here (once for VERSION, once for VERSION_NUM), spawning two subshells and two git describe calls. The stripped form can be derived from the first result directly.

Suggested change
VERSION_NUM="$("$SCRIPT_DIR/getversion.sh" --strip-v)"
echo "Version (with v): $VERSION"
VERSION="$("$SCRIPT_DIR/getversion.sh")"
VERSION_NUM="$(echo "$VERSION" | sed -e 's/^v//')"

The same pattern also appears in both workflow files (lines 62-63 in build.yml and equivalently in build-tauri.yml) and in package-all.sh (via get_version + get_version_no_prefix), so this is a widespread pattern in the PR.

Comment on lines 94 to 98
exit 1
fi

# Windows installer version should not include 'v' prefix, see: https://github.com/microsoft/winget-pkgs/pull/17564
version_no_prefix="$(echo $version | sed -e 's/^v//')"
if [[ $TAURI_BUILD == "true" ]]; then
env AW_VERSION=$version_no_prefix "$innosetupdir/iscc.exe" scripts/package/aw-tauri.iss
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Removed context comment for Windows installer v-prefix requirement

The deleted line explained why the v prefix must be stripped for the Windows installer:

# Windows installer version should not include 'v' prefix, see: https://github.com/microsoft/winget-pkgs/pull/17564

This is a non-obvious requirement and the link to the winget-pkgs PR was valuable context for future maintainers. Consider restoring it near the build_setup function or the version_no_prefix assignment at the top of the file.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@ErikBjare ErikBjare closed this Apr 18, 2026
TimeToBuildBob pushed a commit to TimeToBuildBob/activitywatch that referenced this pull request Apr 19, 2026
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants