Skip to content

Conversation

pchuri
Copy link
Contributor

@pchuri pchuri commented Aug 27, 2025

This PR fixes Android/Termux builds and replaces nightly file locking with a stable approach.

Why

  • Android/Termux lacks platform support for arboard, causing build failures in the TUI crate.
  • The core crate used nightly-only file locking APIs; now uses stable std::fs file locking (Rust 1.89+).

Changes

  • tui: gate arboard under cfg(all(unix, not(target_os = "android"))).
  • tui: add Android stubs in clipboard_paste.rs returning a clear ClipboardUnavailable error.
  • core: use stabilized std::fs::File::try_lock() in message_history.rs for advisory file locks.
  • tests: updated a single TUI snapshot due to improved wrapping of long diff lines.
  • fix: restore missing tempfile::Builder import that was accidentally removed
  • refactor: modernize format! macros with string interpolation throughout codebase

Technical Details

Android Support

  • Problem: arboard crate doesn't support Android/Termux, causing compilation failures
  • Solution: Conditional compilation with cfg(all(unix, not(target_os = "android")))
  • Fallback: Clear error messages for Android users attempting clipboard operations

File Locking Migration

  • From: Nightly-only file locking APIs
  • To: Stabilized std::fs::File::try_lock() and try_lock_shared() (Rust 1.89+)
  • Benefits: No external dependencies, cross-platform compatibility, stable API
  • Compatibility: Maintains same advisory locking semantics with retry logic

Impact

  • Android/Termux builds succeed.
  • Clipboard image paste is disabled on Android with a clear message; all other platforms unaffected.
  • Stable file locking across platforms using standard library (no external dependencies).

Validation

  • Ran just fmt and just fix -p codex-core / -p codex-tui.
  • cargo test -p codex-tui: all pass after snapshot update.
  • cargo check --target aarch64-linux-android -p codex-tui: arboard dependency correctly gated
  • Local builds on macOS succeed with all tests passing
  • Full suite has environment-specific failures in exec tests on Termux (e.g., missing /bin/bash) not related to these changes.

pchuri and others added 4 commits August 28, 2025 00:26
- Add missing tempfile::Builder import in clipboard_paste.rs
- Remove unnecessary parentheses in chat_composer.rs
- Update diff render snapshot for improved line wrapping

Fixes compilation error where Builder was used but not imported.
Resolves test failures due to updated diff rendering behavior.
@pchuri pchuri changed the title tui: gate arboard on Android; core: use fd-lock for history locking feat: add Android/Termux support and stable file locking Aug 28, 2025
@bolinfest
Copy link
Collaborator

The core crate used nightly-only file locking APIs; replace with stable fd-lock for cross-platform advisory locks.

I thought this was stable in 1.89?

https://blog.rust-lang.org/2025/08/07/Rust-1.89.0/#stabilized-apis

- Remove fd-lock dependency and use stabilized std::fs::File::lock APIs
- Fix platform compatibility issues on Windows and Linux gnu targets
- Maintain same retry logic and advisory file locking behavior
- Fully compatible with Rust 1.89+ stable file locking features
@pchuri
Copy link
Contributor Author

pchuri commented Aug 28, 2025

You're absolutely right! Thank you for pointing that out.

I initially used fd-lock as an external dependency, but you're correct that std::fs::File::lock was stabilized in Rust 1.89.

I've just updated the implementation in commit 6a29eb8 to use the stabilized standard library APIs (try_lock() and try_lock_shared()) instead of the external fd-lock crate.

- Use conditional compilation for file locking on Windows
- Windows doesn't distinguish between shared/exclusive locks in try_lock_shared
- Maintains same retry logic and advisory locking behavior across platforms
@bolinfest
Copy link
Collaborator

@pchuri I just enabled uninlined_format_args for the workspace in #2845, so please rebase your PR on top of it so your PR doesn't have unrelated changes.

Relatedly, could you please split this into two PRs: one for the file lock stuff and one for gating the arboard dependency? These are separate concerns and I expect we can get things resolved quicker this way.

- Use platform-specific file locking APIs for better Windows compatibility
- Unix systems use try_lock_shared() for advisory read locks
- Windows systems use try_lock() due to different lock semantics
- Remove trailing whitespace to fix cargo fmt violations
@pchuri
Copy link
Contributor Author

pchuri commented Aug 29, 2025

@bolinfest Done! I've split this into two separate PRs as requested:

  1. File Locking PR feat: add stable file locking using std::fs APIs #2894:

    • Uses Rust 1.89+ stabilized std::fs::File locking APIs
    • Cross-platform implementation with Windows/Unix compatibility
    • No external dependencies
  2. Android/Termux Support PR feat: add Android/Termux support by gating arboard dependency #2895:

    • Gates arboard dependency for Android targets
    • Fixes compilation issues on Android/Termux
    • Maintains clipboard functionality on other platforms

Both PRs are rebased on the latest main branch that includes the uninlined_format_args changes from #2845. This should make the review process much cleaner and faster.

Thank you for the guidance on splitting these concerns

@pchuri
Copy link
Contributor Author

pchuri commented Aug 29, 2025

Closing this PR in favor of two separate PRs that address distinct concerns:

This provides cleaner separation of concerns and easier review process as requested by @bolinfest.

@pchuri pchuri closed this Aug 29, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants