dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.67k stars 1.06k forks source link

NoWarn the new .NET SDK netstandard1.x warning doesn't work #41787

Closed ViktorHofer closed 2 months ago

ViktorHofer commented 3 months ago

/vmr/.dotnet/sdk/9.0.100-preview.7.24325.1/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(187,5): error NETSDK1215: Targeting .NET Standard prior to 2.0 is no longer recommended. See https://aka.ms/dotnet/dotnet-standard-guidance for more details. [/vmr/src/source-build-reference-packages/src/referencePackages/src/humanizer.core/2.2.0/Humanizer.Core.2.2.0.csproj::TargetFramework=netstandard1.0]

<NoWarn>$(NoWarn);NETSDK1215</NoWarn> doesn't work. I see there's a separate msbuild property to disable the check but why can't I use NoWarn?

@marcpopMSFT @terrajobst

KalleOlaviNiemitalo commented 3 months ago

Why does your log show "error NETSDK1215" rather than a warning?

Microsoft.Common.CurrentVersion.targets sets MSBuildWarningsAsMessages to match $(NoWarn), which should downgrade the warning. But does your project or build environment set some options that interfere with this?

ViktorHofer commented 3 months ago

Presumably because we upgrade warnings to errors via the TreatWarningsAsErrors=true flag?

ViktorHofer commented 3 months ago

The https://github.com/dotnet/sdk/blob/ff9735447ce8ca62879d70d322a5bd1cd8f4e7d8/src/Tasks/Common/NETSdkWarning.cs#L22 task is used which doesn't add a warning code, it just logs a string.

I think we would need to use https://apisof.net/catalog/e25be74c0e0f3ae3f0230429a6d2c04b to log a warning with a warning code in order for NoWarn / MSBuildWarningsAsMessages to work.

cc @rainersigwald

baronfel commented 3 months ago

not quite @ViktorHofer - that method is actually not using the MSBuild Logger, it's using the SDK version of Logger: https://github.com/dotnet/sdk/blob/5433bc4745265d7c16611f2b389b780e834eacc0/src/Tasks/Common/Logger.cs#L66-L67. We use this a lot of places to extract a code from a format string and prevent having to keep things in sync. So it should be applying the code as expected.

ViktorHofer commented 3 months ago

Ah I see. So the SDK sends the warning level correctly to msbuild but for some reason NoWarn doesn't work.

rainersigwald commented 3 months ago

Can I get a repro or a log? I wonder if there's a mix of NoWarn/MSBuildWarningsAsMessages involved, that's caused friction before.

marcpopMSFT commented 3 months ago

FWIW, I cannot reproduce this. I had tested NoWarn with the original PR and it was able to remove the warning. Here's the project file I have though note that if I remove NoWarn, I don't get NETSDK1215 as an error which is suspicious:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard1.6</TargetFramework>
    <CheckNotRecommendedTargetFramework>true</CheckNotRecommendedTargetFramework>
    <NoWarn>$(NoWarn);NETSDK1215</NoWarn>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  </PropertyGroup>

</Project>
ViktorHofer commented 3 months ago

Interesting, this didn't work for SBRP. I'll take a look again tomorrow.

marcpopMSFT commented 3 months ago

There might be some other setting that's interacting strangely with nowarn.

rainersigwald commented 3 months ago

IIRC we've also seen some timing things, maybe it's setting NoWarn after it's already been read into MSBuildWarningsAsMessages?

terrajobst commented 3 months ago

Damn. Not being able to suppress this warning makes the warning problematic. If we can't make NoWarn work, we should add a property that allows suppressing the target that raises it.

ViktorHofer commented 3 months ago

We already have a property switch: https://github.com/dotnet/source-build-reference-packages/blob/0b53e839fa2f09a5994cc6006533dcc3d45a4226/Directory.Build.props#L20-L21

This issue is just about making sure that NoWarn also works.

terrajobst commented 3 months ago

Awesome, glad we did that!

ViktorHofer commented 2 months ago

I just tried this out in a sample app and NoWarn works. I noticed this in the source-build-reference-packages repo so I assume that somewhere there we are accidentally overriding other NoWarn entries. Sorry for the noise everyone.