From 82636e32f757aa59b2c7b56e2065d9a356d4a5cd Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 4 Aug 2017 21:21:19 -0700 Subject: [PATCH 1/7] Clean up ShellExecuteHelper and enable ShellExecute in NativeCommandProcessor --- .../commands/management/Process.cs | 32 +-- .../CoreCLR/CorePsPlatform.cs | 12 + .../engine/NativeCommandProcessor.cs | 60 +++-- .../engine/Utils.cs | 238 ------------------ .../help/HelpCommands.cs | 8 +- .../namespaces/FileSystemProvider.cs | 28 +-- 6 files changed, 61 insertions(+), 317 deletions(-) diff --git a/src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs b/src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs index c231e762e92..955c86df628 100644 --- a/src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs +++ b/src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs @@ -1665,18 +1665,6 @@ public sealed class StartProcessCommand : PSCmdlet, IDisposable private ManualResetEvent _waithandle = null; private bool _isDefaultSetParameterSpecified = false; - private bool _useShellExecute; - private readonly bool _isOnFullWinSku; - - /// - /// Constructor - /// - public StartProcessCommand() - { - _isOnFullWinSku = Platform.IsWindows && !Platform.IsNanoServer && !Platform.IsIoT; - _useShellExecute = _isOnFullWinSku; - } - #region Parameters /// @@ -1877,7 +1865,7 @@ protected override void BeginProcessing() string message = string.Empty; // -Verb and -WindowStyle are not supported on non-Windows platforms as well as Windows headless SKUs - if (_isOnFullWinSku) + if (Platform.IsWindowsDesktop) { // Parameters '-NoNewWindow' and '-WindowStyle' are both valid on full windows SKUs. if (_nonewwindow && _windowstyleSpecified) @@ -1908,6 +1896,8 @@ protected override void BeginProcessing() //create an instance of the ProcessStartInfo Class ProcessStartInfo startInfo = new ProcessStartInfo(); + //use ShellExecute by default if we are running on full windows SKUs + startInfo.UseShellExecute = Platform.IsWindowsDesktop; //Path = Mandatory parameter -> Will not be empty. try @@ -1959,7 +1949,6 @@ protected override void BeginProcessing() { if (_isDefaultSetParameterSpecified) { - _useShellExecute = false; startInfo.UseShellExecute = false; } @@ -1970,10 +1959,10 @@ protected override void BeginProcessing() LoadEnvironmentVariable(startInfo, Environment.GetEnvironmentVariables(EnvironmentVariableTarget.Machine)); LoadEnvironmentVariable(startInfo, Environment.GetEnvironmentVariables(EnvironmentVariableTarget.User)); } -#if !CORECLR + //WindowStyle startInfo.WindowStyle = _windowstyle; -#endif + //NewWindow if (_nonewwindow) { @@ -2054,7 +2043,6 @@ protected override void BeginProcessing() } } } -#if !CORECLR // Properties 'Verb' and 'WindowStyle' are missing in CoreCLR else if (ParameterSetName.Equals("UseShellExecute")) { //Verb @@ -2062,7 +2050,7 @@ protected override void BeginProcessing() //WindowStyle startInfo.WindowStyle = _windowstyle; } -#endif + //Starts the Process Process process = Start(startInfo); @@ -2212,7 +2200,7 @@ private Process Start(ProcessStartInfo startInfo) return process; #else Process process = null; - if (_useShellExecute) + if (startInfo.UseShellExecute) { process = StartWithShellExecute(startInfo); } @@ -2467,7 +2455,7 @@ private Process StartWithCreateProcess(ProcessStartInfo startinfo) lpStartupInfo.dwFlags |= 0x00000001; // On headless SKUs like NanoServer and IoT, window style can only be the default value 'Normal'. - switch (WindowStyle) + switch (startinfo.WindowStyle) { case ProcessWindowStyle.Normal: //SW_SHOWNORMAL @@ -2595,11 +2583,7 @@ private Process StartWithShellExecute(ProcessStartInfo startInfo) Process result = null; try { -#if CORECLR - result = ShellExecuteHelper.Start(startInfo, WindowStyle, Verb); -#else result = Process.Start(startInfo); -#endif } catch (Win32Exception ex) { diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 81df0316e99..78555693b54 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -148,6 +148,18 @@ internal static bool IsInbox } } + internal static bool IsWindowsDesktop + { + get + { +#if UNIX + return false; +#else + return IsWindows && !IsNanoServer && !IsIoT; +#endif + } + } + #if !UNIX private static bool? _isNanoServer = null; private static bool? _isIoT = null; diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index 677a0a64e54..4a2b882c0c4 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -436,21 +436,27 @@ private void InitNativeProcess() throw new PipelineStoppedException(); } + if (!Platform.IsWindows && startInfo.UseShellExecute) + { + // UseShellExecute is not properly supported on Unix. It runs the file with '/bin/sh'. + // Before the behavior is improved (tracked by dotnet/corefx#19956), we use xdg-open/open as the default programs + string executable = Platform.IsLinux ? "xdg-open" : /* OS X */ "open"; + startInfo.Arguments = "\"" + startInfo.FileName + "\" " + startInfo.Arguments; + startInfo.FileName = executable; + } + try { - _nativeProcess = new Process(); - _nativeProcess.StartInfo = startInfo; + _nativeProcess = new Process() { StartInfo = startInfo }; _nativeProcess.Start(); } catch (Win32Exception) { -#if CORECLR // Shell doesn't exist on OneCore, so a file cannot be associated with an executable, - // and we cannot run an executable as 'ShellExecute' either. - throw; -#else - // See if there is a file association for this command. If so - // then we'll use that. If there's no file association, then - // try shell execute... + // On Unix platforms, nothing can be further done, so just throw + // On headless Windows SKUs, there is no shell to fall back to, so just throw + if (!Platform.IsWindowsDesktop) { throw; } + + // on Windows desktops, see if there is a file association for this command. If so then we'll use that. string executable = FindExecutable(startInfo.FileName); bool notDone = true; if (!String.IsNullOrEmpty(executable)) @@ -495,7 +501,6 @@ private void InitNativeProcess() throw; } } -#endif } } @@ -931,11 +936,7 @@ private static bool IsConsoleApplication(string fileName) [ArchitectureSensitive] private static bool IsWindowsApplication(string fileName) { -#if UNIX - return false; -#else - if (Platform.IsNanoServer) - return false; + if (!Platform.IsWindowsDesktop) { return false; } SHFILEINFO shinfo = new SHFILEINFO(); IntPtr type = SHGetFileInfo(fileName, 0, ref shinfo, (uint)Marshal.SizeOf(shinfo), SHGFI_EXETYPE); @@ -955,7 +956,6 @@ private static bool IsWindowsApplication(string fileName) // anything else - is a windows program... return true; } -#endif } #endregion checkForConsoleApplication @@ -1104,11 +1104,14 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE } else { -#if CORECLR // Shell doesn't exist on OneCore, so documents cannot be associated with an application. - // Therefore, we cannot run document directly on OneCore. - throw InterpreterError.NewInterpreterException(this.Path, typeof(RuntimeException), - this.Command.InvocationExtent, "CantActivateDocumentInPowerShellCore", ParserStrings.CantActivateDocumentInPowerShellCore, this.Path); -#else + if (Platform.IsNanoServer || Platform.IsIoT) + { + // Shell doesn't exist on headless SKUs, so documents cannot be associated with an application. + // Therefore, we cannot run document in this case. + throw InterpreterError.NewInterpreterException(this.Path, typeof(RuntimeException), + this.Command.InvocationExtent, "CantActivateDocumentInPowerShellCore", ParserStrings.CantActivateDocumentInPowerShellCore, this.Path); + } + // We only want to ShellExecute something that is standalone... if (!soloCommand) { @@ -1117,7 +1120,6 @@ private ProcessStartInfo GetProcessStartInfo(bool redirectOutput, bool redirectE } startInfo.UseShellExecute = true; -#endif } //For minishell value of -outoutFormat parameter depends on value of redirectOutput. @@ -1243,11 +1245,10 @@ private void CalculateIORedirection(out bool redirectOutput, out bool redirectEr redirectOutput = true; redirectError = true; } -#if !CORECLR // UI doesn't exist on OneCore, so all applications running on an OneCore client should be console applications. - // The powershell on the OneCore client should already have a console attached. - else if (IsConsoleApplication(this.Path)) + else if (Platform.IsWindowsDesktop && IsConsoleApplication(this.Path)) { - // Allocate a console if there isn't one attached already... + // On Windows desktops, if the command to run is a console application, + // then allocate a console if there isn't one attached already... ConsoleVisibility.AllocateHiddenConsole(); if (ConsoleVisibility.AlwaysCaptureApplicationIO) @@ -1256,7 +1257,7 @@ private void CalculateIORedirection(out bool redirectOutput, out bool redirectEr redirectError = true; } } -#endif + if (!(redirectInput || redirectOutput)) _runStandAlone = true; } @@ -1288,7 +1289,6 @@ private bool ValidateExtension(string path) return false; } -#if !UNIX // Shell doesn't exist on OneCore, so documents cannot be associated with applications. #region Interop for FindExecutable... // Constant used to determine the buffer size for a path @@ -1368,7 +1368,6 @@ private static extern IntPtr SHGetFileInfo(string pszPath, uint dwFileAttributes ref SHFILEINFO psfi, uint cbSizeFileInfo, uint uFlags); #endregion -#endif #region Minishell Interop @@ -1843,8 +1842,6 @@ internal void Done() } } -#if !CORECLR // There is no GUI application on OneCore, so powershell on OneCore should always have a console attached. - /// /// Static class that allows you to show and hide the console window /// associated with this process. @@ -1989,7 +1986,6 @@ public static void Hide() } } } -#endif /// /// Exception used to wrap the error coming from diff --git a/src/System.Management.Automation/engine/Utils.cs b/src/System.Management.Automation/engine/Utils.cs index dd935c4ff31..0c1e39de866 100644 --- a/src/System.Management.Automation/engine/Utils.cs +++ b/src/System.Management.Automation/engine/Utils.cs @@ -1494,242 +1494,4 @@ public static void SetTestHook(string property, bool value) } } } - -#if !UNIX - /// - /// Helper to start process using ShellExecuteEx. This is used only in PowerShell Core on Full Windows. - /// - internal class ShellExecuteHelper - { - private NativeMethods.ShellExecuteInfo _executeInfo; - private int _errorCode; - private bool _succeeded; - - /// - /// Constructor for ShellExecuteHelper - /// - private ShellExecuteHelper(NativeMethods.ShellExecuteInfo executeInfo) { _executeInfo = executeInfo; } - - /// - /// Start a process using ShellExecuteEx with default settings about WindowStyle and Verb. - /// - internal static Process Start(ProcessStartInfo startInfo) - { - return Start(startInfo, ProcessWindowStyle.Normal, string.Empty); - } - - /// - /// Start a process using ShellExecuteEx - /// - internal static Process Start(ProcessStartInfo startInfo, ProcessWindowStyle windowStyle, string verb) - { - var shellExecuteInfo = new NativeMethods.ShellExecuteInfo(); - shellExecuteInfo.fMask = NativeMethods.SEE_MASK_NOCLOSEPROCESS; - shellExecuteInfo.fMask |= NativeMethods.SEE_MASK_FLAG_NO_UI; - - switch (windowStyle) - { - case ProcessWindowStyle.Hidden: - shellExecuteInfo.nShow = NativeMethods.SW_HIDE; - break; - case ProcessWindowStyle.Minimized: - shellExecuteInfo.nShow = NativeMethods.SW_SHOWMINIMIZED; - break; - case ProcessWindowStyle.Maximized: - shellExecuteInfo.nShow = NativeMethods.SW_SHOWMAXIMIZED; - break; - default: - shellExecuteInfo.nShow = NativeMethods.SW_SHOWNORMAL; - break; - } - - try - { - if (startInfo.FileName.Length != 0) - shellExecuteInfo.lpFile = Marshal.StringToHGlobalUni(startInfo.FileName); - if (!string.IsNullOrEmpty(verb)) - shellExecuteInfo.lpVerb = Marshal.StringToHGlobalUni(verb); - if (startInfo.Arguments.Length != 0) - shellExecuteInfo.lpParameters = Marshal.StringToHGlobalUni(startInfo.Arguments); - if (startInfo.WorkingDirectory.Length != 0) - shellExecuteInfo.lpDirectory = Marshal.StringToHGlobalUni(startInfo.WorkingDirectory); - - shellExecuteInfo.fMask |= NativeMethods.SEE_MASK_FLAG_DDEWAIT; - ShellExecuteHelper helper = new ShellExecuteHelper(shellExecuteInfo); - if (!helper.ExecuteOnSTAThread()) - { - if(helper.ErrorCode == NativeMethods.ERROR_BAD_EXE_FORMAT || helper.ErrorCode == NativeMethods.ERROR_EXE_MACHINE_TYPE_MISMATCH) - { - throw new Win32Exception(helper.ErrorCode, "InvalidApplication"); - } - else - { - throw new Win32Exception(helper.ErrorCode); - } - } - } - finally - { - if (shellExecuteInfo.lpFile != (IntPtr)0) Marshal.FreeHGlobal(shellExecuteInfo.lpFile); - if (shellExecuteInfo.lpVerb != (IntPtr)0) Marshal.FreeHGlobal(shellExecuteInfo.lpVerb); - if (shellExecuteInfo.lpParameters != (IntPtr)0) Marshal.FreeHGlobal(shellExecuteInfo.lpParameters); - if (shellExecuteInfo.lpDirectory != (IntPtr)0) Marshal.FreeHGlobal(shellExecuteInfo.lpDirectory); - } - - Process processToReturn = null; - if (shellExecuteInfo.hProcess != IntPtr.Zero) - { - var handle = new SafeProcessHandle(shellExecuteInfo.hProcess, true); - try { - int processId = GetProcessIdFromHandle(handle); - processToReturn = Process.GetProcessById(processId); - } finally { - handle.Dispose(); - } - } - - return processToReturn; - } - - private void ShellExecuteFunction() - { - if (!(_succeeded = NativeMethods.ShellExecuteEx(_executeInfo))) - { - _errorCode = Marshal.GetLastWin32Error(); - if (_errorCode == 0) - { - switch ((long)_executeInfo.hInstApp) - { - case NativeMethods.SE_ERR_FNF: _errorCode = NativeMethods.ERROR_FILE_NOT_FOUND; break; - case NativeMethods.SE_ERR_PNF: _errorCode = NativeMethods.ERROR_PATH_NOT_FOUND; break; - case NativeMethods.SE_ERR_ACCESSDENIED: _errorCode = NativeMethods.ERROR_ACCESS_DENIED; break; - case NativeMethods.SE_ERR_OOM: _errorCode = NativeMethods.ERROR_NOT_ENOUGH_MEMORY; break; - case NativeMethods.SE_ERR_DDEFAIL: - case NativeMethods.SE_ERR_DDEBUSY: - case NativeMethods.SE_ERR_DDETIMEOUT: _errorCode = NativeMethods.ERROR_DDE_FAIL; break; - case NativeMethods.SE_ERR_SHARE: _errorCode = NativeMethods.ERROR_SHARING_VIOLATION; break; - case NativeMethods.SE_ERR_NOASSOC: _errorCode = NativeMethods.ERROR_NO_ASSOCIATION; break; - case NativeMethods.SE_ERR_DLLNOTFOUND: _errorCode = NativeMethods.ERROR_DLL_NOT_FOUND; break; - default: _errorCode = (int)_executeInfo.hInstApp; break; - } - } - } - } - - private bool ExecuteOnSTAThread() - { - if (Thread.CurrentThread.GetApartmentState() != System.Threading.ApartmentState.STA) - { - ThreadStart threadStart = new ThreadStart(this.ShellExecuteFunction); - Thread thread = new Thread(threadStart); - thread.SetApartmentState(System.Threading.ApartmentState.STA); - thread.Start(); - thread.Join(); - } - else - { - ShellExecuteFunction(); - } - return _succeeded; - } - - private int ErrorCode - { - get - { - return _errorCode; - } - } - - private static int GetProcessIdFromHandle(SafeProcessHandle processHandle) - { - NativeMethods.NtProcessBasicInfo info = new NativeMethods.NtProcessBasicInfo(); - int status = NativeMethods.NtQueryInformationProcess(processHandle, NativeMethods.NtQueryProcessBasicInfo, info, (int)Marshal.SizeOf(info), null); - if (status != 0) { - throw new InvalidOperationException("CantGetProcessId", new Win32Exception(status)); - } - // We should change the signature of this function and ID property in process class. - return info.UniqueProcessId.ToInt32(); - } - - private static class NativeMethods - { - public const int SEE_MASK_NOCLOSEPROCESS = 0x00000040; - public const int SEE_MASK_FLAG_NO_UI = 0x00000400; - public const int SEE_MASK_FLAG_DDEWAIT = 0x00000100; - - public const int SW_HIDE = 0; - public const int SW_SHOWMINIMIZED = 2; - public const int SW_SHOWMAXIMIZED = 3; - public const int SW_SHOWNORMAL = 1; - - public const int SE_ERR_FNF = 2; - public const int SE_ERR_PNF = 3; - public const int SE_ERR_ACCESSDENIED = 5; - public const int SE_ERR_OOM = 8; - public const int SE_ERR_DLLNOTFOUND = 32; - public const int SE_ERR_SHARE = 26; - public const int SE_ERR_DDETIMEOUT = 28; - public const int SE_ERR_DDEFAIL = 29; - public const int SE_ERR_DDEBUSY = 30; - public const int SE_ERR_NOASSOC = 31; - - public const int ERROR_FILE_NOT_FOUND = 2; - public const int ERROR_PATH_NOT_FOUND = 3; - public const int ERROR_ACCESS_DENIED = 5; - public const int ERROR_NOT_ENOUGH_MEMORY = 8; - public const int ERROR_SHARING_VIOLATION = 32; - public const int ERROR_OPERATION_ABORTED = 995; - public const int ERROR_NO_ASSOCIATION = 1155; - public const int ERROR_DLL_NOT_FOUND = 1157; - public const int ERROR_DDE_FAIL = 1156; - - public const int ERROR_BAD_EXE_FORMAT = 193; - public const int ERROR_EXE_MACHINE_TYPE_MISMATCH = 216; - - public const int NtQueryProcessBasicInfo = 0; - - [StructLayout(LayoutKind.Sequential)] - internal class ShellExecuteInfo - { - public int cbSize = 0; - public int fMask = 0; - public IntPtr hwnd = (IntPtr)0; - public IntPtr lpVerb = (IntPtr)0; - public IntPtr lpFile = (IntPtr)0; - public IntPtr lpParameters = (IntPtr)0; - public IntPtr lpDirectory = (IntPtr)0; - public int nShow = 0; - public IntPtr hInstApp = (IntPtr)0; - public IntPtr lpIDList = (IntPtr)0; - public IntPtr lpClass = (IntPtr)0; - public IntPtr hkeyClass = (IntPtr)0; - public int dwHotKey = 0; - public IntPtr hIcon = (IntPtr)0; - public IntPtr hProcess = (IntPtr)0; - - public ShellExecuteInfo() - { - cbSize = Marshal.SizeOf(this); - } - } - - [StructLayout(LayoutKind.Sequential)] - internal class NtProcessBasicInfo { - public int ExitStatus = 0; - public IntPtr PebBaseAddress = (IntPtr)0; - public IntPtr AffinityMask = (IntPtr)0; - public int BasePriority = 0; - public IntPtr UniqueProcessId = (IntPtr)0; - public IntPtr InheritedFromUniqueProcessId = (IntPtr)0; - } - - [DllImport("Shell32", CharSet=CharSet.Unicode, SetLastError=true)] - public static extern bool ShellExecuteEx(ShellExecuteInfo info); - - [DllImport("Ntdll", CharSet=CharSet.Unicode)] - public static extern int NtQueryInformationProcess(SafeProcessHandle processHandle, int query, NtProcessBasicInfo info, int size, int[] returnedSize); - } - } -#endif } diff --git a/src/System.Management.Automation/help/HelpCommands.cs b/src/System.Management.Automation/help/HelpCommands.cs index 1b39db326c0..eedaf0ee67f 100644 --- a/src/System.Management.Automation/help/HelpCommands.cs +++ b/src/System.Management.Automation/help/HelpCommands.cs @@ -642,7 +642,7 @@ private void LaunchOnlineHelp(Uri uriToLaunch) browserProcess.StartInfo.FileName = Platform.IsLinux ? "xdg-open" : /* OS X */ "open"; browserProcess.StartInfo.Arguments = uriToLaunch.OriginalString; browserProcess.Start(); -#elif CORECLR +#else if (Platform.IsNanoServer || Platform.IsIoT) { // We cannot open the URL in browser on headless SKUs. @@ -653,11 +653,9 @@ private void LaunchOnlineHelp(Uri uriToLaunch) { // We can call ShellExecute directly on Full Windows. browserProcess.StartInfo.FileName = uriToLaunch.OriginalString; - ShellExecuteHelper.Start(browserProcess.StartInfo); + browserProcess.StartInfo.UseShellExecute = true; + browserProcess.Start(); } -#else - browserProcess.StartInfo.FileName = uriToLaunch.OriginalString; - browserProcess.Start(); #endif } catch (InvalidOperationException ioe) diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 7205120bdd1..50a7611ebb3 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -1307,14 +1307,6 @@ private FileSystemInfo GetFileSystemItem(string path, ref bool isContainer, bool /// protected override void InvokeDefaultAction(string path) { -#if UNIX - // Error code 13 -- Permission denied - const int NOT_EXECUTABLE = 13; -#else - // Error code 193 -- BAD_EXE_FORMAT (not a valid Win32 application) - const int NOT_EXECUTABLE = 193; -#endif - if (String.IsNullOrEmpty(path)) { throw PSTraceSource.NewArgumentException("path"); @@ -1330,11 +1322,11 @@ protected override void InvokeDefaultAction(string path) { var invokeProcess = new System.Diagnostics.Process(); invokeProcess.StartInfo.FileName = path; +#if UNIX bool invokeDefaultProgram = false; - - if (Directory.Exists(path) && !Platform.IsNanoServer && !Platform.IsIoT) + if (Directory.Exists(path)) { - // Path points to a directory and it's not NanoServer or IoT, so we can opne the file explorer + // Path points to a directory. We have to use xdg-open/open on Linux/OSX. invokeDefaultProgram = true; } else @@ -1344,18 +1336,16 @@ protected override void InvokeDefaultAction(string path) // Try Process.Start first. This works for executables on Win/Unix platforms invokeProcess.Start(); } - catch (Win32Exception ex) when (ex.NativeErrorCode == NOT_EXECUTABLE) + catch (Win32Exception ex) when (ex.NativeErrorCode == 13) { - // The file is possibly not an executable. If it's headless SKUs, rethrow. - if (Platform.IsNanoServer || Platform.IsIoT) { throw; } - // Otherwise, try invoking the default program that handles this file. + // Error code 13 -- Permission denied + // The file is possibly not an executable. We try xdg-open/open on Linux/OSX. invokeDefaultProgram = true; } } if (invokeDefaultProgram) { -#if UNIX const string quoteFormat = "\"{0}\""; invokeProcess.StartInfo.FileName = Platform.IsLinux ? "xdg-open" : /* OS X */ "open"; if (NativeCommandParameterBinder.NeedQuotes(path)) @@ -1364,10 +1354,12 @@ protected override void InvokeDefaultAction(string path) } invokeProcess.StartInfo.Arguments = path; invokeProcess.Start(); + } #else - ShellExecuteHelper.Start(invokeProcess.StartInfo); + // Use ShellExecute when it's not a headless SKU + invokeProcess.StartInfo.UseShellExecute = !Platform.IsNanoServer && !Platform.IsIoT; + invokeProcess.Start(); #endif - } } } // InvokeDefaultAction From 6ac56abd229c310a0c628d816d165dda48d03fa7 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 4 Aug 2017 21:24:01 -0700 Subject: [PATCH 2/7] Minor fix --- .../engine/NativeCommandProcessor.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index 4a2b882c0c4..de884416849 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -443,6 +443,7 @@ private void InitNativeProcess() string executable = Platform.IsLinux ? "xdg-open" : /* OS X */ "open"; startInfo.Arguments = "\"" + startInfo.FileName + "\" " + startInfo.Arguments; startInfo.FileName = executable; + startInfo.UseShellExecute = false; } try From f99c35ae35ee13a0f25dd085671fb2f3b6e4b0a5 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Mon, 7 Aug 2017 16:06:18 -0700 Subject: [PATCH 3/7] [Feature] Fix NativeCommandProcessor to clean up in case an exception is thron --- .../engine/NativeCommandProcessor.cs | 53 ++++++--- .../NativeCommandProcessor.Tests.ps1 | 103 ++++++++++++++++++ 2 files changed, 142 insertions(+), 14 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index de884416849..533299697ca 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -304,7 +304,16 @@ internal override void Prepare(IDictionary psDefaultParameterValues) this.NativeParameterBinderController.BindParameters(arguments); } - InitNativeProcess(); + try + { + InitNativeProcess(); + } + catch (Exception) + { + // Do the cleanup and rethrow the exception + CleanUp(); + throw; + } } /// @@ -312,12 +321,21 @@ internal override void Prepare(IDictionary psDefaultParameterValues) /// internal override void ProcessRecord() { - while (Read()) + try { - _inputWriter.Add(Command.CurrentPipelineObject); - } + while (Read()) + { + _inputWriter.Add(Command.CurrentPipelineObject); + } - ConsumeAvailableNativeProcessOutput(blocking: false); + ConsumeAvailableNativeProcessOutput(blocking: false); + } + catch (Exception) + { + // Do the cleanup and rethrow the exception + CleanUp(); + throw; + } } /// @@ -342,6 +360,12 @@ internal override void ProcessRecord() /// private bool _isRunningInBackground; + /// + /// Indicate if we have called 'NotifyBeginApplication()' on the host, so that + /// we can call the counterpart 'NotifyEndApplication' as approriate. + /// + private bool _hasNotifiedBeginApplication; + /// /// This output queue helps us keep the output and error (if redirected) order correct. /// We could do a blocking read in the Complete block instead, @@ -401,9 +425,10 @@ private void InitNativeProcess() // If this process is being run standalone, tell the host, which might want // to save off the window title or other such state as might be tweaked by // the native process - if (!redirectOutput) + if (_runStandAlone) { this.Command.Context.EngineHostInterface.NotifyBeginApplication(); + _hasNotifiedBeginApplication = true; // Also, store the Raw UI coordinates so that we can scrape the screen after // if we are transcribing. @@ -730,11 +755,7 @@ internal override void Complete() } finally { - if (!_nativeProcess.StartInfo.RedirectStandardOutput) - { - this.Command.Context.EngineHostInterface.NotifyEndApplication(); - } - // Do the clean up... + // Do the cleanup... CleanUp(); } @@ -995,6 +1016,12 @@ internal void StopProcessing() /// private void CleanUp() { + // We need to call 'NotifyEndApplication' as appropriate during cleanup + if (_hasNotifiedBeginApplication) + { + this.Command.Context.EngineHostInterface.NotifyEndApplication(); + } + try { if (_nativeProcess != null) @@ -1179,7 +1206,6 @@ private void CalculateIORedirection(out bool redirectOutput, out bool redirectEr redirectOutput = true; redirectError = true; - // Figure out if we're going to run this process "standalone" i.e. without // redirecting anything. This is a bit tricky as we always run redirected so // we have to see if the redirection is actually being done at the topmost level or not. @@ -1259,8 +1285,7 @@ private void CalculateIORedirection(out bool redirectOutput, out bool redirectEr } } - if (!(redirectInput || redirectOutput)) - _runStandAlone = true; + _runStandAlone = !redirectInput && !redirectOutput && !redirectError; } private bool ValidateExtension(string path) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 24678d46d6b..2e0da969bae 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -119,5 +119,108 @@ Describe "Native Command Processor" -tags "Feature" { $ps.Dispose() } } +} + +Describe "Open a text file with NativeCommandProcessor" -tags "Feature" { + BeforeAll { + if ($IsWindows) { + $TestFile = Join-Path -Path $TestDrive -ChildPath "TextFileTest.foo" + } else { + $TestFile = Join-Path -Path $TestDrive -ChildPath "TextFileTest.txt" + } + Set-Content -Path $TestFile -Value "Hello" -Force + $supportedEnvironment = $true + + if ($IsLinux) { + $appFolder = "$HOME/.local/share/applications" + $supportedEnvironment = Test-Path $appFolder + if ($supportedEnvironment) { + $mimeDefault = xdg-mime query default text/plain + Remove-Item $HOME/nativeCommandProcessor.Success -Force -ErrorAction SilentlyContinue + Set-Content -Path "$appFolder/nativeCommandProcessor.desktop" -Force -Value @" +[Desktop Entry] +Version=1.0 +Name=nativeCommandProcessor +Comment=Validate_native_command_processor_open_text_file +Exec=/bin/sh -c 'echo %u > ~/nativeCommandProcessor.Success' +Icon=utilities-terminal +Terminal=true +Type=Application +Categories=Application; +"@ + xdg-mime default nativeCommandProcessor.desktop text/plain + } + } + elseif ($IsWindows) { + $supportedEnvironment = ![System.Management.Automation.Platform]::IsNanoServer -and ![System.Management.Automation.Platform]::IsIoT + if ($supportedEnvironment) { + cmd /c assoc .foo=foofile + cmd /c ftype foofile=cmd /c echo %1^> $TestDrive\foo.txt + Remove-Item $TestDrive\foo.txt -Force -ErrorAction SilentlyContinue + } + } + } + + AfterAll { + Remove-Item -Path $TestFile -Force -ErrorAction SilentlyContinue + + if ($IsLinux -and $supportedEnvironment) { + xdg-mime default $mimeDefault text/plain + Remove-Item $appFolder/nativeCommandProcessor.desktop -Force -ErrorAction SilentlyContinue + Remove-Item $HOME/nativeCommandProcessor.Success -Force -ErrorAction SilentlyContinue + } + elseif ($IsWindows -and $supportedEnvironment) { + cmd /c assoc .foo= + cmd /c ftype foofile= + } + } + It "Should open text file without error" -Skip:(!$supportedEnvironment) { + if ($IsOSX) { + $expectedTitle = Split-Path $TestFile -Leaf + open -F -a TextEdit + $beforeCount = [int]('tell application "TextEdit" to count of windows' | osascript) + & $TestFile + $startTime = Get-Date + $title = [String]::Empty + while (((Get-Date) - $startTime).TotalSeconds -lt 30 -and ($title -ne $expectedTitle)) { + Start-Sleep -Milliseconds 100 + $title = 'tell application "TextEdit" to get name of front window' | osascript + } + $afterCount = [int]('tell application "TextEdit" to count of windows' | osascript) + $afterCount | Should Be ($beforeCount + 1) + $title | Should Be $expectedTitle + "tell application ""TextEdit"" to close window ""$expectedTitle""" | osascript + 'tell application "TextEdit" to quit' | osascript + } + elseif ($IsLinux) { + # Validate on Linux by reassociating default app for text file + & $TestFile + $startTime = Get-Date + # It may take time for handler to start + while (((Get-Date) - $startTime).TotalSeconds -lt 10 -and (-not (Test-Path "$HOME/nativeCommandProcessor.Success"))) { + Start-Sleep -Milliseconds 100 + } + Get-Content $HOME/nativeCommandProcessor.Success | Should Be $TestFile + } + else { + & $TestFile + $startTime = Get-Date + while (((Get-Date) - $startTime).TotalSeconds -lt 10 -and (!(Test-Path $TestDrive\foo.txt))) { + Start-Sleep -Milliseconds 100 + } + "$TestDrive\foo.txt" | Should Exist + Get-Content $TestDrive\foo.txt | Should BeExactly $TestFile + } + } + + It "Opening a file with an unregistered extension on Windows should fail" -Skip:(!$IsWindows) { + try { + $dllFile = "$PSHOME\System.Management.Automation.dll" + & $dllFile + throw "No Exception!" + } catch { + $_.FullyQualifiedErrorId | should be "NativeCommandFailed" + } + } } From 7d375eb83bba35b3685f162c5d1aac4184ca9b89 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Mon, 7 Aug 2017 17:42:29 -0700 Subject: [PATCH 4/7] [Feature] Update tests --- .../NativeCommandProcessor.Tests.ps1 | 2 +- .../Invoke-Item.Tests.ps1 | 23 +++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 2e0da969bae..4a5092c7374 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -121,7 +121,7 @@ Describe "Native Command Processor" -tags "Feature" { } } -Describe "Open a text file with NativeCommandProcessor" -tags "Feature" { +Describe "Open a text file with NativeCommandProcessor" -tags @("Feature", "RequireAdminOnWindows") { BeforeAll { if ($IsWindows) { $TestFile = Join-Path -Path $TestDrive -ChildPath "TextFileTest.foo" diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 index 2a1fc0f25f4..a230261a0e8 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 @@ -51,17 +51,30 @@ Describe "Invoke-Item basic tests" -Tags "Feature" { } It "Should invoke an executable file without error" { - $executable = Get-Command "ping" -CommandType Application | ForEach-Object Source + $ping = Get-Command "ping" -CommandType Application | ForEach-Object Source $redirectFile = Join-Path -Path $TestDrive -ChildPath "redirect2.txt" if ($IsWindows) { - ## 'ping.exe' on Windows writes out usage to stdout. - & $powershell -noprofile -c "Invoke-Item '$executable'" > $redirectFile + if ([System.Management.Automation.Platform]::IsNanoServer -or [System.Management.Automation.Platform]::IsIoT) { + ## On headless SKUs, we use `UseShellExecute = false` + ## 'ping.exe' on Windows writes out usage to stdout. + & $powershell -noprofile -c "Invoke-Item '$ping'" > $redirectFile + Get-Content $redirectFile -Raw | Should Match "usage: ping" + } else { + ## On full desktop, we use `UseShellExecute = true` to align with Windows PowerShell + $notepad = Get-Command "notepad.exe" -CommandType Application | ForEach-Object Source + Get-Process -Name notepad | Stop-Process -Force + Invoke-Item -Path $notepad + $notepadProcess = Get-Process -Name notepad + $notepadProcess.Name | Should Be notepad + Stop-Process -InputObject $notepadProcess + } } else { + ## On Unix, we use `UseShellExecute = false` ## 'ping' on Unix write out usage to stderr - & $powershell -noprofile -c "Invoke-Item '$executable'" 2> $redirectFile + & $powershell -noprofile -c "Invoke-Item '$ping'" 2> $redirectFile + Get-Content $redirectFile -Raw | Should Match "usage: ping" } - Get-Content $redirectFile -Raw | Should Match "usage: ping" } Context "Invoke a folder" { From 5494c7faf80504a31c548e92c1b79e3d7653bf84 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Mon, 7 Aug 2017 18:07:09 -0700 Subject: [PATCH 5/7] [Feature] Address comments --- .../CoreCLR/CorePsPlatform.cs | 11 +++++++++-- .../namespaces/FileSystemProvider.cs | 2 +- .../NativeExecution/NativeCommandProcessor.Tests.ps1 | 10 ++-------- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs index 78555693b54..fae4d8c7fe6 100644 --- a/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs +++ b/src/System.Management.Automation/CoreCLR/CorePsPlatform.cs @@ -148,14 +148,20 @@ internal static bool IsInbox } } - internal static bool IsWindowsDesktop + /// + /// True if underlying system is Windows Desktop. + /// + public static bool IsWindowsDesktop { get { #if UNIX return false; #else - return IsWindows && !IsNanoServer && !IsIoT; + if (_isWindowsDesktop.HasValue) { return _isWindowsDesktop.Value; } + + _isWindowsDesktop = !IsNanoServer && !IsIoT; + return _isWindowsDesktop.Value; #endif } } @@ -164,6 +170,7 @@ internal static bool IsWindowsDesktop private static bool? _isNanoServer = null; private static bool? _isIoT = null; private static bool? _isInbox = null; + private static bool? _isWindowsDesktop = null; #endif // format files diff --git a/src/System.Management.Automation/namespaces/FileSystemProvider.cs b/src/System.Management.Automation/namespaces/FileSystemProvider.cs index 50a7611ebb3..4e38dbdf45a 100644 --- a/src/System.Management.Automation/namespaces/FileSystemProvider.cs +++ b/src/System.Management.Automation/namespaces/FileSystemProvider.cs @@ -1357,7 +1357,7 @@ protected override void InvokeDefaultAction(string path) } #else // Use ShellExecute when it's not a headless SKU - invokeProcess.StartInfo.UseShellExecute = !Platform.IsNanoServer && !Platform.IsIoT; + invokeProcess.StartInfo.UseShellExecute = Platform.IsWindowsDesktop; invokeProcess.Start(); #endif } diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 4a5092c7374..f09c6727430 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -152,7 +152,7 @@ Categories=Application; } } elseif ($IsWindows) { - $supportedEnvironment = ![System.Management.Automation.Platform]::IsNanoServer -and ![System.Management.Automation.Platform]::IsIoT + $supportedEnvironment = [System.Management.Automation.Platform]::IsWindowsDesktop if ($supportedEnvironment) { cmd /c assoc .foo=foofile cmd /c ftype foofile=cmd /c echo %1^> $TestDrive\foo.txt @@ -215,12 +215,6 @@ Categories=Application; } It "Opening a file with an unregistered extension on Windows should fail" -Skip:(!$IsWindows) { - try { - $dllFile = "$PSHOME\System.Management.Automation.dll" - & $dllFile - throw "No Exception!" - } catch { - $_.FullyQualifiedErrorId | should be "NativeCommandFailed" - } + { $dllFile = "$PSHOME\System.Management.Automation.dll"; & $dllFile } | ShouldBeErrorId "NativeCommandFailed" } } From 16f459ee134f6f216d666a6f3d46b6ca80bcf1cf Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 8 Aug 2017 08:41:13 -0700 Subject: [PATCH 6/7] address one more comment --- .../Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 index a230261a0e8..2053aef8857 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 @@ -66,7 +66,7 @@ Describe "Invoke-Item basic tests" -Tags "Feature" { Get-Process -Name notepad | Stop-Process -Force Invoke-Item -Path $notepad $notepadProcess = Get-Process -Name notepad - $notepadProcess.Name | Should Be notepad + $notepadProcess.Name | Should Be "notepad" Stop-Process -InputObject $notepadProcess } } else { From c7c939498ffc0f5c77a6fc48c979aa0252b11317 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 9 Aug 2017 09:40:38 -0700 Subject: [PATCH 7/7] Address some more comments --- .../engine/NativeCommandProcessor.cs | 7 ++++--- .../Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/NativeCommandProcessor.cs b/src/System.Management.Automation/engine/NativeCommandProcessor.cs index 533299697ca..18c36b9667f 100644 --- a/src/System.Management.Automation/engine/NativeCommandProcessor.cs +++ b/src/System.Management.Automation/engine/NativeCommandProcessor.cs @@ -310,7 +310,7 @@ internal override void Prepare(IDictionary psDefaultParameterValues) } catch (Exception) { - // Do the cleanup and rethrow the exception + // Do cleanup in case of exception CleanUp(); throw; } @@ -332,7 +332,7 @@ internal override void ProcessRecord() } catch (Exception) { - // Do the cleanup and rethrow the exception + // Do cleanup in case of exception CleanUp(); throw; } @@ -755,7 +755,7 @@ internal override void Complete() } finally { - // Do the cleanup... + // Do some cleanup CleanUp(); } @@ -1024,6 +1024,7 @@ private void CleanUp() try { + // Dispose the process if it's already created if (_nativeProcess != null) { _nativeProcess.Dispose(); diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 index 2053aef8857..ce97092b8c6 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 @@ -63,10 +63,11 @@ Describe "Invoke-Item basic tests" -Tags "Feature" { } else { ## On full desktop, we use `UseShellExecute = true` to align with Windows PowerShell $notepad = Get-Command "notepad.exe" -CommandType Application | ForEach-Object Source - Get-Process -Name notepad | Stop-Process -Force + $notepadProcessName = "notepad" + Get-Process -Name $notepadProcessName | Stop-Process -Force Invoke-Item -Path $notepad - $notepadProcess = Get-Process -Name notepad - $notepadProcess.Name | Should Be "notepad" + $notepadProcess = Get-Process -Name $notepadProcessName + $notepadProcess.Name | Should Be $notepadProcessName Stop-Process -InputObject $notepadProcess } } else {