[iOS] Fix the mess we had with the logs. (#208)

The initial implementation of the logs, yet useful, it was wrong. It made it very easy to pass Logs that do have certain missing properties and methods to other classes that used those methods, resulting in runtime issues. We move to have different interfaces that will allow to say which is the min amount of methods to be implemented (ILog) or the max one (IFileBackedLog) so that the compiler will help us to not pass a MemoryLog or a ConsoleLog to those methods that will like to use, for example, the FullPath of the log. This might look like a breaking change for xamarin-macios, but whatever it breaks was a hidden bug. Co-authored-by: Přemek Vysoký <[email protected]>
dotnet · May 15, 2020 · 7f79598 · 7f79598
1 parent 27e5dcb
commit 7f79598
Show file tree

Hide file tree

Showing 33 changed files with 143 additions and 148 deletions.
diff --git a/src/Microsoft.DotNet.XHarness.CLI/Commands/iOS/iOSPackageCommand.cs b/src/Microsoft.DotNet.XHarness.CLI/Commands/iOS/iOSPackageCommand.cs
@@ -98,7 +98,7 @@ protected async override Task<ExitCode> InvokeInternal(ILogger logger)
                 Timestamp = false
             };
 
-            var aggregatedLog = Log.CreateAggregatedLogWithDefault(runLog, consoleLog);
+            var aggregatedLog = Log.CreateReadableAggregatedLog(runLog, consoleLog);
             aggregatedLog.WriteLine("Generating scaffold app with:");
             aggregatedLog.WriteLine($"\tAppname: '{_arguments.AppPackageName}'");
             aggregatedLog.WriteLine($"\tAssemblies: '{string.Join(" ", _arguments.Assemblies)}'");

diff --git a/src/Microsoft.DotNet.XHarness.CLI/Commands/iOS/iOSTestCommand.cs b/src/Microsoft.DotNet.XHarness.CLI/Commands/iOS/iOSTestCommand.cs
@@ -33,7 +33,7 @@ internal class iOSTestCommand : TestCommand
         private readonly iOSTestCommandArguments _arguments = new iOSTestCommandArguments();
         private readonly ErrorKnowledgeBase _errorKnowledgeBase = new ErrorKnowledgeBase();
         private static readonly string _mlaunchLldbConfigFile = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Personal), ".mtouch-launch-with-lldb");
-        private bool _createdLldbFile = false;
+        private bool _createdLldbFile;
 
         protected override string CommandUsage { get; } = "ios test [OPTIONS]";
         protected override string CommandDescription { get; } = CommandHelp;
@@ -119,7 +119,8 @@ private async Task<ExitCode> RunTest(
             logger.LogInformation($"Starting test for {target.AsString()}{ (_arguments.DeviceName != null ? " targeting " + _arguments.DeviceName : null) }..");
 
             string mainLogFile = Path.Join(_arguments.OutputDirectory, $"run-{target}{(_arguments.DeviceName != null ? "-" + _arguments.DeviceName : null)}.log");
-            ILog mainLog = Log.CreateAggregatedLogWithDefault(
+
+            IFileBackedLog mainLog = Log.CreateReadableAggregatedLog(
                 logs.Create(mainLogFile, LogType.ExecutionLog.ToString(), true),
                 new CallbackLog(message => logger.LogDebug(message)));
 

diff --git a/src/Microsoft.DotNet.XHarness.Common/Logging/ILog.cs b/src/Microsoft.DotNet.XHarness.Common/Logging/ILog.cs
@@ -11,17 +11,24 @@ namespace Microsoft.DotNet.XHarness.Common.Logging
 
     public interface ILog : IDisposable
     {
-        string Description { get; set; }
+        string? Description { get; set; }
         bool Timestamp { get; set; }
-        string FullPath { get; }
         Encoding Encoding { get; }
-
         void Write(byte[] buffer, int offset, int count);
-        StreamReader GetReader();
         void Write(string value);
         void WriteLine(string value);
         void WriteLine(StringBuilder value);
         void WriteLine(string format, params object[] args);
         void Flush();
     }
+
+    public interface IReadableLog : ILog
+    {
+        StreamReader GetReader();
+    }
+
+    public interface IFileBackedLog : IReadableLog
+    {
+        string FullPath { get; }
+    }
 }
diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/CrashSnapshotReporter.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/CrashSnapshotReporter.cs
@@ -89,10 +89,10 @@ public async Task EndCaptureAsync(TimeSpan timeout)
 
                 log.WriteLine("Found {0} new crash report(s)", newCrashFiles.Count);
 
-                IEnumerable<ILog> crashReports;
+                IEnumerable<IFileBackedLog> crashReports;
                 if (!isDevice)
                 {
-                    crashReports = new List<ILog>(newCrashFiles.Count);
+                    crashReports = new List<IFileBackedLog>(newCrashFiles.Count);
                     foreach (var path in newCrashFiles)
                     {
                         logs.AddFile(path, $"Crash report: {Path.GetFileName(path)}");
@@ -119,7 +119,7 @@ public async Task EndCaptureAsync(TimeSpan timeout)
             } while (true);
         }
 
-        async Task<ILog> ProcessCrash(string crashFile)
+        async Task<IFileBackedLog> ProcessCrash(string crashFile)
         {
             var name = Path.GetFileName(crashFile);
             var crashReportFile = logs.Create(name, $"Crash report: {name}", timestamp: false);
@@ -143,7 +143,7 @@ async Task<ILog> ProcessCrash(string crashFile)
             }
         }
 
-        async Task<ILog> GetSymbolicateCrashReportAsync(ILog report)
+        async Task<IFileBackedLog> GetSymbolicateCrashReportAsync(IFileBackedLog report)
         {
             if (symbolicateCrashPath == null)
             {

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/IErrorKnowledgeBase.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/IErrorKnowledgeBase.cs
@@ -20,22 +20,22 @@ public interface IErrorKnowledgeBase
         /// <param name="installLog">The installation log.</param>
         /// <param name="knownFailureMessage">A string message for the user to understand the reason for the failure.</param>
         /// <returns>True if the failure is due to a known reason, false otherwise.</returns>
-        bool IsKnownInstallIssue(ILog installLog, [NotNullWhen(true)] out string? knownFailureMessage);
+        bool IsKnownInstallIssue(IFileBackedLog installLog, [NotNullWhen(true)] out string? knownFailureMessage);
 
         /// <summary>
         /// Identifies via the logs if the build failure is due to a known issue that the user can act upon.
         /// </summary>
         /// <param name="buildLog">The build log.</param>
         /// <param name="knownFailureMessage">A string message for the user to understand the reason for the failure.</param>
         /// <returns>True if the failure is due to a known reason, false otherwise.</returns>
-        bool IsKnownBuildIssue(ILog buildLog, [NotNullWhen(true)] out string? knownFailureMessage);
+        bool IsKnownBuildIssue(IFileBackedLog buildLog, [NotNullWhen(true)] out string? knownFailureMessage);
 
         /// <summary>
         /// Identifies via the logs if the run failure is due to a known issue that the user can act upon.
         /// </summary>
         /// <param name="runLog">The run log.</param>
         /// <param name="knownFailureMessage">A string message for the user to understand the reason for the failure.</param>
         /// <returns>True if the failure is due to a known reason, false otherwise.</returns>
-        bool IsKnownTestIssue(ILog runLog, [NotNullWhen(true)] out string? knownFailureMessage);
+        bool IsKnownTestIssue(IFileBackedLog runLog, [NotNullWhen(true)] out string? knownFailureMessage);
     }
 }
diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleFileListener.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleFileListener.cs
@@ -6,7 +6,6 @@
 using System.IO;
 using System.Threading;
 using Microsoft.DotNet.XHarness.Common.Logging;
-using Microsoft.DotNet.XHarness.iOS.Shared.Logging;
 
 namespace Microsoft.DotNet.XHarness.iOS.Shared.Listeners
 {
@@ -18,7 +17,7 @@ public class SimpleFileListener : SimpleListener
 
         public string Path { get; private set; }
 
-        public SimpleFileListener(string path, ILog log, ILog testLog, bool xmlOutput) : base(log, testLog)
+        public SimpleFileListener(string path, ILog log, IFileBackedLog testLog, bool xmlOutput) : base(log, testLog)
         {
             Path = path ?? throw new ArgumentNullException(nameof(path));
             _xmlOutput = xmlOutput;

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleHttpListener.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleHttpListener.cs
@@ -17,7 +17,7 @@ public class SimpleHttpListener : SimpleListener
         private HttpListener _server;
         private bool _connected_once;
 
-        public SimpleHttpListener(ILog log, ILog testLog, bool autoExit) : base(log, testLog)
+        public SimpleHttpListener(ILog log, IFileBackedLog testLog, bool autoExit) : base(log, testLog)
         {
             _autoExit = autoExit;
         }

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleListener.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleListener.cs
@@ -16,7 +16,7 @@ public interface ISimpleListener : IDisposable
         Task CompletionTask { get; }
         Task ConnectedTask { get; }
         int Port { get; }
-        ILog TestLog { get; }
+        IFileBackedLog TestLog { get; }
 
         void Cancel();
         void Initialize();
@@ -28,7 +28,7 @@ public abstract class SimpleListener : ISimpleListener
         private readonly TaskCompletionSource<bool> _stopped = new TaskCompletionSource<bool>();
         private readonly TaskCompletionSource<bool> _connected = new TaskCompletionSource<bool>();
 
-        public ILog TestLog { get; private set; }
+        public IFileBackedLog TestLog { get; private set; }
 
         protected readonly IPAddress Address = IPAddress.Any;
         protected ILog Log { get; }
@@ -39,7 +39,7 @@ public abstract class SimpleListener : ISimpleListener
         public int Port { get; protected set; }
         public abstract void Initialize();
 
-        protected SimpleListener(ILog log, ILog testLog)
+        protected SimpleListener(ILog log, IFileBackedLog testLog)
         {
             Log = log ?? throw new ArgumentNullException(nameof(log));
             TestLog = testLog ?? throw new ArgumentNullException(nameof(testLog));

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleListenerFactory.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleListenerFactory.cs
@@ -4,7 +4,6 @@
 
 using System;
 using Microsoft.DotNet.XHarness.Common.Logging;
-using Microsoft.DotNet.XHarness.iOS.Shared.Logging;
 
 namespace Microsoft.DotNet.XHarness.iOS.Shared.Listeners
 {
@@ -20,7 +19,7 @@ public interface ISimpleListenerFactory
         (ListenerTransport transport, ISimpleListener listener, string listenerTempFile) Create(
             RunMode mode,
             ILog log,
-            ILog testLog,
+            IFileBackedLog testLog,
             bool isSimulator,
             bool autoExit,
             bool xmlOutput);
@@ -42,7 +41,7 @@ public SimpleListenerFactory(ITunnelBore tunnelBore = null) =>
         public (ListenerTransport transport, ISimpleListener listener, string listenerTempFile) Create(
             RunMode mode,
             ILog log,
-            ILog testLog,
+            IFileBackedLog testLog,
             bool isSimulator,
             bool autoExit,
             bool xmlOutput)

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleTcpListener.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Listeners/SimpleTcpListener.cs
@@ -27,13 +27,13 @@ public class SimpleTcpListener : SimpleListener, ITunnelListener
 
         public TaskCompletionSource<bool> TunnelHoleThrough { get; } = new TaskCompletionSource<bool>();
 
-        public SimpleTcpListener(ILog log, ILog testLog, bool autoExit, bool tunnel = false) : base(log, testLog)
+        public SimpleTcpListener(ILog log, IFileBackedLog testLog, bool autoExit, bool tunnel = false) : base(log, testLog)
         {
             _autoExit = autoExit;
             _useTcpTunnel = tunnel;
         }
 
-        public SimpleTcpListener(int port, ILog log, ILog testLog, bool autoExit, bool tunnel = false) : this(log, testLog, autoExit, tunnel)
+        public SimpleTcpListener(int port, ILog log, IFileBackedLog testLog, bool autoExit, bool tunnel = false) : this(log, testLog, autoExit, tunnel)
         {
             Port = port;
         }
@@ -119,7 +119,7 @@ private void StartTcpTunnel()
             bool processed;
             try
             {
-                int timeout = TimeOutInit; ;
+                int timeout = TimeOutInit;
                 var watch = new System.Diagnostics.Stopwatch();
                 watch.Start();
                 while (true)

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/AggregatedLog.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/AggregatedLog.cs
@@ -14,12 +14,11 @@ namespace Microsoft.DotNet.XHarness.iOS.Shared.Logging
     public abstract partial class Log
     {
 
-        public static ILog CreateAggregatedLogWithDefault(ILog defaultLog, params ILog[] logs)
+        public static IFileBackedLog CreateReadableAggregatedLog (IFileBackedLog defaultLog, params ILog[] logs)
         {
-            return new AggregatedLog(defaultLog, logs);
+            return new ReadableAggregatedLog(defaultLog, logs);
         }
 
-        [Obsolete ("AggregatedLogs without a default log are dangerous. Use 'CreateAggregatedLogWithDefault' instead.")]
         public static ILog CreateAggregatedLog(params ILog[] logs)
         {
             return new AggregatedLog(logs);
@@ -28,32 +27,13 @@ public static ILog CreateAggregatedLog(params ILog[] logs)
         // Log that will duplicate log output to multiple other logs.
         class AggregatedLog : Log
         {
-            readonly ILog? _defaultLog;
-            readonly List<ILog> _logs = new List<ILog>();
-
-            public AggregatedLog(ILog defaultLog, params ILog[] logs)
-            {
-                _defaultLog = defaultLog ?? throw new ArgumentNullException(nameof(defaultLog));
-                _logs.Add(defaultLog);
-                _logs.AddRange(logs);
-            }
+            protected readonly List<ILog> _logs = new List<ILog>();
 
             public AggregatedLog(params ILog[] logs)
-                : base(null)
             {
                 _logs.AddRange(logs);
             }
 
-            public override string FullPath
-            {
-                get
-                {
-                    if (_defaultLog == null)
-                        throw new InvalidOperationException("Default log not set.");
-                    return _defaultLog.FullPath;
-                }
-            }
-
             protected override void WriteImpl(string value)
             {
                 foreach (var log in _logs)
@@ -66,13 +46,6 @@ public override void Write(byte[] buffer, int offset, int count)
                     log.Write(buffer, offset, count);
             }
 
-            public override StreamReader GetReader()
-            {
-                if (_defaultLog == null)
-                    throw new InvalidOperationException("Default log not set.");
-                return _defaultLog.GetReader();
-            }
-
             public override void Flush()
             {
                 foreach (var log in _logs)
@@ -85,5 +58,21 @@ public override void Dispose()
                     log.Dispose();
             }
         }
+
+        class ReadableAggregatedLog : AggregatedLog, IFileBackedLog
+        {
+            readonly IFileBackedLog _defaultLog;
+
+            public ReadableAggregatedLog(IFileBackedLog defaultLog, params ILog[] logs) : base(logs)
+            {
+                _defaultLog = defaultLog ?? throw new ArgumentNullException(nameof(defaultLog));
+                // make sure that we also write in the default log
+                _logs.Add(defaultLog);
+            }
+
+            public StreamReader GetReader() => _defaultLog.GetReader();
+
+            public string FullPath => _defaultLog.FullPath;
+        }
     }
 }
diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/AppInstallMonitorLog.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/AppInstallMonitorLog.cs
@@ -12,10 +12,10 @@
 namespace Microsoft.DotNet.XHarness.iOS.Shared.Logging
 {
     // Monitor the output from 'mlaunch --installdev' and cancel the installation if there's no output for 1 minute.
-    public class AppInstallMonitorLog : Log
+    public class AppInstallMonitorLog : FileBackedLog
     {
 
-        readonly ILog copy_to;
+        readonly IFileBackedLog copy_to;
         readonly CancellationTokenSource cancellationSource;
 
         public override string FullPath => copy_to.FullPath;
@@ -35,7 +35,7 @@ public class AppInstallMonitorLog : Log
         public long AppTotalBytes;
         public long WatchAppTotalBytes;
 
-        public AppInstallMonitorLog(ILog copy_to)
+        public AppInstallMonitorLog(IFileBackedLog copy_to)
                 : base($"Watch transfer log for {copy_to.Description}")
         {
             this.copy_to = copy_to;

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/CallbackLog.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/CallbackLog.cs
@@ -18,8 +18,6 @@ public CallbackLog(Action<string> onWrite)
             this.onWrite = onWrite;
         }
 
-        public override string FullPath => throw new NotSupportedException();
-
         public override void Dispose()
         {
         }
@@ -28,11 +26,6 @@ public override void Flush()
         {
         }
 
-        public override StreamReader GetReader()
-        {
-            throw new NotSupportedException();
-        }
-
         protected override void WriteImpl(string value)
         {
             onWrite(value);

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/CaptureLog.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/CaptureLog.cs
@@ -25,15 +25,15 @@ public ICaptureLog Create(string logDirectory, string systemLogPath, bool entire
         }
     }
 
-    public interface ICaptureLog : ILog
+    public interface ICaptureLog : IFileBackedLog
     {
         void StartCapture();
         void StopCapture();
     }
 
     // A log that captures data written to a separate file between two moments in time
     // (between StartCapture and StopCapture).
-    public class CaptureLog : Log, ICaptureLog
+    public class CaptureLog : FileBackedLog, ICaptureLog
     {
         readonly bool entireFile;
 
@@ -45,7 +45,6 @@ public class CaptureLog : Log, ICaptureLog
         public override string FullPath { get; }
 
         public CaptureLog(string path, string capture_path, bool entireFile = false)
-            : base()
         {
             FullPath = path ?? throw new ArgumentNullException(nameof(path));
             CapturePath = capture_path ?? throw new ArgumentNullException(nameof(path));

diff --git a/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/ConsoleLog.cs b/src/Microsoft.DotNet.XHarness.iOS.Shared/Logging/ConsoleLog.cs
@@ -9,7 +9,7 @@
 namespace Microsoft.DotNet.XHarness.iOS.Shared.Logging
 {
     // A log that writes to standard output
-    public class ConsoleLog : Log
+    public class ConsoleLog : ReadableLog
     {
         readonly StringBuilder captured = new StringBuilder();
 
@@ -19,8 +19,6 @@ protected override void WriteImpl(string value)
             Console.Write(value);
         }
 
-        public override string FullPath => throw new NotSupportedException();
-
         public override StreamReader GetReader()
         {
             var str = new MemoryStream(Encoding.GetBytes(captured.ToString()));