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

Jump to bottom

[Xamarin.Android.Tools.AndroidSdk] Fix NRT warnings. #206

Merged
merged 3 commits into from
May 12, 2023
Merged

[Xamarin.Android.Tools.AndroidSdk] Fix NRT warnings. #206

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6d2c9d
[Xamarin.Android.Tools.AndroidSdk] Fix NRT warnings.
jpobst May 9, 2023
cff4ee6
Address review suggestion.
jpobst May 11, 2023
5bec215
Review fixes.
jpobst May 11, 2023
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
22 changes: 13 additions & 9 deletions src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,20 @@ public class AndroidAppManifest
throw new ArgumentNullException (nameof (doc));
this.versions = versions;
this.doc = doc;
manifest = doc.Root;
if (manifest.Name != "manifest")

if (doc.Root is null || doc.Root.Name != "manifest")
throw new ArgumentException ("App manifest does not have 'manifest' root element", nameof (doc));

application = manifest.Element ("application");
if (application == null)
manifest = doc.Root;

if (manifest.Element ("application") is XElement app)
application = app;
else
manifest.Add (application = new XElement ("application"));

usesSdk = manifest.Element ("uses-sdk");
if (usesSdk == null)
if (manifest.Element ("uses-sdk") is XElement uses)
usesSdk = uses;
else
manifest.Add (usesSdk = new XElement ("uses-sdk"));
}

Expand Down Expand Up @@ -185,7 +189,7 @@ public int? TargetSdkVersion {
set { usesSdk.SetAttributeValue (aNS + "targetSdkVersion", value == null ? null : value.ToString ()); }
}

int? ParseSdkVersion (XAttribute attribute)
int? ParseSdkVersion (XAttribute? attribute)
{
var version = (string?) attribute;
if (version == null || string.IsNullOrEmpty (version))
Expand Down Expand Up @@ -247,9 +251,9 @@ void AddAndroidPermissions (IEnumerable<string> permissions)
lastPerm = el;
}
} else {
var parentNode = (XNode) manifest.Element ("application") ?? manifest.LastNode;
var parentNode = (XNode?) manifest.Element ("application") ?? manifest.LastNode;
foreach (var el in newElements)
parentNode.AddBeforeSelf (el);
parentNode!.AddBeforeSelf (el);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public static void DetectAndSetPreferredJavaSdkPathToLatest (Action<TraceLevel,
sdk.SetPreferredJavaSdkPath (latestJdk.HomePath);
}

public string TryGetCommandLineToolsPath ()
public string? TryGetCommandLineToolsPath ()
{
return GetCommandLineToolsPaths ("latest").FirstOrDefault ();
}
Expand Down
10 changes: 5 additions & 5 deletions src/Xamarin.Android.Tools.AndroidSdk/AndroidVersion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ public static AndroidVersion Load (string uri)
// </AndroidApiInfo>
static AndroidVersion Load (XDocument doc)
{
var id = (string) doc.Root.Element ("Id");
var level = (int) doc.Root.Element ("Level");
var name = (string) doc.Root.Element ("Name");
var version = (string) doc.Root.Element ("Version");
var stable = (bool) doc.Root.Element ("Stable");
var id = (string?) doc.Root?.Element ("Id") ?? throw new InvalidOperationException ("Missing Id element");
var level = (int?) doc.Root?.Element ("Level") ?? throw new InvalidOperationException ("Missing Level element");
var name = (string?) doc.Root?.Element ("Name") ?? throw new InvalidOperationException ("Missing Name element");
var version = (string?) doc.Root?.Element ("Version") ?? throw new InvalidOperationException ("Missing Version element");
var stable = (bool?) doc.Root?.Element ("Stable") ?? throw new InvalidOperationException ("Missing Stable element");

return new AndroidVersion (level, version.TrimStart ('v'), name, id, stable);
}
Expand Down
30 changes: 18 additions & 12 deletions src/Xamarin.Android.Tools.AndroidSdk/JdkInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,15 @@ public JdkInfo (string homePath, string? locator = null, Action<TraceLevel, stri
this.logger = logger ?? AndroidSdkInfo.DefaultConsoleLogger;

var binPath = Path.Combine (HomePath, "bin");
JarPath = ProcessUtils.FindExecutablesInDirectory (binPath, "jar").FirstOrDefault ();
JavaPath = ProcessUtils.FindExecutablesInDirectory (binPath, "java").FirstOrDefault ();
JavacPath = ProcessUtils.FindExecutablesInDirectory (binPath, "javac").FirstOrDefault ();
JarPath = RequireExecutableInDirectory (binPath, "jar");
JavaPath = RequireExecutableInDirectory (binPath, "java");
JavacPath = RequireExecutableInDirectory (binPath, "javac");

string? jdkJvmPath = GetJdkJvmPath ();

ValidateFile ("jar", JarPath);
ValidateFile ("java", JavaPath);
ValidateFile ("javac", JavacPath);
ValidateFile ("jvm", jdkJvmPath);

JdkJvmPath = jdkJvmPath!;
JdkJvmPath = jdkJvmPath;

var includes = new List<string> ();
var jdkInclude = Path.Combine (HomePath, "include");
Expand Down Expand Up @@ -153,12 +150,21 @@ static IEnumerable<string> FindLibrariesInDirectory (string dir, string libraryN
return Directory.EnumerateFiles (dir, library, SearchOption.AllDirectories);
}

void ValidateFile (string name, string? path)
void ValidateFile (string name, [NotNull]string? path)
{
if (path == null || !File.Exists (path))
throw new ArgumentException ($"Could not find required file `{name}` within `{HomePath}`; is this a valid JDK?", "homePath");
}

string RequireExecutableInDirectory (string binPath, string fileName)
{
var file = ProcessUtils.FindExecutablesInDirectory (binPath, fileName).FirstOrDefault ();

ValidateFile (fileName, file);

return file;
}

static Regex NonDigitMatcher = new Regex (@"[^\d]", RegexOptions.Compiled | RegexOptions.CultureInvariant);

Version? GetJavaVersion ()
Expand Down Expand Up @@ -400,12 +406,12 @@ static IEnumerable<string> GetLibexecJdkPaths (Action<TraceLevel, string> logger
yield break;
}
foreach (var info in plist.Elements ("array").Elements ("dict")) {
var JVMHomePath = (XNode) info.Elements ("key").FirstOrDefault (e => e.Value == "JVMHomePath");
var JVMHomePath = (XNode?) info.Elements ("key").FirstOrDefault (e => e.Value == "JVMHomePath");
if (JVMHomePath == null)
continue;
while (JVMHomePath.NextNode.NodeType != XmlNodeType.Element)
JVMHomePath = JVMHomePath.NextNode;
var strElement = (XElement) JVMHomePath.NextNode;
while (JVMHomePath.NextNode!.NodeType != XmlNodeType.Element)
JVMHomePath = JVMHomePath.NextNode!;
var strElement = (XElement) JVMHomePath.NextNode!;
var path = strElement.Value;
yield return path;
}
Expand Down
4 changes: 4 additions & 0 deletions src/Xamarin.Android.Tools.AndroidSdk/Jdks/JdkLocations.MacOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ static IEnumerable<JdkInfo> GetUnixPreferredJdks (Action<TraceLevel, string> log
static IEnumerable<string> GetUnixConfiguredJdkPaths (Action<TraceLevel, string> logger)
{
var config = AndroidSdkUnix.GetUnixConfigFile (logger);

if (config.Root is null)
throw new InvalidOperationException ("Unix config file root is missing");

foreach (var java_sdk in config.Root.Elements ("java-sdk")) {
var path = (string?) java_sdk.Attribute ("path");
if (path != null && !string.IsNullOrEmpty (path)) {
Expand Down
18 changes: 15 additions & 3 deletions src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public override string NdkHostPlatform64Bit {
public override string? PreferedAndroidSdkPath {
get {
var config_file = GetUnixConfigFile (Logger);
var androidEl = config_file.Root.Element ("android-sdk");
var androidEl = config_file.Root?.Element ("android-sdk");

if (androidEl != null) {
var path = (string?)androidEl.Attribute ("path");
Expand All @@ -61,7 +61,7 @@ public override string? PreferedAndroidSdkPath {
public override string? PreferedAndroidNdkPath {
get {
var config_file = GetUnixConfigFile (Logger);
var androidEl = config_file.Root.Element ("android-ndk");
var androidEl = config_file.Root?.Element ("android-ndk");

if (androidEl != null) {
var path = (string?)androidEl.Attribute ("path");
Expand All @@ -76,7 +76,7 @@ public override string? PreferedAndroidNdkPath {
public override string? PreferedJavaSdkPath {
get {
var config_file = GetUnixConfigFile (Logger);
var javaEl = config_file.Root.Element ("java-sdk");
var javaEl = config_file.Root?.Element ("java-sdk");

if (javaEl != null) {
var path = (string?)javaEl.Attribute ("path");
Expand Down Expand Up @@ -126,6 +126,10 @@ public override void SetPreferredAndroidSdkPath (string? path)
path = NullIfEmpty (path);

var doc = GetUnixConfigFile (Logger);

if (doc.Root is null)
throw new InvalidOperationException ("Unix config file root is missing");

var androidEl = doc.Root.Element ("android-sdk");

if (androidEl == null) {
Expand All @@ -142,6 +146,10 @@ public override void SetPreferredJavaSdkPath (string? path)
path = NullIfEmpty (path);

var doc = GetUnixConfigFile (Logger);

if (doc.Root is null)
throw new InvalidOperationException ("Unix config file root is missing");

var javaEl = doc.Root.Element ("java-sdk");

if (javaEl == null) {
Expand All @@ -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");
Copy link
Member

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".

Copy link
Member

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:

https://github.com/xamarin/xamarin-android-tools/blob/7ef8ec51f4ca2ddd2f651b87b487c367ac833ceb/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs#L254-L256

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?

Copy link
Member

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…


var androidEl = doc.Root.Element ("android-ndk");

if (androidEl == null) {
Expand Down