From 38bc568db0408fbb84954cc222f61483a5573349 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 30 Aug 2017 12:00:31 -0400 Subject: [PATCH] [Xamarin.Android.Build.Tasks] Add `$(AndroidErrorOnCustomJavaObject)` Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=56819 Bumps to Java.Interop/master/ab3c2b26. Nothing *prevents* "anybody" from custom implementing `IJavaObject`: class MyBadClass : Android.Runtime.IJavaObject { public IntPtr Handle { get {return IntPtr.Zero;} } public void Dispose () {} } The problem is that the above doesn't actually work: the `IJavaObject.Handle` value contains the JNI Object Reference to pass into JNI. The above code will thus result in always passing `null` into Java code, which could result in a `NullPointerException`, but will always result in *not* doing what was intended. While it is *theoretically* possible to manually implement `IJavaObject`, it's not something *I* would want to contemplate, nor is it for the faint of heart, so long ago we decided to emit a warning if we encounter a type which: 1. Implements `Android.Runtime.IJavaObject`, and 2. Does *not* also inherit `Java.Lang.Object` or `Java.Lang.Throwable`. As such, the above `MyBadClass` elicits the warning: Type 'MyBadClass' implements Android.Runtime.IJavaObject but does not inherit from Java.Lang.Object. It is not supported. This is all well and good, but (effectively) *nobody* reads warnings, *especially* since our toolchain emits so many warnings that enabling `/warnaserror` is for the truly foolhardy, so *lots* of warnings is, unfortunately, normal. Which brings us to Bug #56819: Could we make this an error? Answer: Of course we can. That said, I have no idea how much existing code this will *break* if we turned it into an error. That might be good and useful, but if there's no way to *disable* the error, existing apps which (appear to) work will no longer build. That's not desirable. Add a new `$(AndroidErrorOnCustomJavaObject)` MSBuild property to control what happens when such custom `IJavaObject` implementations are found. When True, the default, emit a new XA4212 *error*. The error can be turned back into a warning by setting `$(AndroidErrorOnCustomJavaObject)` within the `App.csproj`: False --- Documentation/build_process.md | 21 +++++++++++ external/Java.Interop | 2 +- .../Tasks/AsyncTask.cs | 24 ++++++------ .../Tasks/BuildApk.cs | 2 +- .../Tasks/CheckTargetFrameworks.cs | 2 +- .../Tasks/GenerateJavaStubs.cs | 13 +++++-- .../Tasks/GenerateResourceDesigner.cs | 2 +- .../GetAdditionalResourcesFromAssemblies.cs | 2 +- .../Tasks/LinkAssemblies.cs | 2 +- .../Tasks/ResolveAssemblies.cs | 2 +- .../Tasks/ResolveLibraryProjectImports.cs | 2 +- .../Tasks/StripEmbeddedLibraries.cs | 2 +- .../Xamarin.Android.Build.Tests/BuildTest.cs | 30 +++++++++++++++ .../Utilities/MSBuildExtensions.cs | 37 +++++++++++++++++++ .../Xamarin.Android.Common.targets | 3 ++ 15 files changed, 122 insertions(+), 24 deletions(-) diff --git a/Documentation/build_process.md b/Documentation/build_process.md index f038b9e3e84..6ba90ae3452 100644 --- a/Documentation/build_process.md +++ b/Documentation/build_process.md @@ -217,6 +217,27 @@ when packaing Release applications. This property is `False` by default. +- **AndroidErrorOnCustomJavaObject** – A boolean property that + determines whether types may implement `Android.Runtime.IJavaObject` + *without* also inheriting from `Java.Lang.Object` or `Java.Lang.Throwable`: + + class BadType : IJavaObject { + public IntPtr Handle { + get {return IntPtr.Zero;} + } + + public void Dispose() + { + } + } + + When True, such types will generate an XA4212 error, otherwise a + XA4212 warning will be generated. + + Support for this property was added in Xamarin.Android 7.6. + + This property is `True` by default. + - **AndroidFastDeploymentType** – A `:` (colon)-separated list of values to control what types can be deployed to the [Fast Deployment directory](#Fast_Deployment) on the target device diff --git a/external/Java.Interop b/external/Java.Interop index 33436346553..16be390ab82 160000 --- a/external/Java.Interop +++ b/external/Java.Interop @@ -1 +1 @@ -Subproject commit 33436346553b7ad1b2e3b922f04f1703b9a9ec64 +Subproject commit 16be390ab82a5b1dda34aa2cb3237c9634a37266 diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/AsyncTask.cs b/src/Xamarin.Android.Build.Tasks/Tasks/AsyncTask.cs index e5b3366cae3..2fb5794f71b 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/AsyncTask.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/AsyncTask.cs @@ -78,27 +78,27 @@ public void LogDebugTaskItems (string message, ITaskItem[] items) LogDebugMessage (" {0}", item.ItemSpec); } - protected void LogMessage (string message) + public void LogMessage (string message) { LogMessage (message, importance: MessageImportance.Normal); } - protected void LogMessage (string message, params object[] messageArgs) + public void LogMessage (string message, params object[] messageArgs) { LogMessage (string.Format (message, messageArgs)); } - protected void LogDebugMessage (string message) + public void LogDebugMessage (string message) { LogMessage (message , importance: MessageImportance.Low); } - protected void LogDebugMessage (string message, params object[] messageArgs) + public void LogDebugMessage (string message, params object[] messageArgs) { LogMessage (string.Format (message, messageArgs), importance: MessageImportance.Low); } - protected void LogMessage (string message, MessageImportance importance = MessageImportance.Normal) + public void LogMessage (string message, MessageImportance importance = MessageImportance.Normal) { if (UIThreadId == Thread.CurrentThread.ManagedThreadId) { #pragma warning disable 618 @@ -121,27 +121,27 @@ protected void LogMessage (string message, MessageImportance importance = Messag } } - protected void LogError (string message) + public void LogError (string message) { LogError (code: null, message: message, file: null, lineNumber: 0); } - protected void LogError (string message, params object[] messageArgs) + public void LogError (string message, params object[] messageArgs) { LogError (code: null, message: string.Format (message, messageArgs)); } - protected void LogCodedError (string code, string message) + public void LogCodedError (string code, string message) { LogError (code: code, message: message, file: null, lineNumber: 0); } - protected void LogCodedError (string code, string message, params object[] messageArgs) + public void LogCodedError (string code, string message, params object[] messageArgs) { LogError (code: code, message: string.Format (message, messageArgs), file: null, lineNumber: 0); } - protected void LogError (string code, string message, string file = null, int lineNumber = 0) + public void LogError (string code, string message, string file = null, int lineNumber = 0) { if (UIThreadId == Thread.CurrentThread.ManagedThreadId) { #pragma warning disable 618 @@ -180,12 +180,12 @@ protected void LogError (string code, string message, string file = null, int li } } - protected void LogWarning (string message, params object[] messageArgs) + public void LogWarning (string message, params object[] messageArgs) { LogWarning (string.Format (message, messageArgs)); } - protected void LogWarning (string message) + public void LogWarning (string message) { if (UIThreadId == Thread.CurrentThread.ManagedThreadId) { #pragma warning disable 618 diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/BuildApk.cs b/src/Xamarin.Android.Build.Tasks/Tasks/BuildApk.cs index cc37c749077..490f2b78020 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/BuildApk.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/BuildApk.cs @@ -522,7 +522,7 @@ void AddNativeLibrariesFromAssemblies (ZipArchiveEx apk, string supportedAbis) { int count = 0; var abis = supportedAbis.Split (new char[] { ',', ';' }, StringSplitOptions.RemoveEmptyEntries); - using (var res = new DirectoryAssemblyResolver (Console.WriteLine, loadDebugSymbols: false)) { + using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { foreach (var assembly in EmbeddedNativeLibraryAssemblies) res.Load (assembly.ItemSpec); foreach (var assemblyPath in EmbeddedNativeLibraryAssemblies) { diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/CheckTargetFrameworks.cs b/src/Xamarin.Android.Build.Tasks/Tasks/CheckTargetFrameworks.cs index e542f350718..8bf23fec2b0 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/CheckTargetFrameworks.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/CheckTargetFrameworks.cs @@ -50,7 +50,7 @@ public override bool Execute () Log.LogDebugMessage (" ProjectFile: {0}", ProjectFile); Log.LogDebugTaskItems (" ResolvedUserAssemblies: {0}", ResolvedAssemblies); - using (var res = new DirectoryAssemblyResolver (Log.LogWarning, loadDebugSymbols: false)) { + using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { foreach (var assembly in ResolvedAssemblies) { res.Load (Path.GetFullPath (assembly.ItemSpec)); var apiLevel = ExtractApiLevel (res, assembly); diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs index 7b8d58b9ad1..1c3de9ad422 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Linq; using Microsoft.Build.Framework; @@ -51,6 +52,8 @@ public class GenerateJavaStubs : Task public bool UseSharedRuntime { get; set; } + public bool ErrorOnCustomJavaObject { get; set; } + [Required] public string ResourceDirectory { get; set; } @@ -69,6 +72,7 @@ public override bool Execute () Log.LogDebugMessage (" PackageName: {0}", PackageName); Log.LogDebugMessage (" AndroidSdkDir: {0}", AndroidSdkDir); Log.LogDebugMessage (" AndroidSdkPlatform: {0}", AndroidSdkPlatform); + Log.LogDebugMessage ($" {nameof (ErrorOnCustomJavaObject)}: {ErrorOnCustomJavaObject}"); Log.LogDebugMessage (" OutputDirectory: {0}", OutputDirectory); Log.LogDebugMessage (" MergedAndroidManifestOutput: {0}", MergedAndroidManifestOutput); Log.LogDebugMessage (" UseSharedRuntime: {0}", UseSharedRuntime); @@ -83,7 +87,7 @@ public override bool Execute () try { // We're going to do 3 steps here instead of separate tasks so // we can share the list of JLO TypeDefinitions between them - using (var res = new DirectoryAssemblyResolver (Log.LogWarning, loadDebugSymbols: true)) { + using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: true)) { Run (res); } } @@ -117,9 +121,12 @@ void Run (DirectoryAssemblyResolver res) var fxAdditions = MonoAndroidHelper.GetFrameworkAssembliesToTreatAsUserAssemblies (ResolvedAssemblies) .Where (a => assemblies.All (x => Path.GetFileName (x) != Path.GetFileName (a))); assemblies = assemblies.Concat (fxAdditions).ToList (); - + // Step 1 - Find all the JLO types - var all_java_types = JavaTypeScanner.GetJavaTypes (assemblies, res, Log.LogWarning); + var scanner = new JavaTypeScanner (this.CreateTaskLogger ()) { + ErrorOnCustomJavaObject = ErrorOnCustomJavaObject, + }; + var all_java_types = scanner.GetJavaTypes (assemblies, res); WriteTypeMappings (all_java_types); diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs index 5662dccee59..367d846394f 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs @@ -106,7 +106,7 @@ public override bool Execute () var assemblyNames = new List (); if (IsApplication && References != null && References.Any ()) { // FIXME: should this be unified to some better code with ResolveLibraryProjectImports? - using (var resolver = new DirectoryAssemblyResolver (Log.LogWarning, loadDebugSymbols: false)) { + using (var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { foreach (var assemblyName in References) { var suffix = assemblyName.ItemSpec.EndsWith (".dll") ? String.Empty : ".dll"; string hintPath = assemblyName.GetMetadata ("HintPath").Replace (Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GetAdditionalResourcesFromAssemblies.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GetAdditionalResourcesFromAssemblies.cs index 87122055daf..8968c489de2 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/GetAdditionalResourcesFromAssemblies.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/GetAdditionalResourcesFromAssemblies.cs @@ -394,7 +394,7 @@ public override bool Execute () ? CachePath : Path.Combine (Environment.GetFolderPath (Environment.SpecialFolder.LocalApplicationData), CacheBaseDir); - using (var resolver = new DirectoryAssemblyResolver (LogWarning, loadDebugSymbols: false)) { + using (var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { foreach (var assemblyItem in Assemblies) { string fullPath = Path.GetFullPath (assemblyItem.ItemSpec); if (assemblies.Contains (fullPath)) { diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/LinkAssemblies.cs b/src/Xamarin.Android.Build.Tasks/Tasks/LinkAssemblies.cs index ad1ea5ed82f..5756283d14b 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/LinkAssemblies.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/LinkAssemblies.cs @@ -85,7 +85,7 @@ public override bool Execute () var rp = new ReaderParameters { InMemory = true, }; - using (var res = new DirectoryAssemblyResolver (Log.LogWarning, loadDebugSymbols: false, loadReaderParameters: rp)) { + using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false, loadReaderParameters: rp)) { return Execute (res); } } diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/ResolveAssemblies.cs b/src/Xamarin.Android.Build.Tasks/Tasks/ResolveAssemblies.cs index 4f60737711e..cdb2ae84a7c 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/ResolveAssemblies.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/ResolveAssemblies.cs @@ -45,7 +45,7 @@ public class ResolveAssemblies : Task public override bool Execute () { - using (var resolver = new DirectoryAssemblyResolver (Log.LogWarning, loadDebugSymbols: false)) { + using (var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { return Execute (resolver); } } diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs b/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs index dc718e803ae..7cb3ba6f13c 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs @@ -78,7 +78,7 @@ public override bool Execute () assemblyMap.Load (AssemblyIdentityMapFile); - using (var resolver = new DirectoryAssemblyResolver (Log.LogWarning, loadDebugSymbols: false)) { + using (var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: false)) { Extract (resolver, jars, resolvedResourceDirectories, resolvedAssetDirectories, resolvedEnvironmentFiles); } diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/StripEmbeddedLibraries.cs b/src/Xamarin.Android.Build.Tasks/Tasks/StripEmbeddedLibraries.cs index 819dbc265d2..b1e412b04ae 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/StripEmbeddedLibraries.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/StripEmbeddedLibraries.cs @@ -28,7 +28,7 @@ public override bool Execute () Log.LogDebugMessage ("StripEmbeddedLibraries Task"); Log.LogDebugTaskItems (" Assemblies: ", Assemblies); - using (var res = new DirectoryAssemblyResolver (Log.LogWarning, true, new ReaderParameters { ReadWrite = true } )) { + using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), true, new ReaderParameters { ReadWrite = true } )) { return Execute (res); } } diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs index b28dd02c27c..7273c967718 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs @@ -1993,6 +1993,36 @@ public static void Foo () { Assert.IsTrue(sb.BuildProject(app1, "SignAndroidPackage"), "Build of project should have succeeded"); sb.Dispose(); } + + [Test] + public void XA4212 () + { + var proj = new XamarinAndroidApplicationProject () { + }; + proj.Sources.Add (new BuildItem ("Compile", "MyBadJavaObject.cs") { TextContent = () => @" +using System; +using Android.Runtime; +namespace UnnamedProject { + public class MyBadJavaObject : IJavaObject + { + public IntPtr Handle { + get {return IntPtr.Zero;} + } + + public void Dispose () + { + } + } +}" }); + using (var builder = CreateApkBuilder (Path.Combine ("temp", TestContext.CurrentContext.Test.Name))) { + builder.ThrowOnBuildFailure = false; + Assert.IsFalse (builder.Build (proj), "Build should have failed with XA4212."); + StringAssert.Contains ($"error XA4", builder.LastBuildOutput, "Error should be XA4212"); + StringAssert.Contains ($"Type `UnnamedProject.MyBadJavaObject` implements `Android.Runtime.IJavaObject`", builder.LastBuildOutput, "Error should mention MyBadJavaObject"); + Assert.IsTrue (builder.Build (proj, parameters: new [] { "AndroidErrorOnCustomJavaObject=False" }), "Build should have succeeded."); + StringAssert.Contains ($"warning XA4", builder.LastBuildOutput, "warning XA4212"); + } + } } } diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/MSBuildExtensions.cs b/src/Xamarin.Android.Build.Tasks/Utilities/MSBuildExtensions.cs index 336e2c12f1b..b9cb78322a6 100644 --- a/src/Xamarin.Android.Build.Tasks/Utilities/MSBuildExtensions.cs +++ b/src/Xamarin.Android.Build.Tasks/Utilities/MSBuildExtensions.cs @@ -131,6 +131,43 @@ public static void LogCodedWarning (this TaskLoggingHelper log, string code, str messageArgs: messageArgs); } + public static Action CreateTaskLogger (this Task task) + { + Action logger = (level, value) => { + switch (level) { + case TraceLevel.Error: + task.Log.LogError ("{0}", value); + break; + case TraceLevel.Warning: + task.Log.LogWarning ("{0}", value); + break; + default: + task.Log.LogDebugMessage ("{0}", value); + break; + } + }; + return logger; + } + + public static Action CreateTaskLogger (this AsyncTask task) + { + Action logger = (level, value) => { + switch (level) { + case TraceLevel.Error: + task.LogError (value); + break; + case TraceLevel.Warning: + task.LogWarning (value); + break; + default: + task.LogDebugMessage (value); + break; + } + }; + return logger; + } + + public static IEnumerable Concat (params ITaskItem[][] values) { if (values == null) diff --git a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets index 9df34b7a9a6..a8d5f226ef5 100755 --- a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets +++ b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets @@ -182,6 +182,8 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved. False False + True + Normal True @@ -1733,6 +1735,7 @@ because xbuild doesn't support framework reference assemblies.