[Xamarin.Android.Tools.AndroidSdk] Fix NRT warnings. #206
Conversation
| public string JarPath {get;} | ||
| public string JavaPath {get;} | ||
| public string JavacPath {get;} | ||
| public string? JarPath {get;} |
There was a problem hiding this comment.
I don't like these changes, and am not sure why they're needed. For example, if this is "actually" a JDK, then surely it has jar and javac and java, so these must exist. (No?) Additionally, these don't currently elicit warnings, at least not for me. Thus, why are these changes needed?
There was a problem hiding this comment.
We explicitly set these values to null when they cannot be found:
https://github.com/xamarin/xamarin-android-tools/blob/main/src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs#L64-L66
This results in the following warnings without these changes:
1>C:\code\xamarin-android-tools\src\Xamarin.Android.Tools.AndroidSdk\JdkInfo.cs(64,26,64,100): warning CS8601: Possible null reference assignment.
1>C:\code\xamarin-android-tools\src\Xamarin.Android.Tools.AndroidSdk\JdkInfo.cs(65,26,65,101): warning CS8601: Possible null reference assignment.
1>C:\code\xamarin-android-tools\src\Xamarin.Android.Tools.AndroidSdk\JdkInfo.cs(66,26,66,102): warning CS8601: Possible null reference assignment.
1>C:\code\xamarin-android-tools\src\Xamarin.Android.Tools.AndroidSdk\JdkInfo.cs(52,10,52,17): warning CS8618: Non-nullable property 'JarPath' must contain a non-null value when exiting constructor. Consider declaring the property as nullable.
1>C:\code\xamarin-android-tools\src\Xamarin.Android.Tools.AndroidSdk\JdkInfo.cs(52,10,52,17): warning CS8618: Non-nullable property 'JavaPath' must contain a non-null value when exiting constructor. Consider declaring the property as nullable.
1>C:\code\xamarin-android-tools\src\Xamarin.Android.Tools.AndroidSdk\JdkInfo.cs(52,10,52,17): warning CS8618: Non-nullable property 'JavacPath' must contain a non-null value when exiting constructor. Consider declaring the property as nullable.
There was a problem hiding this comment.
I wasn't properly reading my warning messages. :-(
Was expecting something on line 24. :-/
That said, they are required!
It's just that they're "required" in a form that the C# compiler doesn't know about.
So maybe instead we should do:
JarPath = RequireExecutableInDirectory (binPath, "jar");
// …
string RequireExecutableInDirectory (string binPath, string filename)
{
var file = ProcessUtils.FindExecutablesInDirectory (binPath, filename).FirstOrDefault ();
if (file == null)
throw new ArgumentException ($"Could not find required file `{filename}` in {HomePath}; …");
return file;
}That should appease the C# compiler, right?
There was a problem hiding this comment.
👍 Makes sense, Fixed it roughly as suggested.
| @@ -158,6 +166,10 @@ public override void SetPreferredAndroidNdkPath (string? path) | |||
| path = NullIfEmpty (path); | |||
|
|
|||
| var doc = GetUnixConfigFile (Logger); | |||
|
|
|||
| if (doc.Root is null) | |||
| throw new InvalidOperationException ("Unix config file root is missing"); | |||
There was a problem hiding this comment.
Should this include the path that couldn't be found? If this exception "leaks" to the user, it's not particularly "actionable".
There was a problem hiding this comment.
What's "doubly hilarious" is that this codepath should be unreachable, i.e. it isn't possible for doc.Root to be null:
GetUnixConfigFile() ensures that doc.Root isn't null!
And you've had to "duplicate" this check in multiple places.
Perhaps the "better" solution is to change the return type of GetUnixConfigFile() to XElement instead of XDocument, and have it be the "root" element?
There was a problem hiding this comment.
and maybe update SaveConfig() to likewise take XElement instead of XDocument?
I'm not sure we need to use XDocument here…
Changes: dotnet/android-tools@8bc0750...3b8c467 * dotnet/android-tools@3b8c467: [Xamarin.Android.Tools.AndroidSdk] Fix NRT warnings (dotnet/android-tools#206) * dotnet/android-tools@7ef8ec5: [ci] Add weekly schedule for OneLocBuild (dotnet/android-tools#207) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fix NRT warnings that are showing up in CI build logs.