Repository navigation
Add native macOS support - #1
Conversation
Praveensenpai
left a comment
There was a problem hiding this comment.
Thank you for putting this together! Adding native macOS Keychain support and multi-platform CI release builds is a fantastic contribution.
Before we can merge, there is one critical issue on Linux that needs to be resolved, along with a minor architectural convention:
1. 🚨 Linux Keyring Write: Keep Raw JSON (Breaking Change)
In src/account/keyring/linux.rs:
pub fn write_keyring_token(token_json: &str) -> bool {
let encoded_token = encode_keyring_secret(token_json);
...- Issue: The
go-keyring-base64:prefix is specific tozalando/go-keyring's macOS implementation (keyring_darwin.go), where it works around argument escaping with the/usr/bin/securityCLI. On Linux,zalando/go-keyringcommunicates directly with Freedesktop Secret Service over D-Bus (secret_service/ss.go) and stores / expects raw UTF-8 JSON. - Impact: If
agymwritesgo-keyring-base64:...into Secret Service on Linux,agywill read that literal prefixed string instead of valid JSON and fail with an unmarshal error, breaking Antigravity CLI on Linux after an account switch. - Fix: Keep
normalize_keyring_secretfor reading (it gracefully handles plain text or base64), but insrc/account/keyring/linux.rs,write_keyring_tokenshould continue writing rawtoken_json.as_bytes()directly.macos.rscan keepencode_keyring_secret(token_json).
2. 📂 Modern Rust Module Layout
To conform with the repository's Rust 2018/2021 conventions (avoiding mod.rs files per clean-code rules):
- Please move
src/account/keyring/mod.rstosrc/account/keyring.rs, declaringlinuxandmacossubmodules:src/account/ ├── keyring.rs └── keyring/ ├── linux.rs └── macos.rs
3. 💡 Minor Polish (Optional)
install.sh: In theif [ -f "Cargo.toml" ]branch,build_from_source "$PWD"already copies the binary to$INSTALL_DIR/$BINARY_NAME. The secondcpon line 43 is redundant. Also, addingchmod +x "$INSTALL_DIR/$BINARY_NAME"after extracting downloaded release tarballs ensures execution permissions..github/workflows/release.yml: Line 1 is stillname: Release Linux Binary—renaming it toname: Build and Release Binarieswould reflect the new multi-platform builds.
Once the Linux write behavior and module pathing are updated, we'll gladly approve and merge this!
dc22d5c to
e500091
Compare
|
Addressed all requested changes:
The updated branch is |
Praveensenpai
left a comment
There was a problem hiding this comment.
Approved! Great work addressing the feedback. Merging now and patching the Linux dead-code cfg guards on main for v0.0.8 release.
What changed
go-keyring-base64:envelope.Verification
cargo test --all-targetspasses.cargo clippy --all-targets -- -D warningspasses.The fork branch is ready for upstream CI to validate the Linux and Intel macOS builds.