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

Fix MSBuild restore is logging exception as warning #4248

Merged
merged 8 commits into from
Sep 29, 2021
Merged

Fix MSBuild restore is logging exception as warning #4248

merged 8 commits into from
Sep 29, 2021

Conversation

erdembayar
Copy link
Contributor

@erdembayar erdembayar commented Sep 5, 2021 •
edited
Loading

Bug

Fixes: NuGet/Home#11179

Regression? Last working version:

Description

While debugging test I noticed some exceptions in msbuild restore task logging exception message as warning, I believe it should log as error instead.
Before no error, yet a failure return code is not fixed. Also just in case added unit test to assert same issue doesn't exist for nuget.exe.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@erdembayar erdembayar requested a review from a team as a code owner September 5, 2021 16:26
zivkan
zivkan reviewed Sep 9, 2021
View reviewed changes
src/NuGet.Core/NuGet.PackageManagement/IDE/PackageRestoreManager.cs Outdated
@@ -480,7 +480,8 @@ private NuGetPackageManager GetNuGetPackageManager(string solutionDirectory)
{
if (!string.IsNullOrEmpty(exception.Message))
{
nuGetProjectContext.Log(MessageLevel.Warning, exception.Message);
var errorMessage = string.Format(Strings.RestoreErrorNU1000, exception.Message);
nuGetProjectContext.Log(MessageLevel.Error, errorMessage);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you should need to format the NU code into the message yourself. If you look at how other warnings/errors are created, we have helper methods. For example:

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

Line 464 in 2247784

await _logger.LogAsync(RestoreLogMessage.CreateError(NuGetLogCode.NU1011, Strings.Error_CentralPackageVersions_FloatingVersionsAreNotAllowed));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zivkan
Thank you for your suggestion. I used the helper method.

src/NuGet.Core/NuGet.PackageManagement/Strings.resx Outdated Show resolved Hide resolved
@erdembayar
Copy link
Contributor Author

@NuGet/nuget-client
Please review again.

nkolev92
nkolev92 reviewed Sep 20, 2021
View reviewed changes
src/NuGet.Core/NuGet.PackageManagement/IDE/PackageRestoreManager.cs Outdated
@@ -480,7 +480,7 @@ private NuGetPackageManager GetNuGetPackageManager(string solutionDirectory)
{
if (!string.IsNullOrEmpty(exception.Message))
{
nuGetProjectContext.Log(MessageLevel.Warning, exception.Message);
nuGetProjectContext.Log(RestoreLogMessage.CreateError(NuGetLogCode.NU1000, exception.Message));
Copy link
Member

Choose a reason for hiding this comment

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

Is NU1000 redundant?
Isn't that the default anyways, so wouldn't:

                    nuGetProjectContext.Log(MessageLevel.Error, exception.Message);

be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nkolev92
Please check previous comment regarding this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that addresses my concern.

Isn't NU1000 the default code? The tests seem to suggest that as well.

packages.config doesn't really have specific error codes.

Copy link
Contributor Author

@erdembayar erdembayar Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it's default code. But since we don't have specific error code opted to use generic NU1000.
Do you suggest removing any NU code from here? So we can just keep error part.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah my suggestion was to remove it. Codes are just not a thing in packages.config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I addressed your comment. Please review again.

@erdembayar erdembayar requested a review from nkolev92 September 21, 2021 16:24
@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 28, 2021
@ghost
Copy link

ghost commented Sep 28, 2021

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@ghost ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Sep 29, 2021
@erdembayar erdembayar merged commit f93c754 into dev Sep 29, 2021
@erdembayar erdembayar deleted the dev-eryondon-11179 branch September 29, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@zivkan zivkan Awaiting requested review from zivkan

@kartheekp-ms kartheekp-ms Awaiting requested review from kartheekp-ms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: MSBuild restore is logging exception as warning
4 participants
@erdembayar @nkolev92 @zivkan @kartheekp-ms