Build.proj restore filters out TreatWarningsAsErrors

[tmds]

@ViktorHofer I'm looking as to why I'm seeing some warnings as errors when building aspnetcore with --warnaserror false.

For example:

/home/tester/workdir/aspnetcore/src/Hosting/test/testassets/BasicLinkedApp/BasicLinkedApp.csproj : error NU1603: Warning As Error: BasicLinkedApp depends on Microsoft.NET.ILLink.Tasks (>= 10.0.0-preview.6.25277.101) but Microsoft.NET.ILLink.Tasks 10.0.0-preview.6.25277.101 was not found. Microsoft.NET.ILLink.Tasks 10.0.0-preview.6.25277.102 was resolved instead. [/tmp/tmp.X9Undgqi2t/sdk/10.0.100-preview.6.25277.101/NuGet.targets]

I found this:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj

Line 253 in 83c8fe2

RemoveProperties="$(_RemoveProps);TreatWarningsAsErrors"

From #5172 (comment), it seems the intent is to consistenly have TreatWarningsAsErrors=false to avoid re-restoring if the property changed due to the project config.

In my case, it seems this has the unintended effect of filtering out TreatWarningsAsErrors=false.

Could we change the above to setting it to false in Properties instead of filtering it out in RemoveProperties?

    <MSBuild Projects="@(_ProjectToRestore)"
             Properties="@(_SolutionRestoreProps);TreatWarningsAsErrors=false"
             RemoveProperties="$(_RemoveProps)"
             Targets="Restore"
             SkipNonexistentTargets="true"
             BuildInParallel="%(_ProjectToRestore.RestoreInParallel)"
             Condition="'$(Restore)' == 'true'"/>

[ViktorHofer]

I don't have much context here or any preference to be honest. I think the RemoveProperties option was chosen as Properties would pass TreatWarningsAsErrors=false in as a global property which would go against the linked PR's original purpose (making restore inside tools and on CLI the same).

[tmds]

I think this was meant to address this pattern in projects:

<PropertyGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'">
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

By filtering out the TreatWarningsAsErrors, the restore would no longer see TreatWarningsAsErrors=true.

Though, in my case, the filtering causes the restore to not see TreatWarningsAsErrors=false.

So, I think Properties="@(_SolutionRestoreProps);TreatWarningsAsErrors=false" may be a good option.

[ViktorHofer]

I think this was meant to address this pattern in projects:

I'm not sure how that would work with the existing RemoveProperties metadata which is on the MSBuild task in Build.proj. That task doesn't now about such a conditional property in projects as its running from the outer / orchestration layer.

With what you propose (Properties) this would have an actual impact as it would set all these to false. That said, I'm not sure I like restore always running with warningsAsErrors=false in general.

Before we can do anything here, we should better understand the current experience and what would change. I'm still a bit confused.

[tmds]

These are the main comments I'm basing my assumptions on: #5172 (comment), #5172 (comment), #5172 (comment).

I'm not sure who can give additional context?

Would you be ok with a change that leaves out TreatWarningsAsErrors from RemoveProperties when it is set to false?

[tmds]

The issue I'm trying to address is the following.

When I try to build aspnetcore with a daily SDK (from our internal CI) on restore I get errors like:

error NU1603: Warning As Error: BasicTestApp depends on Microsoft.NET.Sdk.WebAssembly.Pack (>= 10.0.0-preview.6.25301.101) but Microsoft.NET.Sdk.WebAssembly.Pack 10.0.0-preview.6.25301.101 was not found. Microsoft.NET.Sdk.WebAssembly.Pack 10.0.0-preview.6.25302.102 was resolved instead.

There is no version for Microsoft.NET.Sdk.WebAssembly.Pack that matches the SDK version, but there is a newer version available from nuget feeds.

This is a warning, which is turned into an error.

Setting --warnaserror false on the build.sh doesn't work due to the TreatWarningsAsErrors being filtered:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj

Line 253 in 83c8fe2

RemoveProperties="$(_RemoveProps);TreatWarningsAsErrors"

[tmds]

Would you be ok with a change that leaves out TreatWarningsAsErrors from RemoveProperties when it is set to false?

This would be something like:

diff --git a/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj b/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj
index 9b1d4e638..337dc5433 100644
--- a/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj
+++ b/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj
@@ -248,9 +248,14 @@
       </_ProjectToRestore>
     </ItemGroup>
 
+    <!-- Ignore TreatWarningsAsErrors during Restore and let the projects configure it unless it is set to 'false' explicitly. -->
+    <PropertyGroup>
+      <_RemovePropsRestore Condition="'$(TreatWarningsAsErrors)' != 'false'">$(_RemoveProps);TreatWarningsAsErrors</_RemovePropsRestore>
+    </PropertyGroup>
+
     <MSBuild Projects="@(_ProjectToRestore)"
              Properties="@(_SolutionRestoreProps)"
-             RemoveProperties="$(_RemoveProps);TreatWarningsAsErrors"
+             RemoveProperties="$(_RemovePropsRestore)"
              Targets="Restore"
              SkipNonexistentTargets="true"
              BuildInParallel="%(_ProjectToRestore.RestoreInParallel)"

[tmds]

@sharwell as you contributed #5172, perhaps you can give some insights as to what change we could make here?

[ViktorHofer]

Sam might not respond. I think the validation that we would need to do is:

1. Make the code change and invoke build from the CLI with the -warnaserror switch and inspect the generated intermediates in the project's obj folders.
2. Restore the solution from VS and inspect those intermediates.
3. Compare them with each other

You can probably do this with any repo, i.e. directly the Arcade one. Just make sure that the bootstrap layout that is used to built Arcade itself (from the nuget package cache) is patched, in addition to the live sources.

[tmds]

I can't verify this because I don't have VS.

My understanding is that the CLI script and VS difference is due to the CLI scripts defaulting to TreatWarningsAsErrors=true.

So if we're only changing the behavior when TreatWarningsAsErrors is false (like #15872 (comment)) then the CLI (default) and VS behavior should remain the same.

[tmds]

@ViktorHofer I think #15872 (comment) is the least intrusive way to make Restore taken into account --warnaserror false.

[ViktorHofer]

OK let's give it a try