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.

Sign up for GitHub

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

Jump to bottom

Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors #6174

Merged
merged 32 commits into from Mar 13, 2021
Merged

Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors #6174

Show file tree
Hide file tree
Changes from 11 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
4 changes: 4 additions & 0 deletions ref/Microsoft.Build.Framework/net/Microsoft.Build.Framework.cs
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions ref/Microsoft.Build.Framework/netstandard/Microsoft.Build.Framework.cs
View file
Open in desktop
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
1 change: 1 addition & 0 deletions ref/Microsoft.Build.Utilities.Core/net/Microsoft.Build.Utilities.Core.cs
View file
Open in desktop
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.IBuildEngine8 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
1 change: 1 addition & 0 deletions ref/Microsoft.Build.Utilities.Core/netstandard/Microsoft.Build.Utilities.Core.cs
View file
Open in desktop
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.IBuildEngine8 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
28 changes: 28 additions & 0 deletions src/Build.UnitTests/BackEnd/LogWarningReturnHasLoggedError.cs
View file
Open in desktop
@@ -0,0 +1,28 @@
// 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.Framework;
using Microsoft.Build.Utilities;
namespace Microsoft.Build.UnitTests
{
/// <summary>
/// This task was created for https://github.com/microsoft/msbuild/issues/2036
/// </summary>
public class LogWarningReturnHasLoggedError : Task
{
[Required]
public string WarningCode { get; set; }

/// <summary>
/// Log a warning and return whether or not the TaskLoggingHelper knows it was turned into an error.
/// </summary>
/// <returns></returns>
public override bool Execute()
{
Log.LogWarning(null, WarningCode, null, null, 0, 0, 0, 0, "Warning Logged!", null);

// This is what tasks should return by default.
return !Log.HasLoggedErrors;
}
}
}
5 changes: 5 additions & 0 deletions src/Build.UnitTests/BackEnd/MockLoggingService.cs
View file
Open in desktop
Expand Up @@ -557,6 +557,11 @@ public bool HasBuildSubmissionLoggedErrors(int submissionId)
return false;
}

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

#endregion
}
}
89 changes: 86 additions & 3 deletions src/Build.UnitTests/BackEnd/TaskHostConfiguration_Tests.cs
View file
Open in desktop
Expand Up @@ -5,14 +5,15 @@
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading;


using Microsoft.Build.BackEnd;
using Microsoft.Build.Framework;
using Microsoft.Build.Shared;
using Microsoft.Build.Utilities;

using Shouldly;
using Xunit;

namespace Microsoft.Build.UnitTests.BackEnd
Expand Down Expand Up @@ -51,6 +52,7 @@ public void ConstructorWithNullName()
null,
@"c:\my tasks\mytask.dll",
null,
null,
null);
}
);
Expand Down Expand Up @@ -79,6 +81,7 @@ public void ConstructorWithEmptyName()
String.Empty,
@"c:\my tasks\mytask.dll",
null,
null,
null);
}
);
Expand Down Expand Up @@ -107,6 +110,7 @@ public void ConstructorWithNullLocation()
"TaskName",
null,
null,
null,
null);
}
);
Expand Down Expand Up @@ -137,6 +141,7 @@ public void ConstructorWithEmptyLocation()
"TaskName",
String.Empty,
null,
null,
null);
}
);
Expand Down Expand Up @@ -165,6 +170,7 @@ public void TestValidConstructors()
"TaskName",
@"c:\MyTasks\MyTask.dll",
null,
null,
null);

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

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

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

HashSet<string> WarningsAsErrors = new HashSet<string>();
WarningsAsErrors.Add("MSB1234");
WarningsAsErrors.Add("MSB1235");
WarningsAsErrors.Add("MSB1236");
WarningsAsErrors.Add("MSB1237");

TaskHostConfiguration config5 = new TaskHostConfiguration(
1,
Directory.GetCurrentDirectory(),
null,
Thread.CurrentThread.CurrentCulture,
Thread.CurrentThread.CurrentUICulture,
#if FEATURE_APPDOMAIN
null,
#endif
1,
1,
@"c:\my project\myproj.proj",
_continueOnErrorDefault,
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters2,
null,
WarningsAsErrors);
}

/// <summary>
Expand Down Expand Up @@ -257,7 +291,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 +330,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 +374,7 @@ public void TestTranslationWithValueTypesInDictionary()
"TaskName",
@"c:\MyTasks\MyTask.dll",
parameters,
null,
null);

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

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
Expand Down Expand Up @@ -419,6 +457,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 All @@ -439,6 +478,50 @@ public void TestTranslationWithITaskItemArrayInDictionary()
TaskHostPacketHelpers.AreEqual(itemArray, deserializedItemArray);
}

/// <summary>
/// Test serialization / deserialization when the parameter dictionary contains an ITaskItem array.
/// </summary>
[Fact]
public void TestTranslationWithWarningsAsErrors()
{
HashSet<string> WarningsAsErrors = new HashSet<string>();
WarningsAsErrors.Add("MSB1234");
WarningsAsErrors.Add("MSB1235");
WarningsAsErrors.Add("MSB1236");
WarningsAsErrors.Add("MSB1237");
TaskHostConfiguration config = new TaskHostConfiguration(
1,
Directory.GetCurrentDirectory(),
null,
Thread.CurrentThread.CurrentCulture,
Thread.CurrentThread.CurrentUICulture,
#if FEATURE_APPDOMAIN
null,
#endif
1,
1,
@"c:\my project\myproj.proj",
_continueOnErrorDefault,
"TaskName",
@"c:\MyTasks\MyTask.dll",
null,
null,
WarningsAsErrors);

((ITranslatable)config).Translate(TranslationHelpers.GetWriteTranslator());
INodePacket packet = TaskHostConfiguration.FactoryForDeserialization(TranslationHelpers.GetReadTranslator());

TaskHostConfiguration deserializedConfig = packet as TaskHostConfiguration;

Assert.Equal(config.TaskName, deserializedConfig.TaskName);
#if !FEATURE_ASSEMBLYLOADCONTEXT
Assert.Equal(config.TaskLocation, deserializedConfig.TaskLocation);
#endif
Assert.NotNull(deserializedConfig.WarningsAsErrors);
config.WarningsAsErrors.SequenceEqual(deserializedConfig.WarningsAsErrors, StringComparer.Ordinal).ShouldBeTrue();

}

/// <summary>
/// Helper methods for testing the task host-related packets.
/// </summary>
Expand Down
28 changes: 28 additions & 0 deletions src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs
View file
Open in desktop
Expand Up @@ -272,6 +272,34 @@ private string GetTestProject(bool? treatAllWarningsAsErrors = null, string warn
</Project>";
}

[Fact]
public void WarningsAsErrors_ExpectBuildToStopWhenTaskLogsWarningAsError()
{
using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<UsingTask TaskName = ""LogWarningReturnHasLoggedError"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<PropertyGroup>
<MSBuildWarningsAsErrors>MSB1234</MSBuildWarningsAsErrors>
</PropertyGroup>
<Target Name='Build'>
<LogWarningReturnHasLoggedError WarningCode=""MSB1234""/>
<ReturnFailureWithoutLoggingErrorTask/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectFailure();

logger.WarningCount.ShouldBe(0);
logger.ErrorCount.ShouldBe(1);

// The build should STOP when a task logs an error, make sure ReturnFailureWithoutLoggingErrorTask doesn't run.
logger.AssertLogDoesntContain("MSB4181");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may not work with tasks that implement ITask directly. Can you please add another test just like this one but calling a task implementing ITask directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. Going to brain dump this before making the test.

Someone implementing ITask wouldn't have access to a TaskLoggingHelper (like Task does) and thus have to log a warning through the BuildEngine.LogWarningEvent. The build engine would log it to the logging service as a warning and at LoggingService.RouteBuildEvent it would be translated into an error.

But I think the build would continue because it entirely depends on what the task returned right? So long as the task returns true despite logging an error, the build will complete.

Question, what context would someone implement itask vs inheriting from task?

Copy link
Contributor

@cdmihai cdmihai Feb 23, 2021 •
edited

Choose a reason for hiding this comment

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

Don't really know why people would do it, but according to this GH search they do it.

So long as the task returns true despite logging an error, the build will complete.

I think MSBuild might either warn or error that the ITask instance returned true but an error was logged. Not super sure about this though, so worth writing a test to pin the behaviour :)
On the other hand, since an ITask implementation cannot know whether any errors were logged (because it does not have access to Log), it can't really condition its return value. So maybe this whole scenario does not make sense and we can ignore it.

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 agree that there's no need to check this on an ITask. Something interesting about the error task. It explicitly returns false otherwise the build would continue:

msbuild/src/Tasks/Error.cs

Line 47 in f0eebf2

// careful to return false. Otherwise the build would continue.
that's a very strange disconnect that sounds to me like it should be an issue to fix. But if that's just how msbuild has been since the beginning of time...should we consider fixing it?

Copy link
Contributor

@cdmihai cdmihai Feb 23, 2021 •
edited

Choose a reason for hiding this comment

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

It comes from an era where an error was an error, so returning false made sense :)

I'd wait until somebody reports an issue and then update both Error and Warning.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, why is that wrong? A task returns false if it logged an error. The Error task is designed to log an error, hence always returns "HasLoggedAnError."

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 should have included it in the code sample, but the error task called Log.LogError and simply returns false, not Log.HasLoggedErrors.

Edit: So if the error task returned true, the build would continue despite it having logged an error. So we'd see the rest of the build but we'd get a Build Failed.

Copy link
Member

@Forgind Forgind Feb 24, 2021 •
edited

Choose a reason for hiding this comment

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

Right...it has always logged an error because it logs an error right before the return statement.

Right, and people who log errors tend to want execution to stop at that point. No point continuing if something went irredeemably wrong.

}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ShouldCauseBuildFailure()
{
Expand Down
2 changes: 2 additions & 0 deletions src/Build/BackEnd/Components/Logging/ILoggingService.cs
View file
Open in desktop
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
View file
Open in desktop
Expand Up @@ -515,6 +515,18 @@ public bool HasBuildSubmissionLoggedErrors(int submissionId)
return _buildSubmissionIdsThatHaveLoggedErrors?.Contains(submissionId) == true;
}

/// <summary>
/// Returns a hashset of warnings to be logged as errors.
/// </summary>
/// <param name="context">The build context through which warnings will be logged as errors.</param>
/// <returns></returns>
public HashSet<string> GetWarningsAsErrors(BuildEventContext context)
{
int key = GetWarningsAsErrorOrMessageKey(context);

return _warningsAsErrorsByProject?[key] as HashSet<string>;
}

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
@@ -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 •
edited

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 :)

}
}