Support relative paths for Python/Node.js executables#4230
Support relative paths for Python/Node.js executables#4230Jack251970 wants to merge 19 commits intodevfrom
Conversation
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
…portability Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
This reverts commit 864eedd.
Removed logic that converted selected Python and Node executable paths to relative if within the program directory. Now, the selected file paths are stored as absolute paths without conversion. This simplifies path handling and improves clarity.
Removed the PathResolutionTest.cs file, which contained unit tests for the Constant class's path resolution methods. These tests covered absolute, relative, UNC, null, and empty path scenarios, as well as round-trip conversions.
Move ResolveAbsolutePath and ConvertToRelativePathIfPossible from Constant to DataLocation for better organization. Update all references accordingly; implementations remain unchanged. This improves code clarity around file path management.
Eliminated the unnecessary using statement for Flow.Launcher.Infrastructure in AbstractPluginEnvironment.cs, as its types or members are no longer referenced in this file. This helps clean up the code and avoid redundant dependencies.
Removed logic that converted absolute paths to relative paths within the ProgramDirectory. Now, plugin settings file paths are always stored as absolute paths. Deleted the ConvertToRelativePathIfPossible method and updated usages accordingly.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResolves plugin settings file paths to absolute paths by adding DataLocation.ResolveAbsolutePath and updating AbstractPluginEnvironment to use the resolved path for file existence checks, conditional plugin installation, and returned settings paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as AbstractPluginEnvironment
participant Locator as DataLocation
participant FS as FileSystem
participant Installer as PluginInstaller
participant Caller as Caller
Env->>Locator: ResolveAbsolutePath(PluginsSettingsFilePath)
Locator-->>Env: resolvedPath
Env->>FS: File.Exists(resolvedPath)?
alt exists
Env->>Installer: EnsureLatestInstalled(resolvedPath) [conditional]
Installer-->>Env: installedPairs
Env-->>Caller: return installedPairs (using resolvedPath)
else not exists
Env-->>Caller: proceed without settings file
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for relative paths in Python and Node.js executable configuration, enabling truly portable Flow Launcher setups. The changes allow users to configure runtime paths relative to the application directory (e.g., .\runtimes\python\pythonw.exe), which are resolved to absolute paths at runtime.
Changes:
- Added
ResolveAbsolutePath()method to handle relative-to-absolute path resolution - Integrated path resolution into plugin environment setup and file existence checks
- Paths are stored as-entered (relative or absolute) and resolved only at execution time
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs | Adds ResolveAbsolutePath() method to resolve relative paths based on ProgramDirectory |
| Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs | Integrates path resolution into plugin setup, using resolved paths for file checks and plugin instantiation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please include in the portability testing of Python/Node.js when installed by flow. If I remember correctly when they are installed from flow, they get reinstalled automatically when path is not correct right? |
This PR focuses on parsing relative paths. Why do we need to check this portability? |
|
Even if reinstallation is invoked, the relative paths can still work. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs">
<violation number="1" location="Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs:59">
P2: `Path.IsPathRooted` evaluates partially qualified paths as true. Use `Path.IsPathFullyQualified` to correctly check for absolute paths.</violation>
</file>
<file name="Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs">
<violation number="1" location="Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs:64">
P1: `resolvedPath` becomes stale after `EnsureLatestInstalled()` updates the environment path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs (1)
59-60: ConsiderPath.IsPathFullyQualifiedoverPath.IsPathRootedfor portability edge cases
Path.IsPathRooted(@"\runtimes\python\pythonw.exe")returnstrue, so a drive-relative path like\runtimes\python\pythonw.exewould be passed through unchanged rather than resolved againstProgramDirectory.Path.IsPathFullyQualifiedonly returnstruefor genuinely absolute paths (e.g.,C:\...or UNC\\server\share\...), which aligns more precisely with the method's stated intent.♻️ Proposed change
- if (Path.IsPathRooted(path)) + if (Path.IsPathFullyQualified(path)) return path;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs` around lines 59 - 60, In DataLocation.cs, in the method that returns the resolved path (the block using Path.IsPathRooted(path)), replace the Path.IsPathRooted check with Path.IsPathFullyQualified(path) so drive-relative paths like "\runtimes\..." are not treated as absolute and will be correctly resolved against ProgramDirectory; ensure System.IO is available and keep the existing return behavior when the path is fully qualified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Flow.Launcher.Infrastructure/UserSettings/DataLocation.cs`:
- Around line 59-60: In DataLocation.cs, in the method that returns the resolved
path (the block using Path.IsPathRooted(path)), replace the Path.IsPathRooted
check with Path.IsPathFullyQualified(path) so drive-relative paths like
"\runtimes\..." are not treated as absolute and will be correctly resolved
against ProgramDirectory; ensure System.IO is available and keep the existing
return behavior when the path is fully qualified.
Updated the catch block in DataLocation.cs to explicitly use System.Exception instead of Exception when handling path resolution errors. This improves code clarity while maintaining the same error handling logic for ArgumentException, NotSupportedException, and PathTooLongException.
|
🥷 Code experts: jjw24 jjw24, Jack251970 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
… a different location
Replaced Path.IsPathRooted with Path.IsPathFullyQualified to more accurately determine if a path is truly absolute, preventing misclassification of certain relative paths.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Outdated
Show resolved
Hide resolved
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Outdated
Show resolved
Hide resolved
Flow.Launcher.Core/ExternalPlugins/Environments/AbstractPluginEnvironment.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Flow Launcher required absolute paths for Python and Node.js executables, breaking portability when the application is moved. Embedded runtimes within the portable directory structure were not supported.
CHANGES
ResolveAbsolutePath(): Resolves relative paths to absolute at runtime, based onProgramDirectoryUsage
Users can now configure:
Paths are stored as-entered (relative or absolute), resolved to absolute only at execution. File picker selections within
ProgramDirectoryare automatically stored as relative paths.Enables truly portable setups with embedded runtimes that survive directory moves, USB installations, and restricted environments.
Resolve #4228