Stop injecting NoWarns into repo's Directory.Build.props

[MichaelSimons]

The source-build infrastructure injects some NoWarns into the repo's Directory.Build.props. This is to handle some scenarios that come up in source build because of the way dependencies flow in source build. This approach is not compatible with the VMR in which source-build is built inside the context of a git repo. Checked in files should not be manipulated in place as this break dev UX. An alternative approach will be necessary going forward. Likely, the most reasonable approach is to make these changes within the repos that require them. This is most feasible in the .NET 9.0 timeframe. For the 8.0 timeframe, an alternative approach may be needed.

Sample manipulated Directory.Build.props:

[root@eec4fefb31f6 tarball]# git diff src/arcade/Directory.Build.props
diff --git a/src/arcade/Directory.Build.props b/src/arcade/Directory.Build.props
index 6bab8b78e..d11fb2749 100644
--- a/src/arcade/Directory.Build.props
+++ b/src/arcade/Directory.Build.props
@@ -27,4 +27,7 @@
     <MsbuildTaskMicrosoftCodeAnalysisCSharpVersion>$(MicrosoftCodeAnalysisCSharpVersion)</MsbuildTaskMicrosoftCodeAnalysisCSharpVersion>
   </PropertyGroup>

+  <PropertyGroup>
+    <NoWarn>$(NoWarn);NU5104;NU1603;</NoWarn>
+  </PropertyGroup>
 </Project>

[NikolaMilosavljevic]

The plan seems to be to create patches for all the affected repos and drive the backport to those repo. Any source-build specific change should be conditioned for full product source-build.

@MichaelSimons does this sound like a plan?

[MichaelSimons]

Things have changes a fair bit since this infrastructure to support source-build wide no-warn which are typically nuget related.

1. Repo specific PVP feature - Repos are able to control with dependencies they take. This likely impacted the number of instances we see NU5104;NU1603 in.
2. VMR - with the vmr we can now add these nowarns to the specific repos they are required for (once edits are supported).

Given these, I think an attempt should be made to remove these nowarns all up and only add them back in the specific cases where they are needed. If that doesn't work and we need a solution to apply a nowarn across the system, I would like to see a common shared solution, for example is it possible to add a directory.build.props in the VMR src directory that would define this common nowarn property?

[NikolaMilosavljevic]

After reviewing the current state, patches wouldn't make any sense - we make nowarn changes in all repos. I will work on consolidating this.

[NikolaMilosavljevic]

Main nuget-related nowarns are not needed at the moment. The original issue implies that this only reproes in some scenarios: #2766

Additionally, there are 3 repo-specific nowarns:

• vstest one is not needed anymore as the root cause was fixed and is now in main: Fix IDE0090 warning microsoft/vstest#4674
• runtime one was reproing in stage 2 builds: Suppress NU5129 warning for runtime repo installer#17022
• nuget-client instance is still needed, as the code looks the same: Patch for net8.0 targeting in nuget.client installer#16541

We cannot have high confidence for removal of any nowarns without being able to test stage 2 builds, which aren't working in main.

Alternative approach is to preserve all current nowarns, but move them to a checked in location. We already have a shared Directory.Build.props file in src root: https://github.com/dotnet/dotnet/blob/main/src/Directory.Build.props If we were to copy this one as part of inner clone, we could provide additional nowarns for repo source-build. This will solve the shared nowarns, but it will likely be possible to also implement repo-specific ones, by conditioning them. This model should work even after we stop creating the inner clone.

[MichaelSimons]

I forgot about the repo specific nowarns. Would be nice to cleanup the nowarns repo and system that are no longer needed. I would have confidence if valdiated in a .NET 8.0 environment.

If we were to copy this one as part of inner clone, we could provide additional nowarns for repo source-build. This will solve the shared nowarns, but it will likely be possible to also implement repo-specific ones, by conditioning them. This model should work even after we stop creating the inner clone.

That could work. Curious if we will encounter any issues with repo's defining their nowarns without concatenating. e.g. $(nowarn);nnn

[NikolaMilosavljevic]

I have validated that we can simply remove nuget-specific, vstest and runtime nowarns. The only remaining one is in nuget.client. I propose that we create a patch for that one and a backport to their repo. We'd condition this nowarn for product source-build.