[generator] Gracefully handle BindingGeneratorException. #845
Conversation
| Run (options); | ||
| } catch (BindingGeneratorException) { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Should other exception types be handled, "just in case", so that we don't get the "unhandled exception" output? Or do we want the unhandled exception output?
|
I'm feeling like a dummy reviewing this, but: How are I also find it to be a "nicer experience" if "as many errors as possible" can be printed, vs. just bailing at the first error, and I fear that |
| // BG4301 | ||
| Report.LogCodedError (Report.ErrorRemoveNodeInvalidXPath, e, metaitem, path); | ||
| Report.LogCodedError (Report.ErrorRemoveNodeInvalidXPath, metaitem, path); |
There was a problem hiding this comment.
Why don't we want to include e anymore?
There was a problem hiding this comment.
We don't use it anywhere, we simply print out the localized message.
|
I made most errors non-terminal so that we'll output them all. The remaining terminal ones are largely various "this shouldn't happen" errors that we probably can't recover from. |
@moljac pointed out that if you have invalid XPath in your metadata, we simply error with the unhelpful: "generator.exe" exited with code -XXXXXX  If you turn on `Diagnostic` logs, you can see the uncaught exception hiding in a lot of ceremony: message BG0000: Unhandled Exception: Java.Interop.Tools.Generator.BindingGeneratorException: C:\code\temp\xamarin.exoplayer\Xamarin.ExoPlayer.Core\Transforms\Metadata.xml(11, 4): error BG4304: Invalid XPath specification: /api/package[@name='com.google.android.exoplayer2']interface[@name='ExoPlayer']/method[@name='addMediaItems' and count(parameter)=2 and parameter[1][@type='int'] and parameter[2][@type='java.util.List<com.google.android.exoplayer2.source.MediaSource>']]/parameter[2]. ---> System.Xml.XPath.XPathException: '/api/package[@name='com.google.android.exoplayer2']interface[@name='ExoPlayer']/method[@name='addMediaItems' and count(parameter)=2 and parameter[1][@type='int'] and parameter[2][@type='java.util.List<com.google.android.exoplayer2.source.MediaSource>']]/parameter[2]' has an invalid token. message BG0000: at MS.Internal.Xml.XPath.XPathParser.ParseXPathExpresion(String xpathExpresion) message BG0000: at System.Xml.XPath.XPathExpression.Compile(String xpath, IXmlNamespaceResolver nsResolver) message BG0000: at System.Xml.XPath.XPathNavigator.Evaluate(String xpath, IXmlNamespaceResolver resolver) message BG0000: at System.Xml.XPath.XPathEvaluator.Evaluate[T](XNode node, String expression, IXmlNamespaceResolver resolver) message BG0000: at System.Xml.XPath.Extensions.XPathSelectElements(XNode node, String expression, IXmlNamespaceResolver resolver) message BG0000: at Java.Interop.Tools.Generator.FixupXmlDocument.Apply(ApiXmlDocument apiDocument, String apiLevelString, Int32 productVersion) message BG0000: --- End of inner exception stack trace --- (TaskId:46) message BG0000: at Java.Interop.Tools.Generator.Report.LogCodedError(LocalizedMessage message, Exception innerException, String sourceFile, Int32 line, Int32 column, String[] args) message BG0000: at Java.Interop.Tools.Generator.Report.LogCodedError(LocalizedMessage message, Exception innerException, XNode node, String[] args) message BG0000: at Java.Interop.Tools.Generator.FixupXmlDocument.Apply(ApiXmlDocument apiDocument, String apiLevelString, Int32 productVersion) message BG0000: at Java.Interop.Tools.Generator.ApiXmlDocument.ApplyFixupFile(FixupXmlDocument fixup) message BG0000: at Xamarin.Android.Binder.CodeGenerator.Run(CodeGeneratorOptions options, DirectoryAssemblyResolver resolver) message BG0000: at Xamarin.Android.Binder.CodeGenerator.Run(CodeGeneratorOptions options) message BG0000: at Xamarin.Android.Binder.CodeGenerator.Main(String[] args) error MSB6006: "generator.exe" exited with code -532462766. It turns out this happens anywhere we throw a `BindingGeneratorException`, as there is no `catch` handler that attempts to gracefully handles this exception. This led to making several improvements: - Gracefully handle `BindingGeneratorException` such that we print out the error as a standard MSBuild error that can be processed by Visual Studio. - Exit `generator` with `-1` if `BindingGeneratorException` is thrown. - Add a separate `Report.LogCodedError(…)` method called `Report.LogCodeErrorAndExit(…)`, which is used for terminal errors ("don't do any further processing and exit"). - Audit existing calls to `Report.LogCodedError()` to allow errors such as `Invalid XPath` to be non-terminal. (Today `generator` will exit after finding the first one.) - Remove spaces in the error file location: `metadata.xml(8, 4): ` becomes `metadata.xml(8,4):`. These were not being processed correctly and could not be double clicked in VS previously. The result is a more useful error message: Metadata.xml(11,4): error BG4304: Invalid XPath specification … We also now handle any unhandled exception a little better. Additionally, `ApiFixup.cs` was deleted as it was no longer used after the refactor in commit f4e68b5.
Mel pointed out that if you have invalid XPath in your metadata, we simply error with the unhelpful
"generator.exe" exited with code -XXXXXX:If you turn on
Diagnosticlogs, you can see the uncaught exception hiding in a lot of ceremony:It turns out this happens anywhere we throw a
BindingGeneratorExceptionas there is nocatchhandler that attempts to gracefully handles this exception. This led to making several improvements:BindingGeneratorExceptionsuch that we print out the error as a standard MSBuild error that can be processed by Visual Studio.generatorwith-1ifBindingGeneratorExceptionis thrown.Report.LogCodedError (...)method calledReport.LogCodeErrorAndExit (...)which is used for terminal errors ("don't do any further processing and exit").Report.LogCodedError (...)to allow errors such asInvalid XPathto be non-terminal. (Todaygeneratorwill exit after finding the first one.)metadata.xml(8, 4):->metadata.xml(8,4):. These were not being processed correctly and could not be double clicked in VS previously.Results:
We additionally handle any unhandled exception a little better:
Additionally
ApiFixup.cswas deleted as it was no longer used after the refactor in commit f4e68b5.