-
Notifications
You must be signed in to change notification settings - Fork 0
Fix/auto port fallback #101
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
Changes from all commits
cd8256f
5c83dec
b693edc
fed3042
11932f2
b7cb948
dde79a9
9df1260
222ea10
7bcc217
0de71a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,16 @@ public override DependencyStatus DetectPython() | |
| } | ||
| } | ||
|
|
||
| // Fallback: try to find python via uv | ||
| if (TryFindPythonViaUv(out version, out fullPath)) | ||
| { | ||
| status.IsAvailable = true; | ||
| status.Version = version; | ||
| status.Path = fullPath; | ||
| status.Details = $"Found Python {version} via uv"; | ||
| return status; | ||
| } | ||
|
|
||
| status.ErrorMessage = "Python not found in PATH"; | ||
| status.Details = "Install Python 3.10+ and ensure it's added to PATH."; | ||
| } | ||
|
|
@@ -86,6 +96,64 @@ public override string GetInstallationRecommendations() | |
| 3. MCP Server: Will be installed automatically by MCP for Unity Bridge"; | ||
| } | ||
|
|
||
| private bool TryFindPythonViaUv(out string version, out string fullPath) | ||
| { | ||
| version = null; | ||
| fullPath = null; | ||
|
|
||
| try | ||
| { | ||
| var psi = new ProcessStartInfo | ||
| { | ||
| FileName = "uv", // Assume uv is in path or user can't use this fallback | ||
| Arguments = "python list", | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| CreateNoWindow = true | ||
| }; | ||
|
|
||
| using var process = Process.Start(psi); | ||
| if (process == null) return false; | ||
|
|
||
| string output = process.StandardOutput.ReadToEnd(); | ||
| process.WaitForExit(5000); | ||
|
|
||
| if (process.ExitCode == 0 && !string.IsNullOrEmpty(output)) | ||
| { | ||
| var lines = output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); | ||
| foreach (var line in lines) | ||
| { | ||
| // Look for installed python paths | ||
| // Format is typically: <version> <path> | ||
| // Skip lines with "<download available>" | ||
| if (line.Contains("<download available>")) continue; | ||
|
|
||
| // The path is typically the last part of the line | ||
| var parts = line.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries); | ||
| if (parts.Length >= 2) | ||
| { | ||
| string potentialPath = parts[parts.Length - 1]; | ||
| if (File.Exists(potentialPath) && | ||
| (potentialPath.EndsWith("python.exe") || potentialPath.EndsWith("python3.exe"))) | ||
| { | ||
| if (TryValidatePython(potentialPath, out version, out fullPath)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+124
to
+146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chain🌐 Web query:
💡 Result: Short answer: uv's CLI doesn't provide a JSON/output-format for "uv python list", so don't rely on its human-readable lines for machine parsing. Instead either use uv python find to get a single interpreter path (safe for programmatic use), or parse a command that supports structured output (e.g. uv pip list --format json) or call the discovery logic from Python. See uv docs. [1][2] Concrete options:
Sources:
Use The |
||
| } | ||
| } | ||
| catch | ||
| { | ||
| // Ignore errors if uv is not installed or fails | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private bool TryValidatePython(string pythonPath, out string version, out string fullPath) | ||
| { | ||
| version = null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -119,17 +119,41 @@ private static int FindAvailablePort() | |||||||||||||||||||||||||||||||||||||||||
| /// <returns>True if port is available</returns> | ||||||||||||||||||||||||||||||||||||||||||
| public static bool IsPortAvailable(int port) | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| // Start with quick loopback check | ||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| var testListener = new TcpListener(IPAddress.Loopback, port); | ||||||||||||||||||||||||||||||||||||||||||
| testListener.Start(); | ||||||||||||||||||||||||||||||||||||||||||
| testListener.Stop(); | ||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| catch (SocketException) | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| #if UNITY_EDITOR_OSX | ||||||||||||||||||||||||||||||||||||||||||
| // On macOS, the OS might report the port as available (SO_REUSEADDR) even if another process | ||||||||||||||||||||||||||||||||||||||||||
| // is using it, unless we also check active connections or try a stricter bind. | ||||||||||||||||||||||||||||||||||||||||||
| // Double check by trying to Connect to it. If we CAN connect, it's NOT available. | ||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| using var client = new TcpClient(); | ||||||||||||||||||||||||||||||||||||||||||
| var connectTask = client.ConnectAsync(IPAddress.Loopback, port); | ||||||||||||||||||||||||||||||||||||||||||
| // If we connect successfully, someone is listening -> Not available | ||||||||||||||||||||||||||||||||||||||||||
| if (connectTask.Wait(50) && client.Connected) | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} bind succeeded but connection also succeeded -> Not available (Conflict)."); | ||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+140
to
+148
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Resource leak:
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: MCPForUnity/Editor/Helpers/PortManager.cs
Line: 140:148
Comment:
**style:** Resource leak: `TcpClient` is created without using statement, but `client.ConnectAsync()` returns a `Task` that's awaited synchronously with `.Wait(50)`. If connection times out or succeeds, the `TcpClient` may not be properly disposed before entering the `catch` block or returning.
```suggestion
try
{
using var client = new TcpClient();
var connectTask = client.ConnectAsync(IPAddress.Loopback, port);
// If we connect successfully, someone is listening -> Not available
if (connectTask.Wait(50) && client.Connected)
{
if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} bind succeeded but connection also succeeded -> Not available (Conflict).");
return false;
}
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||
| catch | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| // Connection failed -> likely available (or firewall blocked, but we assume available) | ||||||||||||||||||||||||||||||||||||||||||
| if (IsDebugEnabled()) McpLog.Info($"[PortManager] Port {port} connection failed -> likely available."); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check WaitForExit return value to handle timeouts.
The
WaitForExit(5000)call returns a boolean indicating whether the process exited within the timeout, but this return value is not checked. If the process times out,ExitCodemay not be valid.Apply this diff to handle the timeout case:
🤖 Prompt for AI Agents