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.

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

Add support for improved TaskParameterEventArgs #780

Merged
merged 2 commits into from
May 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/StructuredLogger/BinaryLog.cs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ public static Build ReadBuild(

if (errorByType.Any(i => i != 0))
{
string summary = string.Join(", ", errorByType.Where((count, index) => count > 0).Select((count, index) => $"{((ReaderErrorType)index)}: {count}"));
string summary = string.Join(", ", errorByType.Where((count, index) => count > 0).Select((count, index) => $"{((ReaderErrorType)index)}: {count} cases"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding the existing message confusing. For example, Skipped some data unknown to this version of Viewer. 7 cases encountered (UnknownEventType: 7). to me suggests that 7 is the event type. Making it clearer that the number is the count.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed very confusing. Thanks!

string message = $"Skipped some data unknown to this version of Viewer. {errorByType.Sum()} case{(errorByType.Sum() > 1 ? "s" : string.Empty)} encountered ({summary}).";

TreeNode node = readerSettings.UnknownDataBehavior switch
Expand Down
4 changes: 3 additions & 1 deletion src/StructuredLogger/BinaryLogger/BinaryLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,16 @@ public sealed class BinaryLogger : ILogger
// - GeneratedFileUsedEventArgs exposed for brief period of time (so let's continue with 20)
// version 20:
// - TaskStartedEventArgs: Added TaskAssemblyLocation property
// version 21:
// - TaskParameterEventArgs: Added ParameterName and PropertyName properties

// This should be never changed.
// The minimum version of the binary log reader that can read log of above version.
internal const int ForwardCompatibilityMinimalVersion = 18;

// The current version of the binary log representation.
// Changes with each update of the binary log format.
internal const int FileFormatVersion = 20;
internal const int FileFormatVersion = 21;
// The minimum version of the binary log reader that can read log of above version.
// This should be changed only when the binary log format is changed in a way that would prevent it from being
// read by older readers. (changing of the individual BuildEventArgs or adding new is fine - as reader can
Expand Down
5 changes: 5 additions & 0 deletions src/StructuredLogger/BinaryLogger/BuildEventArgsReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1149,10 +1149,15 @@ private BuildEventArgs ReadTaskParameterEventArgs()
var kind = (TaskParameterMessageKind)ReadInt32();
var itemType = ReadDeduplicatedString() ?? "N/A";
var items = ReadTaskItemList() as IList ?? Array.Empty<ITaskItem>();
var (parameterName, propertyName) = _fileFormatVersion >= 21
? (ReadDeduplicatedString(), ReadDeduplicatedString())
: (null, null);

var e = ItemGroupLoggingHelper.CreateTaskParameterEventArgs(
fields.BuildEventContext,
kind,
parameterName,
propertyName,
itemType,
items,
logItemMetadata: true,
Expand Down
5 changes: 5 additions & 0 deletions src/StructuredLogger/BinaryLogger/BuildEventArgsWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,11 @@ private BinaryLogRecordKind Write(TaskParameterEventArgs e)
Write((int)e.Kind);
WriteDeduplicatedString(e.ItemType);
WriteTaskItemList(e.Items, e.LogItemMetadata);

TaskParameterEventArgs2 taskParameters2 = e as TaskParameterEventArgs2;
WriteDeduplicatedString(taskParameters2?.ParameterName);
WriteDeduplicatedString(taskParameters2?.PropertyName);

if (e.Kind == TaskParameterMessageKind.AddItem
|| e.Kind == TaskParameterMessageKind.TaskOutput)
{
Expand Down
30 changes: 30 additions & 0 deletions src/StructuredLogger/BinaryLogger/TaskParameterEventArgs2.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
using System;
using System.Collections;

namespace Microsoft.Build.Framework
{
public class TaskParameterEventArgs2 : TaskParameterEventArgs
{
public TaskParameterEventArgs2(
TaskParameterMessageKind kind,
string parameterName,
string propertyName,
string itemType,
IList items,
bool logItemMetadata,
DateTime eventTimestamp)
: base(kind, itemType, items, logItemMetadata, eventTimestamp)
{
ParameterName = parameterName;
PropertyName = propertyName;
}

// Added in MSBuild 17.11
public string ParameterName { get; set; }
public string PropertyName { get; set; }

// the properties in the base class have an internal setter
public new int LineNumber { get; set; }
public new int ColumnNumber { get; set; }
}
}
25 changes: 8 additions & 17 deletions src/StructuredLogger/BinaryLogger/Utilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Reflection;
using System.Runtime.CompilerServices;
using Microsoft.Build.Framework;
using Microsoft.Build.Logging.StructuredLogger;

namespace Microsoft.Build.BackEnd
{
Expand All @@ -14,34 +12,27 @@ internal class ItemGroupLoggingHelper
internal static TaskParameterEventArgs CreateTaskParameterEventArgs(
BuildEventContext buildEventContext,
TaskParameterMessageKind messageKind,
string parameterName,
string propertyName,
string itemType,
IList items,
bool logItemMetadata,
DateTime timestamp,
int line,
int column)
{
var args = new TaskParameterEventArgs(
var args = new TaskParameterEventArgs2(
messageKind,
parameterName,
propertyName,
itemType,
items,
logItemMetadata,
timestamp);
args.BuildEventContext = buildEventContext;

if (line != 0)
{
Reflector.SetLineNumber(args, line);
}

if (column != 0)
{
Reflector.SetColumnNumber(args, column);
}

// Should probably make these public
// args.LineNumber = line;
// args.ColumnNumber = column;
args.LineNumber = line;
args.ColumnNumber = column;
Comment on lines -32 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

return args;
}
}
Expand Down Expand Up @@ -426,4 +417,4 @@ public static BuildEventContext ReadBuildEventContext(this BinaryReader reader)
return buildEventContext;
}
}
}
}
17 changes: 13 additions & 4 deletions src/StructuredLogger/Construction/MessageProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ private void ProcessProjectImported(ProjectImportedEventArgs args)
private void ProcessTaskParameter(TaskParameterEventArgs args)
{
string itemType = args.ItemType;
var (parameterName, propertyName) = args is TaskParameterEventArgs2 taskParameterEventArgs2
? (taskParameterEventArgs2.ParameterName, taskParameterEventArgs2.PropertyName)
: (null, null);
var items = args.Items;
var kind = args.Kind;

Expand All @@ -189,11 +192,17 @@ private void ProcessTaskParameter(TaskParameterEventArgs args)

bool isOutput = kind == TaskParameterMessageKind.TaskOutput;

string folderName = isOutput ? Strings.OutputItems : Strings.Parameters;
string folderName = isOutput
? (propertyName is null ? Strings.OutputItems : Strings.OutputProperties)
: Strings.Parameters;
parent = task.GetOrCreateNodeWithName<Folder>(folderName);
parent.DisableChildrenCache = true;

node = CreateParameterNode(itemType, items, isOutput);
string itemName = isOutput && parameterName is not null
? $"{propertyName ?? itemType} from parameter {parameterName}"
: propertyName ?? itemType;

node = CreateParameterNode(itemName, items, isOutput);
}
else if (
kind == TaskParameterMessageKind.AddItem ||
Expand All @@ -208,14 +217,14 @@ private void ProcessTaskParameter(TaskParameterEventArgs args)
{
named = new AddItem
{
LineNumber = args.LineNumber
LineNumber = (args as TaskParameterEventArgs2)?.LineNumber ?? args.LineNumber
};
}
else if (kind == TaskParameterMessageKind.RemoveItem)
{
named = new RemoveItem
{
LineNumber = args.LineNumber
LineNumber = (args as TaskParameterEventArgs2)?.LineNumber ?? args.LineNumber
};
}
else
Expand Down
14 changes: 1 addition & 13 deletions src/StructuredLogger/Reflector.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,6 @@ public static void SetTimestamp(BuildEventArgs args, DateTime timestamp)
timeStampSetter(args, timestamp);
}

private static Action<BuildMessageEventArgs, int> lineNumberSetter = GetFieldSetter<BuildMessageEventArgs, int>("lineNumber");
public static void SetLineNumber(BuildMessageEventArgs args, int lineNumber)
{
lineNumberSetter(args, lineNumber);
}

private static Action<BuildMessageEventArgs, int> columnNumberSetter = GetFieldSetter<BuildMessageEventArgs, int>("columnNumber");
public static void SetColumnNumber(BuildMessageEventArgs args, int columnNumber)
{
columnNumberSetter(args, columnNumber);
}

Comment on lines -74 to -85
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this anymore as TaskParameterEventArgs2 contains new props representing line and column number.

private static MethodInfo enumerateItemsPerType;
public static MethodInfo GetEnumerateItemsPerTypeMethod(Type itemDictionary)
{
Expand Down Expand Up @@ -114,4 +102,4 @@ public static Action<T, R> GetFieldSetter<T, R>(string fieldName)
return compiled;
}
}
}
}