Arcade: Arcade allows build to pass as green even when target shows error and build output is corrupted

[jmarolf]

In this build I get the following error:

##[error].packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.19521.4\tools\VisualStudio.InsertionManifests.targets(23,5): error : Property VisualStudioDropName must be specified in official build that produces Visual Studio insertion components.

Yet the build passes. This will cause the build output to be invalid and should fail the build

[markwilkie]

@riarenas - do you remember the deal here?

[riarenas]

Looks similar to some issues from a while back where we were not reporting error states correctly. I'll take a look.

[dagood]

I wonder if this is https://github.com/microsoft/msbuild/issues/3345?

That target "hangs off" the build using AfterTargets so errors in there may not fail the all-up build:

https://github.com/dotnet/arcade/blob/894bd005267af53f7d39ac988881e61193e67ae1/src/Microsoft.DotNet.Arcade.Sdk/tools/VisualStudio.InsertionManifests.targets#L12-L13

[riarenas]

Does look related. All our previous instances of something like this were in the powershell side, but that was not the case here. I'll take a look at that issue. Thanks Davis.

[riarenas]

Yeah, definitely related. This is exactly the same issue described in #1800
Doesn't look like we got resolution there.

EDIT: Pasted the wrong link at first. It's becoming a thing...

[dagood]

I sympathize with MSBuild's interpretation of "Build Successful", but yeah, throws an ugly wrench in a lot of code.

[tmat]

I get the interpretation, but I think the current behavior is incorrect and inconsistent with a common sense rule "if a task runs and it fails the build should fail".
I'd be fine if msbuild chose not to run these AfterTargets at all. But running them and ignoring their failure is imo wrong.

Nevertheless, I have been fixing these cases. I guess there are some more left.

[riarenas]

The error is understood. Using AfterTargets here causes the errors in msbuild to be swallowed.

Going to pass this off to FR so someone can fix the target chaining here to something that will actually fail the build. (It looks like using BeforeTargets or DependsOnTargets doesn't get hit by the bug.

[MattGal]

@rainersigwald any suggestions on how best to address this?

[rainersigwald]

Generally, hook BeforeTargets the rollup target, rather than AfterTargets it. That look perfectly cromulent for Pack in this case.

[MattGal]

@Tohron if this is merged, can it be closed?