-
-
Notifications
You must be signed in to change notification settings - Fork 548
Support relative paths for Python/Node.js executables #4230
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
base: dev
Are you sure you want to change the base?
Changes from all commits
438bb8a
d61ec18
864eedd
3de9e53
ab5a0b4
c59249d
9070ff4
d16e43d
d49e5b4
9fa4c37
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 | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -42,5 +42,25 @@ public static bool PortableDataLocationInUse() | |||||||||||||||||||||||||||||||
| public const string PluginEnvironments = "Environments"; | ||||||||||||||||||||||||||||||||
| public const string PluginDeleteFile = "NeedDelete.txt"; | ||||||||||||||||||||||||||||||||
| public static readonly string PluginEnvironmentsPath = Path.Combine(DataDirectory(), PluginEnvironments); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||
| /// Resolves a path that may be relative to an absolute path. | ||||||||||||||||||||||||||||||||
| /// If the path is already absolute, returns it as-is. | ||||||||||||||||||||||||||||||||
| /// If the path is relative (starts with . or doesn't contain a drive), resolves it relative to ProgramDirectory. | ||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||
| /// <param name="path">The path to resolve</param> | ||||||||||||||||||||||||||||||||
| /// <returns>An absolute path</returns> | ||||||||||||||||||||||||||||||||
| public static string ResolveAbsolutePath(string path) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (string.IsNullOrEmpty(path)) | ||||||||||||||||||||||||||||||||
| return path; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // If already absolute, return as-is | ||||||||||||||||||||||||||||||||
| if (Path.IsPathRooted(path)) | ||||||||||||||||||||||||||||||||
| return path; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Resolve relative to ProgramDirectory | ||||||||||||||||||||||||||||||||
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); | ||||||||||||||||||||||||||||||||
|
Comment on lines
+62
to
+63
|
||||||||||||||||||||||||||||||||
| // Resolve relative to ProgramDirectory | |
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); | |
| // Resolve relative to ProgramDirectory, handling invalid path formats gracefully | |
| try | |
| { | |
| return Path.GetFullPath(Path.Combine(Constant.ProgramDirectory, path)); | |
| } | |
| catch (Exception ex) when (ex is ArgumentException || | |
| ex is NotSupportedException || | |
| ex is PathTooLongException) | |
| { | |
| // If the path cannot be resolved (invalid characters, format, or too long), | |
| // return the original path to avoid crashing the application. | |
| return path; | |
| } |
Copilot
AI
Jan 24, 2026
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.
The new ResolveAbsolutePath method lacks comprehensive test coverage. Given that this is a critical feature for portability and involves path resolution logic with edge cases (relative paths starting with ".", paths without drives, UNC paths, etc.), it should have unit tests to verify correct behavior across different scenarios. Consider adding tests in Flow.Launcher.Test to verify: 1) absolute paths are returned unchanged, 2) relative paths like ".\runtime\python.exe" resolve correctly, 3) paths without leading "." also resolve correctly, and 4) edge cases like null/empty strings are handled properly.
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.
The documentation comment states the method handles paths that "start with . or doesn't contain a drive", but the implementation only checks if the path is rooted using Path.IsPathRooted. This check returns false for relative paths (including those starting with ".") but may have unexpected behavior with UNC paths or paths on Unix-like systems. Consider clarifying the documentation to match the actual implementation, which relies on Path.IsPathRooted for the determination.