[main] Update dependencies from dotnet/msbuild

[dotnet-maestro]

This pull request updates the following dependencies

From https://github.com/dotnet/msbuild

• Subscription: 9b9c2fc1-dee5-447e-827b-08d8e9754760
• Build: 20210415.5
• Date Produced: 4/15/2021 10:48 PM
• Commit: 369631b4b21ef485f4d6f35e16b0c839a971b0e9
• Branch: refs/heads/main

• Updates:
  • Microsoft.Build.Localization: from 16.10.0-preview-21205-05 to 16.10.0-preview-21215-05
  • Microsoft.Build: from 16.10.0-preview-21205-05 to 16.10.0-preview-21215-05

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[akoeplinger]

Looks like there are new test failures. The commit diff is pretty short: dotnet/msbuild@7804350...9bcc06c

/cc @jeffkl @benvillalobos

[jeffkl]

@akoeplinger

ItRunsSpecifiedTargetsWithPropertiesCorrectly is now failing because of my change. It used to erroneously pass because its trying to do an implicit restore and then build, but the project does not define a restore target. Can we update the test asset to have an empty Restore target or disable implicit restore in the test? @benvillalobos Is it considered a breaking change if Restore never ran before even though it was invoked which technically is bad and now the build correctly fails? 😄

MsbuildInvocationIsCorrect appears to be failing because an unexpected logger argument is added, not sure what is causing that

Expected string to be 
"exec <msbuildpath> -maxcpucount -verbosity:m -verbosity:normal -target:Clean -verbosity:diag" with a length of 92, but 
"exec <msbuildpath> -maxcpucount -verbosity:m -verbosity:normal -target:Clean -verbosity:diag -distributedlogger:Microsoft.DotNet.Tools.MSBuild.MSBuildLogger,/Users/runner/work/1/s/artifacts/bin/redist/Release/dotnet.dll*Microsoft.DotNet.Tools.MSBuild.MSBuildForwardingLogger,/Users/runner/work/1/s/artifacts/bin/redist/Release/dotnet.dll" has a length of 337.

[akoeplinger]

Can we update the test asset to have an empty Restore target or disable implicit restore in the test?

Good question, I'm not sure what the correct sdk behavior should be here.

MsbuildInvocationIsCorrect appears to be failing because an unexpected logger argument is added, not sure what is causing that

Maybe the fix in dotnet/msbuild@bab9b71 caused arguments that were skipped before to now be escaped? E.g. the argument uses , which is in the list of characters that are now escaped.

[benvillalobos]

At first glance it doesn't look like dotnet/msbuild@bab9b71 caused the regression in MsbuildInvocationIsCorrect . The change to Exec modifies how its batch file gets generated, and only on windows.

Is it considered a breaking change if Restore never ran before even though it was invoked which technically is bad and now the build correctly fails?

Wouldn't more tests fail because of this change? Or is that project without the restore target only used in this test?

Reminds me of the recent HasLoggedError change, where loggers are now properly aware of warnings that were turned into errors. Technically a breaking change, but into correct behavior and needs to be done. I'd put it in that category and modify the test to account for it.

[jeffkl]

Wouldn't more tests fail because of this change?

Any test that is building with implicit restore and its building a project with no Restore target yes. I agree that this makes things work more correctly, hopefully we don't see too many reports of breakage.

[dsplaisted]

We should figure out exactly what the breaking change is and make sure to doc it.

/cc @KathleenDollard

[jeffkl]

We should figure out exactly what the breaking change is and make sure to doc it.

My change fixes a bug where you'd run msbuild /t:restore and it wouldn't actually do anything but succeed causing the build to fail later on for mysterious reasons. Restore now fails if there is no Restore target or if any MSBuild project SDK can't be found (including Microsoft.NET.Sdk).

dotnet/msbuild#6312

I doubt it will cause any actual breakage unless someone was counting on restore "succeeding" even though it failed and then their build works even though Restore didn't do anything.

[benvillalobos]

Checking in on this and trying to debug the failing test MsbuildInvocationIsCorrect but I can't seem to get the test to run at all? It wants me to install net6.0 p4 and I can't seem to global.json myself down?

[benvillalobos]

I did notice @ladipro recently modified the test. I don't think it's the cause since the timing if different, any idea why this test might start getting logger arguments as well?

[ladipro]

@benvillalobos I'm probably not looking at the right logs because I don't see MsbuildInvocationIsCorrect failing in the last run. It appears to be working locally as well.

Instructions for running the test locally:

1. Run build in the SDK repo.
2. Run artifacts\sdk-build-env.bat
3. Run VS from this environment: "\Program Files (x86)\Microsoft Visual Studio\2019\Preview\Common7\IDE\devenv.exe"
4. Open sdk.sln and you should see Test Explorer populated with tests.

The only test I see failing is:

 Microsoft.DotNet.Cli.MSBuild.Tests.GivenDotnetMSBuildBuildsProjects.ItRunsSpecifiedTargetsWithPropertiesCorrectly
Expected command to pass but it did not.\nFile Name: /Users/runner/work/1/s/artifacts/bin/redist/Release/dotnet/dotnet\nArguments: msbuild /t:SayHello /Users/runner/work/1/s/artifacts/tmp/Release/ItRunsSpecifi---927107D0/BareBones.proj /restore\nExit Code: 1\nStdOut:\nMicrosoft (R) Build Engine version 16.10.0-preview-21213-02+47be98296 for .NET\nCopyright (C) Microsoft Corporation. All rights reserved.\n\n/Users/runner/work/1/s/artifacts/tmp/Release/ItRunsSpecifi---927107D0/BareBones.proj : error MSB4057: The target \"Restore\" does not exist in the project.\nStdErr:\n\n
   at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message) in FluentAssertions.dll:token 0x600015c+0x1d
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message) in FluentAssertions.dll:token 0x6000164+0x11
   at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message) in FluentAssertions.Core.dll:token 0x6000372+0x0
   at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args) in FluentAssertions.Core.dll:token 0x600034f+0x49
   at Microsoft.NET.TestFramework.Assertions.CommandResultAssertions.Pass() in /_/src/Tests/Microsoft.NET.TestFramework/Assertions/CommandResultAssertions.cs:line 32
   at Microsoft.DotNet.Cli.MSBuild.Tests.GivenDotnetMSBuildBuildsProjects.ItRunsSpecifiedTargetsWithPropertiesCorrectly() in /_/src/Tests/dotnet-msbuild.Tests/GivenDotnetMSBuildBuildsProjects.cs:line 42
Output:
> /Users/runner/work/1/s/artifacts/bin/redist/Release/dotnet/dotnet msbuild /t:SayHello /Users/runner/work/1/s/artifacts/tmp/Release/ItRunsSpecifi---927107D0/BareBones.proj /restore
Microsoft (R) Build Engine version 16.10.0-preview-21213-02+47be98296 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

/Users/runner/work/1/s/artifacts/tmp/Release/ItRunsSpecifi---927107D0/BareBones.proj : error MSB4057: The target "Restore" does not exist in the project.
Exit Code: 1

[benvillalobos]

@ladipro Ah, I was operating under old test results. You're right, that test is actually fine now :) Thanks for the steps on running the test locally!

[benvillalobos]

What's the status on this? /cc: @dsplaisted

[benvillalobos]

Talked about this in standup. @jeffkl could you update the logic and place it behind a changewave?

The process should be:

1. ChangeWave it
2. Modify the test project to have an empty restore target
3. Release a doc somewhere that includes that in the list of breaking changes

[jeffkl]

@benvillalobos sounds good. I'm OOF this week so I wouldn't be able to work on it until next week, feel free to revert if its blocking.

[dsplaisted]

@benvillalobos @jeffkl I've pushed an update that should fix the failing test here. Please make sure to include this in a change wave and document it as a breaking change.

[dsplaisted]

@benvillalobos @jeffkl Are you folks tracking the breaking change work for this?

[benvillalobos]

@dsplaisted So far we have it documented under our changewaves docs. Was inserted here dotnet/msbuild#6372.

Are there specific places we need document this?

[dsplaisted]

@benvillalobos No, that looks good, I was just checking to make sure the follow-up had happened.