fix： use Rust binary name for portable packaging#108
Merged
zouyonghe merged 4 commits intoAstrBotDevs:mainfrom Apr 1, 2026
Merged
fix： use Rust binary name for portable packaging#108zouyonghe merged 4 commits intoAstrBotDevs:mainfrom
zouyonghe merged 4 commits intoAstrBotDevs:mainfrom
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 2 issues, and left some high level feedback:
- In
load_cargo_package_name, consider replacing the ad-hoc text parsing with Python'stomllib(or a TOML parser) so thatCargo.tomlformatting variations (whitespace, reordering, inline comments) and more complex configs are handled robustly. - The current approach assumes the Windows binary name always matches
[package].name; if the project later uses a different[[bin]]target name, this will fail, so it may be worth allowing an override or explicitly handling[[bin]]entries when resolving the executable name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `load_cargo_package_name`, consider replacing the ad-hoc text parsing with Python's `tomllib` (or a TOML parser) so that `Cargo.toml` formatting variations (whitespace, reordering, inline comments) and more complex configs are handled robustly.
- The current approach assumes the Windows binary name always matches `[package].name`; if the project later uses a different `[[bin]]` target name, this will fail, so it may be worth allowing an override or explicitly handling `[[bin]]` entries when resolving the executable name.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="149-158" />
<code_context>
return json.loads(config_path.read_text(encoding="utf-8"))
+def load_cargo_package_name(project_root: pathlib.Path) -> str:
+ cargo_toml_path = project_root / CARGO_TOML_RELATIVE_PATH
+ if not cargo_toml_path.is_file():
+ raise FileNotFoundError(f"Cargo.toml not found: {cargo_toml_path}")
+
+ package_section = False
+ for raw_line in cargo_toml_path.read_text(encoding="utf-8").splitlines():
+ line = raw_line.strip()
+ if not line or line.startswith("#"):
+ continue
+ if line.startswith("["):
+ package_section = line == "[package]"
+ continue
+ if package_section and line.startswith("name"):
+ _, _, value = line.partition("=")
+ binary_name = value.strip().strip('"').strip("'")
+ if binary_name:
+ return binary_name
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using a TOML parser instead of manual line parsing for Cargo.toml.
Manual parsing here is likely to break on valid TOML, e.g. `name = "foo" # comment`, varied spacing, or future constructs like multiline strings. Using a TOML library (`tomllib` on 3.11+ or `toml`) would be more robust: parse once and read `data["package"]["name"]` instead of iterating lines and tracking section state yourself.
Suggested implementation:
```python
def load_cargo_package_name(project_root: pathlib.Path) -> str:
cargo_toml_path = project_root / CARGO_TOML_RELATIVE_PATH
if not cargo_toml_path.is_file():
raise FileNotFoundError(f"Cargo.toml not found: {cargo_toml_path}")
# Parse Cargo.toml using a TOML parser instead of manual line parsing
# to robustly handle comments, spacing, and other valid TOML constructs.
try:
import tomllib # Python 3.11+
except ModuleNotFoundError:
# Fallback to the third‑party "toml" package if tomllib is unavailable.
import toml as tomllib # type: ignore[import]
with cargo_toml_path.open("rb") as f:
cargo_data = tomllib.load(f)
try:
package_table = cargo_data["package"]
name_value = package_table["name"]
except KeyError as exc:
raise ValueError(f"Missing [package].name in {CARGO_TOML_RELATIVE_PATH}") from exc
binary_name = str(name_value).strip()
if not binary_name:
raise ValueError(f"Empty [package].name in {CARGO_TOML_RELATIVE_PATH}")
return binary_name
```
1. If this project guarantees Python 3.11+ in CI, you can simplify the function by removing the `try/except` around the `tomllib` import and the `toml` fallback.
2. If you rely on the fallback, ensure `toml` is listed in the appropriate dependency file for your CI environment (e.g. `requirements.txt`, `pyproject.toml`, or the CI configuration’s install step).
3. If the file already has a module-level import policy (e.g. all imports at top of file), you may want to move the `tomllib`/`toml` import block up to the top-level imports to match existing style.
</issue_to_address>
### Comment 2
<location path="scripts/ci/package_windows_portable.py" line_range="162-164" />
<code_context>
+ if line.startswith("["):
+ package_section = line == "[package]"
+ continue
+ if package_section and line.startswith("name"):
+ _, _, value = line.partition("=")
+ binary_name = value.strip().strip('"').strip("'")
+ if binary_name:
+ return binary_name
</code_context>
<issue_to_address>
**issue (bug_risk):** Current value extraction will mis-handle `name` lines with inline comments.
When `name` has an inline comment (e.g. `name = "foo" # main binary`), `value.strip().strip('"').strip("'")` leaves the comment in `binary_name` (e.g. `foo" # main binary`). Consider stripping the inline comment before trimming quotes, e.g. split on `#` first, then `strip()` and remove quotes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Windows portable packaging script to use the binary name defined in Cargo.toml instead of the product name from the Tauri configuration when locating the main executable. It introduces a new function, load_cargo_package_name, to parse the package name from the project's manifest and updates the ProjectConfig structure and associated tests accordingly. Feedback was provided regarding the manual parsing logic in load_cargo_package_name, which is currently fragile and may incorrectly handle keys or trailing comments; a more robust parsing approach was suggested.
Comment on lines
+162
to
+167
| if package_section and line.startswith("name"): | ||
| _, _, value = line.partition("=") | ||
| binary_name = value.strip().strip('"').strip("'") | ||
| if binary_name: | ||
| return binary_name | ||
| break |
There was a problem hiding this comment.
The current manual parsing of Cargo.toml is fragile. It can incorrectly match keys that merely start with name (e.g., namesake), and it fails to correctly extract the value if there are trailing comments on the same line (e.g., name = "foo" # comment).
Consider a more robust approach to identify the name key and strip potential comments.
Suggested change
| if package_section and line.startswith("name"): | |
| _, _, value = line.partition("=") | |
| binary_name = value.strip().strip('"').strip("'") | |
| if binary_name: | |
| return binary_name | |
| break | |
| if package_section and "=" in line: | |
| key, value = map(str.strip, line.split("=", 1)) | |
| if key == "name": | |
| # Extract value, removing potential trailing comments and quotes | |
| binary_name = value.split("#", 1)[0].strip().strip('"').strip("'") | |
| if binary_name: | |
| return binary_name | |
| break |
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/ci/test_package_windows_portable.py" line_range="130-144" />
<code_context>
+ self.assertEqual(project_config.binary_name, "astrbot-desktop-tauri")
self.assertEqual(project_config.portable_marker_name, "portable.flag")
+ def test_load_cargo_package_name_supports_inline_comments(self):
+ with tempfile.TemporaryDirectory() as tmpdir:
+ project_root = Path(tmpdir)
+ cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
+ cargo_toml_path.parent.mkdir(parents=True)
+ cargo_toml_path.write_text(
+ '[package]\nname = "astrbot-desktop-tauri" # main binary\n'
+ )
+
+ self.assertEqual(
+ MODULE.load_cargo_package_name(project_root),
+ "astrbot-desktop-tauri",
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `load_cargo_package_name` error paths and fallback behavior
The new tests cover the happy path, but the function’s error and fallback branches are still untested:
- Missing `Cargo.toml` → `FileNotFoundError`
- Missing `[package]` table → `ValueError`
- `[package]` present but missing/empty `name` → `ValueError`
- `[[bin]]` present but empty or without `name` → should fall back to `[package].name`
Please add unit tests that mirror the existing tempdir setup to assert the correct exception types (and optionally messages) for the error cases, and to confirm the fallback returns the package name when `[[bin]]` entries lack `name`.
```suggestion
def test_load_cargo_package_name_supports_inline_comments(self):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
cargo_toml_path.parent.mkdir(parents=True)
cargo_toml_path.write_text(
'[package]\nname = "astrbot-desktop-tauri" # main binary\n'
)
self.assertEqual(
MODULE.load_cargo_package_name(project_root),
"astrbot-desktop-tauri",
)
def test_load_cargo_package_name_missing_cargo_toml_raises_file_not_found(self):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
# Intentionally DO NOT create src-tauri/Cargo.toml
with self.assertRaises(FileNotFoundError):
MODULE.load_cargo_package_name(project_root)
def test_load_cargo_package_name_missing_package_table_raises_value_error(self):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
cargo_toml_path.parent.mkdir(parents=True)
# No [package] table at all
cargo_toml_path.write_text(
'[workspace]\n'
'members = ["crates/*"]\n'
)
with self.assertRaises(ValueError):
MODULE.load_cargo_package_name(project_root)
def test_load_cargo_package_name_missing_package_name_raises_value_error(self):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
cargo_toml_path.parent.mkdir(parents=True)
# [package] table present but missing name
cargo_toml_path.write_text(
"[package]\n"
'version = "0.1.0"\n'
)
with self.assertRaises(ValueError):
MODULE.load_cargo_package_name(project_root)
def test_load_cargo_package_name_empty_package_name_raises_value_error(self):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
cargo_toml_path.parent.mkdir(parents=True)
# [package].name present but empty
cargo_toml_path.write_text(
"[package]\n"
'name = ""\n'
)
with self.assertRaises(ValueError):
MODULE.load_cargo_package_name(project_root)
def test_load_cargo_package_name_falls_back_to_package_when_bin_missing_name(self):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
cargo_toml_path.parent.mkdir(parents=True)
# [[bin]] table exists but has no name; should fall back to package.name
cargo_toml_path.write_text(
"[package]\n"
'name = "astrbot-desktop-tauri"\n'
"\n"
"[[bin]]\n"
'path = "src/main.rs"\n'
)
self.assertEqual(
MODULE.load_cargo_package_name(project_root),
"astrbot-desktop-tauri",
)
def test_load_cargo_package_name_prefers_explicit_bin_name(self):
```
</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
+130
to
+144
| def test_load_cargo_package_name_supports_inline_comments(self): | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| project_root = Path(tmpdir) | ||
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | ||
| cargo_toml_path.parent.mkdir(parents=True) | ||
| cargo_toml_path.write_text( | ||
| '[package]\nname = "astrbot-desktop-tauri" # main binary\n' | ||
| ) | ||
|
|
||
| self.assertEqual( | ||
| MODULE.load_cargo_package_name(project_root), | ||
| "astrbot-desktop-tauri", | ||
| ) | ||
|
|
||
| def test_load_cargo_package_name_prefers_explicit_bin_name(self): |
There was a problem hiding this comment.
suggestion (testing): Add tests for load_cargo_package_name error paths and fallback behavior
The new tests cover the happy path, but the function’s error and fallback branches are still untested:
- Missing
Cargo.toml→FileNotFoundError - Missing
[package]table →ValueError [package]present but missing/emptyname→ValueError[[bin]]present but empty or withoutname→ should fall back to[package].name
Please add unit tests that mirror the existing tempdir setup to assert the correct exception types (and optionally messages) for the error cases, and to confirm the fallback returns the package name when [[bin]] entries lack name.
Suggested change
| def test_load_cargo_package_name_supports_inline_comments(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | |
| cargo_toml_path.parent.mkdir(parents=True) | |
| cargo_toml_path.write_text( | |
| '[package]\nname = "astrbot-desktop-tauri" # main binary\n' | |
| ) | |
| self.assertEqual( | |
| MODULE.load_cargo_package_name(project_root), | |
| "astrbot-desktop-tauri", | |
| ) | |
| def test_load_cargo_package_name_prefers_explicit_bin_name(self): | |
| def test_load_cargo_package_name_supports_inline_comments(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | |
| cargo_toml_path.parent.mkdir(parents=True) | |
| cargo_toml_path.write_text( | |
| '[package]\nname = "astrbot-desktop-tauri" # main binary\n' | |
| ) | |
| self.assertEqual( | |
| MODULE.load_cargo_package_name(project_root), | |
| "astrbot-desktop-tauri", | |
| ) | |
| def test_load_cargo_package_name_missing_cargo_toml_raises_file_not_found(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| # Intentionally DO NOT create src-tauri/Cargo.toml | |
| with self.assertRaises(FileNotFoundError): | |
| MODULE.load_cargo_package_name(project_root) | |
| def test_load_cargo_package_name_missing_package_table_raises_value_error(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | |
| cargo_toml_path.parent.mkdir(parents=True) | |
| # No [package] table at all | |
| cargo_toml_path.write_text( | |
| '[workspace]\n' | |
| 'members = ["crates/*"]\n' | |
| ) | |
| with self.assertRaises(ValueError): | |
| MODULE.load_cargo_package_name(project_root) | |
| def test_load_cargo_package_name_missing_package_name_raises_value_error(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | |
| cargo_toml_path.parent.mkdir(parents=True) | |
| # [package] table present but missing name | |
| cargo_toml_path.write_text( | |
| "[package]\n" | |
| 'version = "0.1.0"\n' | |
| ) | |
| with self.assertRaises(ValueError): | |
| MODULE.load_cargo_package_name(project_root) | |
| def test_load_cargo_package_name_empty_package_name_raises_value_error(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | |
| cargo_toml_path.parent.mkdir(parents=True) | |
| # [package].name present but empty | |
| cargo_toml_path.write_text( | |
| "[package]\n" | |
| 'name = ""\n' | |
| ) | |
| with self.assertRaises(ValueError): | |
| MODULE.load_cargo_package_name(project_root) | |
| def test_load_cargo_package_name_falls_back_to_package_when_bin_missing_name(self): | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| project_root = Path(tmpdir) | |
| cargo_toml_path = project_root / "src-tauri" / "Cargo.toml" | |
| cargo_toml_path.parent.mkdir(parents=True) | |
| # [[bin]] table exists but has no name; should fall back to package.name | |
| cargo_toml_path.write_text( | |
| "[package]\n" | |
| 'name = "astrbot-desktop-tauri"\n' | |
| "\n" | |
| "[[bin]]\n" | |
| 'path = "src/main.rs"\n' | |
| ) | |
| self.assertEqual( | |
| MODULE.load_cargo_package_name(project_root), | |
| "astrbot-desktop-tauri", | |
| ) | |
| def test_load_cargo_package_name_prefers_explicit_bin_name(self): |
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
load_cargo_package_namefunction can return a[[bin]].namethat differs from the package name, so the name and docstring are a bit misleading now; consider renaming it (and associated variables) to something likeload_binary_name_from_cargoto reflect its behavior more accurately. - In
load_cargo_package_name, the error messages referenceCARGO_TOML_RELATIVE_PATHinstead of the resolvedcargo_toml_path, which could make failures harder to debug when the script is run from different locations; consider including the full path in those messages for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `load_cargo_package_name` function can return a `[[bin]].name` that differs from the package name, so the name and docstring are a bit misleading now; consider renaming it (and associated variables) to something like `load_binary_name_from_cargo` to reflect its behavior more accurately.
- In `load_cargo_package_name`, the error messages reference `CARGO_TOML_RELATIVE_PATH` instead of the resolved `cargo_toml_path`, which could make failures harder to debug when the script is run from different locations; consider including the full path in those messages for clarity.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
1 participant
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
productNamediffers from the built.exefilenameProblem
After PR #107 merged, the Windows portable zip step still failed in GitHub Actions. The portable packager looked for
src-tauri/target/release/AstrBot.exebecause it derived the executable name fromproductName, but the Windows build actually producessrc-tauri/target/release/astrbot-desktop-tauri.exe.Root Cause
The packaging script conflated the user-facing Tauri
productNamewith the real Rust package/binary name. On Windows those are different in this repository, so the portable packaging step could never find the built executable in CI.Fix
This change teaches
scripts/ci/package_windows_portable.pyto read the Rust package name fromsrc-tauri/Cargo.tomland use that value when locating the built release executable. The existingproductNamehandling stays intact for display-oriented metadata, while the actual file copy now targets the real binary path.The tests were extended to cover the exact failing case:
productName = AstrBotwhile the built executable isastrbot-desktop-tauri.exe. That locks in the expected behavior for future packaging changes.Validation
python3 -m unittest scripts.ci.test_package_windows_portablepython3 -m unittest discover -s scripts/ci -p 'test_*.py'python3 -m compileall -q scriptsnode --test $(find scripts -type f -name '*.test.mjs' | sort)Summary by Sourcery
Use the Rust binary name from Cargo.toml for Windows portable packaging instead of the Tauri product name.
Bug Fixes:
Tests: