Skip to content

Commit 8220dc7

Browse files
authored
[ToolsCommon] Refactor Utils.cs (#5622)
With more tests validating output of diagnostic tools, errors from CommandUtils helpers should be validated as well. In facilitating error checking, `src/Tools/Common/Commands/Utils.cs` has been refactored to break unnecessary dependencies (e.g. using `CommandUtils.FindProcessIdWithName` requires adding `IConsole.cs` to project Compile items because `LineRewriter` depends on `IConsole`, regardless of whether LineRewriter is used) This PR does the following: - Switch CounterMonitor back to using IConsole for output instead of TextWriters (see commit message for more details #5217 had switched from IConsole to TextWriters) - Rename CommandLineErrorException -> DiagnosticToolException to suggest a more generic invalid usage of diagnostic tools, rather specifically a command line exception. - Update `CommandUtils` helpers to throw DiagnosticToolException to allow callers to decide how to handle the message. - Extend DiagnosticToolException (formerly CommandLineErrorException) to specify ReturnCode - Break ReturnCode + LineRewriter out of Utils.cs to have discrete dependency chains for Project compile items. - Rename Utils.cs -> CommandUtils.cs - Keep `src/Tools/Common/Commands` command-only
1 parent 39435e1 commit 8220dc7

28 files changed

+265
-296
lines changed

src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ public static IpcEndpointConfig Parse(string config)
133133
string[] parts = config.Split(',');
134134
if (parts.Length > 2)
135135
{
136-
throw new FormatException($"Unknow IPC endpoint config format, {config}.");
136+
throw new FormatException($"Unknown IPC endpoint config format, {config}.");
137137
}
138138

139139
if (string.IsNullOrEmpty(parts[0]))
@@ -156,7 +156,7 @@ public static IpcEndpointConfig Parse(string config)
156156
}
157157
else
158158
{
159-
throw new FormatException($"Unknow IPC endpoint config keyword, {parts[1]} in {config}.");
159+
throw new FormatException($"Unknown IPC endpoint config keyword, {parts[1]} in {config}.");
160160
}
161161
}
162162
}

src/Tools/Common/Commands/Utils.cs renamed to src/Tools/Common/CommandUtils.cs

Lines changed: 22 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using Microsoft.Diagnostics.NETCore.Client;
8-
using Microsoft.Diagnostics.Tools.Common;
8+
using Microsoft.Diagnostics.Tools;
99

1010
namespace Microsoft.Internal.Common.Utils
1111
{
@@ -27,15 +27,14 @@ public static int FindProcessIdWithName(string name)
2727
{
2828
if (commonId != -1)
2929
{
30-
Console.WriteLine("There are more than one active processes with the given name: {0}", name);
31-
return -1;
30+
throw new DiagnosticToolException($"There are more than one active processes with the given name: {name}");
3231
}
3332
commonId = processesWithMatchingName[i].Id;
3433
}
3534
}
3635
if (commonId == -1)
3736
{
38-
Console.WriteLine("There is no active process with the given name: {0}", name);
37+
throw new DiagnosticToolException($"There is no active process with the given name: {name}");
3938
}
4039
return commonId;
4140
}
@@ -61,45 +60,39 @@ public static int LaunchDSRouterProcess(string dsrouterCommand)
6160
/// <param name="name">name</param>
6261
/// <param name="port">port</param>
6362
/// <returns></returns>
64-
public static bool ValidateArgumentsForChildProcess(int processId, string name, string port)
63+
public static void ValidateArgumentsForChildProcess(int processId, string name, string port)
6564
{
6665
if (processId != 0 || name != null || !string.IsNullOrEmpty(port))
6766
{
68-
Console.WriteLine("None of the --name, --process-id, or --diagnostic-port options may be specified when launching a child process.");
69-
return false;
67+
throw new DiagnosticToolException("None of the --name, --process-id, or --diagnostic-port options may be specified when launching a child process.");
7068
}
71-
72-
return true;
7369
}
7470

7571
/// <summary>
7672
/// A helper method for validating --process-id, --name options for collect commands and resolving the process ID and name.
7773
/// Only one of these options can be specified, so it checks for duplicate options specified and if there is
78-
/// such duplication, it prints the appropriate error message.
74+
/// such duplication, it throws the appropriate DiagnosticToolException error message.
7975
/// </summary>
8076
/// <param name="processId">process ID</param>
8177
/// <param name="name">name</param>
8278
/// <param name="resolvedProcessId">resolvedProcessId</param>
8379
/// <param name="resolvedProcessName">resolvedProcessName</param>
8480
/// <returns></returns>
85-
public static bool ResolveProcess(int processId, string name, out int resolvedProcessId, out string resolvedProcessName)
81+
public static void ResolveProcess(int processId, string name, out int resolvedProcessId, out string resolvedProcessName)
8682
{
8783
resolvedProcessId = -1;
8884
resolvedProcessName = name;
8985
if (processId == 0 && string.IsNullOrEmpty(name))
9086
{
91-
Console.Error.WriteLine("Must specify either --process-id or --name.");
92-
return false;
87+
throw new DiagnosticToolException("Must specify either --process-id or --name.");
9388
}
9489
else if (processId < 0)
9590
{
96-
Console.Error.WriteLine($"{processId} is not a valid process ID");
97-
return false;
91+
throw new DiagnosticToolException($"{processId} is not a valid process ID");
9892
}
9993
else if ((processId != 0) && !string.IsNullOrEmpty(name))
10094
{
101-
Console.Error.WriteLine("Only one of the --name or --process-id options may be specified.");
102-
return false;
95+
throw new DiagnosticToolException("Only one of the --name or --process-id options may be specified.");
10396
}
10497
try
10598
{
@@ -116,159 +109,70 @@ public static bool ResolveProcess(int processId, string name, out int resolvedPr
116109
}
117110
catch (ArgumentException)
118111
{
119-
Console.Error.WriteLine($"No process with ID {processId} is currently running.");
120-
return false;
112+
throw new DiagnosticToolException($"No process with ID {processId} is currently running.");
121113
}
122-
123-
return resolvedProcessId != -1;
124114
}
125115

126116
/// <summary>
127117
/// A helper method for validating --process-id, --name, --diagnostic-port, --dsrouter options for collect commands and resolving the process ID.
128118
/// Only one of these options can be specified, so it checks for duplicate options specified and if there is
129-
/// such duplication, it prints the appropriate error message.
119+
/// such duplication, it throws the appropriate DiagnosticToolException error message.
130120
/// </summary>
131121
/// <param name="processId">process ID</param>
132122
/// <param name="name">name</param>
133123
/// <param name="port">port</param>
134124
/// <param name="dsrouter">dsrouter</param>
135125
/// <param name="resolvedProcessId">resolvedProcessId</param>
136126
/// <returns></returns>
137-
public static bool ResolveProcessForAttach(int processId, string name, string port, string dsrouter, out int resolvedProcessId)
127+
public static void ResolveProcessForAttach(int processId, string name, string port, string dsrouter, out int resolvedProcessId)
138128
{
139129
resolvedProcessId = -1;
140130
if (processId == 0 && string.IsNullOrEmpty(name) && string.IsNullOrEmpty(port) && string.IsNullOrEmpty(dsrouter))
141131
{
142-
Console.WriteLine("Must specify either --process-id, --name, --diagnostic-port, or --dsrouter.");
143-
return false;
132+
throw new DiagnosticToolException("Must specify either --process-id, --name, --diagnostic-port, or --dsrouter.");
144133
}
145134
else if (processId < 0)
146135
{
147-
Console.WriteLine($"{processId} is not a valid process ID");
148-
return false;
136+
throw new DiagnosticToolException($"{processId} is not a valid process ID");
149137
}
150138
else if ((processId != 0 ? 1 : 0) +
151139
(!string.IsNullOrEmpty(name) ? 1 : 0) +
152140
(!string.IsNullOrEmpty(port) ? 1 : 0) +
153141
(!string.IsNullOrEmpty(dsrouter) ? 1 : 0)
154142
!= 1)
155143
{
156-
Console.WriteLine("Only one of the --name, --process-id, --diagnostic-port, or --dsrouter options may be specified.");
157-
return false;
144+
throw new DiagnosticToolException("Only one of the --name, --process-id, --diagnostic-port, or --dsrouter options may be specified.");
158145
}
159146
// If we got this far it means only one of --name/--diagnostic-port/--process-id/--dsrouter was specified
160147
else if (!string.IsNullOrEmpty(port))
161148
{
162-
return true;
149+
return;
163150
}
164151
// Resolve name option
165152
else if (!string.IsNullOrEmpty(name))
166153
{
167-
if ((processId = FindProcessIdWithName(name)) < 0)
168-
{
169-
return false;
170-
}
154+
processId = FindProcessIdWithName(name);
171155
}
172156
else if (!string.IsNullOrEmpty(dsrouter))
173157
{
174158
if (dsrouter != "ios" && dsrouter != "android" && dsrouter != "ios-sim" && dsrouter != "android-emu")
175159
{
176-
Console.WriteLine("Invalid value for --dsrouter. Valid values are 'ios', 'ios-sim', 'android' and 'android-emu'.");
177-
return false;
160+
throw new DiagnosticToolException("Invalid value for --dsrouter. Valid values are 'ios', 'ios-sim', 'android' and 'android-emu'.");
178161
}
179162
if ((processId = LaunchDSRouterProcess(dsrouter)) < 0)
180163
{
181164
if (processId == -2)
182165
{
183-
Console.WriteLine($"Failed to launch dsrouter: {dsrouter}. Make sure that dotnet-dsrouter is not already running. You can connect to an already running dsrouter with -p.");
166+
throw new DiagnosticToolException($"Failed to launch dsrouter: {dsrouter}. Make sure that dotnet-dsrouter is not already running. You can connect to an already running dsrouter with -p.", ReturnCode.TracingError);
184167
}
185168
else
186169
{
187-
Console.WriteLine($"Failed to launch dsrouter: {dsrouter}. Please make sure that dotnet-dsrouter is installed and available in the same directory as dotnet-trace.");
188-
Console.WriteLine("You can install dotnet-dsrouter by running 'dotnet tool install --global dotnet-dsrouter'. More info at https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-dsrouter");
170+
throw new DiagnosticToolException($"Failed to launch dsrouter: {dsrouter}. Please make sure that dotnet-dsrouter is installed and available in the same directory as dotnet-trace.\n" +
171+
"You can install dotnet-dsrouter by running 'dotnet tool install --global dotnet-dsrouter'. More info at https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-dsrouter", ReturnCode.TracingError);
189172
}
190-
return false;
191173
}
192174
}
193175
resolvedProcessId = processId;
194-
return true;
195176
}
196177
}
197-
198-
internal sealed class LineRewriter
199-
{
200-
public int LineToClear { get; set; }
201-
202-
private IConsole Console { get; }
203-
204-
public LineRewriter(IConsole console)
205-
{
206-
Console = console;
207-
}
208-
209-
// ANSI escape codes:
210-
// [2K => clear current line
211-
// [{LineToClear};0H => move cursor to column 0 of row `LineToClear`
212-
public void RewriteConsoleLine()
213-
{
214-
bool useConsoleFallback = true;
215-
if (!Console.IsInputRedirected)
216-
{
217-
// in case of console input redirection, the control ANSI codes would appear
218-
219-
// first attempt ANSI Codes
220-
int before = Console.CursorTop;
221-
Console.Out.Write($"\u001b[2K\u001b[{LineToClear};0H");
222-
int after = Console.CursorTop;
223-
224-
// Some consoles claim to be VT100 compliant, but don't respect
225-
// all of the ANSI codes, so fallback to the System.Console impl in that case
226-
useConsoleFallback = (before == after);
227-
}
228-
229-
if (useConsoleFallback)
230-
{
231-
SystemConsoleLineRewriter();
232-
}
233-
}
234-
235-
private void SystemConsoleLineRewriter() => Console.SetCursorPosition(0, LineToClear);
236-
237-
private static bool? _isSetCursorPositionSupported;
238-
public bool IsRewriteConsoleLineSupported
239-
{
240-
get
241-
{
242-
bool isSupported = _isSetCursorPositionSupported ?? EnsureInitialized();
243-
return isSupported;
244-
245-
bool EnsureInitialized()
246-
{
247-
try
248-
{
249-
int left = Console.CursorLeft;
250-
int top = Console.CursorTop;
251-
Console.SetCursorPosition(0, LineToClear);
252-
Console.SetCursorPosition(left, top);
253-
_isSetCursorPositionSupported = true;
254-
}
255-
catch
256-
{
257-
_isSetCursorPositionSupported = false;
258-
}
259-
return (bool)_isSetCursorPositionSupported;
260-
}
261-
}
262-
}
263-
}
264-
265-
internal enum ReturnCode
266-
{
267-
Ok,
268-
SessionCreationError,
269-
TracingError,
270-
ArgumentError,
271-
PlatformNotSupportedError,
272-
UnknownError
273-
}
274178
}

src/Tools/Common/DefaultConsole.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ namespace Microsoft.Diagnostics.Tools.Common
1515
internal class DefaultConsole : IConsole
1616
{
1717
private readonly bool _useAnsi;
18-
public DefaultConsole(bool useAnsi)
18+
public DefaultConsole(bool useAnsi = false)
1919
{
2020
_useAnsi = useAnsi;
2121
}

src/Tools/Common/CommandLineErrorException.cs renamed to src/Tools/Common/DiagnosticToolException.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System;
5+
using Microsoft.Internal.Common.Utils;
56

67
namespace Microsoft.Diagnostics.Tools
78
{
@@ -16,8 +17,12 @@ namespace Microsoft.Diagnostics.Tools
1617
//
1718
// For any other error conditions that were unanticipated or do not have
1819
// contextualized error messages, don't use this type.
19-
internal sealed class CommandLineErrorException : Exception
20+
internal sealed class DiagnosticToolException : Exception
2021
{
21-
public CommandLineErrorException(string errorMessage) : base(errorMessage) { }
22+
public ReturnCode ReturnCode { get; }
23+
public DiagnosticToolException(string errorMessage, ReturnCode returnCode = ReturnCode.ArgumentError ) : base(errorMessage)
24+
{
25+
ReturnCode = returnCode;
26+
}
2227
}
2328
}

src/Tools/Common/LineRewriter.cs

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using Microsoft.Diagnostics.Tools.Common;
5+
6+
namespace Microsoft.Internal.Common.Utils
7+
{
8+
internal sealed class LineRewriter
9+
{
10+
public int LineToClear { get; set; }
11+
12+
private IConsole Console { get; }
13+
14+
public LineRewriter(IConsole console)
15+
{
16+
Console = console;
17+
}
18+
19+
// ANSI escape codes:
20+
// [2K => clear current line
21+
// [{LineToClear};0H => move cursor to column 0 of row `LineToClear`
22+
public void RewriteConsoleLine()
23+
{
24+
bool useConsoleFallback = true;
25+
if (!Console.IsInputRedirected)
26+
{
27+
// in case of console input redirection, the control ANSI codes would appear
28+
29+
// first attempt ANSI Codes
30+
int before = Console.CursorTop;
31+
Console.Out.Write($"\u001b[2K\u001b[{LineToClear};0H");
32+
int after = Console.CursorTop;
33+
34+
// Some consoles claim to be VT100 compliant, but don't respect
35+
// all of the ANSI codes, so fallback to the System.Console impl in that case
36+
useConsoleFallback = (before == after);
37+
}
38+
39+
if (useConsoleFallback)
40+
{
41+
SystemConsoleLineRewriter();
42+
}
43+
}
44+
45+
private void SystemConsoleLineRewriter() => Console.SetCursorPosition(0, LineToClear);
46+
47+
private static bool? _isSetCursorPositionSupported;
48+
public bool IsRewriteConsoleLineSupported
49+
{
50+
get
51+
{
52+
bool isSupported = _isSetCursorPositionSupported ?? EnsureInitialized();
53+
return isSupported;
54+
55+
bool EnsureInitialized()
56+
{
57+
try
58+
{
59+
int left = Console.CursorLeft;
60+
int top = Console.CursorTop;
61+
Console.SetCursorPosition(0, LineToClear);
62+
Console.SetCursorPosition(left, top);
63+
_isSetCursorPositionSupported = true;
64+
}
65+
catch
66+
{
67+
_isSetCursorPositionSupported = false;
68+
}
69+
return (bool)_isSetCursorPositionSupported;
70+
}
71+
}
72+
}
73+
}
74+
}

src/Tools/Common/ReturnCode.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
namespace Microsoft.Internal.Common.Utils
5+
{
6+
internal enum ReturnCode
7+
{
8+
Ok,
9+
SessionCreationError,
10+
TracingError,
11+
ArgumentError,
12+
PlatformNotSupportedError,
13+
UnknownError
14+
}
15+
}

0 commit comments

Comments
 (0)