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

Change Copy task LogDiagnostics Warning so it is suppressible #9217

Merged

Conversation

rokonec
Copy link
Contributor

@rokonec rokonec commented Sep 13, 2023

Fixes #9210

Context

Copy task implement LogDiagnostics as warnings. This is causing issues for /WarnAsError use cases.

Changes Made

Change Copy task LogDiagnostics from Warning to low importance Message.

/// <summary>
/// If MSBUILDALWAYSRETRY is set, also log useful diagnostic information -- as 
/// a warning, so it's easily visible. 
/// </summary>
private void LogDiagnostic(string message, params object[] messageArgs)
{
    if (s_alwaysRetryCopy)
    {
        Log.LogMessage(MessageImportance.Low, message, messageArgs);
    }
}

The reasoning for Warning level "so it's easily visible" is, IMO, not strong enough.

Testing

Unit tests.
Locally.

Notes

@rainersigwald Since we see increasing number of people reporting issues with Copy conflicts, I believe it is candidate for 17.8

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.

Please remove/alter the existing comment (it's confusing now) - than I'm OK with change. As soon as the MessageImportance.High was considered and there are reasons not to use it.

src/Tasks/Copy.cs Outdated Show resolved Hide resolved
@rokonec rokonec closed this Sep 13, 2023
@rokonec rokonec force-pushed the rokonec/copy-task-diagnostics-messages branch from b320586 to caf06d1 Compare September 13, 2023 18:04
@rokonec
Copy link
Contributor Author

rokonec commented Sep 13, 2023

Not closed actually, but hard reset + force push have confused github

@rokonec rokonec reopened this Sep 13, 2023
@rokonec rokonec changed the title Change Copy task LogDiagnostics from Warning to low importance message Change Copy task LogDiagnostics Warning so it is suppressible Sep 13, 2023
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!

We just need to pick unique error codes

src/Tasks/Resources/Strings.resx Show resolved Hide resolved
@rokonec rokonec added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Sep 14, 2023
@rainersigwald rainersigwald added this to the VS 17.8 milestone Sep 18, 2023
@rainersigwald rainersigwald merged commit 3847162 into dotnet:main Sep 21, 2023
8 checks passed
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Sep 27, 2023
These strings were introduced in dotnet#9217 but caused loc breaks.

Fixes dotnet#9283.
rainersigwald added a commit that referenced this pull request Sep 27, 2023
These strings were introduced in #9217 but caused loc breaks.

Fixes #9283.
bulatgrzegorz pushed a commit to bulatgrzegorz/selective-condition-evaluator that referenced this pull request Oct 16, 2023
These strings were introduced in dotnet#9217 but caused loc breaks.

Fixes dotnet#9283.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Copy.DoCopyWithRetries logs warnings without a diagnostic code
4 participants