-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: graceful fallback for symlink creation failures #2582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: graceful fallback for symlink creation failures #2582
Conversation
|
Please merge this fix - adk is broken in most common windows environments |
Agreed - please merge this!! This issue is a big inconvenience for those using Windows + ADK. Thanks Lorenzo for providing the fix! |
Merge #2582 Relate to #3306 ## Description Fixes issues #6 and #1785 where ADK commands crash on Windows due to symlink creation requiring admin privileges. ## Problem On Windows, running ADK commands (like `adk run`) fails with `OSError: [WinError 1314] A required privilege is not held by the client: ...` because the logging system attempts to create a symlink for the latest log file. Windows requires administrator privileges for symlink creation by default (unless Developer Mode is enabled), causing crashes for non-admin users. ## Root Cause The issue was in [`logs.py`](https://github.com/google/adk-python/blob/main/src/google/adk/cli/utils/logs.py#L72) where `os.symlink()` was called unconditionally without error handling, causing the entire CLI to crash when symlink creation failed. ## Solution This PR implements graceful symlink handling. Changes made: - Extracted symlink creation into separate function with error handling - Added graceful fallback when symlink creation fails - Replaced crashes with warnings to keep CLI functional - Improved user messaging about log file locations This solution follows a similar pattern to the one used in [ROS2](https://github.com/ros2) in its [logging module](https://github.com/ros2/launch/blob/rolling/launch/launch/logging/__init__.py#L93). ## Testing ### Before (broken) ```bash > adk run my_agent Log setup complete: C:\Users\username\AppData\Local\Temp\agents_log\agent.20250817_215119.log Traceback (most recent call last): File "google\adk\cli\utils\logs.py", line 72, in log_to_tmp_folder os.symlink(log_filepath, latest_log_link) OSError: [WinError 1314] A required privilege is not held by the client ``` ### After (fixed) ```bash > adk run my_agent Log setup complete: C:\Users\username\AppData\Local\Temp\agents_log\agent.20250817_215319.log UserWarning: Cannot create symlink for latest log file: [WinError 1314] A required privilege is not held by the client To access latest log: tail -F C:\Users\username\AppData\Local\Temp\agents_log\agent.20250817_215319.log Running agent my_agent, type exit to exit. ``` Co-authored-by: Wei Sun (Jack) <[email protected]> COPYBARA_INTEGRATE_REVIEW=#2582 from lorenzofavaro:fix/windows-symlink-permissions 1e99a62 PiperOrigin-RevId: 828690483
|
Thank you @lorenzofavaro for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 63b69fb. Closing this PR as the changes are now in the main branch. |
Description
Fixes issues #6 and #1785 where ADK commands crash on Windows due to symlink creation requiring admin privileges.
Problem
On Windows, running ADK commands (like
adk run) fails withOSError: [WinError 1314] A required privilege is not held by the client: ...because the logging system attempts to create a symlink for the latest log file. Windows requires administrator privileges for symlink creation by default (unless Developer Mode is enabled), causing crashes for non-admin users.Root Cause
The issue was in
logs.pywhereos.symlink()was called unconditionally without error handling, causing the entire CLI to crash when symlink creation failed.Solution
This PR implements graceful symlink handling. Changes made:
This solution follows a similar pattern to the one used in ROS2 in its logging module.
Testing
Before (broken)
After (fixed)