[Xamarin.Android.Tools.AndroidSdk] Parse Properties after header #143
Conversation
| } | ||
| if (e.Data.StartsWith (ContinuedValuePrefix, StringComparison.Ordinal)) { | ||
| if (curKey == null) | ||
| throw new InvalidOperationException ($"Unknown property key for value {e.Data}!"); |
There was a problem hiding this comment.
How in general should we handle errors here? It looks like now if "Property settings:" isn't found it silently ignores the JDK, returning no properties, but if currKey is null (parsing issue) it throws an exception which (at least sometimes) apparently causes the IDE to crash. Neither mode of error handling seems ideal. Instead can it log something for errors (if possible) then return in error cases (instead of throwing) to avoid a crash?
There was a problem hiding this comment.
@BretJohnson : The fundamental question -- for which there is no answer (see also The Error Model regarding Midori?) -- is thus: how should errors be handled? What is an error?
Do you want the error to be "loud and noisy", so that it's found and fixed? Or "quiet-ish", possibly allowing it to be ignored or forgotten about?
The problem encountered here was an error. The exception was thrown as part of asserting that something was true, and it was found to not be true in certain environments. It thus failed "loud and noisy", triggering a Watson.
The benefit to this is that I found out about the error, and thus was able to fix it. (Sucky for VS in the meantime? Also, what are the semantics of a Watson? Does it only happen if the entire IDE is torn down? Or in less "loud and noisy" circumstances?)
The alternative is for "quiet-ish" error handling, as you describe. This means that no Watson would have ever been reported, and likewise means we likely would have never known about this particular issue, which means it wouldn't be fixed. (Unless these error messages are reported "somewhere", and "someone" regularly reviews the messages to try to track down errors...?)
I personally prefer "loud and noisy" errors -- for various definitions of "loud" and "noisy" -- because it helps ensure that my team actually gets error reports. (Whether that be via telemetry reporting error codes, or your team appropriately forwarding Watson bugs, or...)
I don't currently see or understand how my team would otherwise know these errors even exist.
That's the philosophical bit/rant.
The counter-argument is whether this code is "actually" important enough to warrant "loud and noisy" in the first place. To which my answer is...I'm not actually sure.
One of the "useful" bits of JdkInfo is JdkInfo.Version, which allows sorting JDKs to reasonably determine which is the latest/oldest/whatever. JdkVersion tries to use the release file within a JDK install, but if that file is missing, it will "fall back" to using the java.version property as gleamed from java -XshowSettings:properties -version. I consider this to be a very important bit of functionality.
That said, does it require that java -XshowSettings:properties -version parsing be "robust"? Not necessarily; it could instead require ("loud and noisy") that a release file exist, and only use that for Version information. This might even be desirable! I'm not entirely sure.
| const string NameValueDelim = " = "; | ||
| if (string.IsNullOrEmpty (e.Data)) | ||
| return; | ||
| if (e.Data.StartsWith ("Property settings:", StringComparison.Ordinal)) { |
There was a problem hiding this comment.
So probably what happened, is some JDK doesn't understand -XshowSettings:properties and ignored it?
This seems like the right string to look for:
| foundPS = true; | ||
| } | ||
| if (!foundPS) { | ||
| return; |
There was a problem hiding this comment.
I think if we at least logged a message here, that would be helpful. It should show up in diagnostic MSBuild logs?
There was a problem hiding this comment.
Related: log where? Unlike AndroidSdkInfo, there is no Action<TraceLevel, string> parameter. We could certainly add one, but that would require changes to calling code to receive those messages, otherwise they'd just go to stdout/stderr.
There was a problem hiding this comment.
Seems like we're good then.
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1397171 Context: https://www.4e00.com/blog/java/2016/12/16/java-command-line-options.html We have a Watson report of an `InvalidOperationException` being thrown from `JdkInfo.GetJavaProperties()`: Xamarin.Android.Tools.AndroidSdk!Xamarin.Android.Tools.JdkInfo ... clr!IL_Throw Xamarin.Android.Tools.AndroidSdk!Xamarin.Android.Tools.JdkInfo.__c__DisplayClass46_0._GetJavaProperties_b__0 The dump provides an additional glimmer of information: the `InvalidOperationException` message text: Unknown property key for value (to execute a class)! `JdkInfo.GetJavaProperties()` only contains one `throw`, for when we think we're processing a multi-line value, but we don't have a key for the value encountered. Which brings us to [java-command-line-options.html][0], which isn't English, and isn't recent, but *does* show `java` output which contains our "offending" string of "(to execute a class)": $ java -showversion -help java version "1.8.0_66" Java(TM) SE Runtime Environment (build 1.8.0_66-b17) Java HotSpot(TM) 64-Bit Server VM (build 25.66-b17, mixed mode) Usage: java [-options] class [args...] (to execute a class) ... What appears to be happening is that `JdkInfo` is encountering a JDK for which `java -XshowSettings:properties -version` shows something resembling the above `java -showversion -help` output, and the " (to execute a class)" is treated as part of a multi-line value, which it isn't. Update `JdkInfo.GetJavaProperties()` so that we *require* that we see "Property settings:" *before* we start looking for keys & values. This should ensure that we appropriately ignore "(to execute a class)". Additionally, add a `JdkInfo` constructor overload which takes an `Action<TraceLevel, string>? logger` parameter, a'la `AndroidSdkInfo`, so that `JdkInfo` can provide additional "contextual" logging without requiring the use of exceptions. Turn the lack of a key for a multi- line value into a warning, instead of an exception. Finally, update `tools/ls-jdks` so that it will accept a path to a JDK to look at, and dump out the parsed properties. [0]: https://www.4e00.com/blog/java/2016/12/16/java-command-line-options.html
jonpryor
force-pushed
the
jonp-require-properties-header
branch
from
c0e0769 to
09c3152
Compare
…net#143) Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1397171 Context: https://www.4e00.com/blog/java/2016/12/16/java-command-line-options.html We have a Watson report of an `InvalidOperationException` being thrown from `JdkInfo.GetJavaProperties()`: Xamarin.Android.Tools.AndroidSdk!Xamarin.Android.Tools.JdkInfo ... clr!IL_Throw Xamarin.Android.Tools.AndroidSdk!Xamarin.Android.Tools.JdkInfo.__c__DisplayClass46_0._GetJavaProperties_b__0 The dump provides an additional glimmer of information: the `InvalidOperationException` message text: Unknown property key for value (to execute a class)! `JdkInfo.GetJavaProperties()` only contains one `throw`, for when we think we're processing a multi-line value, but we don't have a key for the value encountered. Which brings us to [java-command-line-options.html][0], which isn't English, and isn't recent, but *does* show `java` output which contains our "offending" string of "(to execute a class)": $ java -showversion -help java version "1.8.0_66" Java(TM) SE Runtime Environment (build 1.8.0_66-b17) Java HotSpot(TM) 64-Bit Server VM (build 25.66-b17, mixed mode) Usage: java [-options] class [args...] (to execute a class) ... What appears to be happening is that `JdkInfo` is encountering a JDK for which `java -XshowSettings:properties -version` shows something resembling the above `java -showversion -help` output, and the " (to execute a class)" is treated as part of a multi-line value, which it isn't. Update `JdkInfo.GetJavaProperties()` so that we *require* that we see "Property settings:" *before* we start looking for keys & values. This should ensure that we appropriately ignore " (to execute a class)". Additionally, add a `JdkInfo` constructor overload which takes an `Action<TraceLevel, string>? logger` parameter, a'la `AndroidSdkInfo`, so that `JdkInfo` can provide additional "contextual" logging without requiring the use of exceptions. Turn the lack of a key for a multi- line value into a warning, instead of an exception. Finally, update `tools/ls-jdks` so that it will accept a path to a JDK to look at, and dump out the parsed properties. [0]: https://www.4e00.com/blog/java/2016/12/16/java-command-line-options.html
3 participants
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1397171
Context: https://www.4e00.com/blog/java/2016/12/16/java-command-line-options.html
We have a Watson report of an
InvalidOperationExceptionbeingthrown from
JdkInfo.GetJavaProperties():The dump provides an additional glimmer of information: the
InvalidOperationExceptionmessage text:JdkInfo.GetJavaProperties()only contains onethrow, for when wethink we're processing a multi-line value, but we don't have a key
for the value encountered.
Which brings us to java-command-line-options.html, which isn't
English, and isn't recent, but does show
javaoutput whichcontains our "offending" string "(to execute a class)":
What appears to be happening is that
JdkInfois encountering a JDKfor which
java -XshowSettings:properties -versionshows somethingresembling the above
java -showversion -helpoutput, and the" (to execute a class)" is treated as part of a multi-line
value, which it isn't.
Update
JdkInfo.GetJavaProperties()so that we require that we see"Property settings:" before we start looking for keys & values.
This should ensure that we appropriately ignore
"(to execute a class)".
Additionally, update
tools/ls-jdksso that it will accept a pathto a JDK to look at, and dump out the parsed properties.