feat: add windows portable zip artifacts#107
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
updater_mode.rstests, consider usingtempfile::TempDirinstead of manually constructing and cleaning a path understd::env::temp_dir()to avoid potential name collisions and guarantee cleanup even if the test fails early. - The
package_windows_portable.pyscript depends on7zbeing available but only discovers it heuristically; you might want to make that requirement explicit in the Windows workflow (e.g., install 7-Zip or setSEVEN_ZIP_EXE) to avoid environment-specific flakiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updater_mode.rs` tests, consider using `tempfile::TempDir` instead of manually constructing and cleaning a path under `std::env::temp_dir()` to avoid potential name collisions and guarantee cleanup even if the test fails early.
- The `package_windows_portable.py` script depends on `7z` being available but only discovers it heuristically; you might want to make that requirement explicit in the Windows workflow (e.g., install 7-Zip or set `SEVEN_ZIP_EXE`) to avoid environment-specific flakiness.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="18-20" />
<code_context>
+from scripts.ci.lib.release_artifacts import WINDOWS_UPDATER_PATTERNS, match_any
+
+
+WINDOWS_CANONICAL_INSTALLER_RE = re.compile(
+ r"(?P<name>.+?)_(?P<version>[^_]+)_windows_(?P<arch>x86_64|x64|amd64|arm64|aarch64)"
+ r"_setup(?P<nightly_suffix>_nightly_[0-9A-Fa-f]{7,40})?\.exe$"
+)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Nightly suffix length here can diverge from the 8‑char nightly pattern used in normalize_release_artifact_filenames, making portable zips non-normalizable.
Here `nightly_suffix` allows 7–40 hex chars, while `normalize_release_artifact_filenames.py` expects exactly 8 (`_nightly_[0-9a-fA-F]{8}`). If a nightly installer uses a non‑8‑char suffix, `installer_to_portable_name` will generate a portable zip name the normalizer can’t match, so it won’t be canonicalized. Please either restrict this to 8 chars or update the normalizer to accept the same range so both stay consistent.
</issue_to_address>
### Comment 2
<location path="scripts/ci/test_package_windows_portable.py" line_range="8" />
<code_context>
+from scripts.ci import package_windows_portable as MODULE
+
+
+class PackageWindowsPortableTests(unittest.TestCase):
+ def test_installer_to_portable_name_accepts_canonical_windows_name(self):
+ self.assertEqual(
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for error paths and edge cases in the packaging helper functions.
The tests only cover happy-path behavior for the portable packager; please add coverage for failure cases:
- Exercise the error path in `installer_to_portable_name` by passing an unexpected installer name (e.g. `"AstrBot-setup.exe"`) and asserting it raises `ValueError` with the expected message.
- For `find_nsis_payload_archive`, add tests that (a) run in a directory with no `.7z` files and assert `FileNotFoundError`, and (b) run in a directory with multiple candidate archives (e.g. `app-64.7z` and `app-32.7z` or several non-`app-` archives) and assert the expected `RuntimeError`.
- For `validate_portable_root_requires_expected_files`, add a test asserting that missing a top-level `*.exe` also causes a failure, so the top-level-exe requirement is enforced.
These will cover the main failure conditions without depending on external tools like 7-Zip.
</issue_to_address>
### Comment 3
<location path="scripts/ci/test_normalize_release_artifact_filenames.py" line_range="14" />
<code_context>
+SCRIPT_PATH = Path(MODULE.__file__)
+
+
+class NormalizeReleaseArtifactFilenamesTests(unittest.TestCase):
+ def test_main_normalizes_legacy_windows_portable_zip_name(self):
+ with tempfile.TemporaryDirectory() as tmpdir:
</code_context>
<issue_to_address>
**suggestion (testing):** Extend tests to cover canonical Windows portable zips and nightly suffix handling.
The new normalization rule now supports both legacy and canonical Windows portable zip names and adds an optional `nightly_suffix` group defaulted to an empty string.
To fully exercise this behavior, please add tests that:
- Cover a canonical portable zip with `_portable` and a nightly suffix (e.g. `AstrBot_4.29.0_windows_x64_portable_nightly_deadbeef12.zip` → `AstrBot_4.29.0_windows_amd64_portable_nightly_deadbeef12.zip`).
- Cover a canonical portable zip **without** a nightly suffix, ensuring it normalizes correctly and does not include the literal string `"None"` (exercising the `groups = {key: value or "" ...}` logic).
This will solidify the expected formats and guard against regressions in the regex and group handling.
Suggested implementation:
```python
class NormalizeReleaseArtifactFilenamesTests(unittest.TestCase):
def test_main_normalizes_canonical_windows_portable_zip_with_nightly_suffix(self):
with tempfile.TemporaryDirectory() as tmpdir:
root = Path(tmpdir)
original_name = "AstrBot_4.29.0_windows_x64_portable_nightly_deadbeef12.zip"
original_path = root / original_name
original_path.write_text("portable-nightly")
expected_name = "AstrBot_4.29.0_windows_amd64_portable_nightly_deadbeef12.zip"
expected_path = root / expected_name
stdout = io.StringIO()
stderr = io.StringIO()
argv = [
str(SCRIPT_PATH),
"--root",
str(root),
]
with redirect_stdout(stdout), redirect_stderr(stderr):
MODULE.main(argv)
# Original should be gone, normalized file should exist
self.assertFalse(original_path.exists(), original_path)
self.assertTrue(expected_path.exists(), expected_path)
# Sanity: no output or filenames should contain literal "None"
self.assertNotIn("None", stdout.getvalue())
self.assertNotIn("None", stderr.getvalue())
for path in root.iterdir():
self.assertNotIn("None", path.name)
def test_main_normalizes_canonical_windows_portable_zip_without_nightly_suffix(self):
with tempfile.TemporaryDirectory() as tmpdir:
root = Path(tmpdir)
original_name = "AstrBot_4.29.0_windows_x64_portable.zip"
original_path = root / original_name
original_path.write_text("portable")
expected_name = "AstrBot_4.29.0_windows_amd64_portable.zip"
expected_path = root / expected_name
stdout = io.StringIO()
stderr = io.StringIO()
argv = [
str(SCRIPT_PATH),
"--root",
str(root),
]
with redirect_stdout(stdout), redirect_stderr(stderr):
MODULE.main(argv)
# Original should be gone, normalized file should exist
self.assertFalse(original_path.exists(), original_path)
self.assertTrue(expected_path.exists(), expected_path)
# Sanity: no output or filenames should contain literal "None"
self.assertNotIn("None", stdout.getvalue())
self.assertNotIn("None", stderr.getvalue())
for path in root.iterdir():
self.assertNotIn("None", path.name)
```
These tests assume the following, based on the existing test structure:
1. `MODULE.main(argv)` is the correct way to invoke the normalization logic and accepts an `argv` list starting with the script path followed by `--root <path>`. If the actual invocation differs (e.g., `MODULE.main()` uses `sys.argv` directly or takes `(argv, stdout, stderr)`), adjust the call inside the `with redirect_stdout(...)` block to match the existing tests' pattern.
2. `io`, `tempfile`, `unittest`, and `redirect_stdout`/`redirect_stderr` are already imported at the top of this file as suggested by the snippet. If any of these are missing, add the appropriate imports.
3. If the script does not delete the original file but instead only renames or copies, update the assertions on `original_path.exists()` to match the intended behavior (e.g., remove that assertion or adapt it to the actual contract).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
changed the title
zouyonghe
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds support for Windows portable packages. It includes a new CI script to package NSIS installers as ZIP files with a portable marker, updates the desktop app to detect this marker and switch to manual updates, and adjusts artifact normalization and verification scripts. Feedback focuses on aligning nightly suffix regex patterns with project standards, improving CI debuggability by avoiding output redirection during extraction, and simplifying directory copying with shutil.copytree.
|
|
||
| WINDOWS_CANONICAL_INSTALLER_RE = re.compile( | ||
| r"(?P<name>.+?)_(?P<version>[^_]+)_windows_(?P<arch>x86_64|x64|amd64|arm64|aarch64)" | ||
| r"_setup(?P<nightly_suffix>_nightly_[0-9A-Fa-f]{7,40})?\.exe$" |
Contributor
There was a problem hiding this comment.
The nightly suffix regex here uses {7,40} for the short SHA, but the project standard (as seen in lib/release_artifacts.py and normalize_release_artifact_filenames.py) is exactly 8 characters. Aligning this ensures the normalize script correctly identifies the resulting portable zip.
Suggested change
| r"_setup(?P<nightly_suffix>_nightly_[0-9A-Fa-f]{7,40})?\.exe$" | |
| r"_setup(?P<nightly_suffix>_nightly_[0-9A-Fa-f]{8})?\.exe$" |
Comment on lines
+115
to
+121
| subprocess.run( | ||
| [seven_zip, "x", "-y", f"-o{output_dir}", str(archive_path)], | ||
| check=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| text=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
Capturing stdout and stderr without printing them on failure makes it difficult to debug CI issues when 7z fails. Removing the redirection allows the output to flow to the CI console.
subprocess.run(
[seven_zip, "x", "-y", f"-o{output_dir}", str(archive_path)],
check=True,
)
Comment on lines
+124
to
+133
| def copy_tree_contents( | ||
| source_root: pathlib.Path, destination_root: pathlib.Path | ||
| ) -> None: | ||
| destination_root.mkdir(parents=True, exist_ok=True) | ||
| for child in source_root.iterdir(): | ||
| target = destination_root / child.name | ||
| if child.is_dir(): | ||
| shutil.copytree(child, target) | ||
| else: | ||
| shutil.copy2(child, target) |
Contributor
There was a problem hiding this comment.
This manual iteration can be simplified using shutil.copytree with the dirs_exist_ok=True parameter (available in Python 3.8+). This handles creating the destination directory and merging contents in a single call.
Suggested change
| def copy_tree_contents( | |
| source_root: pathlib.Path, destination_root: pathlib.Path | |
| ) -> None: | |
| destination_root.mkdir(parents=True, exist_ok=True) | |
| for child in source_root.iterdir(): | |
| target = destination_root / child.name | |
| if child.is_dir(): | |
| shutil.copytree(child, target) | |
| else: | |
| shutil.copy2(child, target) | |
| def copy_tree_contents( | |
| source_root: pathlib.Path, destination_root: pathlib.Path | |
| ) -> None: | |
| shutil.copytree(source_root, destination_root, dirs_exist_ok=True) |
zouyonghe
changed the title
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
package_windows_portable.py,iter_installer_pathscurrently treats every*.exeinbundle_diras an installer; consider narrowing this (e.g., only matching canonical_setup.exenames) so the script won’t accidentally try to repack unrelated helper executables that may get dropped alongside the NSIS installer in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `package_windows_portable.py`, `iter_installer_paths` currently treats every `*.exe` in `bundle_dir` as an installer; consider narrowing this (e.g., only matching canonical `_setup.exe` names) so the script won’t accidentally try to repack unrelated helper executables that may get dropped alongside the NSIS installer in the future.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="168-169" />
<code_context>
+ )
+
+
+def iter_installer_paths(bundle_dir: pathlib.Path) -> Iterable[pathlib.Path]:
+ return sorted(path for path in bundle_dir.glob("*.exe") if path.is_file())
+
+
</code_context>
<issue_to_address>
**suggestion:** Consider narrowing the installer glob to avoid accidentally treating helper executables as installers
Because this returns all `*.exe` files, any future helper binaries in `bundle_dir` (e.g., auxiliary tools shipped with the installer) could cause `installer_to_portable_name` to raise `ValueError` or generate incorrect portable names. Restricting the pattern (e.g., `*setup*.exe` or using `WINDOWS_CANONICAL_INSTALLER_RE`) would make sure only real installer executables are considered.
Suggested implementation:
```python
def iter_installer_paths(bundle_dir: pathlib.Path) -> Iterable[pathlib.Path]:
"""Yield installer executables from a bundle directory.
Uses WINDOWS_CANONICAL_INSTALLER_RE to distinguish real installers from
helper executables that may also live in the bundle directory.
"""
return sorted(
path
for path in bundle_dir.glob("*.exe")
if path.is_file() and WINDOWS_CANONICAL_INSTALLER_RE.match(path.name)
)
```
If `WINDOWS_CANONICAL_INSTALLER_RE` is not already defined in this module, you will need to:
1. Add an appropriate compiled regex constant (e.g., `WINDOWS_CANONICAL_INSTALLER_RE = re.compile(r".*setup.*\.exe$", re.IGNORECASE)`), ideally near other constants.
2. Ensure any existing code using a different pattern for Windows installers is updated to use this shared constant, to keep behavior consistent.
</issue_to_address>
### Comment 2
<location path="scripts/ci/package_windows_portable.py" line_range="115-125" />
<code_context>
+ )
+
+
+def extract_archive(
+ archive_path: pathlib.Path, output_dir: pathlib.Path, seven_zip: str
+) -> None:
+ output_dir.mkdir(parents=True, exist_ok=True)
+ subprocess.run(
+ [seven_zip, "x", "-y", f"-o{output_dir}", str(archive_path)],
+ check=True,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ text=True,
+ )
+
</code_context>
<issue_to_address>
**suggestion (performance):** Capturing full 7-Zip output into memory may be unnecessary and could impact performance on large archives
`subprocess.run` currently captures all 7-Zip output in memory via `stdout=subprocess.PIPE` and `stderr=subprocess.STDOUT`, but the result isn’t used. For large archives this can consume unnecessary memory. Consider redirecting output to `subprocess.DEVNULL` (or inheriting stdout only in verbose mode) to avoid this overhead and handle very verbose 7-Zip output more robustly.
```suggestion
def extract_archive(
archive_path: pathlib.Path, output_dir: pathlib.Path, seven_zip: str
) -> None:
output_dir.mkdir(parents=True, exist_ok=True)
subprocess.run(
[seven_zip, "x", "-y", f"-o{output_dir}", str(archive_path)],
check=True,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
```
</issue_to_address>
### Comment 3
<location path=".github/workflows/build-desktop-tauri.yml" line_range="501-507" />
<code_context>
+
+ "SEVEN_ZIP_EXE=$sevenZip" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append
+
+ - name: Package portable zip artifact (Windows)
+ shell: bash
+ run: |
+ set -euo pipefail
+ python3 -m scripts.ci.package_windows_portable \
+ --bundle-dir src-tauri/target/release/bundle/nsis \
+ --output-dir src-tauri/target/release/bundle/nsis
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `python3` on Windows runners might be fragile; consider aligning with how Python is provisioned there
Since this job runs on `windows-*`, prefer the same Python entrypoint we use elsewhere on Windows (e.g., `python` or `setup-python`’s configured interpreter). That will avoid flaky failures on runners where `python3` isn’t installed or is mapped differently.
```suggestion
- name: Package portable zip artifact (Windows)
shell: bash
run: |
set -euo pipefail
python -m scripts.ci.package_windows_portable \
--bundle-dir src-tauri/target/release/bundle/nsis \
--output-dir src-tauri/target/release/bundle/nsis
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+501
to
+507
| - name: Package portable zip artifact (Windows) | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| python3 -m scripts.ci.package_windows_portable \ | ||
| --bundle-dir src-tauri/target/release/bundle/nsis \ | ||
| --output-dir src-tauri/target/release/bundle/nsis |
There was a problem hiding this comment.
suggestion (bug_risk): Using python3 on Windows runners might be fragile; consider aligning with how Python is provisioned there
Since this job runs on windows-*, prefer the same Python entrypoint we use elsewhere on Windows (e.g., python or setup-python’s configured interpreter). That will avoid flaky failures on runners where python3 isn’t installed or is mapped differently.
Suggested change
| - name: Package portable zip artifact (Windows) | |
| shell: bash | |
| run: | | |
| set -euo pipefail | |
| python3 -m scripts.ci.package_windows_portable \ | |
| --bundle-dir src-tauri/target/release/bundle/nsis \ | |
| --output-dir src-tauri/target/release/bundle/nsis | |
| - name: Package portable zip artifact (Windows) | |
| shell: bash | |
| run: | | |
| set -euo pipefail | |
| python -m scripts.ci.package_windows_portable \ | |
| --bundle-dir src-tauri/target/release/bundle/nsis \ | |
| --output-dir src-tauri/target/release/bundle/nsis |
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the Rust tests for
updater_mode, consider replacing the customScopedTempDirwithtempfile::TempDirto avoid maintaining your own temp directory management and to simplify cleanup. - In
package_windows_portable.py,resolve_project_root()assumes the script will always live underscripts/ci; if this layout changes the logic will silently break, so it may be safer to derive the project root from a more stable anchor (e.g. walking up untilsrc-tauriandtauri.conf.jsonare found).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Rust tests for `updater_mode`, consider replacing the custom `ScopedTempDir` with `tempfile::TempDir` to avoid maintaining your own temp directory management and to simplify cleanup.
- In `package_windows_portable.py`, `resolve_project_root()` assumes the script will always live under `scripts/ci`; if this layout changes the logic will silently break, so it may be safer to derive the project root from a more stable anchor (e.g. walking up until `src-tauri` and `tauri.conf.json` are found).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
package_windows_portable.py,iter_installer_pathscurrently packages every*.exeunder the NSIS bundle directory; consider restricting this to installer-style names (e.g. matching the canonical/legacy installer patterns or*_setup.exe) so that helper executables are never accidentally repackaged as portable zips. - The portable runtime detection in
resolve_desktop_update_modealways evaluatesis_windows_portable_runtime()even on non‑Windows targets; you could gate this behind acfg!(target_os = "windows")(or similar) to avoid unnecessary filesystem work and simplify reasoning on other platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `package_windows_portable.py`, `iter_installer_paths` currently packages every `*.exe` under the NSIS bundle directory; consider restricting this to installer-style names (e.g. matching the canonical/legacy installer patterns or `*_setup.exe`) so that helper executables are never accidentally repackaged as portable zips.
- The portable runtime detection in `resolve_desktop_update_mode` always evaluates `is_windows_portable_runtime()` even on non‑Windows targets; you could gate this behind a `cfg!(target_os = "windows")` (or similar) to avoid unnecessary filesystem work and simplify reasoning on other platforms.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The portable runtime marker filename is currently duplicated as a string in Rust (
PORTABLE_RUNTIME_MARKER) and Python (PORTABLE_MARKER_NAME); consider wiring both through a single shared constant (e.g., via build-time env or config) to avoid drift in future changes. - In
package_windows_portable.py,WINDOWS_UPDATER_PATTERNSare reused to recognize legacy installer names; if those patterns are ever expanded to cover updater binaries as well, this script could start treating updater executables as installers—consider either tightening the pattern here or adding a small dedicated pattern for legacy setup EXEs to make the intent explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The portable runtime marker filename is currently duplicated as a string in Rust (`PORTABLE_RUNTIME_MARKER`) and Python (`PORTABLE_MARKER_NAME`); consider wiring both through a single shared constant (e.g., via build-time env or config) to avoid drift in future changes.
- In `package_windows_portable.py`, `WINDOWS_UPDATER_PATTERNS` are reused to recognize legacy installer names; if those patterns are ever expanded to cover updater binaries as well, this script could start treating updater executables as installers—consider either tightening the pattern here or adding a small dedicated pattern for legacy setup EXEs to make the intent explicit.
## Individual Comments
### Comment 1
<location path="scripts/ci/test_package_windows_portable.py" line_range="9-12" />
<code_context>
+
+
+class PackageWindowsPortableTests(unittest.TestCase):
+ def test_installer_to_portable_name_accepts_canonical_windows_name(self):
+ self.assertEqual(
+ MODULE.installer_to_portable_name("AstrBot_4.29.0_windows_amd64_setup.exe"),
+ "AstrBot_4.29.0_windows_amd64_portable.zip",
+ )
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend installer name tests to cover additional architectures (e.g. arm64/aarch64) for both canonical and legacy patterns
Since `WINDOWS_CANONICAL_INSTALLER_RE` and `normalize_arch` also handle `arm64`/`aarch64`, please add tests that exercise those architectures for both canonical and legacy patterns. For instance, asserting that `AstrBot_4.29.0_windows_arm64_setup.exe` and `AstrBot_4.29.0_aarch64-setup.exe` both normalize to `AstrBot_4.29.0_windows_arm64_portable.zip` would directly validate the arch normalization logic.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/bridge/updater_mode.rs" line_range="54" />
<code_context>
+ is_windows_portable_runtime_with_exe_dir(exe_dir.as_deref())
+}
+
+fn windows_portable_runtime_for_target<F>(target_os: &str, detector: F) -> bool
+where
+ F: FnOnce() -> bool,
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the generic `windows_portable_runtime_for_target<F>` and its closure-based usage with a simple, non-generic helper that directly calls `is_windows_portable_runtime()` on Windows and returns false otherwise, plus simpler tests.
The `windows_portable_runtime_for_target<F>` helper adds indirection without buying much, and the generic + closure + `AtomicBool` test setup is harder to reason about than necessary.
You can keep all behavior (including “don’t run the portable detector on non-Windows”) with a simpler, non-generic helper:
```rust
fn windows_portable_runtime_for_target(target_os: &str) -> bool {
if target_os == "windows" {
is_windows_portable_runtime()
} else {
false
}
}
```
Use it in `resolve_desktop_update_mode`:
```rust
pub(crate) fn resolve_desktop_update_mode() -> DesktopUpdateMode {
let target_os = if cfg!(target_os = "windows") {
"windows"
} else if cfg!(target_os = "macos") {
"macos"
} else if cfg!(target_os = "linux") {
"linux"
} else {
"other"
};
resolve_desktop_update_mode_for_target(
target_os,
is_linux_appimage_runtime(),
windows_portable_runtime_for_target(target_os),
)
}
```
Then the test for “don’t run detection on non-Windows” can be expressed without `AtomicBool`/closures by unit-testing the helper behavior directly, e.g.:
```rust
#[test]
fn windows_portable_runtime_for_target_non_windows_is_false() {
assert!(!windows_portable_runtime_for_target("linux"));
assert!(!windows_portable_runtime_for_target("freebsd"));
}
#[test]
fn windows_portable_runtime_for_target_windows_uses_detector() {
// This can be an integration test on Windows, or exercised indirectly
// via resolve_desktop_update_mode on a Windows build.
}
```
This preserves all functionality (including skipping the filesystem check on non-Windows targets) while reducing generic/higher-order complexity and making the tests more straightforward.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+9
to
+12
| def test_installer_to_portable_name_accepts_canonical_windows_name(self): | ||
| self.assertEqual( | ||
| MODULE.installer_to_portable_name("AstrBot_4.29.0_windows_amd64_setup.exe"), | ||
| "AstrBot_4.29.0_windows_amd64_portable.zip", |
There was a problem hiding this comment.
suggestion (testing): Extend installer name tests to cover additional architectures (e.g. arm64/aarch64) for both canonical and legacy patterns
Since WINDOWS_CANONICAL_INSTALLER_RE and normalize_arch also handle arm64/aarch64, please add tests that exercise those architectures for both canonical and legacy patterns. For instance, asserting that AstrBot_4.29.0_windows_arm64_setup.exe and AstrBot_4.29.0_aarch64-setup.exe both normalize to AstrBot_4.29.0_windows_arm64_portable.zip would directly validate the arch normalization logic.
Member
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Apr 1, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/ci/test_normalize_release_artifact_filenames.py" line_range="39" />
<code_context>
+ def test_main_normalizes_canonical_windows_portable_zip_with_nightly_suffix(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for legacy nightly portable zip names with `-portable` syntax
Given the new `should_preserve_original_nightly_suffix` logic and the `WINDOWS_ARTIFACT_STEM_PATTERN_FRAGMENT` that supports both `-portable` and `_portable`, please add a test case for a legacy nightly portable zip such as `AstrBot_4.29.0_x64-portable_nightly_deadbeef.zip`. This will verify that nightly suffix preservation and normalization also work for the legacy `-portable` naming style.
```suggestion
def test_main_normalizes_legacy_windows_portable_zip_with_nightly_suffix(self):
with tempfile.TemporaryDirectory() as tmpdir:
root = Path(tmpdir)
original_name = "AstrBot_4.29.0_x64-portable_nightly_deadbeef.zip"
original_path = root / original_name
original_path.write_text("portable-nightly")
stdout = io.StringIO()
stderr = io.StringIO()
argv = [
str(SCRIPT_PATH),
"--root",
str(root),
"--entrypoint",
"AstrBot",
"--platform",
"windows",
"--version",
"4.29.0",
"--arch",
"x64",
"--portable",
"--nightly",
"--dry-run",
]
with mock.patch("sys.argv", argv):
with redirect_stdout(stdout), redirect_stderr(stderr):
exit_code = MODULE.main()
self.assertEqual(exit_code, 0)
# Legacy nightly portable zip should be normalized with nightly suffix preserved
self.assertFalse((root / original_name).exists())
self.assertTrue(
(root / "AstrBot_4.29.0_windows_x64_portable_nightly_deadbeef.zip").exists()
)
def test_main_normalizes_canonical_windows_portable_zip_with_nightly_suffix(self):
```
</issue_to_address>
### Comment 2
<location path="scripts/ci/package_windows_portable.py" line_range="38" />
<code_context>
+WINDOWS_CLEANUP_SCRIPT_RELATIVE_PATH = (
+ pathlib.Path("src-tauri") / "windows" / "kill-backend-processes.ps1"
+)
+PORTABLE_RUNTIME_MARKER_RELATIVE_PATH = (
+ pathlib.Path("src-tauri") / "windows" / "portable-runtime-marker.txt"
+)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring to avoid import-time filesystem side effects and inlining very thin copy helpers so that control flow and layout assumptions are clearer in one place.
You can keep behavior identical while reducing structural complexity with two small refactors:
---
### 1. Avoid import‑time filesystem I/O for the marker
Move marker loading into `main()` and thread the value down instead of using a global. This keeps imports side‑effect‑free and makes testing easier.
**Before:**
```python
PORTABLE_RUNTIME_MARKER_RELATIVE_PATH = (
pathlib.Path("src-tauri") / "windows" / "portable-runtime-marker.txt"
)
def load_portable_runtime_marker(project_root: pathlib.Path) -> str:
marker_path = project_root / PORTABLE_RUNTIME_MARKER_RELATIVE_PATH
if not marker_path.is_file():
raise FileNotFoundError(
f"Portable runtime marker file not found: {marker_path}"
)
marker_name = marker_path.read_text(encoding="utf-8").strip()
if not marker_name:
raise ValueError(f"Portable runtime marker file is empty: {marker_path}")
return marker_name
PORTABLE_MARKER_NAME = load_portable_runtime_marker(resolve_project_root())
```
```python
def add_portable_runtime_files(destination_root: pathlib.Path) -> None:
(destination_root / PORTABLE_MARKER_NAME).write_text("", encoding="utf-8")
(destination_root / PORTABLE_README_NAME).write_text(
PORTABLE_README_TEXT,
encoding="utf-8",
)
```
```python
def populate_portable_root(
bundle_dir: pathlib.Path,
destination_root: pathlib.Path,
project_root: pathlib.Path,
) -> None:
...
add_portable_runtime_files(destination_root)
validate_portable_root(destination_root)
```
**After (no global I/O, explicit threading):**
```python
PORTABLE_RUNTIME_MARKER_RELATIVE_PATH = (
pathlib.Path("src-tauri") / "windows" / "portable-runtime-marker.txt"
)
def load_portable_runtime_marker(project_root: pathlib.Path) -> str:
marker_path = project_root / PORTABLE_RUNTIME_MARKER_RELATIVE_PATH
if not marker_path.is_file():
raise FileNotFoundError(
f"Portable runtime marker file not found: {marker_path}"
)
marker_name = marker_path.read_text(encoding="utf-8").strip()
if not marker_name:
raise ValueError(f"Portable runtime marker file is empty: {marker_path}")
return marker_name
```
```python
def add_portable_runtime_files(
destination_root: pathlib.Path, portable_marker_name: str
) -> None:
(destination_root / portable_marker_name).write_text("", encoding="utf-8")
(destination_root / PORTABLE_README_NAME).write_text(
PORTABLE_README_TEXT,
encoding="utf-8",
)
```
```python
def populate_portable_root(
bundle_dir: pathlib.Path,
destination_root: pathlib.Path,
project_root: pathlib.Path,
portable_marker_name: str,
) -> None:
...
add_portable_runtime_files(destination_root, portable_marker_name)
validate_portable_root(destination_root)
```
```python
def package_installer(
installer_path: pathlib.Path,
output_dir: pathlib.Path,
project_root: pathlib.Path,
portable_marker_name: str,
) -> pathlib.Path:
...
populate_portable_root(
bundle_dir=installer_path.parent,
destination_root=archive_root,
project_root=project_root,
portable_marker_name=portable_marker_name,
)
...
```
```python
def main() -> int:
args = parse_args()
...
project_root = resolve_project_root()
portable_marker_name = load_portable_runtime_marker(project_root)
for installer_path in installer_paths:
archive_path = package_installer(
installer_path,
output_dir,
project_root,
portable_marker_name,
)
print(f"[windows-portable] created: {archive_path.name}")
return 0
```
This removes the import‑time `resolve_project_root()` and marker file read, while keeping behavior identical.
---
### 2. Inline very thin copy helpers into `populate_portable_root`
`copy_required_directory` and `copy_optional_file` are only used in `populate_portable_root` and mostly wrap single `shutil` calls, which makes the layout assumptions harder to see.
**Before:**
```python
def copy_required_directory(
source_root: pathlib.Path, destination_root: pathlib.Path
) -> None:
if not source_root.is_dir():
raise FileNotFoundError(f"Required directory not found: {source_root}")
shutil.copytree(source_root, destination_root)
def copy_optional_file(
source_path: pathlib.Path, destination_path: pathlib.Path
) -> None:
if source_path.is_file():
destination_path.parent.mkdir(parents=True, exist_ok=True)
shutil.copy2(source_path, destination_path)
```
```python
def populate_portable_root(
bundle_dir: pathlib.Path,
destination_root: pathlib.Path,
project_root: pathlib.Path,
portable_marker_name: str,
) -> None:
release_dir = resolve_release_dir(bundle_dir)
main_executable_path = resolve_main_executable_path(bundle_dir, project_root)
destination_root.mkdir(parents=True, exist_ok=True)
shutil.copy2(main_executable_path, destination_root / main_executable_path.name)
copy_optional_file(
release_dir / "WebView2Loader.dll",
destination_root / "WebView2Loader.dll",
)
copy_optional_file(
project_root / WINDOWS_CLEANUP_SCRIPT_RELATIVE_PATH,
destination_root / "kill-backend-processes.ps1",
)
resources_root = destination_root / "resources"
copy_required_directory(
project_root / BACKEND_RESOURCE_RELATIVE_PATH,
resources_root / "backend",
)
copy_required_directory(
project_root / WEBUI_RESOURCE_RELATIVE_PATH,
resources_root / "webui",
)
add_portable_runtime_files(destination_root, portable_marker_name)
validate_portable_root(destination_root)
```
**After (helpers inlined, logic visible in one place):**
```python
def populate_portable_root(
bundle_dir: pathlib.Path,
destination_root: pathlib.Path,
project_root: pathlib.Path,
portable_marker_name: str,
) -> None:
release_dir = resolve_release_dir(bundle_dir)
main_executable_path = resolve_main_executable_path(bundle_dir, project_root)
destination_root.mkdir(parents=True, exist_ok=True)
shutil.copy2(main_executable_path, destination_root / main_executable_path.name)
# Optional files
webview_loader = release_dir / "WebView2Loader.dll"
if webview_loader.is_file():
shutil.copy2(webview_loader, destination_root / "WebView2Loader.dll")
cleanup_script = project_root / WINDOWS_CLEANUP_SCRIPT_RELATIVE_PATH
if cleanup_script.is_file():
(destination_root).mkdir(parents=True, exist_ok=True)
shutil.copy2(cleanup_script, destination_root / "kill-backend-processes.ps1")
# Required resource directories
resources_root = destination_root / "resources"
backend_src = project_root / BACKEND_RESOURCE_RELATIVE_PATH
if not backend_src.is_dir():
raise FileNotFoundError(f"Required directory not found: {backend_src}")
shutil.copytree(backend_src, resources_root / "backend")
webui_src = project_root / WEBUI_RESOURCE_RELATIVE_PATH
if not webui_src.is_dir():
raise FileNotFoundError(f"Required directory not found: {webui_src}")
shutil.copytree(webui_src, resources_root / "webui")
add_portable_runtime_files(destination_root, portable_marker_name)
validate_portable_root(destination_root)
```
This keeps all validations and behavior but removes two indirections, making the directory/layout assumptions obvious at the call site.
</issue_to_address>Hi @zouyonghe! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.| (root / "AstrBot_4.29.0_windows_amd64_portable.zip").exists() | ||
| ) | ||
|
|
||
| def test_main_normalizes_canonical_windows_portable_zip_with_nightly_suffix(self): |
There was a problem hiding this comment.
suggestion (testing): Add coverage for legacy nightly portable zip names with -portable syntax
Given the new should_preserve_original_nightly_suffix logic and the WINDOWS_ARTIFACT_STEM_PATTERN_FRAGMENT that supports both -portable and _portable, please add a test case for a legacy nightly portable zip such as AstrBot_4.29.0_x64-portable_nightly_deadbeef.zip. This will verify that nightly suffix preservation and normalization also work for the legacy -portable naming style.
Suggested change
| def test_main_normalizes_canonical_windows_portable_zip_with_nightly_suffix(self): | |
| def test_main_normalizes_legacy_windows_portable_zip_with_nightly_suffix(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| root = Path(tmpdir) | |
| original_name = "AstrBot_4.29.0_x64-portable_nightly_deadbeef.zip" | |
| original_path = root / original_name | |
| original_path.write_text("portable-nightly") | |
| stdout = io.StringIO() | |
| stderr = io.StringIO() | |
| argv = [ | |
| str(SCRIPT_PATH), | |
| "--root", | |
| str(root), | |
| "--entrypoint", | |
| "AstrBot", | |
| "--platform", | |
| "windows", | |
| "--version", | |
| "4.29.0", | |
| "--arch", | |
| "x64", | |
| "--portable", | |
| "--nightly", | |
| "--dry-run", | |
| ] | |
| with mock.patch("sys.argv", argv): | |
| with redirect_stdout(stdout), redirect_stderr(stderr): | |
| exit_code = MODULE.main() | |
| self.assertEqual(exit_code, 0) | |
| # Legacy nightly portable zip should be normalized with nightly suffix preserved | |
| self.assertFalse((root / original_name).exists()) | |
| self.assertTrue( | |
| (root / "AstrBot_4.29.0_windows_x64_portable_nightly_deadbeef.zip").exists() | |
| ) | |
| def test_main_normalizes_canonical_windows_portable_zip_with_nightly_suffix(self): |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
package_windows_portable.py,PORTABLE_MARKER_NAMEis computed at import time by reading from the real project root; consider deferringload_portable_runtime_marker()untilmain()(or when first needed) so the module can be imported in more contexts without immediately failing if the marker file is missing or temporarily invalid. - The portable packager currently infers the main
.exeviaproductNameintauri.conf.jsonand a fixedtarget/.../releaselayout; you may want to pass the resolved executable path (or name) in via CLI or a single helper so layout or naming changes don’t silently break the portable build logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `package_windows_portable.py`, `PORTABLE_MARKER_NAME` is computed at import time by reading from the real project root; consider deferring `load_portable_runtime_marker()` until `main()` (or when first needed) so the module can be imported in more contexts without immediately failing if the marker file is missing or temporarily invalid.
- The portable packager currently infers the main `.exe` via `productName` in `tauri.conf.json` and a fixed `target/.../release` layout; you may want to pass the resolved executable path (or name) in via CLI or a single helper so layout or naming changes don’t silently break the portable build logic.
## Individual Comments
### Comment 1
<location path="src-tauri/build.rs" line_range="10-12" />
<code_context>
+ let marker = fs::read_to_string(&marker_path)
+ .unwrap_or_else(|error| panic!("failed to read {}: {error}", marker_path.display()));
+ let marker = marker.trim();
+ assert!(
+ !marker.is_empty(),
+ "portable runtime marker file is empty: {}",
+ marker_path.display()
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** The new assertion requires a non-empty marker file, but the added portable-runtime-marker.txt is currently empty, which will break the build.
The build now trims and uses the contents of `windows/portable-runtime-marker.txt` as the marker name and panics with `portable runtime marker file is empty` if it has no content. To avoid a failing build, either:
- Commit `portable-runtime-marker.txt` with the intended marker filename (e.g. `.astrbot-portable-runtime`), or
- Relax the assertion to tolerate an empty file and fall back to a default marker name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The portable runtime marker name is resolved at import time in
package_windows_portable.py(PORTABLE_MARKER_NAME = load_portable_runtime_marker(resolve_project_root())), which makes the module tightly coupled to the repo layout and filesystem; consider deferring this I/O tomain()or an explicit initializer so the module is easier to reuse and test in isolation or alternate layouts. is_windows_portable_runtime()recomputes the current exe directory and hits the filesystem every time it is called; if this ends up being used in more than one place, consider caching the boolean result once at startup to avoid repeated I/O.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The portable runtime marker name is resolved at import time in `package_windows_portable.py` (`PORTABLE_MARKER_NAME = load_portable_runtime_marker(resolve_project_root())`), which makes the module tightly coupled to the repo layout and filesystem; consider deferring this I/O to `main()` or an explicit initializer so the module is easier to reuse and test in isolation or alternate layouts.
- `is_windows_portable_runtime()` recomputes the current exe directory and hits the filesystem every time it is called; if this ends up being used in more than one place, consider caching the boolean result once at startup to avoid repeated I/O.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="80" />
<code_context>
+ return marker_name
+
+
+PORTABLE_MARKER_NAME = load_portable_runtime_marker(resolve_project_root())
+
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a lazily loaded ProjectConfig object that is created in main() and threaded through helper functions instead of using global state and repeated config lookups.
You can reduce complexity and side‑effects by centralizing the “project config” and loading it lazily in `main()` instead of at import time, while also avoiding repeated config/IO calls.
### 1. Remove side‑effectful global and centralize config
Instead of:
```python
def resolve_project_root() -> pathlib.Path:
return resolve_project_root_from(pathlib.Path(__file__))
def load_portable_runtime_marker(project_root: pathlib.Path) -> str:
...
PORTABLE_MARKER_NAME = load_portable_runtime_marker(resolve_project_root())
```
introduce a small config object that’s created in `main()` and passed down:
```python
from dataclasses import dataclass
@dataclass
class ProjectConfig:
root: pathlib.Path
product_name: str
portable_marker_name: str
def load_project_config() -> ProjectConfig:
project_root = resolve_project_root_from(pathlib.Path(__file__))
config = load_tauri_config(project_root)
product_name = str(config.get("productName", "")).strip()
if not product_name:
raise ValueError(f"Missing productName in {TAURI_CONFIG_RELATIVE_PATH}")
marker_path = project_root / PORTABLE_RUNTIME_MARKER_RELATIVE_PATH
if not marker_path.is_file():
raise FileNotFoundError(f"Portable runtime marker file not found: {marker_path}")
marker_name = marker_path.read_text(encoding="utf-8").strip()
if not marker_name:
raise ValueError(f"Portable runtime marker file is empty: {marker_path}")
return ProjectConfig(
root=project_root,
product_name=product_name,
portable_marker_name=marker_name,
)
```
Then in `main()`:
```python
def main() -> int:
args = parse_args()
bundle_dir = pathlib.Path(args.bundle_dir).resolve()
...
project_config = load_project_config()
for installer_path in installer_paths:
archive_path = package_installer(
installer_path=installer_path,
output_dir=output_dir,
project_config=project_config,
)
print(f"[windows-portable] created: {archive_path.name}")
return 0
```
### 2. Thread config instead of multiple helpers/globals
Adjust the functions that currently derive things separately:
```python
def resolve_main_executable_path(
bundle_dir: pathlib.Path, project_config: ProjectConfig
) -> pathlib.Path:
release_dir = resolve_release_dir(bundle_dir)
main_executable_path = release_dir / f"{project_config.product_name}.exe"
if not main_executable_path.is_file():
raise FileNotFoundError(f"Main executable not found: {main_executable_path}")
return main_executable_path
```
```python
def populate_portable_root(
bundle_dir: pathlib.Path,
destination_root: pathlib.Path,
project_config: ProjectConfig,
) -> None:
release_dir = resolve_release_dir(bundle_dir)
main_executable_path = resolve_main_executable_path(bundle_dir, project_config)
destination_root.mkdir(parents=True, exist_ok=True)
shutil.copy2(main_executable_path, destination_root / main_executable_path.name)
copy_optional_file(
release_dir / "WebView2Loader.dll",
destination_root / "WebView2Loader.dll",
)
copy_optional_file(
project_config.root / WINDOWS_CLEANUP_SCRIPT_RELATIVE_PATH,
destination_root / "kill-backend-processes.ps1",
)
resources_root = destination_root / "resources"
copy_required_directory(
project_config.root / BACKEND_RESOURCE_RELATIVE_PATH,
resources_root / "backend",
)
copy_required_directory(
project_config.root / WEBUI_RESOURCE_RELATIVE_PATH,
resources_root / "webui",
)
add_portable_runtime_files(destination_root, project_config)
validate_portable_root(destination_root)
```
And make `add_portable_runtime_files` use the config instead of the global:
```python
def add_portable_runtime_files(
destination_root: pathlib.Path, project_config: ProjectConfig
) -> None:
(destination_root / project_config.portable_marker_name).write_text(
"", encoding="utf-8"
)
(destination_root / PORTABLE_README_NAME).write_text(
PORTABLE_README_TEXT,
encoding="utf-8",
)
```
Update `package_installer` to accept `project_config` instead of `project_root`:
```python
def package_installer(
installer_path: pathlib.Path,
output_dir: pathlib.Path,
project_config: ProjectConfig,
) -> pathlib.Path:
...
populate_portable_root(
bundle_dir=installer_path.parent,
destination_root=archive_root,
project_config=project_config,
)
```
This keeps all existing behavior, but:
- eliminates import‑time filesystem IO and the global `PORTABLE_MARKER_NAME`,
- consolidates project‑root / tauri config / marker resolution into a single `load_project_config`,
- avoids re‑reading the tauri config or recomputing the product name in several helpers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
portable.ziprelease assetProblem
Issue #103 asks for a downloadable Windows archive that users can unzip directly instead of installing through NSIS. Today the release pipeline only publishes Windows setup executables, so users who want a portable copy have to unpack the installer themselves.
Root Cause
The workflow only collected NSIS
.exeoutputs, and the runtime had no way to distinguish a repacked portable folder from a normal installed build. That meant any hand-made portable archive would still inherit installer-oriented update behavior.Fix
This change adds
scripts/ci/package_windows_portable.py, which extracts the NSIS payload, validates the packaged layout, drops in a portable marker/README, and emits a canonicalAstrBot_<version>_windows_<arch>_portable.zipartifact. The Windows build job now publishes that zip alongside the existing installer, and the artifact normalization/check scripts understand the new naming.To keep behavior safe at runtime, Windows builds that contain
portable.flagnow resolve to the manual-download updater mode instead of the native installer updater path. The portable README and release notes also call out that WebView2 must already exist on the host machine.Validation
python3 -m unittest discover -s scripts/ci -p 'test_*.py'python3 -m compileall -q scriptscargo fmt --manifest-path src-tauri/Cargo.toml --all -- --checkcargo test --manifest-path src-tauri/Cargo.toml --lockedCloses #103.
Summary by Sourcery
Add Windows portable ZIP packaging and runtime detection so Windows portable builds are published alongside installers and use manual-download updates.
New Features:
Bug Fixes:
Enhancements:
Build:
CI: