-
Notifications
You must be signed in to change notification settings - Fork 774
Fix: Resolve correct pwsh path if installed via Windows Store #3246
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
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,7 +32,6 @@ public class PowerShellHostViewModel : ViewModelBase, IProfileManager | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #region Variables | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static readonly ILog Log = LogManager.GetLogger(typeof(PowerShellHostViewModel)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private readonly IDialogCoordinator _dialogCoordinator; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private readonly DispatcherTimer _searchDispatcherTimer = new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public IInterTabClient InterTabClient { get; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -307,16 +306,15 @@ public bool ProfileContextMenuIsOpen | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #region Constructor, load settings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public PowerShellHostViewModel(IDialogCoordinator instance) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public PowerShellHostViewModel() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _isLoading = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _dialogCoordinator = instance; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if PowerShell executable is configured | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CheckExecutable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Try to find PowerShell executable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!IsExecutableConfigured) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TryFindExecutable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -569,8 +567,24 @@ private void TryFindExecutable() | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var applicationFilePath = ApplicationHelper.Find(PowerShell.PwshFileName); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Workaround for: https://github.com/BornToBeRoot/NETworkManager/issues/3223 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (applicationFilePath.EndsWith("AppData\\Local\\Microsoft\\WindowsApps\\pwsh.exe")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Log.Info("Found pwsh.exe in AppData (Microsoft Store installation). Trying to resolve real path..."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var realPwshPath = FindRealPwshPath(applicationFilePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (realPwshPath != null) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| applicationFilePath = realPwshPath; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+577
to
+578
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (realPwshPath != null) | |
| applicationFilePath = realPwshPath; | |
| if (realPwshPath != null) | |
| { | |
| applicationFilePath = realPwshPath; | |
| } | |
| else | |
| { | |
| Log.Warn("Failed to resolve real pwsh path. Falling back to Windows PowerShell."); | |
| applicationFilePath = ApplicationHelper.Find(PowerShell.WindowsPowerShellFileName); | |
| } |
Copilot
AI
Nov 22, 2025
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.
[nitpick] Missing space after if keyword. Should be if (!process.WaitForExit(10000)) for consistency with C# formatting conventions.
| if(!process.WaitForExit(10000)) | |
| if (!process.WaitForExit(10000)) |
Copilot
AI
Nov 22, 2025
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.
Reading StandardOutput before calling WaitForExit() can cause a deadlock if the output buffer fills up. The process will block waiting for the buffer to be read, while this code blocks waiting for the process to exit. Use WaitForExit() first, then read the output, or read the output asynchronously.
Copilot
AI
Nov 22, 2025
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 string replacement logic is redundant and inefficient. Lines 643-647 replace \r, \n, and \r\n multiple times in different forms. Line 645 replaces \r\n which makes lines 646-647 redundant since they would have no effect. Consider simplifying this to a single set of replacements: first \r\n, then \r, then \n.
| .Replace(@"\r", string.Empty) | |
| .Replace(@"\n", string.Empty) | |
| .Replace("\r\n", string.Empty) | |
| .Replace("\n", string.Empty) | |
| .Replace("\r", string.Empty); | |
| .Replace("\r\n", string.Empty) | |
| .Replace("\r", string.Empty) | |
| .Replace("\n", string.Empty); |
Copilot
AI
Nov 22, 2025
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.
There's a potential resource leak here. If Process.Start(psi) returns null (which can happen if the process fails to start), the null reference will cause an exception before reaching the using statement's disposal. Additionally, if WaitForExit returns false and the process is killed, StandardError is never read, which could lead to deadlock if the error stream buffer fills up. Consider checking for null after Process.Start() and reading StandardError before or alongside StandardOutput.
| using Process process = Process.Start(psi); | |
| string output = process.StandardOutput.ReadToEnd(); | |
| if(!process.WaitForExit(10000)) | |
| { | |
| process.Kill(); | |
| Log.Warn("Timeout while trying to resolve real pwsh path."); | |
| return null; | |
| } | |
| if (string.IsNullOrEmpty(output)) | |
| return null; | |
| output = output.Replace(@"\\", @"\") | |
| .Replace(@"\r", string.Empty) | |
| .Replace(@"\n", string.Empty) | |
| .Replace("\r\n", string.Empty) | |
| .Replace("\n", string.Empty) | |
| .Replace("\r", string.Empty); | |
| return output.Trim(); | |
| Process process = Process.Start(psi); | |
| if (process == null) | |
| { | |
| Log.Error("Failed to start pwsh process to resolve real pwsh path."); | |
| return null; | |
| } | |
| using (process) | |
| { | |
| // Read both output and error streams asynchronously to avoid deadlocks | |
| Task<string> outputTask = process.StandardOutput.ReadToEndAsync(); | |
| Task<string> errorTask = process.StandardError.ReadToEndAsync(); | |
| bool exited = process.WaitForExit(10000); | |
| if (!exited) | |
| { | |
| process.Kill(); | |
| Log.Warn("Timeout while trying to resolve real pwsh path."); | |
| } | |
| // Ensure both output and error are read | |
| Task.WaitAll(outputTask, errorTask); | |
| string output = outputTask.Result; | |
| string error = errorTask.Result; | |
| if (!string.IsNullOrEmpty(error)) | |
| { | |
| Log.Warn($"Error while resolving real pwsh path: {error}"); | |
| } | |
| if (string.IsNullOrEmpty(output)) | |
| return null; | |
| output = output.Replace(@"\\", @"\") | |
| .Replace(@"\r", string.Empty) | |
| .Replace(@"\n", string.Empty) | |
| .Replace("\r\n", string.Empty) | |
| .Replace("\n", string.Empty) | |
| .Replace("\r", string.Empty); | |
| return output.Trim(); | |
| } |
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 path check using
.EndsWith()is case-sensitive and may not handle all variations of the path (e.g., lowercase drive letters, forward slashes). Consider using a more robust check:This handles case variations and ensures we're matching the correct directory structure.