Host UI errors (#4844)

Implements reporting errors with message box for Windows GUI apps in apphost.

In apphost, if it's running as GUI, error writing will be redirected to a buffer. Upon exit, if the error buffer is not empty, it will be showed on screen as a message box.
The error writer is propagated from apphost to hostfxr and thus to hostpolicy.

This solves the problem that GUI apps which don't have console die silently without reporting any errors if there are issues during host execution.

src/corehost/cli/fxr/hostfxr.cpp

@@ -100,37 +100,6 @@ int load_host_library_with_return(
         : StatusCode::CoreHostEntryPointFailure;
 }
 
-// Helper class to make it easy to propagate error writer to the hostpolicy
-class propagate_error_writer_to_corehost_t
-{
-private:
-    corehost_set_error_writer_fn m_set_error_writer_fn;
-    bool m_error_writer_set;
-
-public:
-    propagate_error_writer_to_corehost_t(corehost_set_error_writer_fn set_error_writer_fn)
-    {
-        m_set_error_writer_fn = set_error_writer_fn;
-        m_error_writer_set = false;
-
-        trace::error_writer_fn error_writer = trace::get_error_writer();
-        if (error_writer != nullptr && m_set_error_writer_fn != nullptr)
-        {
-            m_set_error_writer_fn(error_writer);
-            m_error_writer_set = true;
-        }
-    }
-
-    ~propagate_error_writer_to_corehost_t()
-    {
-        if (m_error_writer_set && m_set_error_writer_fn != nullptr)
-        {
-            m_set_error_writer_fn(nullptr);
-            m_error_writer_set = false;
-        }
-    }
-};
-
 int execute_app(
     const pal::string_t& impl_dll_dir,
     corehost_init_t* init,
@@ -154,7 +123,7 @@ int execute_app(
     trace::flush();
 
     {
-        propagate_error_writer_to_corehost_t propagate_error_writer_to_corehost(host_set_error_writer);
+        propagate_error_writer_t propagate_error_writer_to_corehost(host_set_error_writer);
 
         const host_interface_t& intf = init->get_host_init_data();
         if ((code = host_load(&intf)) == 0)
@@ -196,7 +165,7 @@ int execute_host_command(
     trace::flush();
 
     {
-        propagate_error_writer_to_corehost_t propagate_error_writer_to_corehost(host_set_error_writer);
+        propagate_error_writer_t propagate_error_writer_to_corehost(host_set_error_writer);
 
         const host_interface_t& intf = init->get_host_init_data();
         if ((code = host_load(&intf)) == 0)

src/corehost/common/pal.h

@@ -125,7 +125,7 @@ namespace pal
     inline size_t strlen(const char_t* str) { return ::wcslen(str); }
     inline FILE * file_open(const pal::string_t& path, const char_t* mode) { return ::_wfopen(path.c_str(), mode); }
     inline void file_vprintf(FILE* f, const char_t* format, va_list vl) { ::vfwprintf(f, format, vl); ::fputwc(_X('\n'), f); }
-    inline void err_vprintf(const char_t* format, va_list vl) { ::vfwprintf(stderr, format, vl); ::fputwc(_X('\n'), stderr); }
+    inline void err_fputs(const char_t* message) { ::fputws(message, stderr); ::fputwc(_X('\n'), stderr); }
     inline void out_vprintf(const char_t* format, va_list vl) { ::vfwprintf(stdout, format, vl); ::fputwc(_X('\n'), stdout); }
     inline int str_vprintf(char_t* buffer, size_t count, const char_t* format, va_list vl) { return ::_vsnwprintf(buffer, count, format, vl); }
 
@@ -172,7 +172,7 @@ namespace pal
     inline size_t strlen(const char_t* str) { return ::strlen(str); }
     inline FILE * file_open(const pal::string_t& path, const char_t* mode) { return fopen(path.c_str(), mode); }
     inline void file_vprintf(FILE* f, const char_t* format, va_list vl) { ::vfprintf(f, format, vl); ::fputc('\n', f); }
-    inline void err_vprintf(const char_t* format, va_list vl) { ::vfprintf(stderr, format, vl); ::fputc('\n', stderr); }
+    inline void err_fputs(const char_t* message) { ::fputs(message, stderr); ::fputc(_X('\n'), stderr); }
     inline void out_vprintf(const char_t* format, va_list vl) { ::vfprintf(stdout, format, vl); ::fputc('\n', stdout); }
     inline int str_vprintf(char_t* str, size_t size, const char_t* format, va_list vl) { return ::vsnprintf(str, size, format, vl); }
 

src/corehost/common/trace.cpp

@@ -124,23 +124,31 @@ void trace::error(const pal::char_t* format, ...)
     va_list args;
     va_start(args, format);
 
+    va_list trace_args;
+    va_copy(trace_args, args);
+
+    va_list dup_args;
+    va_copy(dup_args, args);
+    int count = pal::str_vprintf(NULL, 0, format, args) + 1;
+    std::vector<pal::char_t> buffer(count);
+    pal::str_vprintf(&buffer[0], count, format, dup_args);
+
     if (g_error_writer == nullptr)
     {
-        pal::err_vprintf(format, args);
+        pal::err_fputs(buffer.data());
     }
     else
     {
-        va_list dup_args;
-        va_copy(dup_args, args);
-        int count = pal::str_vprintf(NULL, 0, format, args) + 1;
-        std::vector<pal::char_t> buffer(count);
-        pal::str_vprintf(&buffer[0], count, format, dup_args);
         g_error_writer(buffer.data());
     }
 
-    if (g_trace_verbosity && (g_trace_file != stderr))
+#if defined(_WIN32)
+    ::OutputDebugStringW(buffer.data());
+#endif
+
+    if (g_trace_verbosity && ((g_trace_file != stderr) || g_error_writer != nullptr))
     {
-        pal::file_vprintf(g_trace_file, format, args);
+        pal::file_vprintf(g_trace_file, format, trace_args);
     }
     va_end(args);
 }

src/corehost/common/utils.h

@@ -5,6 +5,7 @@
 #define UTILS_H
 
 #include "pal.h"
+#include "trace.h"
 struct host_option
 {
     pal::string_t option;
@@ -55,4 +56,38 @@ size_t index_of_non_numeric(const pal::string_t& str, unsigned i);
 bool try_stou(const pal::string_t& str, unsigned* num);
 pal::string_t get_dotnet_root_env_var_name();
 pal::string_t get_deps_from_app_binary(const pal::string_t& app_base, const pal::string_t& app);
+
+// Helper class to make it easy to propagate error writer to the hostpolicy
+class propagate_error_writer_t
+{
+public:
+	typedef trace::error_writer_fn(*set_error_writer_fn)(trace::error_writer_fn error_writer);
+
+private:
+	set_error_writer_fn m_set_error_writer;
+	bool m_error_writer_set;
+
+public:
+	propagate_error_writer_t(set_error_writer_fn set_error_writer)
+	{
+		m_set_error_writer = set_error_writer;
+		m_error_writer_set = false;
+
+		trace::error_writer_fn error_writer = trace::get_error_writer();
+		if (error_writer != nullptr && m_set_error_writer != nullptr)
+		{
+			m_set_error_writer(error_writer);
+			m_error_writer_set = true;
+		}
+	}
+
+	~propagate_error_writer_t()
+	{
+		if (m_error_writer_set && m_set_error_writer != nullptr)
+		{
+			m_set_error_writer(nullptr);
+			m_error_writer_set = false;
+		}
+	}
+};
 #endif

src/corehost/corehost.cpp

@@ -17,6 +17,8 @@
 
 typedef int(*hostfxr_main_fn) (const int argc, const pal::char_t* argv[]);
 typedef int(*hostfxr_main_startupinfo_fn) (const int argc, const pal::char_t* argv[], const pal::char_t* host_path, const pal::char_t* dotnet_root, const pal::char_t* app_path);
+typedef void(*hostfxr_error_writer_fn) (const pal::char_t* message);
+typedef hostfxr_error_writer_fn(*hostfxr_set_error_writer_fn) (hostfxr_error_writer_fn error_writer);
 
 #if FEATURE_APPHOST
 
@@ -282,9 +284,13 @@ int run(const int argc, const pal::char_t* argv[])
         trace::info(_X("Dotnet path: [%s]"), dotnet_root.c_str());
         trace::info(_X("App path: [%s]"), app_path.c_str());
 
+        hostfxr_set_error_writer_fn set_error_writer_fn = (hostfxr_set_error_writer_fn)pal::get_symbol(fxr, "hostfxr_set_error_writer");
+
         // Previous corehost trace messages must be printed before calling trace::setup in hostfxr
         trace::flush();
 
+        propagate_error_writer_t propagate_error_writer_to_hostfxr(set_error_writer_fn);
+
         rc = main_fn_v2(argc, argv, host_path_cstr, dotnet_root_cstr, app_path_cstr);
     }
     else
@@ -320,6 +326,30 @@ int run(const int argc, const pal::char_t* argv[])
     return rc;
 }
 
+#if defined(_WIN32) && defined(FEATURE_APPHOST)
+pal::string_t g_buffered_errors;
+
+void buffering_trace_writer(const pal::char_t* message)
+{
+    g_buffered_errors.append(message).append(_X("\n"));
+}
+
+// Determines if the current module (should be the apphost.exe) is marked as Windows GUI application
+// in case it's not a GUI application (so should be CUI) or in case of any error the function returns false.
+bool get_windows_graphical_user_interface_bit()
+{
+    HMODULE module = ::GetModuleHandleW(NULL);
+    BYTE *bytes = (BYTE *)module;
+
+    // https://en.wikipedia.org/wiki/Portable_Executable
+    UINT32 pe_header_offset = ((IMAGE_DOS_HEADER *)bytes)->e_lfanew;
+    UINT16 subsystem = ((IMAGE_NT_HEADERS *)(bytes + pe_header_offset))->OptionalHeader.Subsystem;
+
+    return subsystem == IMAGE_SUBSYSTEM_WINDOWS_GUI;
+}
+
+#endif
+
 #if defined(_WIN32)
 int __cdecl wmain(const int argc, const pal::char_t* argv[])
 #else
@@ -338,5 +368,35 @@ int main(const int argc, const pal::char_t* argv[])
         trace::info(_X("}"));
     }
 
-    return run(argc, argv);
+#if defined(_WIN32) && defined(FEATURE_APPHOST)
+    if (get_windows_graphical_user_interface_bit())
+    {
+        // If this is a GUI application, buffer errors to display them later. Without this any errors are effectively lost
+        // unless the caller explicitly redirects stderr. This leads to bad experience of running the GUI app and nothing happening.
+        trace::set_error_writer(buffering_trace_writer);
+    }
+#endif
+
+    int exit_code = run(argc, argv);
+
+    // Flush traces before exit - just to be sure, and also if we're showing a popup below the error should show up in traces
+    // by the time the popup is displayed.
+    trace::flush();
+
+#if defined(_WIN32) && defined(FEATURE_APPHOST)
+    // No need to unregister the error writer since we're exiting anyway.
+    if (!g_buffered_errors.empty())
+    {
+        // If there are errors buffered, display them as a dialog. We only buffer if there's no console attached.
+        pal::string_t executable_name;
+        if (pal::get_own_executable_path(&executable_name))
+        {
+            executable_name = get_filename(executable_name);
+        }
+
+        ::MessageBoxW(NULL, g_buffered_errors.c_str(), executable_name.c_str(), MB_OK);
+    }
+#endif
+
+    return exit_code;
 }

src/test/HostActivationTests/GivenThatICareAboutStandaloneAppActivation.cs

@@ -2,23 +2,25 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
-using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Runtime.InteropServices;
-using System.Text.RegularExpressions;
 using Xunit;
 using FluentAssertions;
 using Microsoft.DotNet.CoreSetup.Test;
 using Microsoft.DotNet.Cli.Build.Framework;
-using Newtonsoft.Json.Linq;
 using System.Security.Cryptography;
 using System.Text;
-using Microsoft.DotNet.InternalAbstractions;
+using System.Diagnostics;
+using System.Collections.Generic;
+using System.Threading;
 
 namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.StandaloneApp
 {
     public class GivenThatICareAboutStandaloneAppActivation : IClassFixture<GivenThatICareAboutStandaloneAppActivation.SharedTestState>
     {
+        private readonly string AppHostExeName = "apphost" + Constants.ExeSuffix;
+
         private SharedTestState sharedTestState;
 
         public GivenThatICareAboutStandaloneAppActivation(GivenThatICareAboutStandaloneAppActivation.SharedTestState fixture)
@@ -237,30 +239,13 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.StandaloneApp
             var fixture = sharedTestState.PreviouslyPublishedAndRestoredStandaloneTestProjectFixture
                 .Copy();
 
-            var appExe = fixture.TestProject.AppExe;
+            string appExe = fixture.TestProject.AppExe;
 
-            string hostExeName = $"apphost{Constants.ExeSuffix}";
-            string builtAppHost = Path.Combine(sharedTestState.RepoDirectories.HostArtifacts, hostExeName);
-            string appName = Path.GetFileNameWithoutExtension(appExe);
-            string appDll = $"{appName}.dll";
-            string appDir = Path.GetDirectoryName(appExe);
-            string appDirHostExe = Path.Combine(appDir, hostExeName);
-
-            // Make a copy of apphost first, replace hash and overwrite app.exe, rather than
-            // overwrite app.exe and edit in place, because the file is opened as "write" for
-            // the replacement -- the test fails with ETXTBSY (exit code: 26) in Linux when
-            // executing a file opened in "write" mode.
-            File.Copy(builtAppHost, appDirHostExe, true);
-            using (var sha256 = SHA256.Create())
-            {
-                // Replace the hash with the managed DLL name.
-                var hash = sha256.ComputeHash(Encoding.UTF8.GetBytes("foobar"));
-                var hashStr = BitConverter.ToString(hash).Replace("-", "").ToLower();
-                AppHostExtensions.SearchAndReplace(appDirHostExe, Encoding.UTF8.GetBytes(hashStr), Encoding.UTF8.GetBytes(appDll), true);
-            }
-            File.Copy(appDirHostExe, appExe, true);
+            UseBuiltAppHost(appExe);
+            BindAppHost(appExe);
 
             Command.Create(appExe)
+                .EnvironmentVariable("COREHOST_TRACE", "1")
                 .CaptureStdErr()
                 .CaptureStdOut()
                 .Execute()
@@ -272,6 +257,160 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.StandaloneApp
                 .HaveStdOutContaining($"Framework Version:{sharedTestState.RepoDirectories.MicrosoftNETCoreAppVersion}");
         }
 
+        [Fact]
+        public void Running_AppHost_with_GUI_Reports_Errors_In_Window()
+        {
+            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            {
+                // GUI app host is only supported on Windows.
+                return;
+            }
+
+            var fixture = sharedTestState.PreviouslyPublishedAndRestoredStandaloneTestProjectFixture
+                .Copy();
+
+            string appExe = fixture.TestProject.AppExe;
+
+            // Mark the apphost as GUI, but don't bind it to anything - this will cause it to fail
+            UseBuiltAppHost(appExe);
+            MarkAppHostAsGUI(appExe);
+
+            Command command = Command.Create(appExe)
+                .CaptureStdErr()
+                .CaptureStdOut()
+                .Start();
+
+            IntPtr windowHandle = WaitForPopupFromProcess(command.Process);
+            Assert.NotEqual(IntPtr.Zero, windowHandle);
+
+            // In theory we should close the window - but it's just easier to kill the process.
+            // The popup should be the last thing the process does anyway.
+            command.Process.Kill();
+
+            CommandResult result = command.WaitForExit(true);
+
+            // There should be no output written by the process.
+            Assert.Equal(string.Empty, result.StdOut);
+            Assert.Equal(string.Empty, result.StdErr);
+
+            result.Should().Fail();
+        }
+
+        [Fact]
+        public void Running_AppHost_with_GUI_Reports_Errors_In_Window_and_Traces()
+        {
+            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            {
+                // GUI app host is only supported on Windows.
+                return;
+            }
+
+            var fixture = sharedTestState.PreviouslyPublishedAndRestoredStandaloneTestProjectFixture
+                .Copy();
+
+            string appExe = fixture.TestProject.AppExe;
+
+            // Mark the apphost as GUI, but don't bind it to anything - this will cause it to fail
+            UseBuiltAppHost(appExe);
+            MarkAppHostAsGUI(appExe);
+
+            string traceFilePath = Path.Combine(Path.GetDirectoryName(appExe), "trace.log");
+
+            Command command = Command.Create(appExe)
+                .EnvironmentVariable("COREHOST_TRACE", "1")
+                .EnvironmentVariable("COREHOST_TRACEFILE", traceFilePath)
+                .Start();
+
+            IntPtr windowHandle = WaitForPopupFromProcess(command.Process);
+            Assert.NotEqual(IntPtr.Zero, windowHandle);
+
+            // In theory we should close the window - but it's just easier to kill the process.
+            // The popup should be the last thing the process does anyway.
+            command.Process.Kill();
+
+            CommandResult result = command.WaitForExit(true);
+
+            result.Should().Fail()
+                .And.FileExists(traceFilePath)
+                .And.FileContains(traceFilePath, "This executable is not bound to a managed DLL to execute.");
+        }
+
+#if WINDOWS
+        private delegate bool EnumThreadWindowsDelegate(IntPtr hWnd, IntPtr lParam);
+
+        [DllImport("user32.dll")]
+        private static extern bool EnumThreadWindows(int dwThreadId, EnumThreadWindowsDelegate plfn, IntPtr lParam);
+
+        private IntPtr WaitForPopupFromProcess(Process process, int timeout = 5000)
+        {
+            IntPtr windowHandle = IntPtr.Zero;
+            while (timeout > 0)
+            {
+                foreach (ProcessThread thread in process.Threads)
+                {
+                    // Note we take the last window we find - there really should only be one at most anyway.
+                    EnumThreadWindows(thread.Id,
+                        (hWnd, lParam) => { windowHandle = hWnd; return true; }, IntPtr.Zero);
+                }
+
+                if (windowHandle != IntPtr.Zero)
+                {
+                    break;
+                }
+
+                Thread.Sleep(100);
+                timeout -= 100;
+            }
+
+            return windowHandle;
+        }
+#else
+        private IntPtr WaitForPopupFromProcess(Process process, int timeout = 5000)
+        {
+            throw new PlatformNotSupportedException();
+        }
+#endif
+
+        private void UseBuiltAppHost(string appExe)
+        {
+            File.Copy(Path.Combine(sharedTestState.RepoDirectories.HostArtifacts, AppHostExeName), appExe, true);
+        }
+
+        private void BindAppHost(string appExe)
+        {
+            string appName = Path.GetFileNameWithoutExtension(appExe);
+            string appDll = $"{appName}.dll";
+            string appDir = Path.GetDirectoryName(appExe);
+            string appDirHostExe = Path.Combine(appDir, AppHostExeName);
+
+            // Make a copy of apphost first, replace hash and overwrite app.exe, rather than
+            // overwrite app.exe and edit in place, because the file is opened as "write" for
+            // the replacement -- the test fails with ETXTBSY (exit code: 26) in Linux when
+            // executing a file opened in "write" mode.
+            File.Copy(appExe, appDirHostExe, true);
+            using (var sha256 = SHA256.Create())
+            {
+                // Replace the hash with the managed DLL name.
+                var hash = sha256.ComputeHash(Encoding.UTF8.GetBytes("foobar"));
+                var hashStr = BitConverter.ToString(hash).Replace("-", "").ToLower();
+                AppHostExtensions.SearchAndReplace(appDirHostExe, Encoding.UTF8.GetBytes(hashStr), Encoding.UTF8.GetBytes(appDll), true);
+            }
+            File.Copy(appDirHostExe, appExe, true);
+        }
+
+        private void MarkAppHostAsGUI(string appExe)
+        {
+            string appDir = Path.GetDirectoryName(appExe);
+            string appDirHostExe = Path.Combine(appDir, AppHostExeName);
+
+            File.Copy(appExe, appDirHostExe, true);
+            using (var sha256 = SHA256.Create())
+            {
+                AppHostExtensions.SetWindowsGraphicalUserInterfaceBit(appDirHostExe);
+            }
+            File.Copy(appDirHostExe, appExe, true);
+        }
+
         public class SharedTestState : IDisposable
         {
             public TestProjectFixture PreviouslyBuiltAndRestoredStandaloneTestProjectFixture { get; set; }

src/test/HostActivationTests/HostActivationTests.csproj

@@ -5,6 +5,7 @@
     <AssemblyName>HostActivationTests</AssemblyName>
     <PackageId>HostActivationTests</PackageId>
     <GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>
+    <DefineConstants Condition="'$(OSGroup)' == 'Windows_NT'">$(DefineConstants);WINDOWS</DefineConstants>
   </PropertyGroup>
 
   <ItemGroup>

src/test/TestUtils/AppHostExtensions.cs

@@ -113,6 +113,94 @@ namespace Microsoft.DotNet.CoreSetup.Test
                 }
             }
         }
+
+        /// <summary>
+        /// The first two bytes of a PE file are a constant signature.
+        /// </summary>
+        private const UInt16 PEFileSignature = 0x5A4D;
+
+        /// <summary>
+        /// The offset of the PE header pointer in the DOS header.
+        /// </summary>
+        private const int PEHeaderPointerOffset = 0x3C;
+
+        /// <summary>
+        /// The offset of the Subsystem field in the PE header.
+        /// </summary>
+        private const int SubsystemOffset = 0x5C;
+
+        /// <summary>
+        /// The value of the sybsystem field which indicates Windows GUI (Graphical UI)
+        /// </summary>
+        private const UInt16 WindowsGUISubsystem = 0x2;
+
+        /// <summary>
+        /// The value of the subsystem field which indicates Windows CUI (Console)
+        /// </summary>
+        private const UInt16 WindowsCUISubsystem = 0x3;
+
+        public static void SetWindowsGraphicalUserInterfaceBit(string appHostPath)
+        {
+            // Re-write ModifiedAppHostPath with the proper contents.
+            using (var memoryMappedFile = MemoryMappedFile.CreateFromFile(appHostPath))
+            {
+                using (MemoryMappedViewAccessor accessor = memoryMappedFile.CreateViewAccessor())
+                {
+                    SetWindowsGraphicalUserInterfaceBit(accessor);
+                }
+            }
+        }
+
+        /// <summary>
+        /// If the apphost file is a windows PE file (checked by looking at the first few bytes)
+        /// this method will set its subsystem to GUI.
+        /// </summary>
+        /// <param name="accessor">The memory accessor which has the apphost file opened.</param>
+        /// <param name="appHostSourcePath">The path to the source apphost.</param>
+        private static unsafe void SetWindowsGraphicalUserInterfaceBit(
+            MemoryMappedViewAccessor accessor)
+        {
+            byte* pointer = null;
+
+            try
+            {
+                accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref pointer);
+                byte* bytes = pointer + accessor.PointerOffset;
+
+                // https://en.wikipedia.org/wiki/Portable_Executable
+                // Validate that we're looking at Windows PE file
+                if (((UInt16*)bytes)[0] != PEFileSignature || accessor.Capacity < PEHeaderPointerOffset + sizeof(UInt32))
+                {
+                    throw new Exception("apphost is not a Windows exe.");
+                }
+
+                UInt32 peHeaderOffset = ((UInt32*)(bytes + PEHeaderPointerOffset))[0];
+
+                if (accessor.Capacity < peHeaderOffset + SubsystemOffset + sizeof(UInt16))
+                {
+                    throw new Exception("apphost is not a Windows exe.");
+                }
+
+                UInt16* subsystem = ((UInt16*)(bytes + peHeaderOffset + SubsystemOffset));
+
+                // https://docs.microsoft.com/en-us/windows/desktop/Debug/pe-format#windows-subsystem
+                // The subsystem of the prebuilt apphost should be set to CUI
+                if (subsystem[0] != WindowsCUISubsystem)
+                {
+                    throw new Exception("apphost is not a Windows CLI.");
+                }
+
+                // Set the subsystem to GUI
+                subsystem[0] = WindowsGUISubsystem;
+            }
+            finally
+            {
+                if (pointer != null)
+                {
+                    accessor.SafeMemoryMappedViewHandle.ReleasePointer();
+                }
+            }
+        }
     }
 }
 

src/test/TestUtils/Assertions/CommandResultAssertions.cs

@@ -126,7 +126,7 @@ namespace Microsoft.DotNet.CoreSetup.Test
         public AndConstraint<CommandResultAssertions> FileContains(string path, string pattern)
         {
             Execute.Assertion.ForCondition(System.IO.File.ReadAllText(path).Contains(pattern))
-                .FailWith("The command did not write the expected result '{1}' to the file: {1}{2}", pattern, path, GetDiagnosticsInfo());
+                .FailWith("The command did not write the expected result '{0}' to the file: {1}{2}", pattern, path, GetDiagnosticsInfo());
             return new AndConstraint<CommandResultAssertions>(this);
         }
 

src/test/TestUtils/Command.cs

@@ -28,6 +28,8 @@ namespace Microsoft.DotNet.Cli.Build.Framework
         private bool _running = false;
         private bool _quietBuildReporter = false;
 
+        public Process Process { get => _process; }
+
         private Command(string executable, string args)
         {
             // Set the things we need
@@ -166,7 +168,7 @@ namespace Microsoft.DotNet.Cli.Build.Framework
             return Execute(false);
         }
 
-        public CommandResult Execute(bool fExpectedToFail)
+        public Command Start()
         {
             ThrowIfRunning();
             _running = true;
@@ -189,7 +191,6 @@ namespace Microsoft.DotNet.Cli.Build.Framework
 
             _process.EnableRaisingEvents = true;
 
-            var sw = Stopwatch.StartNew();
             ReportExecBegin();
 
             _process.Start();
@@ -204,6 +205,11 @@ namespace Microsoft.DotNet.Cli.Build.Framework
                 _process.BeginErrorReadLine();
             }
 
+            return this;
+        }
+
+        public CommandResult WaitForExit(bool fExpectedToFail)
+        {
             _process.WaitForExit();
 
             var exitCode = _process.ExitCode;
@@ -217,6 +223,12 @@ namespace Microsoft.DotNet.Cli.Build.Framework
                 _stdErrCapture?.GetStringBuilder()?.ToString());
         }
 
+        public CommandResult Execute(bool fExpectedToFail)
+        {
+            Start();
+            return WaitForExit(fExpectedToFail);
+        }
+
         public Command WorkingDirectory(string projectDirectory)
         {
             _process.StartInfo.WorkingDirectory = projectDirectory;