Skip to content
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.

Sign up for GitHub

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

Improve GUI error dialog when no runtime is installed #57089

Merged
merged 2 commits into from
Aug 10, 2021
Merged

Improve GUI error dialog when no runtime is installed #57089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb898c1
Add architecture and runtime version to error dialog
Aug 9, 2021
4151487
PR feedback
Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 43 additions & 26 deletions src/installer/tests/HostActivation.Tests/PortableAppActivation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,11 +494,9 @@ public void AppHost_CLI_FrameworkDependent_MissingRuntimeFramework_ErrorReported
}
}

[Theory]
[Fact]
[PlatformSpecific(TestPlatforms.Windows)] // GUI app host is only supported on Windows.
[InlineData(true)]
[InlineData(false)]
public void AppHost_GUI_FrameworkDependent_MissingRuntimeFramework_ErrorReportedInDialog(bool missingHostfxr)
public void AppHost_GUI_FrameworkDependent_MissingRuntimeFramework_ErrorReportedInDialog()
{
var fixture = sharedTestState.PortableAppFixture_Built
.Copy();
Expand All @@ -515,22 +513,12 @@ public void AppHost_GUI_FrameworkDependent_MissingRuntimeFramework_ErrorReported

string expectedErrorCode;
string expectedUrlQuery;
string expectedUrlParameter = null;
if (missingHostfxr)
{
expectedErrorCode = Constants.ErrorCode.CoreHostLibMissingFailure.ToString("x");
expectedUrlQuery = "missing_runtime=true&";
expectedUrlParameter = $"&apphost_version={sharedTestState.RepoDirectories.MicrosoftNETCoreAppVersion}";
}
else
{
invalidDotNet = new DotNetBuilder(invalidDotNet, sharedTestState.RepoDirectories.BuiltDotnet, "missingFramework")
.Build()
.BinPath;
expectedErrorCode = Constants.ErrorCode.FrameworkMissingFailure.ToString("x");
expectedUrlQuery = $"framework={Constants.MicrosoftNETCoreApp}&framework_version={sharedTestState.RepoDirectories.MicrosoftNETCoreAppVersion}";
}
invalidDotNet = new DotNetBuilder(invalidDotNet, sharedTestState.RepoDirectories.BuiltDotnet, "missingFramework")
.Build()
.BinPath;

expectedErrorCode = Constants.ErrorCode.FrameworkMissingFailure.ToString("x");
expectedUrlQuery = $"framework={Constants.MicrosoftNETCoreApp}&framework_version={sharedTestState.RepoDirectories.MicrosoftNETCoreAppVersion}";
Command command = Command.Create(appExe)
.EnableTracingAndCaptureOutputs()
.DotNetRoot(invalidDotNet)
Expand All @@ -541,16 +529,45 @@ public void AppHost_GUI_FrameworkDependent_MissingRuntimeFramework_ErrorReported
command.Process.Kill();

var result = command.WaitForExit(true)
.Should().Fail();

result.And.HaveStdErrContaining($"Showing error dialog for application: '{Path.GetFileName(appExe)}' - error code: 0x{expectedErrorCode}")
.Should().Fail()
.And.HaveStdErrContaining($"Showing error dialog for application: '{Path.GetFileName(appExe)}' - error code: 0x{expectedErrorCode}")
.And.HaveStdErrContaining($"url: 'https://aka.ms/dotnet-core-applaunch?{expectedUrlQuery}")
.And.HaveStdErrContaining("&gui=true");
}
}

if (expectedUrlParameter != null)
{
result.And.HaveStdErrContaining(expectedUrlParameter);
}
[Fact]
[PlatformSpecific(TestPlatforms.Windows)]
public void AppHost_GUI_MissingRuntime_ErrorReportedInDialog()
{
var fixture = sharedTestState.PortableAppFixture_Built
.Copy();

string appExe = fixture.TestProject.AppExe;
File.Copy(sharedTestState.BuiltAppHost, appExe, overwrite: true);
AppHostExtensions.BindAppHost(appExe);
AppHostExtensions.SetWindowsGraphicalUserInterfaceBit(appExe);

string invalidDotNet = SharedFramework.CalculateUniqueTestDirectory(Path.Combine(TestArtifact.TestArtifactsPath, "guiErrors"));
using (new TestArtifact(invalidDotNet))
{
Directory.CreateDirectory(invalidDotNet);
var command = Command.Create(appExe)
.EnableTracingAndCaptureOutputs()
.DotNetRoot(invalidDotNet)
.MultilevelLookup(false)
.Start();

WaitForPopupFromProcess(command.Process);
command.Process.Kill();

var expectedErrorCode = Constants.ErrorCode.CoreHostLibMissingFailure.ToString("x");
var result = command.WaitForExit(true)
.Should().Fail()
.And.HaveStdErrContaining($"Showing error dialog for application: '{Path.GetFileName(appExe)}' - error code: 0x{expectedErrorCode}")
.And.HaveStdErrContaining($"url: 'https://aka.ms/dotnet-core-applaunch?missing_runtime=true")
.And.HaveStdErrContaining("gui=true")
.And.HaveStdErrContaining($"&apphost_version={sharedTestState.RepoDirectories.MicrosoftNETCoreAppVersion}");
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/native/corehost/apphost/apphost.windows.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,12 @@ namespace
if (pal::getenv(_X("DOTNET_DISABLE_GUI_ERRORS"), &gui_errors_disabled) && pal::xtoi(gui_errors_disabled.c_str()) == 1)
return;

pal::string_t dialogMsg = _X("To run this application, you must install .NET.\n\n");
pal::string_t dialogMsg;
pal::string_t url;
const pal::string_t url_prefix = _X(" - ") DOTNET_CORE_APPLAUNCH_URL _X("?");
if (error_code == StatusCode::CoreHostLibMissingFailure)
{
dialogMsg = pal::string_t(_X("To run this application, you must install .NET Desktop Runtime ")) + _STRINGIFY(COMMON_HOST_PKG_VER) + _X(" (") + get_arch() + _X(").\n\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the architecture to the error. I think we should also add this to the missing framework error.
There are multiple places for this, but the most important ones are:

runtime/src/native/corehost/fxr/fx_resolver.messages.cpp

Line 115 in e6e7ef9

trace::error(_X("The framework '%s', version '%s' was not found."), fx_name.c_str(), fx_version.c_str());

and

runtime/src/native/corehost/fxr/fx_resolver.messages.cpp

Line 119 in e6e7ef9

trace::error(_X("The framework '%s' was not found."), fx_name.c_str());

I would just add "(x86)" to the message after the framework name (or version).

It's a minor change, but I think this will be crucial for the ARM64 cases.

pal::string_t line;
pal::stringstream_t ss(g_buffered_errors);
while (std::getline(ss, line, _X('\n'))) {
Expand All @@ -81,6 +82,7 @@ namespace
{
// We don't have a great way of passing out different kinds of detailed error info across components, so
// just match the expected error string. See fx_resolver.messages.cpp.
dialogMsg = pal::string_t(_X("To run this application, you must install .NET.\n\n"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably reword this to something like: "To run this application, you must install missing framework for .NET." - The "missing .NET" case is handled above, this is the "missing framework case" (and we MUST NOT use the ".NET framework" wording - since that would make it sound like the .NET Framework product.

pal::string_t line;
pal::stringstream_t ss(g_buffered_errors);
while (std::getline(ss, line, _X('\n'))){
Expand Down