-
Notifications
You must be signed in to change notification settings - Fork 27
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.
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.