Skip to content
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
5 changes: 5 additions & 0 deletions src/Build.UnitTests/BackEnd/BuildManager_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,11 @@ public void OutOfProcNodeForwardCertainproperties()
_env.SetEnvironmentVariable("MsBuildForwardPropertiesFromChild", "InitialProperty3;IAMNOTREAL");
_env.SetEnvironmentVariable("MSBUILDNOINPROCNODE", "1");

// ProjectEvaluationFinished automatically and always forwards all properties, so we'd
// end up with all ~136 properties. Since this test is explicitly testing forwarding specific
// properties on ProjectStarted, turn off the new behavior.
_env.SetEnvironmentVariable("MSBUILDLOGPROPERTIESANDITEMSAFTEREVALUATION", "0");

var project = CreateProject(contents, null, _projectCollection, false);
var data = new BuildRequestData(project.FullPath, new Dictionary<string, string>(),
MSBuildDefaultToolsVersion, new string[] { }, null);
Expand Down
2 changes: 1 addition & 1 deletion src/Build.UnitTests/BackEnd/TaskBuilder_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ public void NullMetadataOnOutputItems_InlineTask()
/// <summary>
/// If an item being output from a task has null metadata, we shouldn't crash.
/// </summary>
[Fact]
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/6521")]
[Trait("Category", "non-mono-tests")]
public void NullMetadataOnLegacyOutputItems_InlineTask()
{
Expand Down
5 changes: 3 additions & 2 deletions src/Build.UnitTests/ConsoleLogger_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public void ErrorMessageWithMultiplePropertiesInMessage(bool includeEvaluationPr
output.ShouldContain("source_of_error : error : Hello from project 2 [" + project.ProjectFile + ":: Number=2 TargetFramework=netcoreapp2.1]");
}

[Fact]
[Fact(Skip = "https://github.com/dotnet/msbuild/issues/6518")]
[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp, "Minimal path validation in Core allows expanding path containing quoted slashes.")]
[SkipOnTargetFramework(TargetFrameworkMonikers.Mono, "Minimal path validation in Mono allows expanding path containing quoted slashes.")]
public void TestItemsWithUnexpandableMetadata()
Expand All @@ -338,7 +338,8 @@ public void TestItemsWithUnexpandableMetadata()
<Target Name=""X"" />
</Project>", logger);

sc.ToString().ShouldContain("\"a\\b\\%(Filename).c\"");
var text = sc.ToString();
text.ShouldContain("\"a\\b\\%(Filename).c\"");
}

/// <summary>
Expand Down
78 changes: 50 additions & 28 deletions src/Build/Logging/BaseConsoleLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,21 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text;

using Microsoft.Build.Evaluation;
using Microsoft.Build.Framework;
using Microsoft.Build.Internal;
using Microsoft.Build.Shared;
using System.Collections;
using System.Globalization;
using System.IO;

using ColorSetter = Microsoft.Build.Logging.ColorSetter;
using ColorResetter = Microsoft.Build.Logging.ColorResetter;
using WriteHandler = Microsoft.Build.Logging.WriteHandler;
using Microsoft.Build.Exceptions;

namespace Microsoft.Build.BackEnd.Logging
{
Expand Down Expand Up @@ -642,42 +644,56 @@ internal SortedList ExtractItemList(IEnumerable items)
/// </summary>
internal virtual void OutputItems(string itemType, ArrayList itemTypeList)
{
// Write each item, one per line
bool haveWrittenItemType = false;
setColor(ConsoleColor.DarkGray);
foreach (ITaskItem item in itemTypeList)
WriteItemType(itemType);

foreach (var item in itemTypeList)
{
if (!haveWrittenItemType)
string itemSpec = item switch
{
setColor(ConsoleColor.Gray);
WriteLinePretty(itemType);
haveWrittenItemType = true;
setColor(ConsoleColor.DarkGray);
}
WriteLinePretty(" " /* indent slightly*/ + item.ItemSpec);
ITaskItem taskItem => taskItem.ItemSpec,
IItem iitem => iitem.EvaluatedInclude,
{ } misc => Convert.ToString(misc),
null => "null"
};

IDictionary metadata = item.CloneCustomMetadata();
WriteItemSpec(itemSpec);

foreach (DictionaryEntry metadatum in metadata)
var metadata = item switch
{
string valueOrError;
try
{
valueOrError = item.GetMetadata(metadatum.Key as string);
}
catch (InvalidProjectFileException e)
IMetadataContainer metadataContainer => metadataContainer.EnumerateMetadata(),
IItem<ProjectMetadata> iitem => iitem.Metadata?.Select(m => new KeyValuePair<string, string>(m.Name, m.EvaluatedValue)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks cleaner, but I'd curious about whether you can inline this to avoid allocating an IEnumerable. Probably not because of the null unless you wanted it really messy.

Also, do you need to unescape the value?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure about inlining

good question about unescaping, need to see whether the values on iitem.Metadata are already unescaped...

_ => null
};

if (metadata != null)
{
foreach (var metadatum in metadata)
{
valueOrError = e.Message;
WriteMetadata(metadatum.Key, metadatum.Value);
}

// A metadatum's "value" is its escaped value, since that's how we represent them internally.
// So unescape before returning to the world at large.
WriteLinePretty(" " + metadatum.Key + " = " + valueOrError);
}
}

resetColor();
}

protected virtual void WriteItemType(string itemType)
{
setColor(ConsoleColor.Gray);
WriteLinePretty(itemType);
setColor(ConsoleColor.DarkGray);
}

protected virtual void WriteItemSpec(string itemSpec)
{
WriteLinePretty(" " + itemSpec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check—this will not allocate the string of spaces every time, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

it won't allocate the string literal, but it will allocate the string for the concatenated total, and it's terrible.

Fortunately we may have an intern this summer who may help look into rewriting the console loggers to avoid allocations.

}

protected virtual void WriteMetadata(string name, string value)
{
WriteLinePretty(" " + name + " = " + value);
}

/// <summary>
/// Returns a performance counter for a given scope (either task name or target name)
/// from the given table.
Expand Down Expand Up @@ -959,6 +975,12 @@ public virtual void Initialize(IEventSource eventSource)
eventSource.MessageRaised += MessageHandler;
eventSource.CustomEventRaised += CustomEventHandler;
eventSource.StatusEventRaised += StatusEventHandler;

bool logPropertiesAndItemsAfterEvaluation = Utilities.Traits.Instance.EscapeHatches.LogPropertiesAndItemsAfterEvaluation ?? true;
if (logPropertiesAndItemsAfterEvaluation && eventSource is IEventSource4 eventSource4)
{
eventSource4.IncludeEvaluationPropertiesAndItems();
}
}
}

Expand Down
113 changes: 64 additions & 49 deletions src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -540,20 +540,38 @@ public override void ProjectStartedHandler(object sender, ProjectStartedEventArg
}
}

ReadProjectConfigurationDescription(e.BuildEventContext, e.Items);
var projectKey = (e.BuildEventContext.NodeId, e.BuildEventContext.ProjectContextId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can e.BuildEventContext be null here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, BuildEventContext should always be set


// If the value is available at all, it will be either in the items
// from ProjectStarted (old behavior), or the items from ProjectEvaluationFinished (new behavior).
// First try the old behavior, and fallback to the new behavior.
var result = ReadProjectConfigurationDescription(e.Items);
if (result != null)
{
// Found the items directly on ProjectStarted
propertyOutputMap[projectKey] = result;
}
else
{
// Try to see if we saw the items on the corresponding ProjectEvaluationFinished
var evaluationKey = GetEvaluationKey(e.BuildEventContext);

// if the value was set from ProjectEvaluationFinished, copy it into the entry
// for this project
if (propertyOutputMap.TryGetValue(evaluationKey, out string value))
{
propertyOutputMap[projectKey] = value;
}
}
}

private void ReadProjectConfigurationDescription(BuildEventContext buildEventContext, IEnumerable items)
private string ReadProjectConfigurationDescription(IEnumerable items)
{
if (buildEventContext == null || items == null)
if (items == null)
{
return;
return null;
}

// node and project context ids for the propertyOutputMap key.
int nodeID = buildEventContext.NodeId;
int projectContextId = buildEventContext.ProjectContextId;

ReuseableStringBuilder projectConfigurationDescription = null;

Internal.Utilities.EnumerateItems(items, item =>
Expand All @@ -578,14 +596,27 @@ private void ReadProjectConfigurationDescription(BuildEventContext buildEventCon
}
});

// Add the finished dictionary to propertyOutputMap.
if (projectConfigurationDescription != null)
{
propertyOutputMap.Add((nodeID, projectContextId), projectConfigurationDescription.ToString());
var result = projectConfigurationDescription.ToString();
(projectConfigurationDescription as IDisposable)?.Dispose();
return result;
}

return null;
}

/// <summary>
/// In case the items are stored on ProjectEvaluationFinishedEventArgs
/// (new behavior), we first store the value per evaluation, and then
/// in ProjectStarted, find the value from the project's evaluation
/// and use that.
/// </summary>
private (int, int) GetEvaluationKey(BuildEventContext buildEventContext)
// note that we use a negative number for evaluations so that we don't conflict
// with project context ids.
=> (buildEventContext.NodeId, -buildEventContext.EvaluationId);

/// <summary>
/// Handler for project finished events
/// </summary>
Expand Down Expand Up @@ -751,44 +782,23 @@ internal void WriteItems(BuildEventArgs e, IEnumerable items)
ShownBuildEventContext(e.BuildEventContext);
}

internal override void OutputItems(string itemType, ArrayList itemTypeList)
protected override void WriteItemType(string itemType)
{
// Write each item, one per line
bool haveWrittenItemType = false;
foreach (ITaskItem item in itemTypeList)
{
if (!haveWrittenItemType)
{
setColor(ConsoleColor.DarkGray);
WriteMessageAligned(itemType, false);
haveWrittenItemType = true;
}
setColor(ConsoleColor.Gray);

// Indent the text by two tab lengths
StringBuilder result = new StringBuilder((2 * tabWidth) + item.ItemSpec.Length);
result.Append(' ', 2 * tabWidth).Append(item.ItemSpec);
WriteMessageAligned(result.ToString(), false);

IDictionary metadata = item.CloneCustomMetadata();
setColor(ConsoleColor.DarkGray);
WriteMessageAligned(itemType, prefixAlreadyWritten: false);
setColor(ConsoleColor.Gray);
}

foreach (DictionaryEntry metadatum in metadata)
{
string valueOrError;
try
{
valueOrError = item.GetMetadata(metadatum.Key as string);
}
catch (InvalidProjectFileException e)
{
valueOrError = e.Message;
}
protected override void WriteItemSpec(string itemSpec)
{
WriteMessageAligned(new string(' ', 2 * tabWidth) + itemSpec, prefixAlreadyWritten: false);
}

WriteMessageAligned($"{new string(' ', 4 * tabWidth)}{metadatum.Key} = {valueOrError}", false);
}
}
resetColor();
protected override void WriteMetadata(string name, string value)
{
WriteMessageAligned($"{new string(' ', 4 * tabWidth)}{name} = {value}", prefixAlreadyWritten: false);
}

/// <summary>
/// Handler for target started events
/// </summary>
Expand Down Expand Up @@ -962,16 +972,16 @@ public override void TaskFinishedHandler(object sender, TaskFinishedEventArgs e)
/// <summary>
/// Finds the LogOutProperty string to be printed in messages.
/// </summary>
/// <param name="e"> Build event to extract context information from.</param>
/// <param name="e">Build event to extract context information from.</param>
internal string FindLogOutputProperties(BuildEventArgs e)
{
string projectConfigurationDescription = String.Empty;
if (e.BuildEventContext != null)
{
int nodeId = e.BuildEventContext.NodeId;
int projectContextId = e.BuildEventContext.ProjectContextId;
propertyOutputMap.TryGetValue((nodeId, projectContextId), out projectConfigurationDescription);
var key = (e.BuildEventContext.NodeId, e.BuildEventContext.ProjectContextId);
propertyOutputMap.TryGetValue(key, out projectConfigurationDescription);
}

return projectConfigurationDescription;
}

Expand Down Expand Up @@ -1168,7 +1178,12 @@ public override void StatusEventHandler(object sender, BuildStatusEventArgs e)
}
}

ReadProjectConfigurationDescription(projectEvaluationFinished.BuildEventContext, projectEvaluationFinished.Items);
var value = ReadProjectConfigurationDescription(projectEvaluationFinished.Items);
if (value != null)
{
var evaluationKey = GetEvaluationKey(e.BuildEventContext);
propertyOutputMap[evaluationKey] = value;
}
}
}

Expand Down