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

[nnyeah] Centralize error handling #15064

Merged
merged 5 commits into from
May 23, 2022
Merged

[nnyeah] Centralize error handling #15064

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4935f5e
[nnyeah] Centralize error handling
chamons May 19, 2022
bced388
Merge branch 'main' into nnyeah_error_refactor
chamons May 19, 2022
9f51ec9
Update tools/nnyeah/nnyeah/Program.cs
chamons May 19, 2022
2597435
Catch all exceptions
chamons May 19, 2022
0b89a17
Improve uncaught exception behavior
chamons May 20, 2022
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
2 changes: 1 addition & 1 deletion tools/nnyeah/nnyeah/MemberNotFoundException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
using System;
namespace Microsoft.MaciOS.Nnyeah {
public class MemberNotFoundException : Exception {
public class MemberNotFoundException : ConversionException {
public MemberNotFoundException (string memberName)
: base ($"Member {memberName} not found.")
{
Expand Down
61 changes: 35 additions & 26 deletions tools/nnyeah/nnyeah/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,29 @@
using System.Diagnostics.CodeAnalysis;

namespace Microsoft.MaciOS.Nnyeah {
public class ConversionException : Exception {
public ConversionException (string message) : base (message)
{
}

public ConversionException (string message, params object? [] args) : base (string.Format(message, args))
{
}
}

public class Program {
static void Main (string [] args)
static int Main (string [] args)
{
try {
Main2 (args);
return 0;
} catch (Exception e) {
Console.Error.WriteLine (e.Message);
return 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

You'll want to catch all exceptions too, otherwise the process will exit with a very ugly message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I wasn't sure here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

I'd add the following, do catch our base exception, then catch all exceptions and do a console.writeline with the message and then the url to provide an issue. If we got an unexpected exception it should be a bug/issue just like we do in xamarin.

}

static void Main2 (string [] args)
{
var doHelp = false;
string? infile = null, outfile = null;
Expand All @@ -35,22 +56,20 @@ static void Main (string [] args)
}

if (infile is null || outfile is null) {
Console.Error.WriteLine (Errors.E0014);
Environment.Exit (1);
throw new ConversionException (Errors.E0014);
}

// TODO - Long term this should default to files packaged within the tool but allow overrides
if (xamarinAssembly is null || microsoftAssembly is null) {
Console.Error.WriteLine (Errors.E0015);
Environment.Exit (1);
throw new ConversionException (Errors.E0015);
}

if (doHelp) {
PrintOptions (options, Console.Out);
Environment.Exit (0);
}
ProcessAssembly (xamarinAssembly!, microsoftAssembly!, infile!,
outfile!, verbose, forceOverwrite, suppressWarnings);
else {
ProcessAssembly (xamarinAssembly!, microsoftAssembly!, infile!, outfile!, verbose, forceOverwrite, suppressWarnings);
}
Comment on lines +75 to +77
Copy link
Member

Choose a reason for hiding this comment

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

If you changed this method to return the exit code, you won't need this change, you can just return 0; after PrintOptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that before. I felt that showing the flow as explicit "if show help do that, else do all of the processing" was better than an early return.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's more of a style choice, either way is fine with me.

}

public static void ProcessAssembly (string xamarinAssembly,
Expand Down Expand Up @@ -134,9 +153,7 @@ static bool TryLoadTypeAndModuleMap (string earlier, string later, bool publicOn

return Reworker.CreateReworker (stm, moduleContainer, typeMap);
} catch (Exception e) {
Console.Error.WriteLine (Errors.E0003, infile, e.Message);
Environment.Exit (1);
throw;
throw new ConversionException (Errors.E0003, infile, e.Message);
}
}

Expand All @@ -149,13 +166,11 @@ static void ReworkFile (string infile, string outfile, bool verbose, bool forceO
var transforms = new List<string> ();

if (!File.Exists (infile)) {
Console.Error.WriteLine (Errors.E0001, infile);
Environment.Exit (1);
throw new ConversionException (Errors.E0001, infile);
}

if (File.Exists (outfile) && !forceOverwrite) {
Console.Error.WriteLine (Errors.E0002, outfile);
Environment.Exit (1);
throw new ConversionException (Errors.E0002, outfile);
}

if (CreateReworker (infile, typeMap, xamarinModule, microsoftModule) is Reworker reworker) {
Expand All @@ -172,14 +187,11 @@ static void ReworkFile (string infile, string outfile, bool verbose, bool forceO
warnings.ForEach (Console.WriteLine);
}
} catch (TypeNotFoundException e) {
Console.Error.Write (Errors.E0012, e.TypeName);
Environment.Exit (1);
throw new ConversionException (Errors.E0012, e.TypeName);
} catch (MemberNotFoundException e) {
Console.Error.WriteLine (Errors.E0013, e.MemberName);
Environment.Exit (1);
throw new ConversionException (Errors.E0013, e.MemberName);
} catch (Exception e) {
Console.Error.WriteLine (Errors.E0004, e.Message);
Environment.Exit (1);
throw new ConversionException (Errors.E0004, e.Message);
}
} else {
if (verbose) {
Expand All @@ -193,15 +205,12 @@ static Stream TryOpenRead (string path)
try {
var stm = new FileStream (path, FileMode.Open, FileAccess.Read);
if (stm is null) {
Console.Error.WriteLine (Errors.E0006, path, "");
Environment.Exit (1);
throw new ConversionException (Errors.E0006, path, "");
}
return stm;
} catch (Exception e) {
Console.Error.WriteLine (Errors.E0006, path, e.Message);
Environment.Exit (1);
throw new ConversionException (Errors.E0006, path, e.Message);
}
return new MemoryStream (); // never reached, thanks code analysis
}

static void PrintOptions (OptionSet options, TextWriter writer)
Expand Down
2 changes: 1 addition & 1 deletion tools/nnyeah/nnyeah/TypeNotFoundException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
using System;
namespace Microsoft.MaciOS.Nnyeah {
public class TypeNotFoundException : Exception {
public class TypeNotFoundException : ConversionException {
public TypeNotFoundException (string typeName)
: base ($"The type {typeName} was not found.")
{
Expand Down