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

Ensure buildcheck lifetime per build #10649

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Ensure buildcheck lifetime per build #10649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,11 @@ public void CleanupForBuild()
throw new AggregateException(deactivateExceptions);
}

var buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance;
IBuildCheckManagerProvider buildCheckProvider = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider);
var buildCheckManager = buildCheckProvider!.Instance;
buildCheckManager.FinalizeProcessing(_nodeLoggingContext);
// Clears the instance so that next call (on node reuse) to 'GetComponent' leads to reinitialization.
buildCheckProvider.ShutdownComponent();
},
isLastTask: true);

Expand Down
2 changes: 2 additions & 0 deletions src/Build/BackEnd/Components/Logging/LoggingService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,8 @@ public void InitializeComponent(IBuildComponentHost buildComponentHost)
_onlyLogCriticalEvents = buildComponentHost.BuildParameters.OnlyLogCriticalEvents;

_serviceState = LoggingServiceState.Initialized;

_buildEngineDataRouter = (buildComponentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)?.BuildEngineDataRouter;
}
}

Expand Down
10 changes: 7 additions & 3 deletions src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,13 +798,17 @@ public void LogIncludeFile(BuildEventContext buildEventContext, string filePath)

#endregion

#nullable enable
private IBuildEngineDataRouter? _buildEngineDataRouter;

public void ProcessPropertyRead(PropertyReadInfo propertyReadInfo, CheckLoggingContext checkContext)
=> BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyRead(propertyReadInfo, checkContext);
=> _buildEngineDataRouter?.ProcessPropertyRead(propertyReadInfo, checkContext);

public void ProcessPropertyWrite(PropertyWriteInfo propertyWriteInfo, CheckLoggingContext checkContext)
=> BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext);
=> _buildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext);

public void ProcessProjectEvaluationStarted(ICheckContext checkContext, string projectFullPath)
=> BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath);
=> _buildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath);
#nullable disable
}
}
27 changes: 11 additions & 16 deletions src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,11 @@ namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
/// </summary>
internal sealed class BuildCheckManagerProvider : IBuildCheckManagerProvider
{
private static IBuildCheckManager? s_globalInstance;
private IBuildCheckManager? _instance;

internal static IBuildCheckManager GlobalInstance => s_globalInstance ?? throw new InvalidOperationException("BuildCheckManagerProvider not initialized");
public IBuildCheckManager Instance => _instance ?? new NullBuildCheckManager();

public IBuildCheckManager Instance => GlobalInstance;

public IBuildEngineDataRouter BuildEngineDataRouter => (IBuildEngineDataRouter)GlobalInstance;

public static IBuildEngineDataRouter? GlobalBuildEngineDataRouter => (IBuildEngineDataRouter?)s_globalInstance;
public IBuildEngineDataRouter BuildEngineDataRouter => (IBuildEngineDataRouter)Instance;

internal static IBuildComponent CreateComponent(BuildComponentType type)
{
Expand All @@ -46,25 +42,24 @@ public void InitializeComponent(IBuildComponentHost host)
{
ErrorUtilities.VerifyThrow(host != null, "BuildComponentHost was null");

if (s_globalInstance == null)
if (_instance == null)
{
IBuildCheckManager instance;
if (host!.BuildParameters.IsBuildCheckEnabled)
{
instance = new BuildCheckManager();
_instance = new BuildCheckManager();
}
else
{
instance = new NullBuildCheckManager();
_instance = new NullBuildCheckManager();
}

// We are fine with the possibility of double creation here - as the construction is cheap
// and without side effects and the actual backing field is effectively immutable after the first assignment.
Interlocked.CompareExchange(ref s_globalInstance, instance, null);
}
}

public void ShutdownComponent() => GlobalInstance.Shutdown();
public void ShutdownComponent()
{
_instance?.Shutdown();
_instance = null;
}

internal sealed class BuildCheckManager : IBuildCheckManager, IBuildEngineDataRouter
{
Expand Down
64 changes: 63 additions & 1 deletion src/BuildCheck.UnitTests/EndToEndTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode)
PrepareSampleProjectsAndConfig(
buildInOutOfProcessNode,
out TransientTestFile projectFile,
out _,
"PropsCheckTest.csproj");

string output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out bool success);
Expand All @@ -62,6 +63,65 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode)
Regex.Matches(output, "BC0203 .* Property").Count.ShouldBe(2);
}

[Fact]
public void ConfigChangeReflectedOnReuse()
{
PrepareSampleProjectsAndConfig(
// we need out of proc build - to test node reuse
true,
out TransientTestFile projectFile,
out TransientTestFile editorconfigFile,
"PropsCheckTest.csproj");

// Build without BuildCheck - no findings should be reported
string output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path}", out bool success);
_env.Output.WriteLine(output);
_env.Output.WriteLine("=========================");
success.ShouldBeTrue(output);
output.ShouldNotContain("BC0201");
output.ShouldNotContain("BC0202");
output.ShouldNotContain("BC0203");

// Build with BuildCheck - findings should be reported
output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out success);
_env.Output.WriteLine(output);
_env.Output.WriteLine("=========================");
success.ShouldBeTrue(output);
output.ShouldContain("warning BC0201");
output.ShouldContain("warning BC0202");
output.ShouldContain("warning BC0203");

// Flip config in editorconfig
string editorConfigChange = """

build_check.BC0201.Severity=error
build_check.BC0202.Severity=error
build_check.BC0203.Severity=error
""";

File.AppendAllText(editorconfigFile.Path, editorConfigChange);

// Build with BuildCheck - findings with new severity should be reported
output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out success);
_env.Output.WriteLine(output);
_env.Output.WriteLine("=========================");
// build should fail due to error checks
success.ShouldBeFalse(output);
output.ShouldContain("error BC0201");
output.ShouldContain("error BC0202");
output.ShouldContain("error BC0203");

// Build without BuildCheck - no findings should be reported
output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path}", out success);
_env.Output.WriteLine(output);
_env.Output.WriteLine("=========================");
success.ShouldBeTrue(output);
output.ShouldNotContain("BC0201");
output.ShouldNotContain("BC0202");
output.ShouldNotContain("BC0203");
}


[Theory]
[InlineData(true, true)]
[InlineData(false, true)]
Expand Down Expand Up @@ -467,6 +527,7 @@ private void PopulateXmlAttribute(XmlDocument doc, XmlNode node, string attribut
private void PrepareSampleProjectsAndConfig(
bool buildInOutOfProcessNode,
out TransientTestFile projectFile,
out TransientTestFile editorconfigFile,
string entryProjectAssetName,
IEnumerable<string>? supplementalAssetNames = null,
IEnumerable<(string RuleId, string Severity)>? ruleToSeverity = null,
Expand All @@ -485,7 +546,7 @@ private void PrepareSampleProjectsAndConfig(
TransientTestFile supplementalFile = _env.CreateFile(workFolder, supplementalAssetName, supplementalContent);
}

_env.CreateFile(workFolder, ".editorconfig", ReadEditorConfig(ruleToSeverity, ruleToCustomConfig, testAssetsFolderName));
editorconfigFile = _env.CreateFile(workFolder, ".editorconfig", ReadEditorConfig(ruleToSeverity, ruleToCustomConfig, testAssetsFolderName));

// OSX links /var into /private, which makes Path.GetTempPath() return "/var..." but Directory.GetCurrentDirectory return "/private/var...".
// This discrepancy breaks path equality checks in MSBuild checks if we pass to MSBuild full path to the initial project.
Expand Down Expand Up @@ -514,6 +575,7 @@ private void PrepareSampleProjectsAndConfig(
=> PrepareSampleProjectsAndConfig(
buildInOutOfProcessNode,
out projectFile,
out _,
"Project1.csproj",
new[] { "Project2.csproj", "ImportedFile1.props" },
ruleToSeverity,
Expand Down