From c7e5c0a46d5795461b6f254f8c127f71adb6b64a Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 22 Mar 2022 16:06:41 -0700 Subject: [PATCH 01/37] Log environment variables read as properties --- src/Build/Evaluation/Expander.cs | 4 +- .../BinaryLogger/BuildEventArgsReader.cs | 4 +- .../BinaryLogger/BuildEventArgsWriter.cs | 1 + .../BuildFinishedEventArgs_Tests.cs | 2 +- src/Framework/BuildFinishedEventArgs.cs | 43 ++++++++++++++++++- .../PublicAPI/net/PublicAPI.Unshipped.txt | 2 + .../netstandard/PublicAPI.Unshipped.txt | 2 + src/Shared/EnvironmentUtilities.cs | 3 ++ 8 files changed, 57 insertions(+), 4 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 1bacf84bbe7..a20aa36c571 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1469,7 +1469,9 @@ private static object LookupProperty(IPropertyProvider properties, string pro /// private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties) { - T property = properties.GetProperty(propertyName, startIndex, endIndex); + string propertyNameValue = propertyName.Substring(startIndex, endIndex - startIndex + 1); + T property = properties.GetProperty(propertyNameValue, 0, propertyNameValue.Length - 1); + EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[propertyNameValue] = Environment.GetEnvironmentVariable(propertyNameValue); object propertyValue; diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs index 64a26269a78..9aeecc7347e 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs @@ -362,12 +362,14 @@ private BuildEventArgs ReadBuildFinishedEventArgs() { var fields = ReadBuildEventArgsFields(); var succeeded = ReadBoolean(); + var environmentProperties = ReadStringDictionary(); var e = new BuildFinishedEventArgs( fields.Message, fields.HelpKeyword, succeeded, - fields.Timestamp); + fields.Timestamp, + environmentVariables: environmentProperties); SetCommonFields(e, fields); return e; } diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs index e9db5412a24..4ea9d105985 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs @@ -259,6 +259,7 @@ private void Write(BuildFinishedEventArgs e) Write(BinaryLogRecordKind.BuildFinished); WriteBuildEventArgsFields(e); Write(e.Succeeded); + Write(EnvironmentUtilities.EnvironmentVariablesUsedAsProperties); } private void Write(ProjectEvaluationStartedEventArgs e) diff --git a/src/Framework.UnitTests/BuildFinishedEventArgs_Tests.cs b/src/Framework.UnitTests/BuildFinishedEventArgs_Tests.cs index 2bf43e98406..eca867cd1cc 100644 --- a/src/Framework.UnitTests/BuildFinishedEventArgs_Tests.cs +++ b/src/Framework.UnitTests/BuildFinishedEventArgs_Tests.cs @@ -27,7 +27,7 @@ public void EventArgsCtors() buildFinishedEvent = new BuildFinishedEventArgs("{0}", "HelpKeyword", true, new DateTime(), "Message"); buildFinishedEvent = new BuildFinishedEventArgs(null, null, true); buildFinishedEvent = new BuildFinishedEventArgs(null, null, true, new DateTime()); - buildFinishedEvent = new BuildFinishedEventArgs(null, null, true, new DateTime(), null); + buildFinishedEvent = new BuildFinishedEventArgs(null, null, true, new DateTime(), messageArgs: null); } /// diff --git a/src/Framework/BuildFinishedEventArgs.cs b/src/Framework/BuildFinishedEventArgs.cs index 1647591416c..1e3accf4353 100644 --- a/src/Framework/BuildFinishedEventArgs.cs +++ b/src/Framework/BuildFinishedEventArgs.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.Collections.Generic; using System.IO; #nullable disable @@ -25,6 +26,11 @@ public class BuildFinishedEventArgs : BuildStatusEventArgs /// private bool succeeded; + /// + /// Environment variable-derived properties + /// + private IDictionary environmentVariables; + /// /// Default constructor /// @@ -65,7 +71,7 @@ public BuildFinishedEventArgs bool succeeded, DateTime eventTimestamp ) - : this(message, helpKeyword, succeeded, eventTimestamp, null) + : this(message, helpKeyword, succeeded, eventTimestamp, messageArgs: null) { // do nothing } @@ -91,6 +97,30 @@ params object[] messageArgs this.succeeded = succeeded; } + /// + /// Constructor which allows environment variable-derived properties to be set + /// + /// text message + /// help keyword + /// True indicates a successful build + /// Timestamp when the event was created + /// Properties derived from environment variables + /// message arguments + public BuildFinishedEventArgs + ( + string message, + string helpKeyword, + bool succeeded, + DateTime eventTimestamp, + IDictionary environmentVariables, + params object[] messageArgs + ) + : base(message, helpKeyword, "MSBuild", eventTimestamp, messageArgs) + { + this.succeeded = succeeded; + this.environmentVariables = environmentVariables; + } + #region CustomSerializationToStream /// @@ -125,5 +155,16 @@ public bool Succeeded return succeeded; } } + + /// + /// Gets all environment variables read when trying to evaluate properties along with their values. + /// + public IDictionary EnvironmentVariables + { + get + { + return environmentVariables; + } + } } } diff --git a/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt b/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt index e69de29bb2d..813fa92a36d 100644 --- a/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt +++ b/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt @@ -0,0 +1,2 @@ +Microsoft.Build.Framework.BuildFinishedEventArgs.BuildFinishedEventArgs(string message, string helpKeyword, bool succeeded, System.DateTime eventTimestamp, System.Collections.Generic.IDictionary environmentVariables, params object[] messageArgs) -> void +Microsoft.Build.Framework.BuildFinishedEventArgs.EnvironmentVariables.get -> System.Collections.Generic.IDictionary \ No newline at end of file diff --git a/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt b/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt index e69de29bb2d..813fa92a36d 100644 --- a/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt +++ b/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt @@ -0,0 +1,2 @@ +Microsoft.Build.Framework.BuildFinishedEventArgs.BuildFinishedEventArgs(string message, string helpKeyword, bool succeeded, System.DateTime eventTimestamp, System.Collections.Generic.IDictionary environmentVariables, params object[] messageArgs) -> void +Microsoft.Build.Framework.BuildFinishedEventArgs.EnvironmentVariables.get -> System.Collections.Generic.IDictionary \ No newline at end of file diff --git a/src/Shared/EnvironmentUtilities.cs b/src/Shared/EnvironmentUtilities.cs index e9e0482619f..0e875a72fe3 100644 --- a/src/Shared/EnvironmentUtilities.cs +++ b/src/Shared/EnvironmentUtilities.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.Collections.Generic; using System.Runtime.InteropServices; #nullable disable @@ -14,5 +15,7 @@ internal static partial class EnvironmentUtilities public static bool Is64BitOperatingSystem => Environment.Is64BitOperatingSystem; + + public static Dictionary EnvironmentVariablesUsedAsProperties { get; } = new(); } } From 418b210dafd258d4b5432c1db4c9a0c3f499f3f9 Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 23 Mar 2022 08:54:46 -0700 Subject: [PATCH 02/37] Only log random environment variables if asked This is both a privacy issue and a waste from a performance perspective. --- src/Build/Logging/BinaryLogger/BinaryLogger.cs | 4 +++- .../Logging/BinaryLogger/BuildEventArgsWriter.cs | 13 ++++++++----- src/Framework/Traits.cs | 5 +++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 7fbddddf7ee..37e88e1afb0 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -53,7 +53,9 @@ public sealed class BinaryLogger : ILogger // - TargetSkippedEventArgs: added OriginallySucceeded, Condition, EvaluatedCondition // version 14: // - TargetSkippedEventArgs: added SkipReason, OriginalBuildEventContext - internal const int FileFormatVersion = 14; + // version 15: + // - Log only environment variables accessed as properties + internal const int FileFormatVersion = 15; private Stream stream; private BinaryWriter binaryWriter; diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs index 4ea9d105985..07b13b482ad 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs @@ -4,18 +4,14 @@ using System; using System.Collections; using System.Collections.Generic; -using System.Diagnostics; using System.Globalization; using System.IO; -using System.Linq; using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Collections; using Microsoft.Build.Evaluation; -using Microsoft.Build.Exceptions; using Microsoft.Build.Execution; using Microsoft.Build.Framework; using Microsoft.Build.Framework.Profiler; -using Microsoft.Build.Internal; using Microsoft.Build.Shared; #nullable disable @@ -251,7 +247,14 @@ private void Write(BuildStartedEventArgs e) { Write(BinaryLogRecordKind.BuildStarted); WriteBuildEventArgsFields(e); - Write(e.BuildEnvironment); + if (Traits.Instance.LogAllEnvironmentVariables) + { + Write(e.BuildEnvironment); + } + else + { + Write(new Dictionary(0)); + } } private void Write(BuildFinishedEventArgs e) diff --git a/src/Framework/Traits.cs b/src/Framework/Traits.cs index cf60eb140c9..11e24117e6b 100644 --- a/src/Framework/Traits.cs +++ b/src/Framework/Traits.cs @@ -92,6 +92,11 @@ public Traits() /// public readonly bool LogPropertyFunctionsRequiringReflection = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MSBuildLogPropertyFunctionsRequiringReflection")); + /// + /// Log all environment variables whether or not they are used in a build in the binary log. + /// + public readonly bool LogAllEnvironmentVariables = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MSBUILDLOGALLENVIRONMENTVARIABLES")); + /// /// Log property tracking information. /// From fb96241a71ef5d66fb652ce5c226977439350634 Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 23 Mar 2022 10:27:29 -0700 Subject: [PATCH 03/37] Don't write a full (empty) dictionary --- src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs index 07b13b482ad..b5737686fef 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs @@ -253,7 +253,7 @@ private void Write(BuildStartedEventArgs e) } else { - Write(new Dictionary(0)); + Write(0); } } From cf9c72290c06429b9dde62a0b1b34347355a788c Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 23 Mar 2022 14:01:51 -0700 Subject: [PATCH 04/37] Only log nonempty env vars --- src/Build/Evaluation/Expander.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index a20aa36c571..d4794c4c3e5 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1469,9 +1469,11 @@ private static object LookupProperty(IPropertyProvider properties, string pro /// private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties) { - string propertyNameValue = propertyName.Substring(startIndex, endIndex - startIndex + 1); - T property = properties.GetProperty(propertyNameValue, 0, propertyNameValue.Length - 1); - EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[propertyNameValue] = Environment.GetEnvironmentVariable(propertyNameValue); + T property = properties.GetProperty(propertyName, startIndex, endIndex); + if (!string.IsNullOrEmpty(property.EvaluatedValue)) + { + EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[property.Name] = property.EvaluatedValue; + } object propertyValue; From d7c03b2b98587e60ba51d168c6deef8e08ce79aa Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 23 Mar 2022 14:46:01 -0700 Subject: [PATCH 05/37] Fix NRE --- src/Build/Evaluation/Expander.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index d4794c4c3e5..7f49a862652 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1470,7 +1470,7 @@ private static object LookupProperty(IPropertyProvider properties, string pro private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties) { T property = properties.GetProperty(propertyName, startIndex, endIndex); - if (!string.IsNullOrEmpty(property.EvaluatedValue)) + if (!string.IsNullOrEmpty(property?.EvaluatedValue)) { EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[property.Name] = property.EvaluatedValue; } From 5b58243448962877fb84d2c31d44fe6005a00f28 Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 23 Mar 2022 16:04:49 -0700 Subject: [PATCH 06/37] Make tests pass --- src/Build.UnitTests/BinaryLogger_Tests.cs | 4 ++++ src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs | 7 +------ src/Shared/UnitTests/EngineTestEnvironment.cs | 2 ++ 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Build.UnitTests/BinaryLogger_Tests.cs b/src/Build.UnitTests/BinaryLogger_Tests.cs index cb255f7ae9b..4be660b2231 100644 --- a/src/Build.UnitTests/BinaryLogger_Tests.cs +++ b/src/Build.UnitTests/BinaryLogger_Tests.cs @@ -124,6 +124,10 @@ public void TestBinaryLoggerRoundtrip(string projectText) var serialActual = serialFromPlaybackText.ToString(); var parallelExpected = parallelFromBuildText.ToString(); var parallelActual = parallelFromPlaybackText.ToString(); + serialExpected = serialExpected.Substring(serialExpected.IndexOf("Project")); + serialActual = serialActual.Substring(serialActual.IndexOf("Project")); + parallelExpected = parallelExpected.Substring(parallelExpected.IndexOf("Project")); + parallelActual = parallelActual.Substring(parallelActual.IndexOf("Project")); serialActual.ShouldContainWithoutWhitespace(serialExpected); parallelActual.ShouldContainWithoutWhitespace(parallelExpected); diff --git a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs index 4ad1bb35cac..da9e6e8b383 100644 --- a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs +++ b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs @@ -38,13 +38,8 @@ public void RoundtripBuildStartedEventArgs() args = new BuildStartedEventArgs( "M", - null, - new Dictionary - { - { "SampleName", "SampleValue" } - }); + null); Roundtrip(args, - e => TranslationHelpers.ToString(e.BuildEnvironment), e => e.HelpKeyword, e => e.ThreadId.ToString(), e => e.SenderName); diff --git a/src/Shared/UnitTests/EngineTestEnvironment.cs b/src/Shared/UnitTests/EngineTestEnvironment.cs index 649d0ff35db..5f895e75fd4 100644 --- a/src/Shared/UnitTests/EngineTestEnvironment.cs +++ b/src/Shared/UnitTests/EngineTestEnvironment.cs @@ -235,7 +235,9 @@ private bool BuildProject( foreach (var pair in pairs) { var expectedText = pair.expected.textGetter(); + expectedText = expectedText.Substring(expectedText.IndexOf("Project")); var actualText = pair.actual.textGetter(); + actualText = actualText.Substring(actualText.IndexOf("Project")); actualText.ShouldContainWithoutWhitespace(expectedText); } } From fdee1138759322169182e1dd1e29764ccdcd25ff Mon Sep 17 00:00:00 2001 From: Forgind Date: Mon, 28 Mar 2022 15:47:21 -0700 Subject: [PATCH 07/37] Only find environment properties --- src/Build/Definition/ProjectProperty.cs | 2 ++ src/Build/Evaluation/Expander.cs | 2 +- src/Build/Evaluation/IProperty.cs | 6 ++++++ src/Build/Instance/ProjectInstance.cs | 4 ++-- src/Build/Instance/ProjectPropertyInstance.cs | 20 +++++++++---------- 5 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index b3e808c3c9a..f42cf55f80e 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -204,6 +204,8 @@ string IValued.EscapedValue get => EvaluatedValueEscapedInternal; } + bool IProperty.IsEnvironmentProperty { get => IsEnvironmentProperty; set => throw new NotImplementedException(); } + #region IEquatable Members /// diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 7f49a862652..4d76f99f815 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1470,7 +1470,7 @@ private static object LookupProperty(IPropertyProvider properties, string pro private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties) { T property = properties.GetProperty(propertyName, startIndex, endIndex); - if (!string.IsNullOrEmpty(property?.EvaluatedValue)) + if (!string.IsNullOrEmpty(property?.EvaluatedValue) && property.IsEnvironmentProperty) { EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[property.Name] = property.EvaluatedValue; } diff --git a/src/Build/Evaluation/IProperty.cs b/src/Build/Evaluation/IProperty.cs index ed57eef4ea3..a5342841b89 100644 --- a/src/Build/Evaluation/IProperty.cs +++ b/src/Build/Evaluation/IProperty.cs @@ -35,5 +35,11 @@ string EvaluatedValueEscaped { get; } + + bool IsEnvironmentProperty + { + get; + set; + } } } diff --git a/src/Build/Instance/ProjectInstance.cs b/src/Build/Instance/ProjectInstance.cs index 94344843b96..6b8c88568c0 100644 --- a/src/Build/Instance/ProjectInstance.cs +++ b/src/Build/Instance/ProjectInstance.cs @@ -1478,7 +1478,7 @@ IItemDefinition IEvaluatorData.SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable) { // Mutability not verified as this is being populated during evaluation - ProjectPropertyInstance property = ProjectPropertyInstance.Create(name, evaluatedValueEscaped, mayBeReserved, _isImmutable); + ProjectPropertyInstance property = ProjectPropertyInstance.Create(name, evaluatedValueEscaped, mayBeReserved, _isImmutable, isEnvironmentVariable); _properties.Set(property); return property; } @@ -2966,7 +2966,7 @@ private void CreatePropertiesSnapshot(ICollection properties, b { // Allow reserved property names, since this is how they are added to the project instance. // The caller has prevented users setting them themselves. - ProjectPropertyInstance instance = ProjectPropertyInstance.Create(property.Name, ((IProperty)property).EvaluatedValueEscaped, true /* MAY be reserved name */, isImmutable); + ProjectPropertyInstance instance = ProjectPropertyInstance.Create(property.Name, ((IProperty)property).EvaluatedValueEscaped, true /* MAY be reserved name */, isImmutable, property.IsEnvironmentProperty); _properties.Set(instance); } } diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index e935b611067..2ceac193e4b 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -98,6 +98,8 @@ public string EvaluatedValue [DebuggerBrowsable(DebuggerBrowsableState.Never)] string IValued.EscapedValue => _escapedValue; + bool IProperty.IsEnvironmentProperty { get; set; } + #region IEquatable Members /// @@ -182,9 +184,9 @@ internal static ProjectPropertyInstance Create(string name, string escapedValue, /// This flags should ONLY be set by the evaluator or by cloning; after the ProjectInstance is created, they must be illegal. /// If name is invalid or reserved, throws ArgumentException. /// - internal static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, bool isImmutable) + internal static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, bool isImmutable, bool isEnvironmentProperty = false) { - return Create(name, escapedValue, mayBeReserved, null, isImmutable); + return Create(name, escapedValue, mayBeReserved, null, isImmutable, isEnvironmentProperty); } /// @@ -212,7 +214,7 @@ internal static ProjectPropertyInstance Create(string name, string escapedValue, /// internal static ProjectPropertyInstance Create(ProjectPropertyInstance that) { - return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: that.IsImmutable); + return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: that.IsImmutable, ((IProperty)that).IsEnvironmentProperty); } /// @@ -221,7 +223,7 @@ internal static ProjectPropertyInstance Create(ProjectPropertyInstance that) /// internal static ProjectPropertyInstance Create(ProjectPropertyInstance that, bool isImmutable) { - return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: isImmutable); + return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: isImmutable, ((IProperty)that).IsEnvironmentProperty); } /// @@ -278,7 +280,7 @@ internal ProjectPropertyElement ToProjectPropertyElement(ProjectElementContainer /// as it should never be needed for any subsequent messages, and is just extra bulk. /// Inherits mutability from project if any. /// - private static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, ElementLocation location, bool isImmutable) + private static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, ElementLocation location, bool isImmutable, bool isEnvironmentProperty = false) { // Does not check immutability as this is only called during build (which is already protected) or evaluation ErrorUtilities.VerifyThrowArgumentNull(escapedValue, nameof(escapedValue)); @@ -295,11 +297,9 @@ private static ProjectPropertyInstance Create(string name, string escapedValue, XmlUtilities.VerifyThrowProjectValidElementName(name, location); } - if (isImmutable) - { - return new ProjectPropertyInstanceImmutable(name, escapedValue); - } - return new ProjectPropertyInstance(name, escapedValue); + ProjectPropertyInstance instance = isImmutable ? new ProjectPropertyInstanceImmutable(name, escapedValue) : new ProjectPropertyInstance(name, escapedValue); + ((IProperty)instance).IsEnvironmentProperty = isEnvironmentProperty; + return instance; } /// From be584ed98c109853ad74fefa08aa6aab918ee238 Mon Sep 17 00:00:00 2001 From: Forgind Date: Mon, 28 Mar 2022 17:05:03 -0700 Subject: [PATCH 08/37] PR comments --- src/Build/Evaluation/Expander.cs | 8 ++++---- src/Build/Logging/BinaryLogger/BinaryLogger.cs | 3 ++- src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs | 2 +- src/Framework/BuildFinishedEventArgs.cs | 8 +------- src/Shared/EnvironmentUtilities.cs | 5 ++--- 5 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 4d76f99f815..9c3fa0f510d 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1470,10 +1470,6 @@ private static object LookupProperty(IPropertyProvider properties, string pro private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties) { T property = properties.GetProperty(propertyName, startIndex, endIndex); - if (!string.IsNullOrEmpty(property?.EvaluatedValue) && property.IsEnvironmentProperty) - { - EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[property.Name] = property.EvaluatedValue; - } object propertyValue; @@ -1517,6 +1513,10 @@ private static object LookupProperty(IPropertyProvider properties, string pro else { propertyValue = property.EvaluatedValueEscaped; + if (!string.IsNullOrEmpty(property.EvaluatedValueEscaped) && property.IsEnvironmentProperty) + { + EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[property.Name] = property.EvaluatedValueEscaped; + } } return propertyValue; diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 37e88e1afb0..0cf98ee1bb8 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -54,7 +54,8 @@ public sealed class BinaryLogger : ILogger // version 14: // - TargetSkippedEventArgs: added SkipReason, OriginalBuildEventContext // version 15: - // - Log only environment variables accessed as properties + // - Don't log all environment variables at BuildStarted + // - Log environment variables accessed as properties at BuildFinished internal const int FileFormatVersion = 15; private Stream stream; diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs index 9aeecc7347e..ab422aca622 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs @@ -362,7 +362,7 @@ private BuildEventArgs ReadBuildFinishedEventArgs() { var fields = ReadBuildEventArgsFields(); var succeeded = ReadBoolean(); - var environmentProperties = ReadStringDictionary(); + var environmentProperties = fileFormatVersion >= 15 ? ReadStringDictionary() : null; var e = new BuildFinishedEventArgs( fields.Message, diff --git a/src/Framework/BuildFinishedEventArgs.cs b/src/Framework/BuildFinishedEventArgs.cs index 1e3accf4353..54224f38b52 100644 --- a/src/Framework/BuildFinishedEventArgs.cs +++ b/src/Framework/BuildFinishedEventArgs.cs @@ -159,12 +159,6 @@ public bool Succeeded /// /// Gets all environment variables read when trying to evaluate properties along with their values. /// - public IDictionary EnvironmentVariables - { - get - { - return environmentVariables; - } - } + public IDictionary EnvironmentVariables => environmentVariables; } } diff --git a/src/Shared/EnvironmentUtilities.cs b/src/Shared/EnvironmentUtilities.cs index 0e875a72fe3..97b4e53a1a4 100644 --- a/src/Shared/EnvironmentUtilities.cs +++ b/src/Shared/EnvironmentUtilities.cs @@ -4,8 +4,7 @@ using System; using System.Collections.Generic; using System.Runtime.InteropServices; - -#nullable disable +using Microsoft.Build.Collections; namespace Microsoft.Build.Shared { @@ -16,6 +15,6 @@ internal static partial class EnvironmentUtilities public static bool Is64BitOperatingSystem => Environment.Is64BitOperatingSystem; - public static Dictionary EnvironmentVariablesUsedAsProperties { get; } = new(); + public static Dictionary EnvironmentVariablesUsedAsProperties { get; } = new(MSBuildNameIgnoreCaseComparer.Default); } } From 16332a7061392f4403fe6aadff319be0db514ca9 Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 30 Mar 2022 16:36:32 -0700 Subject: [PATCH 09/37] Log on first property read --- src/Build/Definition/Project.cs | 6 ++--- src/Build/Definition/ProjectProperty.cs | 21 ++++++++++++--- src/Build/Evaluation/Evaluator.cs | 2 +- src/Build/Evaluation/Expander.cs | 4 --- src/Build/Evaluation/IEvaluatorData.cs | 2 +- .../LazyItemEvaluator.EvaluatorData.cs | 5 ++-- .../PropertyTrackingEvaluatorDataWrapper.cs | 4 +-- src/Build/Instance/ProjectInstance.cs | 4 +-- src/Build/Instance/ProjectPropertyInstance.cs | 27 +++++++++++++++---- 9 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/Build/Definition/Project.cs b/src/Build/Definition/Project.cs index 0adf9e61756..5a1a7ce7e18 100644 --- a/src/Build/Definition/Project.cs +++ b/src/Build/Definition/Project.cs @@ -2923,7 +2923,7 @@ public override bool SetGlobalProperty(string name, string escapedValue) string originalValue = (existing == null) ? String.Empty : ((IProperty)existing).EvaluatedValueEscaped; _data.GlobalPropertiesDictionary.Set(ProjectPropertyInstance.Create(name, escapedValue)); - _data.Properties.Set(ProjectProperty.Create(Owner, name, escapedValue, true /* is global */, false /* may not be reserved name */)); + _data.Properties.Set(ProjectProperty.Create(Owner, name, escapedValue, true /* is global */, false /* may not be reserved name */, null)); ProjectCollection.AfterUpdateLoadedProjectGlobalProperties(Owner); MarkDirty(); @@ -4389,9 +4389,9 @@ public IItemDefinition GetItemDefinition(string itemType) /// /// Sets a property which is not derived from Xml. /// - public ProjectProperty SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false) + public ProjectProperty SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false, BackEnd.Logging.LoggingContext loggingContext = null) { - ProjectProperty property = ProjectProperty.Create(Project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved); + ProjectProperty property = ProjectProperty.Create(Project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, loggingContext); Properties.Set(property); AddToAllEvaluatedPropertiesList(property); diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index f42cf55f80e..00e1391e239 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -3,8 +3,10 @@ using System; using System.Diagnostics; +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Collections; using Microsoft.Build.Construction; +using Microsoft.Build.Framework; using Microsoft.Build.Shared; using ReservedPropertyNames = Microsoft.Build.Internal.ReservedPropertyNames; @@ -94,7 +96,16 @@ public string EvaluatedValue string IProperty.EvaluatedValueEscaped { [DebuggerStepThrough] - get => EvaluatedValueEscapedInternal; + get + { + if ((this as IProperty).IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext is not null) + { + EnvironmentVariableReadEventArgs args = new(Name, EvaluatedValueEscapedInternal); + notXmlBacked.loggingContext.LogBuildEvent(args); + } + + return EvaluatedValueEscapedInternal; + } } /// @@ -239,9 +250,9 @@ bool IEquatable.Equals(ProjectProperty other) /// This is ONLY to be used by the Evaluator (and Project.SetGlobalProperty) and ONLY for Global, Environment, and Built-in properties. /// All other properties originate in XML, and should have a backing XML object. /// - internal static ProjectProperty Create(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved) + internal static ProjectProperty Create(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext = null) { - return new ProjectPropertyNotXmlBacked(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved); + return new ProjectPropertyNotXmlBacked(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, loggingContext); } /// @@ -496,6 +507,7 @@ private class ProjectPropertyNotXmlBacked : ProjectProperty /// Name of the property. /// private readonly string _name; + internal LoggingContext loggingContext; /// /// Creates a property without backing XML. @@ -503,7 +515,7 @@ private class ProjectPropertyNotXmlBacked : ProjectProperty /// This is ONLY to be used by the Evaluator (and Project.SetGlobalProperty) and ONLY for Global, Environment, and Built-in properties. /// All other properties originate in XML, and should have a backing XML object. /// - internal ProjectPropertyNotXmlBacked(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved) + internal ProjectPropertyNotXmlBacked(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext) : base(project, evaluatedValueEscaped) { ErrorUtilities.VerifyThrowArgumentLength(name, nameof(name)); @@ -512,6 +524,7 @@ internal ProjectPropertyNotXmlBacked(Project project, string name, string evalua ErrorUtilities.VerifyThrowArgument(mayBeReserved || !ReservedPropertyNames.IsReservedProperty(name), "OM_ReservedName", name); _name = name; + this.loggingContext = loggingContext; } /// diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index da150f24f83..c7c8ceec1a5 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1197,7 +1197,7 @@ private void AddEnvironmentProperties() { foreach (ProjectPropertyInstance environmentProperty in _environmentProperties) { - _data.SetProperty(environmentProperty.Name, ((IProperty)environmentProperty).EvaluatedValueEscaped, isGlobalProperty: false, mayBeReserved: false, isEnvironmentVariable: true); + _data.SetProperty(environmentProperty.Name, ((IProperty)environmentProperty).EvaluatedValueEscaped, isGlobalProperty: false, mayBeReserved: false, isEnvironmentVariable: true, loggingContext: _evaluationLoggingContext); } } diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 9c3fa0f510d..1bacf84bbe7 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -1513,10 +1513,6 @@ private static object LookupProperty(IPropertyProvider properties, string pro else { propertyValue = property.EvaluatedValueEscaped; - if (!string.IsNullOrEmpty(property.EvaluatedValueEscaped) && property.IsEnvironmentProperty) - { - EnvironmentUtilities.EnvironmentVariablesUsedAsProperties[property.Name] = property.EvaluatedValueEscaped; - } } return propertyValue; diff --git a/src/Build/Evaluation/IEvaluatorData.cs b/src/Build/Evaluation/IEvaluatorData.cs index cf0b17d457f..497207e7024 100644 --- a/src/Build/Evaluation/IEvaluatorData.cs +++ b/src/Build/Evaluation/IEvaluatorData.cs @@ -267,7 +267,7 @@ List EvaluatedItemElements /// /// Sets a property which does not come from the Xml. /// - P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false); + P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false, BackEnd.Logging.LoggingContext loggingContext = null); /// /// Sets a property which comes from the Xml. diff --git a/src/Build/Evaluation/LazyItemEvaluator.EvaluatorData.cs b/src/Build/Evaluation/LazyItemEvaluator.EvaluatorData.cs index 0255a8fbf2d..ecae588cd20 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.EvaluatorData.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.EvaluatorData.cs @@ -9,6 +9,7 @@ using Microsoft.Build.Construction; using Microsoft.Build.Evaluation.Context; using Microsoft.Build.Execution; +using Microsoft.Build.BackEnd.Logging; #nullable disable @@ -307,9 +308,9 @@ public P SetProperty(ProjectPropertyElement propertyElement, string evaluatedVal return _wrappedData.SetProperty(propertyElement, evaluatedValueEscaped); } - public P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false) + public P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false, LoggingContext loggingContext = null) { - return _wrappedData.SetProperty(name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved); + return _wrappedData.SetProperty(name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, loggingContext: loggingContext); } } } diff --git a/src/Build/Evaluation/PropertyTrackingEvaluatorDataWrapper.cs b/src/Build/Evaluation/PropertyTrackingEvaluatorDataWrapper.cs index da81aaf21b5..b113e73a6ec 100644 --- a/src/Build/Evaluation/PropertyTrackingEvaluatorDataWrapper.cs +++ b/src/Build/Evaluation/PropertyTrackingEvaluatorDataWrapper.cs @@ -79,10 +79,10 @@ public P GetProperty(string name, int startIndex, int endIndex) /// /// Sets a property which does not come from the Xml. /// - public P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false) + public P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable = false, BackEnd.Logging.LoggingContext loggingContext = null) { P originalProperty = _wrapped.GetProperty(name); - P newProperty = _wrapped.SetProperty(name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, isEnvironmentVariable); + P newProperty = _wrapped.SetProperty(name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, isEnvironmentVariable, loggingContext); this.TrackPropertyWrite( originalProperty, diff --git a/src/Build/Instance/ProjectInstance.cs b/src/Build/Instance/ProjectInstance.cs index 6b8c88568c0..0237d7f0ad8 100644 --- a/src/Build/Instance/ProjectInstance.cs +++ b/src/Build/Instance/ProjectInstance.cs @@ -1475,10 +1475,10 @@ IItemDefinition IEvaluatorData - ProjectPropertyInstance IEvaluatorData.SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable) + ProjectPropertyInstance IEvaluatorData.SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvironmentVariable, LoggingContext loggingContext) { // Mutability not verified as this is being populated during evaluation - ProjectPropertyInstance property = ProjectPropertyInstance.Create(name, evaluatedValueEscaped, mayBeReserved, _isImmutable, isEnvironmentVariable); + ProjectPropertyInstance property = ProjectPropertyInstance.Create(name, evaluatedValueEscaped, mayBeReserved, _isImmutable, isEnvironmentVariable, loggingContext); _properties.Set(property); return property; } diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index 2ceac193e4b..7d6d24267f6 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -10,6 +10,8 @@ using Microsoft.Build.BackEnd; using ReservedPropertyNames = Microsoft.Build.Internal.ReservedPropertyNames; +using Microsoft.Build.BackEnd.Logging; +using Microsoft.Build.Framework; #nullable disable @@ -41,6 +43,8 @@ private ProjectPropertyInstance(string name, string escapedValue) _escapedValue = escapedValue; } + internal LoggingContext loggingContext; + /// /// Name of the property /// @@ -84,8 +88,20 @@ public string EvaluatedValue /// Setter assumes caller has protected global properties, if necessary. /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - string IProperty.EvaluatedValueEscaped => _escapedValue; - + string IProperty.EvaluatedValueEscaped + { + get + { + if ((this as IProperty).IsEnvironmentProperty && loggingContext is not null) + { + EnvironmentVariableReadEventArgs args = new(Name, _escapedValue); + loggingContext.LogBuildEvent(args); + loggingContext = null; + } + + return _escapedValue; + } + } /// /// Implementation of IKeyed exposing the property name /// @@ -184,9 +200,9 @@ internal static ProjectPropertyInstance Create(string name, string escapedValue, /// This flags should ONLY be set by the evaluator or by cloning; after the ProjectInstance is created, they must be illegal. /// If name is invalid or reserved, throws ArgumentException. /// - internal static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, bool isImmutable, bool isEnvironmentProperty = false) + internal static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, bool isImmutable, bool isEnvironmentProperty = false, LoggingContext loggingContext = null) { - return Create(name, escapedValue, mayBeReserved, null, isImmutable, isEnvironmentProperty); + return Create(name, escapedValue, mayBeReserved, null, isImmutable, isEnvironmentProperty, loggingContext); } /// @@ -280,7 +296,7 @@ internal ProjectPropertyElement ToProjectPropertyElement(ProjectElementContainer /// as it should never be needed for any subsequent messages, and is just extra bulk. /// Inherits mutability from project if any. /// - private static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, ElementLocation location, bool isImmutable, bool isEnvironmentProperty = false) + private static ProjectPropertyInstance Create(string name, string escapedValue, bool mayBeReserved, ElementLocation location, bool isImmutable, bool isEnvironmentProperty = false, LoggingContext loggingContext = null) { // Does not check immutability as this is only called during build (which is already protected) or evaluation ErrorUtilities.VerifyThrowArgumentNull(escapedValue, nameof(escapedValue)); @@ -299,6 +315,7 @@ private static ProjectPropertyInstance Create(string name, string escapedValue, ProjectPropertyInstance instance = isImmutable ? new ProjectPropertyInstanceImmutable(name, escapedValue) : new ProjectPropertyInstance(name, escapedValue); ((IProperty)instance).IsEnvironmentProperty = isEnvironmentProperty; + instance.loggingContext = loggingContext; return instance; } From 2a749b8bc1795d5911145034d57e691b5d49bfcd Mon Sep 17 00:00:00 2001 From: Forgind Date: Fri, 1 Apr 2022 14:30:52 -0700 Subject: [PATCH 10/37] Account for prior logging and EvaluationFinished --- src/Build/Definition/ProjectProperty.cs | 5 ++++- src/Build/Instance/ProjectPropertyInstance.cs | 6 ++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index 00e1391e239..a4cebe500c4 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -98,16 +98,19 @@ string IProperty.EvaluatedValueEscaped [DebuggerStepThrough] get { - if ((this as IProperty).IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext is not null) + if ((this as IProperty).IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext?.IsValid == true && !_loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, EvaluatedValueEscapedInternal); notXmlBacked.loggingContext.LogBuildEvent(args); + _loggedEnvProperty = true; } return EvaluatedValueEscapedInternal; } } + private bool _loggedEnvProperty = false; + /// /// Gets or sets the unevaluated property value. /// Updates the evaluated value in the project, although this is not sure to be correct until re-evaluation. diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index 7d6d24267f6..016f21df53e 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -83,6 +83,8 @@ public string EvaluatedValue /// public virtual bool IsImmutable => false; + private bool _loggedEnvProperty = false; + /// /// Evaluated value of the property, escaped as necessary. /// Setter assumes caller has protected global properties, if necessary. @@ -92,11 +94,11 @@ string IProperty.EvaluatedValueEscaped { get { - if ((this as IProperty).IsEnvironmentProperty && loggingContext is not null) + if ((this as IProperty).IsEnvironmentProperty && loggingContext?.IsValid == true && !_loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, _escapedValue); loggingContext.LogBuildEvent(args); - loggingContext = null; + _loggedEnvProperty = true; } return _escapedValue; From 150a4424376044e282c57c52fb253a6ae7991508 Mon Sep 17 00:00:00 2001 From: Forgind Date: Mon, 4 Apr 2022 11:34:02 -0700 Subject: [PATCH 11/37] Tweak tests --- .../Evaluation/Evaluator_Tests.cs | 20 ++++++--- src/Build.UnitTests/Utilities_Tests.cs | 43 +++++++------------ 2 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs b/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs index 06324b51b52..2e36789b6dd 100644 --- a/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs @@ -4531,7 +4531,9 @@ public void VerifyPropertyTrackingLoggingDefault() logger .AllBuildEvents .OfType() - .ShouldBeEmpty(); + .ShouldHaveSingleItem() + .EnvironmentVariableName + .ShouldBe("DEFINED_ENVIRONMENT_VARIABLE2"); logger .AllBuildEvents @@ -4560,7 +4562,9 @@ public void VerifyPropertyTrackingLoggingPropertyReassignment() logger .AllBuildEvents .OfType() - .ShouldBeEmpty(); + .ShouldHaveSingleItem() + .EnvironmentVariableName + .ShouldBe("DEFINED_ENVIRONMENT_VARIABLE2"); logger .AllBuildEvents @@ -4589,7 +4593,9 @@ public void VerifyPropertyTrackingLoggingNone() logger .AllBuildEvents .OfType() - .ShouldBeEmpty(); + .ShouldHaveSingleItem() + .EnvironmentVariableName + .ShouldBe("DEFINED_ENVIRONMENT_VARIABLE2"); logger .AllBuildEvents @@ -4618,7 +4624,9 @@ public void VerifyPropertyTrackingLoggingPropertyInitialValue() logger .AllBuildEvents .OfType() - .ShouldBeEmpty(); + .ShouldHaveSingleItem() + .EnvironmentVariableName + .ShouldBe("DEFINED_ENVIRONMENT_VARIABLE2"); logger .AllBuildEvents @@ -4702,7 +4710,9 @@ public void VerifyPropertyTrackingLoggingUninitializedPropertyRead() logger .AllBuildEvents .OfType() - .ShouldBeEmpty(); + .ShouldHaveSingleItem() + .EnvironmentVariableName + .ShouldBe("DEFINED_ENVIRONMENT_VARIABLE2"); logger .AllBuildEvents diff --git a/src/Build.UnitTests/Utilities_Tests.cs b/src/Build.UnitTests/Utilities_Tests.cs index c6f7af84e57..617fd118090 100644 --- a/src/Build.UnitTests/Utilities_Tests.cs +++ b/src/Build.UnitTests/Utilities_Tests.cs @@ -22,6 +22,7 @@ using System.Collections.Generic; using System.IO; using Xunit.Abstractions; +using Shouldly; #nullable disable @@ -80,40 +81,34 @@ public UtilitiesTestReadOnlyLoad() [Fact] public void CommentsInPreprocessing() { - Microsoft.Build.Construction.XmlDocumentWithLocation.ClearReadOnlyFlags_UnitTestsOnly(); - - string input = FileUtilities.GetTemporaryFile(); - string output = FileUtilities.GetTemporaryFile(); - - string _initialLoadFilesWriteable = Environment.GetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE"); - try + using (TestEnvironment env = TestEnvironment.Create()) { - Environment.SetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE", "1"); + XmlDocumentWithLocation.ClearReadOnlyFlags_UnitTestsOnly(); - string content = ObjectModelHelpers.CleanupFileContents(@" + TransientTestFile inputFile = env.CreateFile("tempInput.tmp", ObjectModelHelpers.CleanupFileContents(@" -"); - File.WriteAllText(input, content); +")); + TransientTestFile outputFile = env.CreateFile("tempOutput.tmp"); + + env.SetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE", "1"); #if FEATURE_GET_COMMANDLINE - Assert.Equal(MSBuildApp.ExitType.Success, MSBuildApp.Execute(@"c:\bin\msbuild.exe """ + input + - (NativeMethodsShared.IsUnixLike ? @""" -pp:""" : @""" /pp:""") + output + @"""")); + MSBuildApp.Execute(@"c:\bin\msbuild.exe """ + inputFile.Path + + (NativeMethodsShared.IsUnixLike ? @""" -pp:""" : @""" /pp:""") + outputFile.Path + @"""") + .ShouldBe(MSBuildApp.ExitType.Success); #else Assert.Equal( MSBuildApp.ExitType.Success, MSBuildApp.Execute( - new[] { @"c:\bin\msbuild.exe", '"' + input + '"', - '"' + (NativeMethodsShared.IsUnixLike ? "-pp:" : "/pp:") + output + '"'})); + new[] { @"c:\bin\msbuild.exe", '"' + inputFile.Path + '"', + '"' + (NativeMethodsShared.IsUnixLike ? "-pp:" : "/pp:") + outputFile.Path + '"'})); #endif bool foundDoNotModify = false; - foreach (string line in File.ReadLines(output)) + foreach (string line in File.ReadLines(outputFile.Path)) { - if (line.Contains("")) // This is what it will look like if we're loading read/only - { - Assert.True(false); - } + line.ShouldNotContain("", "This is what it will look like if we're loading read/only"); if (line.Contains("DO NOT MODIFY")) // this is in a comment in our targets { @@ -121,13 +116,7 @@ public void CommentsInPreprocessing() } } - Assert.True(foundDoNotModify); - } - finally - { - File.Delete(input); - File.Delete(output); - Environment.SetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE", _initialLoadFilesWriteable); + foundDoNotModify.ShouldBeTrue(); } } From dfe55d728503706abec9b83c6cdc8e7013c53221 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 7 Apr 2022 11:02:23 -0700 Subject: [PATCH 12/37] Random cleanup --- src/Build.UnitTests/Utilities_Tests.cs | 2 +- .../Components/Logging/LoggingService.cs | 44 ++++++------------- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/src/Build.UnitTests/Utilities_Tests.cs b/src/Build.UnitTests/Utilities_Tests.cs index 617fd118090..b4787dd1eb6 100644 --- a/src/Build.UnitTests/Utilities_Tests.cs +++ b/src/Build.UnitTests/Utilities_Tests.cs @@ -86,7 +86,7 @@ public void CommentsInPreprocessing() XmlDocumentWithLocation.ClearReadOnlyFlags_UnitTestsOnly(); TransientTestFile inputFile = env.CreateFile("tempInput.tmp", ObjectModelHelpers.CleanupFileContents(@" - + ")); TransientTestFile outputFile = env.CreateFile("tempOutput.tmp"); diff --git a/src/Build/BackEnd/Components/Logging/LoggingService.cs b/src/Build/BackEnd/Components/Logging/LoggingService.cs index 6f1f5152377..8cb996f6371 100644 --- a/src/Build/BackEnd/Components/Logging/LoggingService.cs +++ b/src/Build/BackEnd/Components/Logging/LoggingService.cs @@ -1154,19 +1154,15 @@ public void LogBuildEvent(BuildEventArgs buildEvent) { ErrorUtilities.VerifyThrow(buildEvent != null, "buildEvent is null"); - BuildWarningEventArgs warningEvent = null; - BuildErrorEventArgs errorEvent = null; - BuildMessageEventArgs messageEvent = null; - - if ((warningEvent = buildEvent as BuildWarningEventArgs) != null && warningEvent.BuildEventContext != null && warningEvent.BuildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId) + if (buildEvent is BuildWarningEventArgs warningEvent && warningEvent.BuildEventContext != null && warningEvent.BuildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId) { warningEvent.ProjectFile = GetAndVerifyProjectFileFromContext(warningEvent.BuildEventContext); } - else if ((errorEvent = buildEvent as BuildErrorEventArgs) != null && errorEvent.BuildEventContext != null && errorEvent.BuildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId) + else if (buildEvent is BuildErrorEventArgs errorEvent && errorEvent.BuildEventContext != null && errorEvent.BuildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId) { errorEvent.ProjectFile = GetAndVerifyProjectFileFromContext(errorEvent.BuildEventContext); } - else if ((messageEvent = buildEvent as BuildMessageEventArgs) != null && messageEvent.BuildEventContext != null && messageEvent.BuildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId) + else if (buildEvent is BuildMessageEventArgs messageEvent && messageEvent.BuildEventContext != null && messageEvent.BuildEventContext.ProjectContextId != BuildEventContext.InvalidProjectContextId) { messageEvent.ProjectFile = GetAndVerifyProjectFileFromContext(messageEvent.BuildEventContext); } @@ -1175,10 +1171,10 @@ public void LogBuildEvent(BuildEventArgs buildEvent) { // Only log certain events if OnlyLogCriticalEvents is true if ( - (warningEvent != null) - || (errorEvent != null) - || (buildEvent is CustomBuildEventArgs) - || (buildEvent is CriticalBuildMessageEventArgs) + buildEvent is BuildWarningEventArgs + || buildEvent is BuildErrorEventArgs + || buildEvent is CustomBuildEventArgs + || buildEvent is CriticalBuildMessageEventArgs ) { ProcessLoggingEvent(buildEvent); @@ -1451,20 +1447,8 @@ private void LoggingEventProcessor(object loggingEvent) /// private void RouteBuildEvent(object loggingEvent) { - BuildEventArgs buildEventArgs = null; - - if (loggingEvent is BuildEventArgs bea) - { - buildEventArgs = bea; - } - else if (loggingEvent is KeyValuePair kvp) - { - buildEventArgs = kvp.Value; - } - else - { - ErrorUtilities.ThrowInternalError("Unknown logging item in queue:" + loggingEvent.GetType().FullName); - } + BuildEventArgs buildEventArgs = loggingEvent as BuildEventArgs ?? (loggingEvent as KeyValuePair?)?.Value; + ErrorUtilities.VerifyThrow(buildEventArgs is not null, "Unknown logging item in queue:" + loggingEvent.GetType().FullName); if (buildEventArgs is BuildWarningEventArgs warningEvent) { @@ -1508,14 +1492,12 @@ private void RouteBuildEvent(object loggingEvent) }; } } - - if (loggingEvent is BuildErrorEventArgs errorEvent) + else if (loggingEvent is BuildErrorEventArgs errorEvent) { // Keep track of build submissions that have logged errors. If there is no build context, add BuildEventContext.InvalidSubmissionId. _buildSubmissionIdsThatHaveLoggedErrors.Add(errorEvent.BuildEventContext?.SubmissionId ?? BuildEventContext.InvalidSubmissionId); } - - if (loggingEvent is ProjectFinishedEventArgs projectFinishedEvent && projectFinishedEvent.BuildEventContext != null) + else if (loggingEvent is ProjectFinishedEventArgs projectFinishedEvent && projectFinishedEvent.BuildEventContext != null) { int key = GetWarningsAsErrorOrMessageKey(projectFinishedEvent); _warningsAsErrorsByProject?.Remove(key); @@ -1560,7 +1542,7 @@ private void RouteBuildEvent(BuildEventArgs eventArg) TryRaiseProjectStartedEvent(eventArg); // The event has not been through a filter yet. All events must go through a filter before they make it to a logger - if (_filterEventSource != null) // Loggers may not be registered + if (_filterEventSource != null) // Loggers may not be registered { // Send the event to the filter, the Consume will not return until all of the loggers which have registered to the event have process // them. @@ -1581,7 +1563,7 @@ private void RouteBuildEvent(BuildEventArgs eventArg) { if (!sink.HaveLoggedBuildStartedEvent) { - sink.Consume(eventArg, (int)pair.Key); + sink.Consume(eventArg, pair.Key); } // Reset the HaveLoggedBuildStarted event because no one else will be sending a build started event to any loggers at this time. From bb51b1fa2952d18111b14a346b8842f0662c17e8 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 7 Apr 2022 11:20:41 -0700 Subject: [PATCH 13/37] Make it work across nodes --- .../BuildEventArgsSerialization_Tests.cs | 11 +++ src/Build/Definition/ProjectProperty.cs | 1 + src/Build/Instance/ProjectPropertyInstance.cs | 1 + src/MSBuildTaskHost/MSBuildTaskHost.csproj | 5 +- src/Shared/LogMessagePacketBase.cs | 78 +++++++++++++------ 5 files changed, 70 insertions(+), 26 deletions(-) diff --git a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs index da9e6e8b383..19e4f1dbb46 100644 --- a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs +++ b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs @@ -168,6 +168,17 @@ public void RoundtripTaskStartedEventArgs() e => e.ColumnNumber.ToString()); } + [Fact] + public void RoundtripEnvironmentVariableReadEventArgs() + { + EnvironmentVariableReadEventArgs args = new("VarName", "VarValue"); + args.BuildEventContext = new BuildEventContext(4, 5, 6, 7); + Roundtrip(args, + e => e.Message, + e => e.EnvironmentVariableName, + e => e.BuildEventContext.ToString()); + } + [Fact] public void RoundtripTaskFinishedEventArgs() { diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index a4cebe500c4..971dafabfc6 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -101,6 +101,7 @@ string IProperty.EvaluatedValueEscaped if ((this as IProperty).IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext?.IsValid == true && !_loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, EvaluatedValueEscapedInternal); + args.BuildEventContext = notXmlBacked.loggingContext.BuildEventContext; notXmlBacked.loggingContext.LogBuildEvent(args); _loggedEnvProperty = true; } diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index 016f21df53e..72461b7e62d 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -97,6 +97,7 @@ string IProperty.EvaluatedValueEscaped if ((this as IProperty).IsEnvironmentProperty && loggingContext?.IsValid == true && !_loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, _escapedValue); + args.BuildEventContext = loggingContext.BuildEventContext; loggingContext.LogBuildEvent(args); _loggedEnvProperty = true; } diff --git a/src/MSBuildTaskHost/MSBuildTaskHost.csproj b/src/MSBuildTaskHost/MSBuildTaskHost.csproj index e7dba840e1a..abeb3a27d4c 100644 --- a/src/MSBuildTaskHost/MSBuildTaskHost.csproj +++ b/src/MSBuildTaskHost/MSBuildTaskHost.csproj @@ -1,4 +1,4 @@ - + @@ -39,6 +39,9 @@ BuildEnvironmentHelper.cs + + EnvironmentVariableReadEventArgs.cs + BuildEnvironmentState.cs diff --git a/src/Shared/LogMessagePacketBase.cs b/src/Shared/LogMessagePacketBase.cs index 7e592d7513d..2f6ba5dc23b 100644 --- a/src/Shared/LogMessagePacketBase.cs +++ b/src/Shared/LogMessagePacketBase.cs @@ -130,6 +130,11 @@ internal enum LoggingEventType : int /// Event is a TelemetryEventArgs /// Telemetry = 18, + + /// + /// Event is an EnvironmentVariableReadEventArgs + /// + EnvironmentVariableReadEvent = 19, } #endregion @@ -403,6 +408,7 @@ internal void ReadFromStream(ITranslator translator) else { _buildEvent = ReadEventFromStream(_eventType, translator); + ErrorUtilities.VerifyThrow(_buildEvent is not null, "Not Supported LoggingEventType {0}", _eventType.ToString()); } } else @@ -509,6 +515,7 @@ private BuildEventArgs GetBuildEventArgFromId() LoggingEventType.TaskStartedEvent => new TaskStartedEventArgs(null, null, null, null, null), LoggingEventType.TaskFinishedEvent => new TaskFinishedEventArgs(null, null, null, null, null, false), LoggingEventType.TaskCommandLineEvent => new TaskCommandLineEventArgs(null, null, MessageImportance.Normal), + LoggingEventType.EnvironmentVariableReadEvent => new EnvironmentVariableReadEventArgs(), #if !TASKHOST // MSBuildTaskHost is targeting Microsoft.Build.Framework.dll 3.5 LoggingEventType.TaskParameterEvent => new TaskParameterEventArgs(0, null, null, true, default), LoggingEventType.ProjectEvaluationStartedEvent => new ProjectEvaluationStartedEventArgs(), @@ -607,6 +614,10 @@ private LoggingEventType GetLoggingEventId(BuildEventArgs eventArg) { return LoggingEventType.BuildErrorEvent; } + else if (eventType == typeof(EnvironmentVariableReadEventArgs)) + { + return LoggingEventType.EnvironmentVariableReadEvent; + } else { return LoggingEventType.CustomEvent; @@ -661,12 +672,29 @@ private void WriteEventToStream(BuildEventArgs buildEvent, LoggingEventType even case LoggingEventType.ProjectFinishedEvent: WriteExternalProjectFinishedEventToStream((ExternalProjectFinishedEventArgs)buildEvent, translator); break; + case LoggingEventType.EnvironmentVariableReadEvent: + WriteEnvironmentVariableReadEventArgs((EnvironmentVariableReadEventArgs)buildEvent, translator); + break; default: ErrorUtilities.ThrowInternalError("Not Supported LoggingEventType {0}", eventType.ToString()); break; } } + /// + /// Serializes EnvironmentVariableRead Event argument to the stream. Does not work properly on TaskHosts due to BuildEventContext serialization not being + /// enabled on TaskHosts, but that shouldn't matter, as this should never be called from a TaskHost anyway. + /// + private void WriteEnvironmentVariableReadEventArgs(EnvironmentVariableReadEventArgs environmentVariableReadEventArgs, ITranslator translator) + { + string name = environmentVariableReadEventArgs.EnvironmentVariableName; + translator.Translate(ref name); + BuildEventContext context = environmentVariableReadEventArgs.BuildEventContext; +#if !CLR2COMPATIBILITY + translator.Translate(ref context); +#endif + } + /// /// Serialize ExternalProjectFinished Event Argument to the stream /// @@ -1001,33 +1029,33 @@ private BuildEventArgs ReadEventFromStream(LoggingEventType eventType, ITranslat translator.Translate(ref helpKeyword); translator.Translate(ref senderName); - BuildEventArgs buildEvent = null; - switch (eventType) + return eventType switch { - case LoggingEventType.TaskCommandLineEvent: - buildEvent = ReadTaskCommandLineEventFromStream(translator, message, helpKeyword, senderName); - break; - case LoggingEventType.BuildErrorEvent: - buildEvent = ReadTaskBuildErrorEventFromStream(translator, message, helpKeyword, senderName); - break; - case LoggingEventType.ProjectStartedEvent: - buildEvent = ReadExternalProjectStartedEventFromStream(translator, message, helpKeyword, senderName); - break; - case LoggingEventType.ProjectFinishedEvent: - buildEvent = ReadExternalProjectFinishedEventFromStream(translator, message, helpKeyword, senderName); - break; - case LoggingEventType.BuildMessageEvent: - buildEvent = ReadBuildMessageEventFromStream(translator, message, helpKeyword, senderName); - break; - case LoggingEventType.BuildWarningEvent: - buildEvent = ReadBuildWarningEventFromStream(translator, message, helpKeyword, senderName); - break; - default: - ErrorUtilities.ThrowInternalError("Not Supported LoggingEventType {0}", eventType.ToString()); - break; - } + LoggingEventType.TaskCommandLineEvent => ReadTaskCommandLineEventFromStream(translator, message, helpKeyword, senderName), + LoggingEventType.BuildErrorEvent => ReadTaskBuildErrorEventFromStream(translator, message, helpKeyword, senderName), + LoggingEventType.ProjectStartedEvent => ReadExternalProjectStartedEventFromStream(translator, message, helpKeyword, senderName), + LoggingEventType.ProjectFinishedEvent => ReadExternalProjectFinishedEventFromStream(translator, message, helpKeyword, senderName), + LoggingEventType.BuildMessageEvent => ReadBuildMessageEventFromStream(translator, message, helpKeyword, senderName), + LoggingEventType.BuildWarningEvent => ReadBuildWarningEventFromStream(translator, message, helpKeyword, senderName), + LoggingEventType.EnvironmentVariableReadEvent => ReadEnvironmentVariableReadEventFromStream(translator, message, helpKeyword, senderName), + _ => null, + }; + } - return buildEvent; + /// + /// Read and reconstruct an EnvironmentVariableReadEventArgs from the stream. This message should never be called from a TaskHost, so although the context translation does not work, that's ok. + /// + private EnvironmentVariableReadEventArgs ReadEnvironmentVariableReadEventFromStream(ITranslator translator, string message, string helpKeyword, string senderName) + { + string environmentVariableName = null; + translator.Translate(ref environmentVariableName); + BuildEventContext context = null; +#if !CLR2COMPATIBILITY + translator.Translate(ref context); +#endif + EnvironmentVariableReadEventArgs args = new(environmentVariableName, message); + args.BuildEventContext = context; + return args; } /// From 775a20de09f4448f99d981af5b7a2edf0919ec73 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 7 Apr 2022 17:27:49 -0700 Subject: [PATCH 14/37] EnvironmentVariableReadEventArgs serialization It can't serialize itself, currently. --- src/Shared/LogMessagePacketBase.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Shared/LogMessagePacketBase.cs b/src/Shared/LogMessagePacketBase.cs index 2f6ba5dc23b..bd010ed294f 100644 --- a/src/Shared/LogMessagePacketBase.cs +++ b/src/Shared/LogMessagePacketBase.cs @@ -328,7 +328,8 @@ internal void WriteToStream(ITranslator translator) #if !TASKHOST && !MSBUILDENTRYPOINTEXE if (_buildEvent is ProjectEvaluationStartedEventArgs || - _buildEvent is ProjectEvaluationFinishedEventArgs) + _buildEvent is ProjectEvaluationFinishedEventArgs || + _buildEvent is EnvironmentVariableReadEventArgs) { // switch to serialization methods that we provide in this file // and don't use the WriteToStream inherited from LazyFormattedBuildEventArgs From 2fc3b004decfa1cd2f231a1886f33e50ec22db85 Mon Sep 17 00:00:00 2001 From: Forgind Date: Mon, 11 Apr 2022 16:58:37 -0700 Subject: [PATCH 15/37] Self comments --- .../Logging/BinaryLogger/BinaryLogger.cs | 2 +- .../BinaryLogger/BuildEventArgsReader.cs | 4 +- .../BinaryLogger/BuildEventArgsWriter.cs | 1 - src/Framework/BuildFinishedEventArgs.cs | 37 +------------------ .../PublicAPI/net/PublicAPI.Unshipped.txt | 2 - .../netstandard/PublicAPI.Unshipped.txt | 2 - src/Shared/EnvironmentUtilities.cs | 4 -- 7 files changed, 3 insertions(+), 49 deletions(-) diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 0cf98ee1bb8..7edb0ae0447 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -55,7 +55,7 @@ public sealed class BinaryLogger : ILogger // - TargetSkippedEventArgs: added SkipReason, OriginalBuildEventContext // version 15: // - Don't log all environment variables at BuildStarted - // - Log environment variables accessed as properties at BuildFinished + // - Log environment variables accessed as properties via EnvironmentVariableReadEventArgs internal const int FileFormatVersion = 15; private Stream stream; diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs index ab422aca622..64a26269a78 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs @@ -362,14 +362,12 @@ private BuildEventArgs ReadBuildFinishedEventArgs() { var fields = ReadBuildEventArgsFields(); var succeeded = ReadBoolean(); - var environmentProperties = fileFormatVersion >= 15 ? ReadStringDictionary() : null; var e = new BuildFinishedEventArgs( fields.Message, fields.HelpKeyword, succeeded, - fields.Timestamp, - environmentVariables: environmentProperties); + fields.Timestamp); SetCommonFields(e, fields); return e; } diff --git a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs index b5737686fef..1089888ff12 100644 --- a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs +++ b/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs @@ -262,7 +262,6 @@ private void Write(BuildFinishedEventArgs e) Write(BinaryLogRecordKind.BuildFinished); WriteBuildEventArgsFields(e); Write(e.Succeeded); - Write(EnvironmentUtilities.EnvironmentVariablesUsedAsProperties); } private void Write(ProjectEvaluationStartedEventArgs e) diff --git a/src/Framework/BuildFinishedEventArgs.cs b/src/Framework/BuildFinishedEventArgs.cs index 54224f38b52..1647591416c 100644 --- a/src/Framework/BuildFinishedEventArgs.cs +++ b/src/Framework/BuildFinishedEventArgs.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; -using System.Collections.Generic; using System.IO; #nullable disable @@ -26,11 +25,6 @@ public class BuildFinishedEventArgs : BuildStatusEventArgs /// private bool succeeded; - /// - /// Environment variable-derived properties - /// - private IDictionary environmentVariables; - /// /// Default constructor /// @@ -71,7 +65,7 @@ public BuildFinishedEventArgs bool succeeded, DateTime eventTimestamp ) - : this(message, helpKeyword, succeeded, eventTimestamp, messageArgs: null) + : this(message, helpKeyword, succeeded, eventTimestamp, null) { // do nothing } @@ -97,30 +91,6 @@ params object[] messageArgs this.succeeded = succeeded; } - /// - /// Constructor which allows environment variable-derived properties to be set - /// - /// text message - /// help keyword - /// True indicates a successful build - /// Timestamp when the event was created - /// Properties derived from environment variables - /// message arguments - public BuildFinishedEventArgs - ( - string message, - string helpKeyword, - bool succeeded, - DateTime eventTimestamp, - IDictionary environmentVariables, - params object[] messageArgs - ) - : base(message, helpKeyword, "MSBuild", eventTimestamp, messageArgs) - { - this.succeeded = succeeded; - this.environmentVariables = environmentVariables; - } - #region CustomSerializationToStream /// @@ -155,10 +125,5 @@ public bool Succeeded return succeeded; } } - - /// - /// Gets all environment variables read when trying to evaluate properties along with their values. - /// - public IDictionary EnvironmentVariables => environmentVariables; } } diff --git a/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt b/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt index 813fa92a36d..e69de29bb2d 100644 --- a/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt +++ b/src/Framework/PublicAPI/net/PublicAPI.Unshipped.txt @@ -1,2 +0,0 @@ -Microsoft.Build.Framework.BuildFinishedEventArgs.BuildFinishedEventArgs(string message, string helpKeyword, bool succeeded, System.DateTime eventTimestamp, System.Collections.Generic.IDictionary environmentVariables, params object[] messageArgs) -> void -Microsoft.Build.Framework.BuildFinishedEventArgs.EnvironmentVariables.get -> System.Collections.Generic.IDictionary \ No newline at end of file diff --git a/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt b/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt index 813fa92a36d..e69de29bb2d 100644 --- a/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt +++ b/src/Framework/PublicAPI/netstandard/PublicAPI.Unshipped.txt @@ -1,2 +0,0 @@ -Microsoft.Build.Framework.BuildFinishedEventArgs.BuildFinishedEventArgs(string message, string helpKeyword, bool succeeded, System.DateTime eventTimestamp, System.Collections.Generic.IDictionary environmentVariables, params object[] messageArgs) -> void -Microsoft.Build.Framework.BuildFinishedEventArgs.EnvironmentVariables.get -> System.Collections.Generic.IDictionary \ No newline at end of file diff --git a/src/Shared/EnvironmentUtilities.cs b/src/Shared/EnvironmentUtilities.cs index 97b4e53a1a4..01cf349f2f8 100644 --- a/src/Shared/EnvironmentUtilities.cs +++ b/src/Shared/EnvironmentUtilities.cs @@ -2,9 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; -using System.Collections.Generic; using System.Runtime.InteropServices; -using Microsoft.Build.Collections; namespace Microsoft.Build.Shared { @@ -14,7 +12,5 @@ internal static partial class EnvironmentUtilities public static bool Is64BitOperatingSystem => Environment.Is64BitOperatingSystem; - - public static Dictionary EnvironmentVariablesUsedAsProperties { get; } = new(MSBuildNameIgnoreCaseComparer.Default); } } From 4c0f001c6f1a2c16c1ca6683e19c3e83f14b01c8 Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 13 Apr 2022 10:21:38 -0700 Subject: [PATCH 16/37] PR comments --- .../BuildEventArgsSerialization_Tests.cs | 47 ++++++++++++------- src/Build/Definition/Project.cs | 2 +- .../Logging/BinaryLogger/BinaryLogger.cs | 5 +- 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs index 19e4f1dbb46..9b9deeef04a 100644 --- a/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs +++ b/src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs @@ -24,25 +24,36 @@ public BuildEventArgsSerializationTests() _ = ItemGroupLoggingHelper.ItemGroupIncludeLogMessagePrefix; } - [Fact] - public void RoundtripBuildStartedEventArgs() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void RoundtripBuildStartedEventArgs(bool serializeAllEnvironmentVariables) { - var args = new BuildStartedEventArgs( - "Message", - "HelpKeyword", - DateTime.Parse("3/1/2017 11:11:56 AM")); - Roundtrip(args, - e => e.Message, - e => e.HelpKeyword, - e => e.Timestamp.ToString()); - - args = new BuildStartedEventArgs( - "M", - null); - Roundtrip(args, - e => e.HelpKeyword, - e => e.ThreadId.ToString(), - e => e.SenderName); + using (TestEnvironment env = TestEnvironment.Create()) + { + env.SetEnvironmentVariable("MSBUILDLOGALLENVIRONMENTVARIABLES", serializeAllEnvironmentVariables ? "1" : null); + var args = new BuildStartedEventArgs( + "Message", + "HelpKeyword", + DateTime.Parse("3/1/2017 11:11:56 AM")); + Roundtrip(args, + e => e.Message, + e => e.HelpKeyword, + e => e.Timestamp.ToString()); + + args = new BuildStartedEventArgs( + "M", + null, + new Dictionary + { + { "SampleName", "SampleValue" } + }); + Roundtrip(args, + e => serializeAllEnvironmentVariables ? TranslationHelpers.ToString(e.BuildEnvironment) : null, + e => e.HelpKeyword, + e => e.ThreadId.ToString(), + e => e.SenderName); + } } [Fact] diff --git a/src/Build/Definition/Project.cs b/src/Build/Definition/Project.cs index 5a1a7ce7e18..7d0542f6e56 100644 --- a/src/Build/Definition/Project.cs +++ b/src/Build/Definition/Project.cs @@ -2923,7 +2923,7 @@ public override bool SetGlobalProperty(string name, string escapedValue) string originalValue = (existing == null) ? String.Empty : ((IProperty)existing).EvaluatedValueEscaped; _data.GlobalPropertiesDictionary.Set(ProjectPropertyInstance.Create(name, escapedValue)); - _data.Properties.Set(ProjectProperty.Create(Owner, name, escapedValue, true /* is global */, false /* may not be reserved name */, null)); + _data.Properties.Set(ProjectProperty.Create(Owner, name, escapedValue, isGlobalProperty: true, mayBeReserved: false, loggingContext: null)); ProjectCollection.AfterUpdateLoadedProjectGlobalProperties(Owner); MarkDirty(); diff --git a/src/Build/Logging/BinaryLogger/BinaryLogger.cs b/src/Build/Logging/BinaryLogger/BinaryLogger.cs index 7edb0ae0447..7fbddddf7ee 100644 --- a/src/Build/Logging/BinaryLogger/BinaryLogger.cs +++ b/src/Build/Logging/BinaryLogger/BinaryLogger.cs @@ -53,10 +53,7 @@ public sealed class BinaryLogger : ILogger // - TargetSkippedEventArgs: added OriginallySucceeded, Condition, EvaluatedCondition // version 14: // - TargetSkippedEventArgs: added SkipReason, OriginalBuildEventContext - // version 15: - // - Don't log all environment variables at BuildStarted - // - Log environment variables accessed as properties via EnvironmentVariableReadEventArgs - internal const int FileFormatVersion = 15; + internal const int FileFormatVersion = 14; private Stream stream; private BinaryWriter binaryWriter; From 30cb0e1365764f67ea35cd2adfa5d8624921f1da Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 13 Apr 2022 14:28:37 -0700 Subject: [PATCH 17/37] Remove IProperty.IsEnvironmentProperty --- src/Build/Definition/ProjectProperty.cs | 4 +--- src/Build/Evaluation/IProperty.cs | 6 ------ src/Build/Instance/ProjectPropertyInstance.cs | 12 ++++++------ 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index 971dafabfc6..ae8dfac6675 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -98,7 +98,7 @@ string IProperty.EvaluatedValueEscaped [DebuggerStepThrough] get { - if ((this as IProperty).IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext?.IsValid == true && !_loggedEnvProperty) + if (IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext?.IsValid == true && !_loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, EvaluatedValueEscapedInternal); args.BuildEventContext = notXmlBacked.loggingContext.BuildEventContext; @@ -219,8 +219,6 @@ string IValued.EscapedValue get => EvaluatedValueEscapedInternal; } - bool IProperty.IsEnvironmentProperty { get => IsEnvironmentProperty; set => throw new NotImplementedException(); } - #region IEquatable Members /// diff --git a/src/Build/Evaluation/IProperty.cs b/src/Build/Evaluation/IProperty.cs index a5342841b89..ed57eef4ea3 100644 --- a/src/Build/Evaluation/IProperty.cs +++ b/src/Build/Evaluation/IProperty.cs @@ -35,11 +35,5 @@ string EvaluatedValueEscaped { get; } - - bool IsEnvironmentProperty - { - get; - set; - } } } diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index 72461b7e62d..859c280a704 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -85,6 +85,8 @@ public string EvaluatedValue private bool _loggedEnvProperty = false; + internal bool IsEnvironmentProperty { get; set; } + /// /// Evaluated value of the property, escaped as necessary. /// Setter assumes caller has protected global properties, if necessary. @@ -94,7 +96,7 @@ string IProperty.EvaluatedValueEscaped { get { - if ((this as IProperty).IsEnvironmentProperty && loggingContext?.IsValid == true && !_loggedEnvProperty) + if (IsEnvironmentProperty && loggingContext?.IsValid == true && !_loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, _escapedValue); args.BuildEventContext = loggingContext.BuildEventContext; @@ -117,8 +119,6 @@ string IProperty.EvaluatedValueEscaped [DebuggerBrowsable(DebuggerBrowsableState.Never)] string IValued.EscapedValue => _escapedValue; - bool IProperty.IsEnvironmentProperty { get; set; } - #region IEquatable Members /// @@ -233,7 +233,7 @@ internal static ProjectPropertyInstance Create(string name, string escapedValue, /// internal static ProjectPropertyInstance Create(ProjectPropertyInstance that) { - return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: that.IsImmutable, ((IProperty)that).IsEnvironmentProperty); + return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: that.IsImmutable, that.IsEnvironmentProperty); } /// @@ -242,7 +242,7 @@ internal static ProjectPropertyInstance Create(ProjectPropertyInstance that) /// internal static ProjectPropertyInstance Create(ProjectPropertyInstance that, bool isImmutable) { - return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: isImmutable, ((IProperty)that).IsEnvironmentProperty); + return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: isImmutable, that.IsEnvironmentProperty); } /// @@ -317,7 +317,7 @@ private static ProjectPropertyInstance Create(string name, string escapedValue, } ProjectPropertyInstance instance = isImmutable ? new ProjectPropertyInstanceImmutable(name, escapedValue) : new ProjectPropertyInstance(name, escapedValue); - ((IProperty)instance).IsEnvironmentProperty = isEnvironmentProperty; + instance.IsEnvironmentProperty = isEnvironmentProperty; instance.loggingContext = loggingContext; return instance; } From d13ef4433589b043a49912402bc30ab04ed590b6 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 14 Apr 2022 13:29:01 -0700 Subject: [PATCH 18/37] Reduce allocations in mainline case --- src/Build/Definition/ProjectProperty.cs | 28 +++++++---- src/Build/Instance/ProjectPropertyInstance.cs | 46 +++++++++++++------ 2 files changed, 49 insertions(+), 25 deletions(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index ae8dfac6675..b069a4988d3 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -98,20 +98,18 @@ string IProperty.EvaluatedValueEscaped [DebuggerStepThrough] get { - if (IsEnvironmentProperty && this is ProjectPropertyNotXmlBacked notXmlBacked && notXmlBacked.loggingContext?.IsValid == true && !_loggedEnvProperty) + if (this is EnvironmentDerivedProjectProperty environmentProperty && environmentProperty.loggingContext?.IsValid == true && !environmentProperty._loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, EvaluatedValueEscapedInternal); - args.BuildEventContext = notXmlBacked.loggingContext.BuildEventContext; - notXmlBacked.loggingContext.LogBuildEvent(args); - _loggedEnvProperty = true; + args.BuildEventContext = environmentProperty.loggingContext.BuildEventContext; + environmentProperty.loggingContext.LogBuildEvent(args); + environmentProperty._loggedEnvProperty = true; } return EvaluatedValueEscapedInternal; } } - private bool _loggedEnvProperty = false; - /// /// Gets or sets the unevaluated property value. /// Updates the evaluated value in the project, although this is not sure to be correct until re-evaluation. @@ -254,7 +252,8 @@ bool IEquatable.Equals(ProjectProperty other) /// internal static ProjectProperty Create(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext = null) { - return new ProjectPropertyNotXmlBacked(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, loggingContext); + return !isGlobalProperty && !mayBeReserved ? new EnvironmentDerivedProjectProperty(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved, loggingContext) : + new ProjectPropertyNotXmlBacked(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved); } /// @@ -509,7 +508,6 @@ private class ProjectPropertyNotXmlBacked : ProjectProperty /// Name of the property. /// private readonly string _name; - internal LoggingContext loggingContext; /// /// Creates a property without backing XML. @@ -517,7 +515,7 @@ private class ProjectPropertyNotXmlBacked : ProjectProperty /// This is ONLY to be used by the Evaluator (and Project.SetGlobalProperty) and ONLY for Global, Environment, and Built-in properties. /// All other properties originate in XML, and should have a backing XML object. /// - internal ProjectPropertyNotXmlBacked(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext) + internal ProjectPropertyNotXmlBacked(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved) : base(project, evaluatedValueEscaped) { ErrorUtilities.VerifyThrowArgumentLength(name, nameof(name)); @@ -526,7 +524,6 @@ internal ProjectPropertyNotXmlBacked(Project project, string name, string evalua ErrorUtilities.VerifyThrowArgument(mayBeReserved || !ReservedPropertyNames.IsReservedProperty(name), "OM_ReservedName", name); _name = name; - this.loggingContext = loggingContext; } /// @@ -645,5 +642,16 @@ public override bool IsImported get { return false; } } } + + private class EnvironmentDerivedProjectProperty : ProjectPropertyNotXmlBacked + { + internal bool _loggedEnvProperty = false; + internal LoggingContext loggingContext; + + internal EnvironmentDerivedProjectProperty(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext) : base(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved) + { + this.loggingContext = loggingContext; + } + } } } diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index 859c280a704..a22fc191aca 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -43,8 +43,6 @@ private ProjectPropertyInstance(string name, string escapedValue) _escapedValue = escapedValue; } - internal LoggingContext loggingContext; - /// /// Name of the property /// @@ -83,10 +81,6 @@ public string EvaluatedValue /// public virtual bool IsImmutable => false; - private bool _loggedEnvProperty = false; - - internal bool IsEnvironmentProperty { get; set; } - /// /// Evaluated value of the property, escaped as necessary. /// Setter assumes caller has protected global properties, if necessary. @@ -96,12 +90,12 @@ string IProperty.EvaluatedValueEscaped { get { - if (IsEnvironmentProperty && loggingContext?.IsValid == true && !_loggedEnvProperty) + if (this is EnvironmentDerivedProjectPropertyInstance envProperty && envProperty.loggingContext?.IsValid == true && !envProperty._loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, _escapedValue); - args.BuildEventContext = loggingContext.BuildEventContext; - loggingContext.LogBuildEvent(args); - _loggedEnvProperty = true; + args.BuildEventContext = envProperty.loggingContext.BuildEventContext; + envProperty.loggingContext.LogBuildEvent(args); + envProperty._loggedEnvProperty = true; } return _escapedValue; @@ -233,7 +227,7 @@ internal static ProjectPropertyInstance Create(string name, string escapedValue, /// internal static ProjectPropertyInstance Create(ProjectPropertyInstance that) { - return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: that.IsImmutable, that.IsEnvironmentProperty); + return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: that.IsImmutable, that is EnvironmentDerivedProjectPropertyInstance); } /// @@ -242,7 +236,7 @@ internal static ProjectPropertyInstance Create(ProjectPropertyInstance that) /// internal static ProjectPropertyInstance Create(ProjectPropertyInstance that, bool isImmutable) { - return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: isImmutable, that.IsEnvironmentProperty); + return Create(that._name, that._escapedValue, mayBeReserved: true /* already validated */, isImmutable: isImmutable, that is EnvironmentDerivedProjectPropertyInstance); } /// @@ -316,9 +310,9 @@ private static ProjectPropertyInstance Create(string name, string escapedValue, XmlUtilities.VerifyThrowProjectValidElementName(name, location); } - ProjectPropertyInstance instance = isImmutable ? new ProjectPropertyInstanceImmutable(name, escapedValue) : new ProjectPropertyInstance(name, escapedValue); - instance.IsEnvironmentProperty = isEnvironmentProperty; - instance.loggingContext = loggingContext; + ProjectPropertyInstance instance = isEnvironmentProperty ? new EnvironmentDerivedProjectPropertyInstance(name, escapedValue, loggingContext) : + isImmutable ? new ProjectPropertyInstanceImmutable(name, escapedValue) : + new ProjectPropertyInstance(name, escapedValue); return instance; } @@ -347,5 +341,27 @@ internal ProjectPropertyInstanceImmutable(string name, string escapedValue) /// public override bool IsImmutable => true; } + + private class EnvironmentDerivedProjectPropertyInstance : ProjectPropertyInstance + { + internal EnvironmentDerivedProjectPropertyInstance(string name, string escapedValue, LoggingContext loggingContext) + : base(name, escapedValue) + { + this.loggingContext = loggingContext; + } + + /// + /// Whether this object can be changed. An immutable object cannot be made mutable. + /// + /// + /// The environment is captured at the start of the build, so environment-derived + /// properties can't change. + /// + public override bool IsImmutable => true; + + internal bool _loggedEnvProperty = false; + + internal LoggingContext loggingContext; + } } } From 01d33625bda90f082d09c357ed00a93ac03054de Mon Sep 17 00:00:00 2001 From: Forgind Date: Fri, 15 Apr 2022 12:03:12 -0700 Subject: [PATCH 19/37] Break up long line --- src/Build/Definition/ProjectProperty.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index b069a4988d3..2587bacfd8e 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -648,7 +648,9 @@ private class EnvironmentDerivedProjectProperty : ProjectPropertyNotXmlBacked internal bool _loggedEnvProperty = false; internal LoggingContext loggingContext; - internal EnvironmentDerivedProjectProperty(Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext) : base(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved) + internal EnvironmentDerivedProjectProperty( + Project project, string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, LoggingContext loggingContext) + : base(project, name, evaluatedValueEscaped, isGlobalProperty, mayBeReserved) { this.loggingContext = loggingContext; } From 75dc9dd85d91cec776f20ebb7f3c3003289f7325 Mon Sep 17 00:00:00 2001 From: Forgind Date: Mon, 18 Apr 2022 15:14:41 -0700 Subject: [PATCH 20/37] Make text log align with binlog --- .../Components/Logging/LoggingServiceLogMethods.cs | 11 ++++------- .../Logging/ParallelLogger/ParallelConsoleLogger.cs | 11 ++++++++--- src/Build/Logging/SerialConsoleLogger.cs | 6 ++++-- src/Shared/EventArgsFormatting.cs | 10 ++++++---- src/Utilities/MuxLogger.cs | 2 +- 5 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs b/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs index e1906fde381..0fabc28ed8e 100644 --- a/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs +++ b/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs @@ -429,14 +429,11 @@ public void LogBuildStarted() message = ResourceUtilities.GetResourceString("BuildStarted"); } - IDictionary environmentProperties = null; + IDictionary environmentProperties = _componentHost?.BuildParameters != null && Traits.Instance.LogAllEnvironmentVariables ? + _componentHost.BuildParameters.BuildProcessEnvironment + : null; - if (_componentHost?.BuildParameters != null) - { - environmentProperties = _componentHost.BuildParameters.BuildProcessEnvironment; - } - - BuildStartedEventArgs buildEvent = new BuildStartedEventArgs(message, null /* no help keyword */, environmentProperties); + BuildStartedEventArgs buildEvent = new(message, helpKeyword: null, environmentProperties); // Raise the event with the filters ProcessLoggingEvent(buildEvent); diff --git a/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs b/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs index b0d0c7eb7b3..accc1fcfa53 100644 --- a/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs +++ b/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs @@ -1204,16 +1204,21 @@ private void DisplayDeferredStartedEvents(BuildEventContext e) /// private void PrintMessage(BuildMessageEventArgs e, bool lightenText) { - string nonNullMessage; + string nonNullMessage = null; + + if (e is EnvironmentVariableReadEventArgs environmentPropertyReadEventArgs) + { + nonNullMessage = $"Property {environmentPropertyReadEventArgs.EnvironmentVariableName} with value {e.Message} expanded from the environment."; + } // Include file information if present. if (e.File != null) { - nonNullMessage = EventArgsFormatting.FormatEventMessage(e, showProjectFile, FindLogOutputProperties(e)); + nonNullMessage = EventArgsFormatting.FormatEventMessage(e, showProjectFile, FindLogOutputProperties(e), nonNullMessage); } else { - nonNullMessage = e.Message ?? string.Empty; + nonNullMessage ??= e.Message ?? string.Empty; } int prefixAdjustment = 0; diff --git a/src/Build/Logging/SerialConsoleLogger.cs b/src/Build/Logging/SerialConsoleLogger.cs index e677526406b..0e1c5100f64 100644 --- a/src/Build/Logging/SerialConsoleLogger.cs +++ b/src/Build/Logging/SerialConsoleLogger.cs @@ -511,7 +511,9 @@ public override void MessageHandler(object sender, BuildMessageEventArgs e) setColor(ConsoleColor.DarkGray); } - string nonNullMessage; + string nonNullMessage = e is EnvironmentVariableReadEventArgs environmentDerivedProperty ? + $"Property {environmentDerivedProperty.EnvironmentVariableName} with value {e.Message} expanded from the environment." + : null; // Include file information if present. if (e.File != null) @@ -521,7 +523,7 @@ public override void MessageHandler(object sender, BuildMessageEventArgs e) else { // null messages are ok -- treat as blank line - nonNullMessage = e.Message ?? String.Empty; + nonNullMessage ??= e.Message ?? String.Empty; } WriteLinePretty(nonNullMessage); diff --git a/src/Shared/EventArgsFormatting.cs b/src/Shared/EventArgsFormatting.cs index e3bcdf9fcba..7c0db803f70 100644 --- a/src/Shared/EventArgsFormatting.cs +++ b/src/Shared/EventArgsFormatting.cs @@ -53,10 +53,11 @@ internal static string FormatEventMessage(BuildWarningEventArgs e, bool showProj /// Message to format /// true to show the project file which issued the event, otherwise false. /// Properties to Print along with message + /// The complete message (including property name) for an environment-derived property /// The formatted message string. - internal static string FormatEventMessage(BuildMessageEventArgs e, bool showProjectFile, string projectConfigurationDescription) + internal static string FormatEventMessage(BuildMessageEventArgs e, bool showProjectFile, string projectConfigurationDescription, string nonNullMessage = null) { - return FormatEventMessage("message", e.Subcategory, e.Message, + return FormatEventMessage("message", e.Subcategory, nonNullMessage ?? e.Message, e.Code, e.File, showProjectFile ? e.ProjectFile : null, e.LineNumber, e.EndLineNumber, e.ColumnNumber, e.EndColumnNumber, e.ThreadId, projectConfigurationDescription); } @@ -144,13 +145,14 @@ internal static string FormatEventMessage(BuildMessageEventArgs e) /// /// Message to format /// Show project file or not + /// For an EnvironmentVariableReadEventArgs, adds an explanatory note and the name of the variable. /// The formatted message string. - internal static string FormatEventMessage(BuildMessageEventArgs e, bool showProjectFile) + internal static string FormatEventMessage(BuildMessageEventArgs e, bool showProjectFile, string nonNullMessage = null) { ErrorUtilities.VerifyThrowArgumentNull(e, nameof(e)); // "message" should not be localized - return FormatEventMessage("message", e.Subcategory, e.Message, + return FormatEventMessage("message", e.Subcategory, nonNullMessage ?? e.Message, e.Code, e.File, showProjectFile ? e.ProjectFile : null, e.LineNumber, e.EndLineNumber, e.ColumnNumber, e.EndColumnNumber, e.ThreadId, null); } diff --git a/src/Utilities/MuxLogger.cs b/src/Utilities/MuxLogger.cs index 2629634f436..f9532864e15 100644 --- a/src/Utilities/MuxLogger.cs +++ b/src/Utilities/MuxLogger.cs @@ -862,7 +862,7 @@ private void RaiseProjectStartedEvent(object sender, ProjectStartedEventArgs bui _firstProjectStartedEventContext = buildEvent.BuildEventContext; // We've never seen a project started event, so raise the build started event and save this project started event. - BuildStartedEventArgs startedEvent = new BuildStartedEventArgs(_buildStartedEvent.Message, _buildStartedEvent.HelpKeyword, _buildStartedEvent.BuildEnvironment); + BuildStartedEventArgs startedEvent = new BuildStartedEventArgs(_buildStartedEvent.Message, _buildStartedEvent.HelpKeyword, Traits.Instance.LogAllEnvironmentVariables ? _buildStartedEvent.BuildEnvironment : null); RaiseBuildStartedEvent(sender, startedEvent); } From 7914d878e52d4b778714c3e25d9f72856d930d88 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 26 Apr 2022 16:46:46 -0700 Subject: [PATCH 21/37] PR feedback --- src/Build/Definition/ProjectProperty.cs | 6 +++--- src/Build/Evaluation/Evaluator.cs | 15 ++++++++++++++- src/Build/Instance/ProjectPropertyInstance.cs | 2 +- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/Build/Definition/ProjectProperty.cs b/src/Build/Definition/ProjectProperty.cs index 2587bacfd8e..157fc4366ec 100644 --- a/src/Build/Definition/ProjectProperty.cs +++ b/src/Build/Definition/ProjectProperty.cs @@ -98,11 +98,11 @@ string IProperty.EvaluatedValueEscaped [DebuggerStepThrough] get { - if (this is EnvironmentDerivedProjectProperty environmentProperty && environmentProperty.loggingContext?.IsValid == true && !environmentProperty._loggedEnvProperty) + if (this is EnvironmentDerivedProjectProperty environmentProperty && environmentProperty.loggingContext is { IsValid: true } loggingContext && !environmentProperty._loggedEnvProperty) { EnvironmentVariableReadEventArgs args = new(Name, EvaluatedValueEscapedInternal); - args.BuildEventContext = environmentProperty.loggingContext.BuildEventContext; - environmentProperty.loggingContext.LogBuildEvent(args); + args.BuildEventContext = loggingContext.BuildEventContext; + loggingContext.LogBuildEvent(args); environmentProperty._loggedEnvProperty = true; } diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index c7c8ceec1a5..489261e6349 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -32,6 +32,7 @@ using EngineFileUtilities = Microsoft.Build.Internal.EngineFileUtilities; using ReservedPropertyNames = Microsoft.Build.Internal.ReservedPropertyNames; using SdkReferencePropertyExpansionMode = Microsoft.Build.Framework.EscapeHatches.SdkReferencePropertyExpansionMode; +using static Microsoft.Build.Execution.ProjectPropertyInstance; #nullable disable @@ -811,13 +812,25 @@ private void Evaluate() if (this._evaluationLoggingContext.LoggingService.IncludeEvaluationPropertiesAndItems) { globalProperties = _data.GlobalPropertiesDictionary; - properties = _data.Properties; + properties = Traits.Instance.LogAllEnvironmentVariables ? _data.Properties : FilterOutEnvironmentDerivedProperties(_data.Properties); items = _data.Items; } _evaluationLoggingContext.LogProjectEvaluationFinished(globalProperties, properties, items, _evaluationProfiler.ProfiledResult); } + private IEnumerable FilterOutEnvironmentDerivedProperties(PropertyDictionary

dictionary) + { + foreach (P p in dictionary) + { + if (!((p is ProjectProperty pp && pp.IsEnvironmentProperty) || + (p is EnvironmentDerivedProjectPropertyInstance))) + { + yield return p; + } + } + } + private void CollectProjectCachePlugins() { foreach (var item in _data.GetItems(ItemTypeNames.ProjectCachePlugin)) diff --git a/src/Build/Instance/ProjectPropertyInstance.cs b/src/Build/Instance/ProjectPropertyInstance.cs index a22fc191aca..c6fd6293e5d 100644 --- a/src/Build/Instance/ProjectPropertyInstance.cs +++ b/src/Build/Instance/ProjectPropertyInstance.cs @@ -342,7 +342,7 @@ internal ProjectPropertyInstanceImmutable(string name, string escapedValue) public override bool IsImmutable => true; } - private class EnvironmentDerivedProjectPropertyInstance : ProjectPropertyInstance + internal class EnvironmentDerivedProjectPropertyInstance : ProjectPropertyInstance { internal EnvironmentDerivedProjectPropertyInstance(string name, string escapedValue, LoggingContext loggingContext) : base(name, escapedValue) From 47d80f67daa8d46dbd51863566ee6d341d51ff65 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 26 Apr 2022 17:01:45 -0700 Subject: [PATCH 22/37] Filter from ProjectStarted --- .../Logging/ProjectLoggingContext.cs | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs b/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs index bc39a5ccd32..793b39d28db 100644 --- a/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs +++ b/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs @@ -8,6 +8,7 @@ using Microsoft.Build.Execution; using Microsoft.Build.Framework; using Microsoft.Build.Shared; +using static Microsoft.Build.Execution.ProjectPropertyInstance; using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem; #nullable disable @@ -71,6 +72,17 @@ internal ProjectLoggingContext( { } + private IEnumerable FilterEnvironmentDerivedProperties(PropertyDictionary properties) + { + foreach (ProjectPropertyInstance property in properties) + { + if (!(property is EnvironmentDerivedProjectPropertyInstance)) + { + yield return new DictionaryEntry(property.Name, property.EvaluatedValue); + } + } + } + ///

/// Constructs a project logging contexts. /// @@ -100,7 +112,19 @@ private ProjectLoggingContext( !LoggingService.IncludeEvaluationPropertiesAndItems && (!LoggingService.RunningOnRemoteNode || LoggingService.SerializeAllProperties)) { - properties = projectProperties?.GetCopyOnReadEnumerable(property => new DictionaryEntry(property.Name, property.EvaluatedValue)) ?? Enumerable.Empty(); + if (projectProperties is null) + { + properties = Enumerable.Empty(); + } + else if (Traits.Instance.LogAllEnvironmentVariables) + { + properties = projectProperties.GetCopyOnReadEnumerable(property => new DictionaryEntry(property.Name, property.EvaluatedValue)); + } + else + { + properties = FilterEnvironmentDerivedProperties(projectProperties); + } + items = projectItems?.GetCopyOnReadEnumerable(item => new DictionaryEntry(item.ItemType, new TaskItem(item))) ?? Enumerable.Empty(); } From fff4f97d9826cd2518d93411129c1003cd1565b0 Mon Sep 17 00:00:00 2001 From: Forgind Date: Wed, 27 Apr 2022 16:06:27 -0700 Subject: [PATCH 23/37] Add test --- src/Build.UnitTests/BinaryLogger_Tests.cs | 42 ++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/BinaryLogger_Tests.cs b/src/Build.UnitTests/BinaryLogger_Tests.cs index 4be660b2231..5360c9ae021 100644 --- a/src/Build.UnitTests/BinaryLogger_Tests.cs +++ b/src/Build.UnitTests/BinaryLogger_Tests.cs @@ -1,12 +1,13 @@ using System; using System.Collections.Generic; +using System.IO; using System.Text; using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Execution; using Microsoft.Build.Framework; using Microsoft.Build.Logging; - +using Microsoft.Build.UnitTests.Shared; using Shouldly; using Xunit; using Xunit.Abstractions; @@ -142,6 +143,45 @@ public void BinaryLoggerShouldSupportFilePathExplicitParameter() ObjectModelHelpers.BuildProjectExpectSuccess(s_testProject, binaryLogger); } + [Fact] + public void UnusedEnvironmentVariablesDoNotAppearInBinaryLog() + { + using (TestEnvironment env = TestEnvironment.Create()) + { + env.SetEnvironmentVariable("EnvVar1", "itsValue"); + env.SetEnvironmentVariable("EnvVar2", "value2"); + env.SetEnvironmentVariable("EnvVar3", "value3"); + string contents = @" + + + +value +$(EnvVar2) + + + + + + +"; + TransientTestFolder logFolder = env.CreateFolder(createFolder: true); + TransientTestFile projectFile = env.CreateFile(logFolder, ".proj", contents); + BinaryLogger logger = new(); + logger.Parameters = Path.Combine(logFolder.Path, "binlog.binlog"); + RunnerUtilities.ExecMSBuild($"{projectFile.Path} -bl:{logger.Parameters}", out bool success); + success.ShouldBeTrue(); + RunnerUtilities.ExecMSBuild($"{logger.Parameters} -flp:logfile={Path.Combine(logFolder.Path, "logFile.log")};verbosity=diagnostic", out success); + success.ShouldBeTrue(); + string text = File.ReadAllText(Path.Combine(logFolder.Path, "logFile.log")); + text.ShouldContain("EnvVar2"); + text.ShouldContain("value2"); + text.ShouldContain("EnvVar3"); + text.ShouldContain("value3"); + text.ShouldNotContain("EnvVar1"); + text.ShouldNotContain("itsValue"); + } + } + [Fact] public void BinaryLoggerShouldNotThrowWhenMetadataCannotBeExpanded() { From 0b830e52ed81f5e7aa9e1a73f86f7c42da99a25a Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 28 Apr 2022 16:38:56 -0700 Subject: [PATCH 24/37] Reorder usings This is the correct order according to VS. It failed when I had them in a different order. --- src/Build/Evaluation/Evaluator.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index 97a015ec465..6aed10ece8b 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -4,7 +4,6 @@ using System; using System.Collections; using System.Collections.Generic; -using ObjectModel = System.Collections.ObjectModel; using System.Diagnostics; using System.Globalization; using System.IO; @@ -25,14 +24,15 @@ using Microsoft.Build.Internal; using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; -using ILoggingService = Microsoft.Build.BackEnd.Logging.ILoggingService; -using SdkResult = Microsoft.Build.BackEnd.SdkResolution.SdkResult; -using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException; +using static Microsoft.Build.Execution.ProjectPropertyInstance; using Constants = Microsoft.Build.Internal.Constants; using EngineFileUtilities = Microsoft.Build.Internal.EngineFileUtilities; +using ILoggingService = Microsoft.Build.BackEnd.Logging.ILoggingService; +using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException; +using ObjectModel = System.Collections.ObjectModel; using ReservedPropertyNames = Microsoft.Build.Internal.ReservedPropertyNames; using SdkReferencePropertyExpansionMode = Microsoft.Build.Framework.EscapeHatches.SdkReferencePropertyExpansionMode; -using static Microsoft.Build.Execution.ProjectPropertyInstance; +using SdkResult = Microsoft.Build.BackEnd.SdkResolution.SdkResult; #nullable disable From c2b93b958bd914e829facaa4677678b37b6bd631 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 28 Apr 2022 17:12:25 -0700 Subject: [PATCH 25/37] Make list instead of yield returning I think this yield return might be modifying the PropertyDictionary or something? --- src/Build/Evaluation/Evaluator.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index 6aed10ece8b..bc7ec077b66 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -821,14 +821,17 @@ private void Evaluate() private IEnumerable FilterOutEnvironmentDerivedProperties(PropertyDictionary

dictionary) { + List

list = new(); foreach (P p in dictionary) { if (!((p is ProjectProperty pp && pp.IsEnvironmentProperty) || (p is EnvironmentDerivedProjectPropertyInstance))) { - yield return p; + list.Add(p); } } + + return list; } private void CollectProjectCachePlugins() From d1cad7b2b5aa8371d3410f5ce24ce8e136dda0f2 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 28 Apr 2022 17:36:14 -0700 Subject: [PATCH 26/37] Remove expectation of file _env.ExpectFile(".binlog").Path means every test in this file would have to make a binlog in the folder without an interesting name. That's too restraining. --- src/Build.UnitTests/BinaryLogger_Tests.cs | 4 +--- src/Shared/UnitTests/TestEnvironment.cs | 11 ----------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/Build.UnitTests/BinaryLogger_Tests.cs b/src/Build.UnitTests/BinaryLogger_Tests.cs index 5360c9ae021..9ee2339b895 100644 --- a/src/Build.UnitTests/BinaryLogger_Tests.cs +++ b/src/Build.UnitTests/BinaryLogger_Tests.cs @@ -74,8 +74,6 @@ public BinaryLoggerTests(ITestOutputHelper output) // this is needed to ensure the binary logger does not pollute the environment _env.WithEnvironmentInvariant(); - - _logFile = _env.ExpectFile(".binlog").Path; } [Theory] @@ -165,7 +163,7 @@ public void UnusedEnvironmentVariablesDoNotAppearInBinaryLog() "; TransientTestFolder logFolder = env.CreateFolder(createFolder: true); - TransientTestFile projectFile = env.CreateFile(logFolder, ".proj", contents); + TransientTestFile projectFile = env.CreateFile(logFolder, "myProj.proj", contents); BinaryLogger logger = new(); logger.Parameters = Path.Combine(logFolder.Path, "binlog.binlog"); RunnerUtilities.ExecMSBuild($"{projectFile.Path} -bl:{logger.Parameters}", out bool success); diff --git a/src/Shared/UnitTests/TestEnvironment.cs b/src/Shared/UnitTests/TestEnvironment.cs index 6ede3f2d7fb..49e86433de5 100644 --- a/src/Shared/UnitTests/TestEnvironment.cs +++ b/src/Shared/UnitTests/TestEnvironment.cs @@ -258,17 +258,6 @@ public TransientTestFile ExpectFile(string extension = ".tmp") return WithTransientTestState(new TransientTestFile(extension, createFile: false, expectedAsOutput: true)); } - ///

- /// Create a temp file name under a specific temporary folder. The file is expected to exist when the test completes. - /// - /// Temp folder - /// Extension of the file (defaults to '.tmp') - /// - public TransientTestFile ExpectFile(TransientTestFolder transientTestFolder, string extension = ".tmp") - { - return WithTransientTestState(new TransientTestFile(transientTestFolder.Path, extension, createFile: false, expectedAsOutput: true)); - } - /// /// Creates a test variant used to add a unique temporary folder during a test. Will be deleted when the test /// completes. From d4c2f060c887c6fc983ee6dce1fbff7e1f0e0fd5 Mon Sep 17 00:00:00 2001 From: Forgind Date: Thu, 28 Apr 2022 17:38:10 -0700 Subject: [PATCH 27/37] Use log file instead --- src/Build.UnitTests/BinaryLogger_Tests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/BinaryLogger_Tests.cs b/src/Build.UnitTests/BinaryLogger_Tests.cs index 9ee2339b895..0ec915e2710 100644 --- a/src/Build.UnitTests/BinaryLogger_Tests.cs +++ b/src/Build.UnitTests/BinaryLogger_Tests.cs @@ -74,6 +74,8 @@ public BinaryLoggerTests(ITestOutputHelper output) // this is needed to ensure the binary logger does not pollute the environment _env.WithEnvironmentInvariant(); + + _logFile = _env.ExpectFile(".binlog").Path; } [Theory] @@ -165,7 +167,7 @@ public void UnusedEnvironmentVariablesDoNotAppearInBinaryLog() TransientTestFolder logFolder = env.CreateFolder(createFolder: true); TransientTestFile projectFile = env.CreateFile(logFolder, "myProj.proj", contents); BinaryLogger logger = new(); - logger.Parameters = Path.Combine(logFolder.Path, "binlog.binlog"); + logger.Parameters = _logFile; RunnerUtilities.ExecMSBuild($"{projectFile.Path} -bl:{logger.Parameters}", out bool success); success.ShouldBeTrue(); RunnerUtilities.ExecMSBuild($"{logger.Parameters} -flp:logfile={Path.Combine(logFolder.Path, "logFile.log")};verbosity=diagnostic", out success); From add1258fd7ce2246d39f9b07ea1f6529ae9d5198 Mon Sep 17 00:00:00 2001 From: Forgind Date: Fri, 29 Apr 2022 15:13:32 -0700 Subject: [PATCH 28/37] Update src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs --- src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs b/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs index 793b39d28db..b7f2fcf4bc2 100644 --- a/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs +++ b/src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs @@ -76,7 +76,7 @@ private IEnumerable FilterEnvironmentDerivedProperties(Property { foreach (ProjectPropertyInstance property in properties) { - if (!(property is EnvironmentDerivedProjectPropertyInstance)) + if (property is not EnvironmentDerivedProjectPropertyInstance) { yield return new DictionaryEntry(property.Name, property.EvaluatedValue); } From 679c0034b692c1157ede472feb084fcbf602d026 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 10 May 2022 14:44:32 -0700 Subject: [PATCH 29/37] Notice env properties accessed in tasks --- .../TaskExecutionHost/TaskExecutionHost.cs | 2 +- src/Build/Evaluation/Expander.cs | 27 ++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs index cdb1553f181..bfbe4b83f41 100644 --- a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs +++ b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs @@ -1174,7 +1174,7 @@ out bool taskParameterSet else { // Expand out all the metadata, properties, and item vectors in the string. - string expandedParameterValue = _batchBucket.Expander.ExpandIntoStringAndUnescape(parameterValue, ExpanderOptions.ExpandAll, parameterLocation); + string expandedParameterValue = _batchBucket.Expander.ExpandIntoStringAndUnescape(parameterValue, ExpanderOptions.ExpandAll, parameterLocation, _targetLoggingContext); if (expandedParameterValue.Length == 0) { diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index 46b24084309..f46a55cde59 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -28,6 +28,7 @@ using TaskItemFactory = Microsoft.Build.Execution.ProjectItemInstance.TaskItem.TaskItemFactory; using Microsoft.NET.StringTools; +using Microsoft.Build.BackEnd.Logging; #nullable disable @@ -420,9 +421,9 @@ internal static bool ExpressionContainsItemVector(string expression) /// /// If ExpanderOptions.BreakOnNotEmpty was passed, expression was going to be non-empty, and it broke out early, returns null. Otherwise the result can be trusted. /// - internal string ExpandIntoStringAndUnescape(string expression, ExpanderOptions options, IElementLocation elementLocation) + internal string ExpandIntoStringAndUnescape(string expression, ExpanderOptions options, IElementLocation elementLocation, LoggingContext loggingContext = null) { - string result = ExpandIntoStringLeaveEscaped(expression, options, elementLocation); + string result = ExpandIntoStringLeaveEscaped(expression, options, elementLocation, loggingContext); return (result == null) ? null : EscapingUtilities.UnescapeAll(result); } @@ -434,7 +435,7 @@ internal string ExpandIntoStringAndUnescape(string expression, ExpanderOptions o /// /// If ExpanderOptions.BreakOnNotEmpty was passed, expression was going to be non-empty, and it broke out early, returns null. Otherwise the result can be trusted. /// - internal string ExpandIntoStringLeaveEscaped(string expression, ExpanderOptions options, IElementLocation elementLocation) + internal string ExpandIntoStringLeaveEscaped(string expression, ExpanderOptions options, IElementLocation elementLocation, LoggingContext loggingContext = null) { if (expression.Length == 0) { @@ -444,7 +445,7 @@ internal string ExpandIntoStringLeaveEscaped(string expression, ExpanderOptions ErrorUtilities.VerifyThrowInternalNull(elementLocation, nameof(elementLocation)); string result = MetadataExpander.ExpandMetadataLeaveEscaped(expression, _metadata, options, elementLocation); - result = PropertyExpander

.ExpandPropertiesLeaveEscaped(result, _properties, options, elementLocation, _usedUninitializedProperties, _fileSystem); + result = PropertyExpander

.ExpandPropertiesLeaveEscaped(result, _properties, options, elementLocation, _usedUninitializedProperties, _fileSystem, loggingContext); result = ItemExpander.ExpandItemVectorsIntoString(this, result, _items, options, elementLocation); result = FileUtilities.MaybeAdjustFilePath(result); @@ -1078,7 +1079,8 @@ internal static string ExpandPropertiesLeaveEscaped( ExpanderOptions options, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties, - IFileSystem fileSystem) + IFileSystem fileSystem, + LoggingContext loggingContext = null) { return ConvertToString( @@ -1088,7 +1090,8 @@ internal static string ExpandPropertiesLeaveEscaped( options, elementLocation, usedUninitializedProperties, - fileSystem)); + fileSystem, + loggingContext)); } ///

@@ -1114,7 +1117,8 @@ internal static object ExpandPropertiesLeaveTypedAndEscaped( ExpanderOptions options, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties, - IFileSystem fileSystem) + IFileSystem fileSystem, + LoggingContext loggingContext = null) { if (((options & ExpanderOptions.ExpandProperties) == 0) || String.IsNullOrEmpty(expression)) { @@ -1229,7 +1233,7 @@ internal static object ExpandPropertiesLeaveTypedAndEscaped( } else // This is a regular property { - propertyValue = LookupProperty(properties, expression, propertyStartIndex + 2, propertyEndIndex - 1, elementLocation, usedUninitializedProperties); + propertyValue = LookupProperty(properties, expression, propertyStartIndex + 2, propertyEndIndex - 1, elementLocation, usedUninitializedProperties, loggingContext); } if (propertyValue != null) @@ -1467,7 +1471,7 @@ private static object LookupProperty(IPropertyProvider properties, string pro /// /// Look up a simple property reference by the name of the property, e.g. "Foo" when expanding $(Foo). /// - private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties) + private static object LookupProperty(IPropertyProvider properties, string propertyName, int startIndex, int endIndex, IElementLocation elementLocation, UsedUninitializedProperties usedUninitializedProperties, LoggingContext loggingContext = null) { T property = properties.GetProperty(propertyName, startIndex, endIndex); @@ -1512,6 +1516,11 @@ private static object LookupProperty(IPropertyProvider properties, string pro } else { + if (property is ProjectPropertyInstance.EnvironmentDerivedProjectPropertyInstance environmentDerivedProperty) + { + environmentDerivedProperty.loggingContext = loggingContext; + } + propertyValue = property.EvaluatedValueEscaped; } From aa466612a1cf7514800d84e73d1501ace7982fa1 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 10 May 2022 15:19:41 -0700 Subject: [PATCH 30/37] PR comments --- src/Build/Evaluation/Evaluator.cs | 12 +++++++----- .../Logging/ParallelLogger/ParallelConsoleLogger.cs | 2 +- src/Build/Logging/SerialConsoleLogger.cs | 9 ++------- src/Build/Resources/Strings.resx | 3 +++ src/Build/Resources/xlf/Strings.cs.xlf | 5 +++++ src/Build/Resources/xlf/Strings.de.xlf | 5 +++++ src/Build/Resources/xlf/Strings.es.xlf | 5 +++++ src/Build/Resources/xlf/Strings.fr.xlf | 5 +++++ src/Build/Resources/xlf/Strings.it.xlf | 5 +++++ src/Build/Resources/xlf/Strings.ja.xlf | 5 +++++ src/Build/Resources/xlf/Strings.ko.xlf | 5 +++++ src/Build/Resources/xlf/Strings.pl.xlf | 5 +++++ src/Build/Resources/xlf/Strings.pt-BR.xlf | 5 +++++ src/Build/Resources/xlf/Strings.ru.xlf | 5 +++++ src/Build/Resources/xlf/Strings.tr.xlf | 5 +++++ src/Build/Resources/xlf/Strings.zh-Hans.xlf | 5 +++++ src/Build/Resources/xlf/Strings.zh-Hant.xlf | 5 +++++ 17 files changed, 78 insertions(+), 13 deletions(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index bc7ec077b66..575e00478f2 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -821,14 +821,16 @@ private void Evaluate() private IEnumerable FilterOutEnvironmentDerivedProperties(PropertyDictionary

dictionary) { - List

list = new(); + List

list = new(dictionary.Count); foreach (P p in dictionary) { - if (!((p is ProjectProperty pp && pp.IsEnvironmentProperty) || - (p is EnvironmentDerivedProjectPropertyInstance))) + if (p is EnvironmentDerivedProjectPropertyInstance || + (p is ProjectProperty pp && pp.IsEnvironmentProperty)) { - list.Add(p); + continue; } + + list.Add(p); } return list; @@ -1314,7 +1316,7 @@ private void EvaluatePropertyElement(ProjectPropertyElement propertyElement) // it is the same as what we are setting the value on. Note: This needs to be set before we expand the property we are currently setting. _expander.UsedUninitializedProperties.CurrentlyEvaluatingPropertyElementName = propertyElement.Name; - string evaluatedValue = _expander.ExpandIntoStringLeaveEscaped(propertyElement.Value, ExpanderOptions.ExpandProperties, propertyElement.Location); + string evaluatedValue = _expander.ExpandIntoStringLeaveEscaped(propertyElement.Value, ExpanderOptions.ExpandProperties, propertyElement.Location, _evaluationLoggingContext); // If we are going to set a property to a value other than null or empty we need to check to see if it has been used // during evaluation. diff --git a/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs b/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs index accc1fcfa53..4c147958cb5 100644 --- a/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs +++ b/src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs @@ -1208,7 +1208,7 @@ private void PrintMessage(BuildMessageEventArgs e, bool lightenText) if (e is EnvironmentVariableReadEventArgs environmentPropertyReadEventArgs) { - nonNullMessage = $"Property {environmentPropertyReadEventArgs.EnvironmentVariableName} with value {e.Message} expanded from the environment."; + nonNullMessage = ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("EnvironmentDerivedPropertyRead", environmentPropertyReadEventArgs.EnvironmentVariableName, e.Message); } // Include file information if present. diff --git a/src/Build/Logging/SerialConsoleLogger.cs b/src/Build/Logging/SerialConsoleLogger.cs index 0e1c5100f64..4404357b05e 100644 --- a/src/Build/Logging/SerialConsoleLogger.cs +++ b/src/Build/Logging/SerialConsoleLogger.cs @@ -512,19 +512,14 @@ public override void MessageHandler(object sender, BuildMessageEventArgs e) } string nonNullMessage = e is EnvironmentVariableReadEventArgs environmentDerivedProperty ? - $"Property {environmentDerivedProperty.EnvironmentVariableName} with value {e.Message} expanded from the environment." - : null; + ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("EnvironmentDerivedPropertyRead", environmentDerivedProperty.EnvironmentVariableName, e.Message) + : e.Message ?? String.Empty; // Include file information if present. if (e.File != null) { nonNullMessage = EventArgsFormatting.FormatEventMessage(e, showProjectFile); } - else - { - // null messages are ok -- treat as blank line - nonNullMessage ??= e.Message ?? String.Empty; - } WriteLinePretty(nonNullMessage); diff --git a/src/Build/Resources/Strings.resx b/src/Build/Resources/Strings.resx index 10018e5f8f1..db8651cce57 100644 --- a/src/Build/Resources/Strings.resx +++ b/src/Build/Resources/Strings.resx @@ -142,6 +142,9 @@ The operation cannot be completed because EndBuild has already been called but existing submissions have not yet completed. + + Property '{0}' with value '{1}' expanded from the environment. + The operation cannot be completed because the submission has already been executed. diff --git a/src/Build/Resources/xlf/Strings.cs.xlf b/src/Build/Resources/xlf/Strings.cs.xlf index bc4f28a26e1..295e744c203 100644 --- a/src/Build/Resources/xlf/Strings.cs.xlf +++ b/src/Build/Resources/xlf/Strings.cs.xlf @@ -102,6 +102,11 @@ MSB4257: Zadaný výstupní soubor mezipaměti pro výsledky je prázdný. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Číst proměnnou prostředí {0} diff --git a/src/Build/Resources/xlf/Strings.de.xlf b/src/Build/Resources/xlf/Strings.de.xlf index 2d5371cd4e4..354bbd025b6 100644 --- a/src/Build/Resources/xlf/Strings.de.xlf +++ b/src/Build/Resources/xlf/Strings.de.xlf @@ -102,6 +102,11 @@ MSB4257: Die angegebene Cachedatei für Ausgabeergebnisse ist leer. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Umgebungsvariable "{0}" lesen diff --git a/src/Build/Resources/xlf/Strings.es.xlf b/src/Build/Resources/xlf/Strings.es.xlf index ae8a6a1e189..0ff85f2cfc1 100644 --- a/src/Build/Resources/xlf/Strings.es.xlf +++ b/src/Build/Resources/xlf/Strings.es.xlf @@ -102,6 +102,11 @@ MSB4257: El archivo de caché de resultados de salida especificado está vacío. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Leer la variable de entorno "{0}" diff --git a/src/Build/Resources/xlf/Strings.fr.xlf b/src/Build/Resources/xlf/Strings.fr.xlf index 0ffddfbe011..bd86334a1d6 100644 --- a/src/Build/Resources/xlf/Strings.fr.xlf +++ b/src/Build/Resources/xlf/Strings.fr.xlf @@ -102,6 +102,11 @@ MSB4257: Le fichier cache des résultats de sortie spécifié est vide. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Lire la variable d'environnement "{0}" diff --git a/src/Build/Resources/xlf/Strings.it.xlf b/src/Build/Resources/xlf/Strings.it.xlf index 11aec49efc6..b3ffd7d5a8f 100644 --- a/src/Build/Resources/xlf/Strings.it.xlf +++ b/src/Build/Resources/xlf/Strings.it.xlf @@ -102,6 +102,11 @@ MSB4257: il file della cache dei risultati di output specificato è vuoto. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Legge la variabile di ambiente "{0}" diff --git a/src/Build/Resources/xlf/Strings.ja.xlf b/src/Build/Resources/xlf/Strings.ja.xlf index 1bf7a2f0d24..20194a207f9 100644 --- a/src/Build/Resources/xlf/Strings.ja.xlf +++ b/src/Build/Resources/xlf/Strings.ja.xlf @@ -102,6 +102,11 @@ MSB4257: 指定された出力結果キャッシュ ファイルは空です。 + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" 環境変数 "{0}" の読み取り diff --git a/src/Build/Resources/xlf/Strings.ko.xlf b/src/Build/Resources/xlf/Strings.ko.xlf index 8f5d34f5cce..64edef66ee4 100644 --- a/src/Build/Resources/xlf/Strings.ko.xlf +++ b/src/Build/Resources/xlf/Strings.ko.xlf @@ -102,6 +102,11 @@ MSB4257: 지정한 출력 결과 캐시 파일이 비어 있습니다. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" 환경 변수 "{0}" 읽기 diff --git a/src/Build/Resources/xlf/Strings.pl.xlf b/src/Build/Resources/xlf/Strings.pl.xlf index b893482a44e..ef425e1af33 100644 --- a/src/Build/Resources/xlf/Strings.pl.xlf +++ b/src/Build/Resources/xlf/Strings.pl.xlf @@ -102,6 +102,11 @@ MSB4257: Określony plik wyjściowej pamięci podręcznej wyników jest pusty. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Odczytaj zmienną środowiskową „{0}” diff --git a/src/Build/Resources/xlf/Strings.pt-BR.xlf b/src/Build/Resources/xlf/Strings.pt-BR.xlf index ad14ba37ca1..57edd51b8b6 100644 --- a/src/Build/Resources/xlf/Strings.pt-BR.xlf +++ b/src/Build/Resources/xlf/Strings.pt-BR.xlf @@ -102,6 +102,11 @@ MSB4257: o arquivo de cache do resultado de saída especificado está vazio. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Ler a variável de ambiente "{0}" diff --git a/src/Build/Resources/xlf/Strings.ru.xlf b/src/Build/Resources/xlf/Strings.ru.xlf index 04004fbdfe1..9355b218d8e 100644 --- a/src/Build/Resources/xlf/Strings.ru.xlf +++ b/src/Build/Resources/xlf/Strings.ru.xlf @@ -102,6 +102,11 @@ MSB4257: указанный выходной файл кэша результатов пустой. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" Чтение переменной среды "{0}" diff --git a/src/Build/Resources/xlf/Strings.tr.xlf b/src/Build/Resources/xlf/Strings.tr.xlf index 5fc8f54ba4e..439af5cefad 100644 --- a/src/Build/Resources/xlf/Strings.tr.xlf +++ b/src/Build/Resources/xlf/Strings.tr.xlf @@ -102,6 +102,11 @@ MSB4257: Belirtilen çıkış sonucu önbellek dosyası boş. + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" "{0}" ortam değişkenini oku diff --git a/src/Build/Resources/xlf/Strings.zh-Hans.xlf b/src/Build/Resources/xlf/Strings.zh-Hans.xlf index 4a9d18a374e..ca550fb5c8d 100644 --- a/src/Build/Resources/xlf/Strings.zh-Hans.xlf +++ b/src/Build/Resources/xlf/Strings.zh-Hans.xlf @@ -102,6 +102,11 @@ MSB4257: 指定的输出结果缓存文件为空。 + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" 读取环境变量“{0}” diff --git a/src/Build/Resources/xlf/Strings.zh-Hant.xlf b/src/Build/Resources/xlf/Strings.zh-Hant.xlf index 214240db01c..f1f0c28e519 100644 --- a/src/Build/Resources/xlf/Strings.zh-Hant.xlf +++ b/src/Build/Resources/xlf/Strings.zh-Hant.xlf @@ -102,6 +102,11 @@ MSB4257: 指定的輸出結果快取檔案是空的。 + + Property '{0}' with value '{1}' expanded from the environment. + Property '{0}' with value '{1}' expanded from the environment. + + Read environment variable "{0}" 讀取環境變數 "{0}" From ce8dad44473498909e69e0ee83b8a87525ac8a21 Mon Sep 17 00:00:00 2001 From: Forgind Date: Fri, 13 May 2022 17:01:50 -0700 Subject: [PATCH 31/37] Account for more cases --- .../IntrinsicTasks/ItemGroupIntrinsicTask.cs | 45 +++++++++++-------- .../Components/RequestBuilder/TargetEntry.cs | 5 ++- src/Build/Evaluation/ConditionEvaluator.cs | 25 ++++++----- .../Conditionals/AndExpressionNode.cs | 7 +-- .../FunctionCallExpressionNode.cs | 3 +- .../Conditionals/GenericExpressionNode.cs | 13 +++--- .../MultipleComparisonExpressionNode.cs | 13 +++--- .../Conditionals/NotExpressionNode.cs | 9 ++-- .../NumericComparisonExpressionNode.cs | 6 +-- .../Conditionals/NumericExpressionNode.cs | 6 +-- .../Conditionals/OperatorExpressionNode.cs | 9 ++-- .../Conditionals/OrExpressionNode.cs | 7 +-- .../Conditionals/StringExpressionNode.cs | 13 +++--- src/Build/Evaluation/Evaluator.cs | 6 +-- src/Build/Evaluation/Expander.cs | 4 +- 15 files changed, 97 insertions(+), 74 deletions(-) diff --git a/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs b/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs index 505bc1d2df1..32b7c6b1b1a 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs @@ -15,6 +15,7 @@ using ProjectItemInstanceFactory = Microsoft.Build.Execution.ProjectItemInstance.TaskItem.ProjectItemInstanceFactory; using EngineFileUtilities = Microsoft.Build.Internal.EngineFileUtilities; using TargetLoggingContext = Microsoft.Build.BackEnd.Logging.TargetLoggingContext; +using Microsoft.Build.BackEnd.Logging; #nullable disable @@ -83,7 +84,7 @@ internal override void ExecuteTask(Lookup lookup) if (!String.IsNullOrEmpty(child.KeepMetadata)) { - var keepMetadataEvaluated = bucket.Expander.ExpandIntoStringListLeaveEscaped(child.KeepMetadata, ExpanderOptions.ExpandAll, child.KeepMetadataLocation).ToList(); + var keepMetadataEvaluated = bucket.Expander.ExpandIntoStringListLeaveEscaped(child.KeepMetadata, ExpanderOptions.ExpandAll, child.KeepMetadataLocation, LoggingContext).ToList(); if (keepMetadataEvaluated.Count > 0) { keepMetadata = new HashSet(keepMetadataEvaluated); @@ -92,7 +93,7 @@ internal override void ExecuteTask(Lookup lookup) if (!String.IsNullOrEmpty(child.RemoveMetadata)) { - var removeMetadataEvaluated = bucket.Expander.ExpandIntoStringListLeaveEscaped(child.RemoveMetadata, ExpanderOptions.ExpandAll, child.RemoveMetadataLocation).ToList(); + var removeMetadataEvaluated = bucket.Expander.ExpandIntoStringListLeaveEscaped(child.RemoveMetadata, ExpanderOptions.ExpandAll, child.RemoveMetadataLocation, LoggingContext).ToList(); if (removeMetadataEvaluated.Count > 0) { removeMetadata = new HashSet(removeMetadataEvaluated); @@ -101,7 +102,7 @@ internal override void ExecuteTask(Lookup lookup) if (!String.IsNullOrEmpty(child.MatchOnMetadata)) { - var matchOnMetadataEvaluated = bucket.Expander.ExpandIntoStringListLeaveEscaped(child.MatchOnMetadata, ExpanderOptions.ExpandAll, child.MatchOnMetadataLocation).ToList(); + var matchOnMetadataEvaluated = bucket.Expander.ExpandIntoStringListLeaveEscaped(child.MatchOnMetadata, ExpanderOptions.ExpandAll, child.MatchOnMetadataLocation, LoggingContext).ToList(); if (matchOnMetadataEvaluated.Count > 0) { matchOnMetadata = new HashSet(matchOnMetadataEvaluated); @@ -114,7 +115,7 @@ internal override void ExecuteTask(Lookup lookup) (child.Exclude.Length != 0)) { // It's an item -- we're "adding" items to the world - ExecuteAdd(child, bucket, keepMetadata, removeMetadata); + ExecuteAdd(child, bucket, keepMetadata, removeMetadata, LoggingContext); } else if (child.Remove.Length != 0) { @@ -124,7 +125,7 @@ internal override void ExecuteTask(Lookup lookup) else { // It's a modify -- changing existing items - ExecuteModify(child, bucket, keepMetadata, removeMetadata); + ExecuteModify(child, bucket, keepMetadata, removeMetadata, LoggingContext); } } } @@ -150,7 +151,8 @@ internal override void ExecuteTask(Lookup lookup) /// The batching bucket. /// An of metadata names to keep. /// An of metadata names to remove. - private void ExecuteAdd(ProjectItemGroupTaskItemInstance child, ItemBucket bucket, ISet keepMetadata, ISet removeMetadata) + /// Context for logging + private void ExecuteAdd(ProjectItemGroupTaskItemInstance child, ItemBucket bucket, ISet keepMetadata, ISet removeMetadata, LoggingContext loggingContext = null) { // First, collect up the appropriate metadata collections. We need the one from the item definition, if any, and // the one we are using for this batching bucket. @@ -164,7 +166,7 @@ private void ExecuteAdd(ProjectItemGroupTaskItemInstance child, ItemBucket bucke bucket.Expander.Metadata = metadataTable; // Second, expand the item include and exclude, and filter existing metadata as appropriate. - List itemsToAdd = ExpandItemIntoItems(child, bucket.Expander, keepMetadata, removeMetadata); + List itemsToAdd = ExpandItemIntoItems(child, bucket.Expander, keepMetadata, removeMetadata, loggingContext); // Third, expand the metadata. foreach (ProjectItemGroupTaskMetadataInstance metadataInstance in child.Metadata) @@ -179,11 +181,12 @@ private void ExecuteAdd(ProjectItemGroupTaskItemInstance child, ItemBucket bucke metadataInstance.Location, LoggingContext.LoggingService, LoggingContext.BuildEventContext, - FileSystems.Default); + FileSystems.Default, + loggingContext: loggingContext); if (condition) { - string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, ExpanderOptions.ExpandAll, metadataInstance.Location); + string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, ExpanderOptions.ExpandAll, metadataInstance.Location, loggingContext); // This both stores the metadata so we can add it to all the items we just created later, and // exposes this metadata to further metadata evaluations in subsequent loop iterations. @@ -245,7 +248,7 @@ private void ExecuteRemove(ProjectItemGroupTaskItemInstance child, ItemBucket bu List itemsToRemove; if (matchOnMetadata == null) { - itemsToRemove = FindItemsMatchingSpecification(group, child.Remove, child.RemoveLocation, bucket.Expander); + itemsToRemove = FindItemsMatchingSpecification(group, child.Remove, child.RemoveLocation, bucket.Expander, LoggingContext); } else { @@ -277,7 +280,8 @@ private void ExecuteRemove(ProjectItemGroupTaskItemInstance child, ItemBucket bu /// The batching bucket. /// An of metadata names to keep. /// An of metadata names to remove. - private void ExecuteModify(ProjectItemGroupTaskItemInstance child, ItemBucket bucket, ISet keepMetadata, ISet removeMetadata) + /// Context for this operation. + private void ExecuteModify(ProjectItemGroupTaskItemInstance child, ItemBucket bucket, ISet keepMetadata, ISet removeMetadata, LoggingContext loggingContext = null) { ICollection group = bucket.Lookup.GetItems(child.ItemType); if (group == null || group.Count == 0) @@ -317,11 +321,12 @@ private void ExecuteModify(ProjectItemGroupTaskItemInstance child, ItemBucket bu metadataInstance.ConditionLocation, LoggingContext.LoggingService, LoggingContext.BuildEventContext, - FileSystems.Default); + FileSystems.Default, + loggingContext: loggingContext); if (condition) { - string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, ExpanderOptions.ExpandAll, metadataInstance.Location); + string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, ExpanderOptions.ExpandAll, metadataInstance.Location, loggingContext); metadataToSet[metadataInstance.Name] = Lookup.MetadataModification.CreateFromNewValue(evaluatedValue); } } @@ -357,6 +362,7 @@ private void GetBatchableValuesFromBuildItemGroupChild(List parameterVal /// The expander to use. /// An of metadata names to keep. /// An of metadata names to remove. + /// Context for logging /// /// This code is very close to that which exists in the Evaluator.EvaluateItemXml method. However, because /// it invokes type constructors, and those constructors take arguments of fundamentally different types, it has not @@ -368,7 +374,8 @@ private List ExpandItemIntoItems ProjectItemGroupTaskItemInstance originalItem, Expander expander, ISet keepMetadata, - ISet removeMetadata + ISet removeMetadata, + LoggingContext loggingContext = null ) { // todo this is duplicated logic with the item computation logic from evaluation (in LazyIncludeOperation.SelectItems) @@ -376,7 +383,7 @@ ISet removeMetadata List items = new List(); // Expand properties and metadata in Include - string evaluatedInclude = expander.ExpandIntoStringLeaveEscaped(originalItem.Include, ExpanderOptions.ExpandPropertiesAndMetadata, originalItem.IncludeLocation); + string evaluatedInclude = expander.ExpandIntoStringLeaveEscaped(originalItem.Include, ExpanderOptions.ExpandPropertiesAndMetadata, originalItem.IncludeLocation, loggingContext); if (evaluatedInclude.Length == 0) { @@ -387,7 +394,7 @@ ISet removeMetadata var excludes = ImmutableList.Empty.ToBuilder(); if (originalItem.Exclude.Length > 0) { - string evaluatedExclude = expander.ExpandIntoStringLeaveEscaped(originalItem.Exclude, ExpanderOptions.ExpandAll, originalItem.ExcludeLocation); + string evaluatedExclude = expander.ExpandIntoStringLeaveEscaped(originalItem.Exclude, ExpanderOptions.ExpandAll, originalItem.ExcludeLocation, loggingContext); if (evaluatedExclude.Length > 0) { @@ -516,13 +523,15 @@ ISet removeMetadata /// The specification to match against the items. /// The specification to match against the provided items /// The expander to use + /// Context for logging /// A list of matching items private List FindItemsMatchingSpecification ( ICollection items, string specification, ElementLocation specificationLocation, - Expander expander + Expander expander, + LoggingContext loggingContext = null ) { if (items.Count == 0 || specification.Length == 0) @@ -535,7 +544,7 @@ Expander expander HashSet specificationsToFind = new HashSet(StringComparer.OrdinalIgnoreCase); // Split by semicolons - var specificationPieces = expander.ExpandIntoStringListLeaveEscaped(specification, ExpanderOptions.ExpandAll, specificationLocation); + var specificationPieces = expander.ExpandIntoStringListLeaveEscaped(specification, ExpanderOptions.ExpandAll, specificationLocation, loggingContext); foreach (string piece in specificationPieces) { diff --git a/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs b/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs index 49a849c4313..e1479768ff2 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TargetEntry.cs @@ -353,7 +353,8 @@ internal List GetDependencies(ProjectLoggingContext project _target.ConditionLocation, projectLoggingContext.LoggingService, projectLoggingContext.BuildEventContext, - FileSystems.Default); + FileSystems.Default, + loggingContext: projectLoggingContext); if (!condition) { @@ -392,7 +393,7 @@ internal List GetDependencies(ProjectLoggingContext project return new List(); } - var dependencies = _expander.ExpandIntoStringListLeaveEscaped(_target.DependsOnTargets, ExpanderOptions.ExpandPropertiesAndItems, _target.DependsOnTargetsLocation); + var dependencies = _expander.ExpandIntoStringListLeaveEscaped(_target.DependsOnTargets, ExpanderOptions.ExpandPropertiesAndItems, _target.DependsOnTargetsLocation, projectLoggingContext); List dependencyTargets = new List(); foreach (string escapedDependency in dependencies) { diff --git a/src/Build/Evaluation/ConditionEvaluator.cs b/src/Build/Evaluation/ConditionEvaluator.cs index 31dd956ed2e..183ce5117d6 100644 --- a/src/Build/Evaluation/ConditionEvaluator.cs +++ b/src/Build/Evaluation/ConditionEvaluator.cs @@ -17,6 +17,7 @@ namespace Microsoft.Build.Evaluation using ElementLocation = Microsoft.Build.Construction.ElementLocation; using Microsoft.Build.Shared; using Microsoft.Build.Shared.FileSystem; + using Microsoft.Build.BackEnd.Logging; internal static class ConditionEvaluator { @@ -181,7 +182,8 @@ internal static bool EvaluateCondition ILoggingService loggingServices, BuildEventContext buildEventContext, IFileSystem fileSystem, - ProjectRootElementCacheBase projectRootElementCache = null) + ProjectRootElementCacheBase projectRootElementCache = null, + LoggingContext loggingContext = null) where P : class, IProperty where I : class, IItem { @@ -196,7 +198,8 @@ internal static bool EvaluateCondition loggingServices, buildEventContext, fileSystem, - projectRootElementCache); + projectRootElementCache, + loggingContext); } ///

@@ -218,7 +221,8 @@ internal static bool EvaluateConditionCollectingConditionedProperties ILoggingService loggingServices, BuildEventContext buildEventContext, IFileSystem fileSystem, - ProjectRootElementCacheBase projectRootElementCache = null + ProjectRootElementCacheBase projectRootElementCache = null, + LoggingContext loggingContext = null ) where P : class, IProperty where I : class, IItem @@ -279,7 +283,7 @@ internal static bool EvaluateConditionCollectingConditionedProperties { try { - result = parsedExpression.Evaluate(state); + result = parsedExpression.Evaluate(state, loggingContext); } finally { @@ -353,7 +357,7 @@ internal interface IConditionEvaluationState /// May return null if the expression would expand to non-empty and it broke out early. /// Otherwise, returns the correctly expanded expression. /// - string ExpandIntoStringBreakEarly(string expression); + string ExpandIntoStringBreakEarly(string expression, LoggingContext loggingContext = null); /// /// Expands the specified expression into a list of TaskItem's. @@ -363,7 +367,7 @@ internal interface IConditionEvaluationState /// /// Expands the specified expression into a string. /// - string ExpandIntoString(string expression); + string ExpandIntoString(string expression, LoggingContext loggingContext = null); /// /// PRE cache @@ -440,11 +444,11 @@ internal ConditionEvaluationState /// May return null if the expression would expand to non-empty and it broke out early. /// Otherwise, returns the correctly expanded expression. /// - public string ExpandIntoStringBreakEarly(string expression) + public string ExpandIntoStringBreakEarly(string expression, LoggingContext loggingContext = null) { var originalValue = _expander.WarnForUninitializedProperties; - expression = _expander.ExpandIntoStringAndUnescape(expression, _expanderOptions | ExpanderOptions.BreakOnNotEmpty, ElementLocation); + expression = _expander.ExpandIntoStringAndUnescape(expression, _expanderOptions | ExpanderOptions.BreakOnNotEmpty, ElementLocation, loggingContext); _expander.WarnForUninitializedProperties = originalValue; @@ -471,12 +475,13 @@ public IList ExpandIntoTaskItems(string expression) /// Expands the specified expression into a string. /// /// The expression to expand. + /// /// The expanded string. - public string ExpandIntoString(string expression) + public string ExpandIntoString(string expression, LoggingContext loggingContext = null) { var originalValue = _expander.WarnForUninitializedProperties; - expression = _expander.ExpandIntoStringAndUnescape(expression, _expanderOptions, ElementLocation); + expression = _expander.ExpandIntoStringAndUnescape(expression, _expanderOptions, ElementLocation, loggingContext); _expander.WarnForUninitializedProperties = originalValue; diff --git a/src/Build/Evaluation/Conditionals/AndExpressionNode.cs b/src/Build/Evaluation/Conditionals/AndExpressionNode.cs index 94513436e9c..47481c625c3 100644 --- a/src/Build/Evaluation/Conditionals/AndExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/AndExpressionNode.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; using System.Diagnostics; @@ -18,9 +19,9 @@ internal sealed class AndExpressionNode : OperatorExpressionNode /// /// Evaluate as boolean /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) + internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { - if (!LeftChild.TryBoolEvaluate(state, out bool leftBool)) + if (!LeftChild.TryBoolEvaluate(state, out bool leftBool, loggingContext)) { ProjectErrorUtilities.ThrowInvalidProject( state.ElementLocation, @@ -37,7 +38,7 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState } else { - if (!RightChild.TryBoolEvaluate(state, out bool rightBool)) + if (!RightChild.TryBoolEvaluate(state, out bool rightBool, loggingContext)) { ProjectErrorUtilities.ThrowInvalidProject( state.ElementLocation, diff --git a/src/Build/Evaluation/Conditionals/FunctionCallExpressionNode.cs b/src/Build/Evaluation/Conditionals/FunctionCallExpressionNode.cs index e0b57181361..4ec222243fd 100644 --- a/src/Build/Evaluation/Conditionals/FunctionCallExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/FunctionCallExpressionNode.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.IO; +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem; @@ -29,7 +30,7 @@ internal FunctionCallExpressionNode(string functionName, List /// Evaluate node as boolean /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) + internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { if (String.Equals(_functionName, "exists", StringComparison.OrdinalIgnoreCase)) { diff --git a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs index d6007133404..0291475add6 100644 --- a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs @@ -2,7 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; - +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; #nullable disable @@ -14,7 +14,7 @@ namespace Microsoft.Build.Evaluation /// internal abstract class GenericExpressionNode { - internal abstract bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result); + internal abstract bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result, LoggingContext loggingContext = null); internal abstract bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result); internal abstract bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result); @@ -25,7 +25,7 @@ internal abstract class GenericExpressionNode /// to empty than to fully evaluate it.) /// Implementations should cache the result so that calls after the first are free. /// - internal virtual bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationState state) + internal virtual bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { return false; } @@ -34,7 +34,7 @@ internal virtual bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationSt /// Value after any item and property expressions are expanded /// /// - internal abstract string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state); + internal abstract string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null); /// /// Value before any item and property expressions are expanded @@ -52,10 +52,11 @@ internal virtual bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationSt /// The main evaluate entry point for expression trees /// /// + /// /// - internal bool Evaluate(ConditionEvaluator.IConditionEvaluationState state) + internal bool Evaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { - if (!TryBoolEvaluate(state, out bool boolValue)) + if (!TryBoolEvaluate(state, out bool boolValue, loggingContext)) { ProjectErrorUtilities.ThrowInvalidProject( state.ElementLocation, diff --git a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs index 65e23c7718f..1ef13748bdc 100644 --- a/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/MultipleComparisonExpressionNode.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; #nullable disable @@ -36,7 +37,7 @@ internal abstract class MultipleComparisonNode : OperatorExpressionNode /// Order in which comparisons are attempted is numeric, boolean, then string. /// Updates conditioned properties table. /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) + internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext) { ProjectErrorUtilities.VerifyThrowInvalidProject (LeftChild != null && RightChild != null, @@ -50,8 +51,8 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState // and we know which do, then we already have enough information to evaluate this expression. // That means we don't have to fully expand a condition like " '@(X)' == '' " // which is a performance advantage if @(X) is a huge item list. - bool leftEmpty = LeftChild.EvaluatesToEmpty(state); - bool rightEmpty = RightChild.EvaluatesToEmpty(state); + bool leftEmpty = LeftChild.EvaluatesToEmpty(state, loggingContext); + bool rightEmpty = RightChild.EvaluatesToEmpty(state, loggingContext); if (leftEmpty || rightEmpty) { UpdateConditionedProperties(state); @@ -68,13 +69,13 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState // is 17.0). return Compare(leftNumericValue, rightNumericValue); } - else if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue) && RightChild.TryBoolEvaluate(state, out bool rightBoolValue)) + else if (LeftChild.TryBoolEvaluate(state, out bool leftBoolValue, loggingContext) && RightChild.TryBoolEvaluate(state, out bool rightBoolValue, loggingContext)) { return Compare(leftBoolValue, rightBoolValue); } - string leftExpandedValue = LeftChild.GetExpandedValue(state); - string rightExpandedValue = RightChild.GetExpandedValue(state); + string leftExpandedValue = LeftChild.GetExpandedValue(state, loggingContext); + string rightExpandedValue = RightChild.GetExpandedValue(state, loggingContext); ProjectErrorUtilities.VerifyThrowInvalidProject (leftExpandedValue != null && rightExpandedValue != null, diff --git a/src/Build/Evaluation/Conditionals/NotExpressionNode.cs b/src/Build/Evaluation/Conditionals/NotExpressionNode.cs index e5b00ac781a..1d2e1d6311e 100644 --- a/src/Build/Evaluation/Conditionals/NotExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NotExpressionNode.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; using System.Diagnostics; @@ -18,9 +19,9 @@ internal sealed class NotExpressionNode : OperatorExpressionNode /// /// Evaluate as boolean /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) + internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { - if (!LeftChild.TryBoolEvaluate(state, out bool boolValue)) + if (!LeftChild.TryBoolEvaluate(state, out bool boolValue, loggingContext)) { ProjectErrorUtilities.ThrowInvalidProject( state.ElementLocation, @@ -44,9 +45,9 @@ internal override string GetUnexpandedValue(ConditionEvaluator.IConditionEvaluat /// /// Returns expanded value with '!' prepended. Useful for error messages. /// - internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state) + internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { - return "!" + LeftChild.GetExpandedValue(state); + return "!" + LeftChild.GetExpandedValue(state, loggingContext); } internal override string DebuggerDisplay => $"(not {LeftChild.DebuggerDisplay})"; diff --git a/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs index 02c242ceef5..f161ad5d18b 100644 --- a/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericComparisonExpressionNode.cs @@ -2,7 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; - +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; #nullable disable @@ -38,7 +38,7 @@ internal abstract class NumericComparisonExpressionNode : OperatorExpressionNode /// /// Evaluate as boolean /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) + internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { bool isLeftNum = LeftChild.TryNumericEvaluate(state, out double leftNum); bool isLeftVersion = LeftChild.TryVersionEvaluate(state, out Version leftVersion); @@ -53,7 +53,7 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state.Condition, /* helpfully display unexpanded token and expanded result in error message */ isLeftNum ? RightChild.GetUnexpandedValue(state) : LeftChild.GetUnexpandedValue(state), - isLeftNum ? RightChild.GetExpandedValue(state) : LeftChild.GetExpandedValue(state)); + isLeftNum ? RightChild.GetExpandedValue(state, loggingContext) : LeftChild.GetExpandedValue(state, loggingContext)); } return (isLeftNum, isLeftVersion, isRightNum, isRightVersion) switch diff --git a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs index 66fa552443d..a2a12c4f41b 100644 --- a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs @@ -3,7 +3,7 @@ using System; using System.Diagnostics; - +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; #nullable disable @@ -23,7 +23,7 @@ internal NumericExpressionNode(string value) _value = value; } - internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) + internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result, LoggingContext loggingContext = null) { result = default; return false; @@ -50,7 +50,7 @@ internal override string GetUnexpandedValue(ConditionEvaluator.IConditionEvaluat /// /// Get the expanded value /// - internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state) + internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { return _value; } diff --git a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs index c6e4b4c349c..077d9d3948c 100644 --- a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using Microsoft.Build.BackEnd.Logging; #nullable disable @@ -12,13 +13,13 @@ namespace Microsoft.Build.Evaluation /// internal abstract class OperatorExpressionNode : GenericExpressionNode { - internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) + internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result, LoggingContext loggingContext = null) { - result = BoolEvaluate(state); + result = BoolEvaluate(state, loggingContext); return true; } - internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state); + internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null); internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) { @@ -36,7 +37,7 @@ internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluatio /// Value after any item and property expressions are expanded /// /// - internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state) + internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { return null; } diff --git a/src/Build/Evaluation/Conditionals/OrExpressionNode.cs b/src/Build/Evaluation/Conditionals/OrExpressionNode.cs index 250e8c9602c..6b3d17a8326 100644 --- a/src/Build/Evaluation/Conditionals/OrExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OrExpressionNode.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; using System.Diagnostics; @@ -18,9 +19,9 @@ internal sealed class OrExpressionNode : OperatorExpressionNode /// /// Evaluate as boolean /// - internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state) + internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { - if (!LeftChild.TryBoolEvaluate(state, out bool leftBool)) + if (!LeftChild.TryBoolEvaluate(state, out bool leftBool, loggingContext)) { ProjectErrorUtilities.ThrowInvalidProject( state.ElementLocation, @@ -37,7 +38,7 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState } else { - if (!RightChild.TryBoolEvaluate(state, out bool rightBool)) + if (!RightChild.TryBoolEvaluate(state, out bool rightBool, loggingContext)) { ProjectErrorUtilities.ThrowInvalidProject( state.ElementLocation, diff --git a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs index 7017b0b5023..798ecc7e32f 100644 --- a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Shared; #nullable disable @@ -30,9 +31,9 @@ internal StringExpressionNode(string value, bool expandable) _expandable = expandable; } - internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result) + internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result, LoggingContext loggingContext = null) { - return ConversionUtilities.TryConvertStringToBool(GetExpandedValue(state), out result); + return ConversionUtilities.TryConvertStringToBool(GetExpandedValue(state, loggingContext), out result); } internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) @@ -68,7 +69,7 @@ internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluatio /// to empty than to fully evaluate it. /// Implementations should cache the result so that calls after the first are free. /// - internal override bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationState state) + internal override bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { if (_cachedExpandedValue == null) { @@ -93,7 +94,7 @@ internal override bool EvaluatesToEmpty(ConditionEvaluator.IConditionEvaluationS break; } - string expandBreakEarly = state.ExpandIntoStringBreakEarly(_value); + string expandBreakEarly = state.ExpandIntoStringBreakEarly(_value, loggingContext); if (expandBreakEarly == null) { @@ -129,13 +130,13 @@ internal override string GetUnexpandedValue(ConditionEvaluator.IConditionEvaluat /// Value after any item and property expressions are expanded /// /// - internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state) + internal override string GetExpandedValue(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { if (_cachedExpandedValue == null) { if (_expandable) { - _cachedExpandedValue = state.ExpandIntoString(_value); + _cachedExpandedValue = state.ExpandIntoString(_value, loggingContext); } else { diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index 575e00478f2..4a1292e1632 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -1069,8 +1069,8 @@ private void ReadTargetElement(ProjectTargetElement targetElement, LinkedList private void AddBeforeAndAfterTargetMappings(ProjectTargetElement targetElement, Dictionary> activeTargets, Dictionary> targetsWhichRunBeforeByTarget, Dictionary> targetsWhichRunAfterByTarget) { - var beforeTargets = _expander.ExpandIntoStringListLeaveEscaped(targetElement.BeforeTargets, ExpanderOptions.ExpandPropertiesAndItems, targetElement.BeforeTargetsLocation); - var afterTargets = _expander.ExpandIntoStringListLeaveEscaped(targetElement.AfterTargets, ExpanderOptions.ExpandPropertiesAndItems, targetElement.AfterTargetsLocation); + var beforeTargets = _expander.ExpandIntoStringListLeaveEscaped(targetElement.BeforeTargets, ExpanderOptions.ExpandPropertiesAndItems, targetElement.BeforeTargetsLocation, _evaluationLoggingContext); + var afterTargets = _expander.ExpandIntoStringListLeaveEscaped(targetElement.AfterTargets, ExpanderOptions.ExpandPropertiesAndItems, targetElement.AfterTargetsLocation, _evaluationLoggingContext); foreach (string beforeTarget in beforeTargets) { @@ -2021,7 +2021,7 @@ private LoadImportsResult ExpandAndLoadImportsFromUnescapedImportExpression(stri { imports = null; - string importExpressionEscaped = _expander.ExpandIntoStringLeaveEscaped(unescapedExpression, ExpanderOptions.ExpandProperties, importElement.ProjectLocation); + string importExpressionEscaped = _expander.ExpandIntoStringLeaveEscaped(unescapedExpression, ExpanderOptions.ExpandProperties, importElement.ProjectLocation, _evaluationLoggingContext); ElementLocation importLocationInProject = importElement.Location; if (String.IsNullOrWhiteSpace(importExpressionEscaped)) diff --git a/src/Build/Evaluation/Expander.cs b/src/Build/Evaluation/Expander.cs index f46a55cde59..5c60e58b0ed 100644 --- a/src/Build/Evaluation/Expander.cs +++ b/src/Build/Evaluation/Expander.cs @@ -475,11 +475,11 @@ internal object ExpandPropertiesLeaveTypedAndEscaped(string expression, Expander /// Use this form when the result is going to be processed further, for example by matching against the file system, /// so literals must be distinguished, and you promise to unescape after that. /// - internal SemiColonTokenizer ExpandIntoStringListLeaveEscaped(string expression, ExpanderOptions options, IElementLocation elementLocation) + internal SemiColonTokenizer ExpandIntoStringListLeaveEscaped(string expression, ExpanderOptions options, IElementLocation elementLocation, LoggingContext loggingContext = null) { ErrorUtilities.VerifyThrow((options & ExpanderOptions.BreakOnNotEmpty) == 0, "not supported"); - return ExpressionShredder.SplitSemiColonSeparatedList(ExpandIntoStringLeaveEscaped(expression, options, elementLocation)); + return ExpressionShredder.SplitSemiColonSeparatedList(ExpandIntoStringLeaveEscaped(expression, options, elementLocation, loggingContext)); } /// From 01ee110a5363b3dc9f011e9f68474290477c19dd Mon Sep 17 00:00:00 2001 From: Forgind Date: Fri, 13 May 2022 17:26:46 -0700 Subject: [PATCH 32/37] One more case --- .../Conditionals/GenericExpressionNode.cs | 6 +++--- .../Conditionals/NumericExpressionNode.cs | 4 ++-- .../Conditionals/OperatorExpressionNode.cs | 4 ++-- .../Conditionals/StringExpressionNode.cs | 16 ++++++++-------- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs index 0291475add6..50efb172cf7 100644 --- a/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/GenericExpressionNode.cs @@ -15,8 +15,8 @@ namespace Microsoft.Build.Evaluation internal abstract class GenericExpressionNode { internal abstract bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, out bool result, LoggingContext loggingContext = null); - internal abstract bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result); - internal abstract bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result); + internal abstract bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result, LoggingContext loggingContext = null); + internal abstract bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result, LoggingContext loggingContext = null); /// /// Returns true if this node evaluates to an empty string, @@ -62,7 +62,7 @@ internal bool Evaluate(ConditionEvaluator.IConditionEvaluationState state, Loggi state.ElementLocation, "ConditionNotBooleanDetail", state.Condition, - GetExpandedValue(state)); + GetExpandedValue(state, loggingContext)); } return boolValue; diff --git a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs index a2a12c4f41b..7725d9962dc 100644 --- a/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/NumericExpressionNode.cs @@ -29,12 +29,12 @@ internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationSt return false; } - internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result, LoggingContext loggingContext = null) { return ConversionUtilities.TryConvertDecimalOrHexToDouble(_value, out result); } - internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result) + internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result, LoggingContext loggingContext = null) { return Version.TryParse(_value, out result); } diff --git a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs index 077d9d3948c..a3f76ff20ff 100644 --- a/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OperatorExpressionNode.cs @@ -21,13 +21,13 @@ internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationSt internal abstract bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null); - internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result, LoggingContext loggingContext = null) { result = default; return false; } - internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result) + internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result, LoggingContext loggingContext = null) { result = default; return false; diff --git a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs index 798ecc7e32f..43dd324e7a8 100644 --- a/src/Build/Evaluation/Conditionals/StringExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/StringExpressionNode.cs @@ -36,29 +36,29 @@ internal override bool TryBoolEvaluate(ConditionEvaluator.IConditionEvaluationSt return ConversionUtilities.TryConvertStringToBool(GetExpandedValue(state, loggingContext), out result); } - internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result) + internal override bool TryNumericEvaluate(ConditionEvaluator.IConditionEvaluationState state, out double result, LoggingContext loggingContext = null) { - if (ShouldBeTreatedAsVisualStudioVersion(state)) + if (ShouldBeTreatedAsVisualStudioVersion(state, loggingContext)) { result = ConversionUtilities.ConvertDecimalOrHexToDouble(MSBuildConstants.CurrentVisualStudioVersion); return true; } else { - return ConversionUtilities.TryConvertDecimalOrHexToDouble(GetExpandedValue(state), out result); + return ConversionUtilities.TryConvertDecimalOrHexToDouble(GetExpandedValue(state, loggingContext), out result); } } - internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result) + internal override bool TryVersionEvaluate(ConditionEvaluator.IConditionEvaluationState state, out Version result, LoggingContext loggingContext = null) { - if (ShouldBeTreatedAsVisualStudioVersion(state)) + if (ShouldBeTreatedAsVisualStudioVersion(state, loggingContext)) { result = Version.Parse(MSBuildConstants.CurrentVisualStudioVersion); return true; } else { - return Version.TryParse(GetExpandedValue(state), out result); + return Version.TryParse(GetExpandedValue(state, loggingContext), out result); } } @@ -169,7 +169,7 @@ internal override void ResetState() /// but now cause the project to throw InvalidProjectException when /// ToolsVersion is "Current". https://github.com/dotnet/msbuild/issues/4150 /// - private bool ShouldBeTreatedAsVisualStudioVersion(ConditionEvaluator.IConditionEvaluationState state) + private bool ShouldBeTreatedAsVisualStudioVersion(ConditionEvaluator.IConditionEvaluationState state, LoggingContext loggingContext = null) { if (!_shouldBeTreatedAsVisualStudioVersion.HasValue) { @@ -177,7 +177,7 @@ private bool ShouldBeTreatedAsVisualStudioVersion(ConditionEvaluator.IConditionE // Do this check first, because if it's not (common) we can early-out and the next // expansion will be cheap because this will populate the cached expanded value. - if (string.Equals(GetExpandedValue(state), MSBuildConstants.CurrentToolsVersion, StringComparison.Ordinal)) + if (string.Equals(GetExpandedValue(state, loggingContext), MSBuildConstants.CurrentToolsVersion, StringComparison.Ordinal)) { // and it is just an expansion of MSBuildToolsVersion _shouldBeTreatedAsVisualStudioVersion = string.Equals(_value, "$(MSBuildToolsVersion)", StringComparison.OrdinalIgnoreCase); From ad9232378a5b9a3749a753982ccda222b358ab74 Mon Sep 17 00:00:00 2001 From: Forgind Date: Mon, 16 May 2022 11:15:48 -0700 Subject: [PATCH 33/37] Fix another case --- src/Build/Evaluation/Evaluator.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index 4a1292e1632..ebce24983ad 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -2446,7 +2446,8 @@ private bool EvaluateCondition(ProjectElement element, string condition, Expande element.ConditionLocation, _evaluationLoggingContext.LoggingService, _evaluationLoggingContext.BuildEventContext, - _evaluationContext.FileSystem + _evaluationContext.FileSystem, + loggingContext: _evaluationLoggingContext ); return result; From 0316ba7c63e9c0971e60146a54138baed1934a04 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 17 May 2022 13:48:10 -0700 Subject: [PATCH 34/37] Fix condition on task execution --- src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs index ec05dfd174f..4835778b4c1 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs @@ -381,7 +381,8 @@ private async Task ExecuteBucket(TaskHost taskHost, ItemBucket b _targetChildInstance.ConditionLocation, _targetLoggingContext.LoggingService, _targetLoggingContext.BuildEventContext, - FileSystems.Default); + FileSystems.Default, + loggingContext: _targetLoggingContext); if (!condition) { @@ -623,7 +624,7 @@ private void LogSkippedTask(ItemBucket bucket, TaskExecutionMode howToExecuteTas if (!_targetLoggingContext.LoggingService.OnlyLogCriticalEvents) { // Expand the expression for the Log. Since we know the condition evaluated to false, leave unexpandable properties in the condition so as not to cause an error - string expanded = bucket.Expander.ExpandIntoStringAndUnescape(_targetChildInstance.Condition, ExpanderOptions.ExpandAll | ExpanderOptions.LeavePropertiesUnexpandedOnError | ExpanderOptions.Truncate, _targetChildInstance.ConditionLocation); + string expanded = bucket.Expander.ExpandIntoStringAndUnescape(_targetChildInstance.Condition, ExpanderOptions.ExpandAll | ExpanderOptions.LeavePropertiesUnexpandedOnError | ExpanderOptions.Truncate, _targetChildInstance.ConditionLocation, loggingContext: _targetLoggingContext); // Whilst we are within the processing of the task, we haven't actually started executing it, so // our skip task message needs to be in the context of the target. However any errors should be reported From 44aa9b660da4baec4b252a37cfabff32e214bd21 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 17 May 2022 14:51:37 -0700 Subject: [PATCH 35/37] Fixed other cases mentioned in PR --- .../Evaluation/Conditionals/OrExpressionNode.cs | 2 +- src/Build/Evaluation/ItemSpec.cs | 12 ++++++++---- .../LazyItemEvaluator.LazyItemOperation.cs | 2 +- src/Build/Evaluation/LazyItemEvaluator.cs | 12 +++++++----- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/Build/Evaluation/Conditionals/OrExpressionNode.cs b/src/Build/Evaluation/Conditionals/OrExpressionNode.cs index 6b3d17a8326..fa1816c2e1d 100644 --- a/src/Build/Evaluation/Conditionals/OrExpressionNode.cs +++ b/src/Build/Evaluation/Conditionals/OrExpressionNode.cs @@ -27,7 +27,7 @@ internal override bool BoolEvaluate(ConditionEvaluator.IConditionEvaluationState state.ElementLocation, "ExpressionDoesNotEvaluateToBoolean", LeftChild.GetUnexpandedValue(state), - LeftChild.GetExpandedValue(state), + LeftChild.GetExpandedValue(state, loggingContext), state.Condition); } diff --git a/src/Build/Evaluation/ItemSpec.cs b/src/Build/Evaluation/ItemSpec.cs index 65c1fd8b486..d7f58c8088a 100644 --- a/src/Build/Evaluation/ItemSpec.cs +++ b/src/Build/Evaluation/ItemSpec.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Globbing; using Microsoft.Build.Internal; using Microsoft.Build.Shared; @@ -156,21 +157,23 @@ private bool InitReferencedItemsIfNecessary() /// The xml location the itemspec comes from /// The directory that the project is in. /// Expand properties before breaking down fragments. Defaults to true + /// Context in which to log public ItemSpec( string itemSpec, Expander expander, IElementLocation itemSpecLocation, string projectDirectory, - bool expandProperties = true) + bool expandProperties = true, + LoggingContext loggingContext = null) { ItemSpecString = itemSpec; Expander = expander; ItemSpecLocation = itemSpecLocation; - Fragments = BuildItemFragments(itemSpecLocation, projectDirectory, expandProperties); + Fragments = BuildItemFragments(itemSpecLocation, projectDirectory, expandProperties, loggingContext); } - private List BuildItemFragments(IElementLocation itemSpecLocation, string projectDirectory, bool expandProperties) + private List BuildItemFragments(IElementLocation itemSpecLocation, string projectDirectory, bool expandProperties, LoggingContext loggingContext) { // Code corresponds to Evaluator.CreateItemsFromInclude var evaluatedItemspecEscaped = ItemSpecString; @@ -186,7 +189,8 @@ private List BuildItemFragments(IElementLocation itemSpecLocat evaluatedItemspecEscaped = Expander.ExpandIntoStringLeaveEscaped( ItemSpecString, ExpanderOptions.ExpandProperties, - itemSpecLocation); + itemSpecLocation, + loggingContext); } var semicolonCount = 0; diff --git a/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs index f78cc28be71..f0c19a5c681 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs @@ -262,7 +262,7 @@ protected void DecorateItemsWithMetadata(IEnumerable itemBa continue; } - string evaluatedValue = _expander.ExpandIntoStringLeaveEscaped(metadataElement.Value, metadataExpansionOptions, metadataElement.Location); + string evaluatedValue = _expander.ExpandIntoStringLeaveEscaped(metadataElement.Value, metadataExpansionOptions, metadataElement.Location, _lazyEvaluator._loggingContext); evaluatedValue = FileUtilities.MaybeAdjustFilePath(evaluatedValue, metadataElement.ContainingProject.DirectoryPath); metadataTable.SetValue(metadataElement, evaluatedValue); diff --git a/src/Build/Evaluation/LazyItemEvaluator.cs b/src/Build/Evaluation/LazyItemEvaluator.cs index 28fe97350da..871ad820e6a 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.cs @@ -99,7 +99,8 @@ LazyItemEvaluator lazyEvaluator element.ConditionLocation, lazyEvaluator._loggingContext.LoggingService, lazyEvaluator._loggingContext.BuildEventContext, - lazyEvaluator.FileSystem + lazyEvaluator.FileSystem, + loggingContext: lazyEvaluator._loggingContext ); MSBuildEventSource.Log.EvaluateConditionStop(condition, result); @@ -624,7 +625,7 @@ private RemoveOperation BuildRemoveOperation(string rootDirectory, ProjectItemEl private void ProcessItemSpec(string rootDirectory, string itemSpec, IElementLocation itemSpecLocation, OperationBuilder builder) { - builder.ItemSpec = new ItemSpec(itemSpec, _outerExpander, itemSpecLocation, rootDirectory); + builder.ItemSpec = new ItemSpec(itemSpec, _outerExpander, itemSpecLocation, rootDirectory, loggingContext: _loggingContext); foreach (ItemSpecFragment fragment in builder.ItemSpec.Fragments) { @@ -635,7 +636,7 @@ private void ProcessItemSpec(string rootDirectory, string itemSpec, IElementLoca } } - private static IEnumerable GetExpandedMetadataValuesAndConditions(ICollection metadata, Expander expander) + private static IEnumerable GetExpandedMetadataValuesAndConditions(ICollection metadata, Expander expander, LoggingContext loggingContext = null) { // Since we're just attempting to expand properties in order to find referenced items and not expanding metadata, // unexpected errors may occur when evaluating property functions on unexpanded metadata. Just ignore them if that happens. @@ -649,7 +650,8 @@ private static IEnumerable GetExpandedMetadataValuesAndConditions(IColle yield return expander.ExpandIntoStringLeaveEscaped( metadatumElement.Value, expanderOptions, - metadatumElement.Location); + metadatumElement.Location, + loggingContext); yield return expander.ExpandIntoStringLeaveEscaped( metadatumElement.Condition, @@ -664,7 +666,7 @@ private void ProcessMetadataElements(ProjectItemElement itemElement, OperationBu { operationBuilder.Metadata.AddRange(itemElement.Metadata); - var itemsAndMetadataFound = ExpressionShredder.GetReferencedItemNamesAndMetadata(GetExpandedMetadataValuesAndConditions(itemElement.Metadata, _expander)); + var itemsAndMetadataFound = ExpressionShredder.GetReferencedItemNamesAndMetadata(GetExpandedMetadataValuesAndConditions(itemElement.Metadata, _expander, _loggingContext)); if (itemsAndMetadataFound.Items != null) { foreach (var itemType in itemsAndMetadataFound.Items) From f7ccf3d4a7308a20604b795e33b7e682d4cd5bfa Mon Sep 17 00:00:00 2001 From: Forgind Date: Fri, 27 May 2022 10:54:41 -0700 Subject: [PATCH 36/37] PR comments --- src/Build.UnitTests/BinaryLogger_Tests.cs | 4 ---- src/Build.UnitTests/Evaluation/Evaluator_Tests.cs | 2 +- src/Framework/Traits.cs | 7 +++++-- src/MSBuildTaskHost/MSBuildTaskHost.csproj | 1 + src/Shared/UnitTests/EngineTestEnvironment.cs | 2 -- 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/Build.UnitTests/BinaryLogger_Tests.cs b/src/Build.UnitTests/BinaryLogger_Tests.cs index 0ec915e2710..71983787418 100644 --- a/src/Build.UnitTests/BinaryLogger_Tests.cs +++ b/src/Build.UnitTests/BinaryLogger_Tests.cs @@ -125,10 +125,6 @@ public void TestBinaryLoggerRoundtrip(string projectText) var serialActual = serialFromPlaybackText.ToString(); var parallelExpected = parallelFromBuildText.ToString(); var parallelActual = parallelFromPlaybackText.ToString(); - serialExpected = serialExpected.Substring(serialExpected.IndexOf("Project")); - serialActual = serialActual.Substring(serialActual.IndexOf("Project")); - parallelExpected = parallelExpected.Substring(parallelExpected.IndexOf("Project")); - parallelActual = parallelActual.Substring(parallelActual.IndexOf("Project")); serialActual.ShouldContainWithoutWhitespace(serialExpected); parallelActual.ShouldContainWithoutWhitespace(parallelExpected); diff --git a/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs b/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs index 2e36789b6dd..cd869baccc2 100644 --- a/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs +++ b/src/Build.UnitTests/Evaluation/Evaluator_Tests.cs @@ -4518,7 +4518,7 @@ public void VerifyMSBuildLogsAMessageWhenLocalPropertyCannotOverrideValueOfGloba [Fact] public void VerifyPropertyTrackingLoggingDefault() { - // Having nothing defined should default to nothing being logged. + // Having just environment variables defined should default to nothing being logged except one environment variable read. this.VerifyPropertyTrackingLoggingScenario( null, logger => diff --git a/src/Framework/Traits.cs b/src/Framework/Traits.cs index 11e24117e6b..e9f5083db68 100644 --- a/src/Framework/Traits.cs +++ b/src/Framework/Traits.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; -using Microsoft.Build.Framework; #nullable disable @@ -95,7 +94,11 @@ public Traits() /// /// Log all environment variables whether or not they are used in a build in the binary log. /// - public readonly bool LogAllEnvironmentVariables = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MSBUILDLOGALLENVIRONMENTVARIABLES")); + public readonly bool LogAllEnvironmentVariables = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MSBUILDLOGALLENVIRONMENTVARIABLES")) +#if !TASKHOST + && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_4) +#endif + ; /// /// Log property tracking information. diff --git a/src/MSBuildTaskHost/MSBuildTaskHost.csproj b/src/MSBuildTaskHost/MSBuildTaskHost.csproj index 228ba8acafe..ec7f04055ae 100644 --- a/src/MSBuildTaskHost/MSBuildTaskHost.csproj +++ b/src/MSBuildTaskHost/MSBuildTaskHost.csproj @@ -39,6 +39,7 @@ BuildEnvironmentHelper.cs + EnvironmentVariableReadEventArgs.cs diff --git a/src/Shared/UnitTests/EngineTestEnvironment.cs b/src/Shared/UnitTests/EngineTestEnvironment.cs index 5f895e75fd4..649d0ff35db 100644 --- a/src/Shared/UnitTests/EngineTestEnvironment.cs +++ b/src/Shared/UnitTests/EngineTestEnvironment.cs @@ -235,9 +235,7 @@ private bool BuildProject( foreach (var pair in pairs) { var expectedText = pair.expected.textGetter(); - expectedText = expectedText.Substring(expectedText.IndexOf("Project")); var actualText = pair.actual.textGetter(); - actualText = actualText.Substring(actualText.IndexOf("Project")); actualText.ShouldContainWithoutWhitespace(expectedText); } } From 22525b63efacc65aab8d3a52aac116b351b725f0 Mon Sep 17 00:00:00 2001 From: Forgind Date: Tue, 31 May 2022 10:45:56 -0700 Subject: [PATCH 37/37] Using statement --- src/Build.UnitTests/Utilities_Tests.cs | 48 ++++++++++++-------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/src/Build.UnitTests/Utilities_Tests.cs b/src/Build.UnitTests/Utilities_Tests.cs index b4787dd1eb6..6159f3af2c1 100644 --- a/src/Build.UnitTests/Utilities_Tests.cs +++ b/src/Build.UnitTests/Utilities_Tests.cs @@ -81,43 +81,41 @@ public UtilitiesTestReadOnlyLoad() [Fact] public void CommentsInPreprocessing() { - using (TestEnvironment env = TestEnvironment.Create()) - { - XmlDocumentWithLocation.ClearReadOnlyFlags_UnitTestsOnly(); + using TestEnvironment env = TestEnvironment.Create(); + XmlDocumentWithLocation.ClearReadOnlyFlags_UnitTestsOnly(); - TransientTestFile inputFile = env.CreateFile("tempInput.tmp", ObjectModelHelpers.CleanupFileContents(@" + TransientTestFile inputFile = env.CreateFile("tempInput.tmp", ObjectModelHelpers.CleanupFileContents(@" - + ")); - TransientTestFile outputFile = env.CreateFile("tempOutput.tmp"); + TransientTestFile outputFile = env.CreateFile("tempOutput.tmp"); - env.SetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE", "1"); + env.SetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE", "1"); #if FEATURE_GET_COMMANDLINE - MSBuildApp.Execute(@"c:\bin\msbuild.exe """ + inputFile.Path + - (NativeMethodsShared.IsUnixLike ? @""" -pp:""" : @""" /pp:""") + outputFile.Path + @"""") - .ShouldBe(MSBuildApp.ExitType.Success); + MSBuildApp.Execute(@"c:\bin\msbuild.exe """ + inputFile.Path + + (NativeMethodsShared.IsUnixLike ? @""" -pp:""" : @""" /pp:""") + outputFile.Path + @"""") + .ShouldBe(MSBuildApp.ExitType.Success); #else - Assert.Equal( - MSBuildApp.ExitType.Success, - MSBuildApp.Execute( - new[] { @"c:\bin\msbuild.exe", '"' + inputFile.Path + '"', - '"' + (NativeMethodsShared.IsUnixLike ? "-pp:" : "/pp:") + outputFile.Path + '"'})); + Assert.Equal( + MSBuildApp.ExitType.Success, + MSBuildApp.Execute( + new[] { @"c:\bin\msbuild.exe", '"' + inputFile.Path + '"', + '"' + (NativeMethodsShared.IsUnixLike ? "-pp:" : "/pp:") + outputFile.Path + '"'})); #endif - bool foundDoNotModify = false; - foreach (string line in File.ReadLines(outputFile.Path)) - { - line.ShouldNotContain("", "This is what it will look like if we're loading read/only"); + bool foundDoNotModify = false; + foreach (string line in File.ReadLines(outputFile.Path)) + { + line.ShouldNotContain("", "This is what it will look like if we're loading read/only"); - if (line.Contains("DO NOT MODIFY")) // this is in a comment in our targets - { - foundDoNotModify = true; - } + if (line.Contains("DO NOT MODIFY")) // this is in a comment in our targets + { + foundDoNotModify = true; } - - foundDoNotModify.ShouldBeTrue(); } + + foundDoNotModify.ShouldBeTrue(); } [Fact]