Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors #6174

Merged
merged 32 commits into from Mar 13, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
8ca466d
Add IBuildEngine8 with a hashset of all warnings to be logged as errors
benvillalobos Feb 8, 2021
ff95db2
Add GetWarningsAsErrors method to ILoggingService. TaskLoggingHelper …
benvillalobos Feb 9, 2021
16cd7f0
Try adding warningsaserrors into taskhostconfiguration so OOP taskhos…
benvillalobos Feb 16, 2021
2b61f12
TaskHostConfiguration now translates warningsaserrors
benvillalobos Feb 16, 2021
07fbbb7
Update src/Utilities/Task.cs
benvillalobos Feb 19, 2021
8da9849
run build.cmd and update ref files
benvillalobos Feb 19, 2021
8f58790
WarningsAsErrors can be null if not specified
benvillalobos Feb 19, 2021
cf90ca0
Null check on warningsaserrors instead of buildengine
benvillalobos Feb 19, 2021
fda18f5
Unit Test: Add check for tasks logging warnings that are turned into …
benvillalobos Feb 19, 2021
4a93c5a
Ensure build stops when task logs a warning->error
benvillalobos Feb 19, 2021
cdb831e
Add test for translation of taskhostconfiguration and warningsaserrors
benvillalobos Feb 20, 2021
6cd3074
Add test that shows even if a task logs an error but returns true the…
benvillalobos Feb 23, 2021
53c5b0e
BuildEngine may not be an IBE8
benvillalobos Feb 23, 2021
4af8ce6
Exclude warnings->messages from warnings->errors because the former t…
benvillalobos Feb 23, 2021
b396ed5
Consider when WarningsAsErrors is an empty set that means we treat AL…
benvillalobos Feb 24, 2021
764df59
Properly remove all warningsasmessages from warningsaserrors. handle …
benvillalobos Feb 25, 2021
fdd2419
Add customization to what CustomLogAndReturn task returns
benvillalobos Mar 1, 2021
5d7b16a
Finalize tests. Remove test that _should not work but does_.
benvillalobos Mar 1, 2021
83f1b66
Code review suggestion
benvillalobos Mar 1, 2021
63be126
Minor code cleanup
benvillalobos Mar 2, 2021
28f55cb
Explicitly call last overload of LogError
benvillalobos Mar 2, 2021
787982a
Add null check for taskloggingcontext for test compatibility
benvillalobos Mar 3, 2021
d2c230c
Add tests for task-batches
benvillalobos Mar 5, 2021
7aa9763
Minor PR feedback
benvillalobos Mar 8, 2021
febb509
Add arg names to taskhostconfig tests
benvillalobos Mar 8, 2021
0ccb51c
Reword 'thrown' to 'logged' to avoid confusion
benvillalobos Mar 8, 2021
7670dea
IBE8 now has ShouldTreatWarningAsError method
benvillalobos Mar 8, 2021
9a485da
WarningsAsErrors set turned into ICollection (taskhost side)
benvillalobos Mar 8, 2021
924423b
Binary translator converts an ICollection of strings. OOPTHN now tran…
benvillalobos Mar 8, 2021
a969d83
Rename arg to collection
benvillalobos Mar 8, 2021
b65e272
Make ICollection translate method more generic
benvillalobos Mar 8, 2021
9856364
PR changes. Adding comments.
benvillalobos Mar 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -211,6 +211,10 @@ public partial interface IBuildEngine7 : Microsoft.Build.Framework.IBuildEngine,
{
bool AllowFailureWithoutError { get; set; }
}
public partial interface IBuildEngine8 : Microsoft.Build.Framework.IBuildEngine, Microsoft.Build.Framework.IBuildEngine2, Microsoft.Build.Framework.IBuildEngine3, Microsoft.Build.Framework.IBuildEngine4, Microsoft.Build.Framework.IBuildEngine5, Microsoft.Build.Framework.IBuildEngine6, Microsoft.Build.Framework.IBuildEngine7
{
System.Collections.Generic.HashSet<string> WarningsAsErrors { get; }
}
public partial interface ICancelableTask : Microsoft.Build.Framework.ITask
{
void Cancel();
Expand Down
Expand Up @@ -211,6 +211,10 @@ public partial interface IBuildEngine7 : Microsoft.Build.Framework.IBuildEngine,
{
bool AllowFailureWithoutError { get; set; }
}
public partial interface IBuildEngine8 : Microsoft.Build.Framework.IBuildEngine, Microsoft.Build.Framework.IBuildEngine2, Microsoft.Build.Framework.IBuildEngine3, Microsoft.Build.Framework.IBuildEngine4, Microsoft.Build.Framework.IBuildEngine5, Microsoft.Build.Framework.IBuildEngine6, Microsoft.Build.Framework.IBuildEngine7
{
System.Collections.Generic.HashSet<string> WarningsAsErrors { get; }
}
public partial interface ICancelableTask : Microsoft.Build.Framework.ITask
{
void Cancel();
Expand Down
Expand Up @@ -353,6 +353,7 @@ public abstract partial class Task : Microsoft.Build.Framework.ITask
public Microsoft.Build.Framework.IBuildEngine5 BuildEngine5 { get { throw null; } }
public Microsoft.Build.Framework.IBuildEngine6 BuildEngine6 { get { throw null; } }
public Microsoft.Build.Framework.IBuildEngine7 BuildEngine7 { get { throw null; } }
public Microsoft.Build.Framework.IBuildEngine7 BuildEngine8 { get { throw null; } }
protected string HelpKeywordPrefix { get { throw null; } set { } }
public Microsoft.Build.Framework.ITaskHost HostObject { get { throw null; } set { } }
public Microsoft.Build.Utilities.TaskLoggingHelper Log { get { throw null; } }
Expand Down
Expand Up @@ -198,6 +198,7 @@ public abstract partial class Task : Microsoft.Build.Framework.ITask
public Microsoft.Build.Framework.IBuildEngine5 BuildEngine5 { get { throw null; } }
public Microsoft.Build.Framework.IBuildEngine6 BuildEngine6 { get { throw null; } }
public Microsoft.Build.Framework.IBuildEngine7 BuildEngine7 { get { throw null; } }
public Microsoft.Build.Framework.IBuildEngine7 BuildEngine8 { get { throw null; } }
protected string HelpKeywordPrefix { get { throw null; } set { } }
public Microsoft.Build.Framework.ITaskHost HostObject { get { throw null; } set { } }
public Microsoft.Build.Utilities.TaskLoggingHelper Log { get { throw null; } }
Expand Down
5 changes: 5 additions & 0 deletions src/Build.UnitTests/BackEnd/MockLoggingService.cs
Expand Up @@ -557,6 +557,11 @@ public bool HasBuildSubmissionLoggedErrors(int submissionId)
return false;
}

public HashSet<string> GetWarningsAsErrors(BuildEventContext context)
{
throw new NotImplementedException();
}

#endregion
}
}
17 changes: 15 additions & 2 deletions src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs
Expand Up @@ -51,6 +51,7 @@ public void ConstructorWithNullName()
null,
@"c:\my tasks\mytask.dll",
null,
null,
null);
}
);
Expand Down Expand Up @@ -79,6 +80,7 @@ public void ConstructorWithEmptyName()
String.Empty,
@"c:\my tasks\mytask.dll",
null,
null,
null);
}
);
Expand Down Expand Up @@ -107,6 +109,7 @@ public void ConstructorWithNullLocation()
"TaskName",
null,
null,
null,
null);
}
);
Expand Down Expand Up @@ -137,6 +140,7 @@ public void ConstructorWithEmptyLocation()
"TaskName",
String.Empty,
null,
null,
null);
}
);
Expand Down Expand Up @@ -165,6 +169,7 @@ public void TestValidConstructors()
"TaskName",
@"c:\MyTasks\MyTask.dll",
null,
null,
null);

TaskHostConfiguration config2 = new TaskHostConfiguration(
Expand All @@ -183,6 +188,7 @@ public void TestValidConstructors()
"TaskName",
@"c:\MyTasks\MyTask.dll",
null,
null,
null);

IDictionary<string, object> parameters = new Dictionary<string, object>();
Expand All @@ -202,6 +208,7 @@ public void TestValidConstructors()
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters,
null,
null);

IDictionary<string, object> parameters2 = new Dictionary<string, object>();
Expand All @@ -226,6 +233,7 @@ public void TestValidConstructors()
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters2,
null,
null);
}

Expand Down Expand Up @@ -257,7 +265,8 @@ public void TestTranslationWithNullDictionary()
"TaskName",
@"c:\MyTasks\MyTask.dll",
null,
expectedGlobalProperties);
expectedGlobalProperties,
null);

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
INodePacket packet = TaskHostConfiguration.FactoryForDeserialization(TranslationHelpers.GetReadTranslator());
Expand Down Expand Up @@ -295,7 +304,8 @@ public void TestTranslationWithEmptyDictionary()
"TaskName",
@"c:\MyTasks\MyTask.dll",
new Dictionary<string, object>(),
new Dictionary<string, string>());
new Dictionary<string, string>(),
null);

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
INodePacket packet = TaskHostConfiguration.FactoryForDeserialization(TranslationHelpers.GetReadTranslator());
Expand Down Expand Up @@ -338,6 +348,7 @@ public void TestTranslationWithValueTypesInDictionary()
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters,
null,
null);

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
Expand Down Expand Up @@ -379,6 +390,7 @@ public void TestTranslationWithITaskItemInDictionary()
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters,
null,
null);

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
Expand Down Expand Up @@ -419,6 +431,7 @@ public void TestTranslationWithITaskItemArrayInDictionary()
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters,
null,
benvillalobos marked this conversation as resolved.
Show resolved Hide resolved
null);

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
Expand Down
2 changes: 2 additions & 0 deletions src/Build/BackEnd/Components/Logging/ILoggingService.cs
Expand Up @@ -219,6 +219,8 @@ bool IncludeTaskInputs
/// <returns><code>true</code> if the build submission logged an errors, otherwise <code>false</code>.</returns>
bool HasBuildSubmissionLoggedErrors(int submissionId);

HashSet<string> GetWarningsAsErrors(BuildEventContext context);

#region Register

/// <summary>
Expand Down
12 changes: 12 additions & 0 deletions src/Build/BackEnd/Components/Logging/LoggingService.cs
Expand Up @@ -515,6 +515,18 @@ public bool HasBuildSubmissionLoggedErrors(int submissionId)
return _buildSubmissionIdsThatHaveLoggedErrors?.Contains(submissionId) == true;
}

public HashSet<string> GetWarningsAsErrors(BuildEventContext context)
{
int key = GetWarningsAsErrorOrMessageKey(context);

if (_warningsAsErrorsByProject.ContainsKey(key))
{
return _warningsAsErrorsByProject[key] as HashSet<string>;
}

return null;
}

public void AddWarningsAsErrors(BuildEventContext buildEventContext, ISet<string> codes)
{
lock (_lockObject)
Expand Down
9 changes: 8 additions & 1 deletion src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs
Expand Up @@ -33,7 +33,7 @@ internal class TaskHost :
#if FEATURE_APPDOMAIN
MarshalByRefObject,
#endif
IBuildEngine7
IBuildEngine8
{
/// <summary>
/// True if the "secret" environment variable MSBUILDNOINPROCNODE is set.
Expand Down Expand Up @@ -676,6 +676,13 @@ public void LogTelemetry(string eventName, IDictionary<string, string> propertie
public bool AllowFailureWithoutError { get; set; } = false;
#endregion

#region IBuildEngine8 Members
/// <summary>
/// Contains all warnings that should be logged as errors.
/// </summary>
public HashSet<string> WarningsAsErrors { get => _taskLoggingContext.LoggingService.GetWarningsAsErrors(_taskLoggingContext.BuildEventContext); }
#endregion

/// <summary>
/// Called by the internal MSBuild task.
/// Does not take the lock because it is called by another request builder thread.
Expand Down
4 changes: 3 additions & 1 deletion src/Build/Instance/TaskFactories/TaskHostTask.cs
Expand Up @@ -271,7 +271,9 @@ public bool Execute()
_taskType.Type.FullName,
AssemblyUtilities.GetAssemblyLocation(_taskType.Type.GetTypeInfo().Assembly),
_setParameters,
new Dictionary<string, string>(_buildComponentHost.BuildParameters.GlobalProperties)
new Dictionary<string, string>(_buildComponentHost.BuildParameters.GlobalProperties),
_taskLoggingContext.LoggingService.GetWarningsAsErrors(_taskLoggingContext.BuildEventContext)

);

try
Expand Down
16 changes: 16 additions & 0 deletions src/Framework/IBuildEngine8.cs
@@ -0,0 +1,16 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;

namespace Microsoft.Build.Framework
{
/// <summary>
/// This interface extends <see cref="IBuildEngine7" /> to allow tasks know if a warning
/// they logged was turned into an error.
/// </summary>
public interface IBuildEngine8 : IBuildEngine7
{
public HashSet<string> WarningsAsErrors { get; }
Copy link
Contributor

@cdmihai cdmihai Mar 5, 2021

Choose a reason for hiding this comment

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

Do we actually need to make this a public API? Isn't it enough to just ensure that HasLoggedErrors works as expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll remove public on warningsaserrors

Copy link
Member

Choose a reason for hiding this comment

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

So do we need IBuildEngine8 then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have been confused by the original question. WarningsAsErrors needs to be visible to the tasklogginghelper so it can determine if it actually logged an error when it tries to log a warning. Without IBE8 having this hashset I'm not aware of a to get that info.

Copy link
Member

Choose a reason for hiding this comment

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

Interface members are public by default so if you remove the public keyword nothing will change, it will still be a public member on a public interface.

So I guess to answer Mihai's question we do need an IBuildEngine8 and it does need to have the WarningsAsErrors member, right? Just to confirm.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the value of the private part of the public/private split interfaces? I do like the final abstract class plan, but marcpopMSFT suggested we should wait for rainersigwald before making that kind of decision, so I shelved my planned implementation of that for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the value of the private part of the public/private split interfaces

I was assuming it somehow prevents fixing TaskLoggingHelper.HasLoggedErrors to return true when logging warnings-as-errors, but it turns out it's not needed. Why do tasks require a new API in addition to the existing TaskLoggingHelper.HasLoggedErrors? In order to allow them to inhibit logging a warning if turns out to be an error? That does not sound like safe behaviour :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, and you're right. I got so focused on the solution that I forgot about the motivating problem. That sounds reasonable to me, though—@benvillalobos, could this interface be internal? Maybe have IBuildEngine7 extend WarningsAsErrorsBuildEngine?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the public/private split is a bit ugly, I'd rather have a public API than yet another tiny wtf in the code :)

This is where I'm at with IBuildEngine. IMO we take one more "this is the way we've done it" IBE PR and stop at IBE9 when we implement the abstract class. Otherwise why would we extend private IBE interfaces after we add the abstract class?

It sounds like it would be even messier to have a single private IBE8, all previous IBE's are public, then we make the abstract class and have no use for private IBE's.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mkay, took another look at the code. So TaskLoggingHelper gets an IBuildEngine, and that's how it interacts with the TaskHost:IBuildEngine{N} which knows about warnings-as-errors from its TaskLoggingContext. Beyond keeping it public, any solution I can think of is pretty messy. So public it is then :)

}
}
11 changes: 9 additions & 2 deletions src/MSBuild/OutOfProcTaskHostNode.cs
Expand Up @@ -33,7 +33,7 @@ internal class OutOfProcTaskHostNode :
#if CLR2COMPATIBILITY
IBuildEngine3
#else
IBuildEngine7
IBuildEngine8
#endif
{
/// <summary>
Expand Down Expand Up @@ -267,6 +267,13 @@ public bool IsRunningMultipleNodes
public bool AllowFailureWithoutError { get; set; } = false;
#endregion

#region IBuildEngine8 Implementation
/// <summary>
/// Contains all warnings that should be logged as errors.
/// </summary>
public HashSet<string> WarningsAsErrors { get; private set; }
#endregion

#region IBuildEngine Implementation (Methods)

/// <summary>
Expand Down Expand Up @@ -793,7 +800,7 @@ private void RunTask(object state)
_debugCommunications = taskConfiguration.BuildProcessEnvironment.ContainsValueAndIsEqual("MSBUILDDEBUGCOMM", "1", StringComparison.OrdinalIgnoreCase);
_updateEnvironment = !taskConfiguration.BuildProcessEnvironment.ContainsValueAndIsEqual("MSBuildTaskHostDoNotUpdateEnvironment", "1", StringComparison.OrdinalIgnoreCase);
_updateEnvironmentAndLog = taskConfiguration.BuildProcessEnvironment.ContainsValueAndIsEqual("MSBuildTaskHostUpdateEnvironmentAndLog", "1", StringComparison.OrdinalIgnoreCase);

WarningsAsErrors = taskConfiguration.WarningsAsErrors;
try
{
// Change to the startup directory
Expand Down
18 changes: 17 additions & 1 deletion src/Shared/TaskHostConfiguration.cs
Expand Up @@ -85,6 +85,8 @@ internal class TaskHostConfiguration : INodePacket

private Dictionary<string, string> _globalParameters;

private HashSet<string> _warningsAsErrors;

#if FEATURE_APPDOMAIN
/// <summary>
/// Constructor
Expand All @@ -103,6 +105,7 @@ internal class TaskHostConfiguration : INodePacket
/// <param name="taskLocation">Location of the assembly the task is to be loaded from.</param>
/// <param name="taskParameters">Parameters to apply to the task.</param>
/// <param name="globalParameters">global properties for the current project.</param>
/// <param name="warningsAsErrors">Warning codes to be thrown as errors for the current project.</param>
benvillalobos marked this conversation as resolved.
Show resolved Hide resolved
#else
/// <summary>
/// Constructor
Expand All @@ -120,6 +123,7 @@ internal class TaskHostConfiguration : INodePacket
/// <param name="taskLocation">Location of the assembly the task is to be loaded from.</param>
/// <param name="taskParameters">Parameters to apply to the task.</param>
/// <param name="globalParameters">global properties for the current project.</param>
/// <param name="warningsAsErrors">Warning codes to be thrown as errors for the current project.</param>
#endif
public TaskHostConfiguration
(
Expand All @@ -138,7 +142,8 @@ public TaskHostConfiguration
string taskName,
string taskLocation,
IDictionary<string, object> taskParameters,
Dictionary<string, string> globalParameters
Dictionary<string, string> globalParameters,
HashSet<string> warningsAsErrors
)
{
ErrorUtilities.VerifyThrowInternalLength(taskName, nameof(taskName));
Expand Down Expand Up @@ -168,6 +173,7 @@ public TaskHostConfiguration
_continueOnError = continueOnError;
_taskName = taskName;
_taskLocation = taskLocation;
_warningsAsErrors = warningsAsErrors;

if (taskParameters != null)
{
Expand Down Expand Up @@ -342,6 +348,15 @@ public NodePacketType Type
{ return NodePacketType.TaskHostConfiguration; }
}

public HashSet<string> WarningsAsErrors
{
[DebuggerStepThrough]
get
{
return _warningsAsErrors;
}
}

/// <summary>
/// Translates the packet to/from binary form.
/// </summary>
Expand All @@ -364,6 +379,7 @@ public void Translate(ITranslator translator)
translator.TranslateDictionary(ref _taskParameters, StringComparer.OrdinalIgnoreCase, TaskParameter.FactoryForDeserialization);
translator.Translate(ref _continueOnError);
translator.TranslateDictionary(ref _globalParameters, StringComparer.OrdinalIgnoreCase);
translator.Translate(ref _warningsAsErrors);
}

/// <summary>
Expand Down
24 changes: 24 additions & 0 deletions src/Shared/TaskLoggingHelper.cs
Expand Up @@ -1016,6 +1016,30 @@ public void LogWarning
// that gives the user something.
bool fillInLocation = (String.IsNullOrEmpty(file) && (lineNumber == 0) && (columnNumber == 0));

if ((BuildEngine as IBuildEngine8).WarningsAsErrors.Contains(warningCode))
{
var err = new BuildErrorEventArgs
(
subcategory,
warningCode,
fillInLocation ? BuildEngine.ProjectFileOfTaskNode : file,
fillInLocation ? BuildEngine.LineNumberOfTaskNode : lineNumber,
fillInLocation ? BuildEngine.ColumnNumberOfTaskNode : columnNumber,
endLineNumber,
endColumnNumber,
message,
helpKeyword,
TaskName,
helpLink,
DateTime.UtcNow,
messageArgs
);

BuildEngine.LogErrorEvent(err);
HasLoggedErrors = true;
return;
}

var e = new BuildWarningEventArgs
(
subcategory,
Expand Down
5 changes: 5 additions & 0 deletions src/Utilities/Task.cs
Expand Up @@ -94,6 +94,11 @@ protected Task(ResourceManager taskResources, string helpKeywordPrefix)
/// </summary>
public IBuildEngine7 BuildEngine7 => (IBuildEngine7)BuildEngine;

/// <summary>
/// Retrieves the <see cref="IBuildEngine8" /> version of the build engine interface provided by the host.
/// </summary>
public IBuildEngine7 BuildEngine8 => (IBuildEngine8)BuildEngine;
benvillalobos marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// The build engine sets this property if the host IDE has associated a host object with this particular task.
/// </summary>
Expand Down