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

Fix build check build submission errors #10227

Merged
merged 7 commits into from
Jul 9, 2024
Merged

Fix build check build submission errors #10227

merged 7 commits into from
Jul 9, 2024

Conversation

JaynieBai
Copy link
Member

@JaynieBai JaynieBai commented Jun 12, 2024

Fixes #10071

Context

While building with buildcheck enabled and MSBUILDNOINPROCNODE=1
Severity of the rule set to Error, does not reported to the all build:
image
This is since buildcheck error is not added into the _warningsAsErrorsByProject, WarningsAsErrors == null && _warningsAsErrorsByProject == null is true all the time. so HasBuildSubmissionLoggedErrors always return false.

public bool HasBuildSubmissionLoggedErrors(int submissionId)
{
// Warnings as errors are not tracked if the user did not specify to do so
if (WarningsAsErrors == null && _warningsAsErrorsByProject == null)
{
return false;
}
// Determine if any of the event sinks have logged an error with this submission ID
return _buildSubmissionIdsThatHaveLoggedErrors?.Contains(submissionId) == true;
}

Treat warning as errors or message, the buildcheckResultWarning doesn't initialize the code. So when the code of BuildWarningEventArgs is null. ShouldTreatWarningAsError returns false all the time.

/// <returns><code>true</code> if the warning should be treated as an error, otherwise <code>false</code>.</returns>
private bool ShouldTreatWarningAsError(BuildWarningEventArgs warningEvent)
{
// This only applies if the user specified /warnaserror from the command-line or added an empty set through the object model
if (WarningsAsErrors != null)
{
// Global warnings as errors apply to all projects. If the list is empty or contains the code, the warning should be treated as an error
if ((WarningsAsErrors.Count == 0 && WarningAsErrorNotOverriden(warningEvent)) || WarningsAsErrors.Contains(warningEvent.Code))
{
return true;
}
}

Changes Made

  1. when buildEventArgs is BuildErrorEventArgs, treat BuildErrorEventArgs' related warnings as errors

  2. Initialize the code of BuildCheckResultWarning that is inherited from BuildWarningEventArgs

Testing

Manually testing on local now

set MSBUILDNOINPROCNODE=1 and change the build_check.BC0101.Severity= Error
dotnet D:\WORK\msbuild\artifacts\bin\bootstrap\net8.0\MSBuild\MSBuild.dll FooBar.csproj /m:1 -nr:False -restore -analyze
image

Change build_check.BC0101.Severity= warning
dotnet D:\WORK\msbuild\artifacts\bin\bootstrap\net8.0\MSBuild\MSBuild.dll FooBar.csproj /m:1 -nr:False -restore -analyze -warnaserror
image

Notes

@JaynieBai JaynieBai marked this pull request as ready for review June 12, 2024 08:41
@JaynieBai
Copy link
Member Author

JaynieBai commented Jun 12, 2024

Will add a test after figured out the reason of flaky test SampleAnalyzerIntegrationTest #10036

@JaynieBai JaynieBai marked this pull request as draft June 26, 2024 08:06
@JaynieBai JaynieBai marked this pull request as ready for review July 3, 2024 05:55
@JaynieBai JaynieBai changed the title Add build check result errors into the list of build submission IDs that have logged error Fix build check build submission errors Jul 5, 2024
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug][BuildCheck]: Build succeedes with MSBUILDNOINPROCNODE enabled
3 participants