Eliminate obsolete API warnings/errors in product source-build

[NikolaMilosavljevic]

Bug

Fixes: NuGet/Home#12988

Regression? No

Description

When building nuget.client repo in full product source-build we see the following error:

/vmr/src/nuget-client/src/NuGet.Core/NuGet.Frameworks/FrameworkException.cs(17,88): error SYSLIB0051: 'Exception.Exception(SerializationInfo, StreamingContext)' is obsolete: 'This API supports obsolete formatter-based serialization. It should not be called or extended by application code.' (https://aka.ms/dotnet-warnings/SYSLIB0051) [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Frameworks/NuGet.Frameworks.csproj::TargetFramework=net8.0]

To work around this, source-build process have been modifying Directory.Build.props during build to add a nowarn for SYSLIB0051. We are moving away from modifying sources during source-build and this needs to be fixed in the repo.

This PR adds the conditional Obsolete attribute to all affected APIs. Attribute is applicable on .NET 8.0+.

[NikolaMilosavljevic]

@jeffkl @MichaelSimons @mthalman

[MichaelSimons]

Taking a step back and looking at the source of the problem. Is there any chance of fixing the underlying issue? I see the type is public so maybe not without a breaking change. If not, would it be better to #pragma warning disable/restore the warning in code around the specific instance? The reason I mention this is to avoid the source-build specialization here.

[zivkan]

The ironic thing is that we added these constructor overloads because roslyn CA code analyzers told us to! 🤦

Something else to note is that these constructors & ISerializable implementation is needed for cross-AppDomain support on .NET Framework, which NuGet still supports (largely due to Visual Studio). Having said that, NuGet doesn't use AppDomains, so we don't explicitly need cross-AppDomain support, but it goes to show the challenge of supporting .NET (Core) & .NET Framework at the same time. Depending on which TFM/LangVersion, you get contradictory analyzers 😭

[NikolaMilosavljevic]

Taking a step back and looking at the source of the problem. Is there any chance of fixing the underlying issue? I see the type is public so maybe not without a breaking change. If not, would it be better to #pragma warning disable/restore the warning in code around the specific instance? The reason I mention this is to avoid the source-build specialization here.

Based on the guidance in the breaking change we should be able to add the following:

#if NET8_0_OR_GREATER
    [Obsolete(DiagnosticId = "SYSLIB0051")] // add this attribute to the serialization ctor
#endif

[NikolaMilosavljevic]

Taking a step back and looking at the source of the problem. Is there any chance of fixing the underlying issue? I see the type is public so maybe not without a breaking change. If not, would it be better to #pragma warning disable/restore the warning in code around the specific instance? The reason I mention this is to avoid the source-build specialization here.

Based on the guidance in the breaking change we should be able to add the following:

#if NET8_0_OR_GREATER
    [Obsolete(DiagnosticId = "SYSLIB0051")] // add this attribute to the serialization ctor
#endif

Fixed with 49ed7f4

Full source-build validation build is in progress: https://dev.azure.com/dnceng/internal/_build/results?buildId=2308863&view=results

cc @zivkan @MichaelSimons @mthalman

[MichaelSimons]

Consider replaceing the // add this attribute to the serialization ctor comment with a reference to the breaking change that remove the StreamingContext overload.

[NikolaMilosavljevic]

Absolutely - this was a simple copy from the guidance doc - will update.

[NikolaMilosavljevic]

Fixed in fcb9c45

I think this should work - it points to the breaking change issue, describing the change and providing guidance for updating source code.

[NikolaMilosavljevic]

There are more instances, about a dozen. I wonder if we should add a new NoWarn here instead of making all the source changes:

NuGet.Client/build/common.project.props

Lines 48 to 49 in 7bff223

	<!-- additional warnings new in .NET 6 that we need to disable when building with source-build -->
	<NoWarn Condition="'$(DotNetBuildFromSource)' == 'true'">$(NoWarn);CS1998;CA1416;CS0618;CS1574</NoWarn>

[NikolaMilosavljevic]

I've updated the PR with conditional obsolete attributes, per guidance in breaking change document: dotnet/docs#34893

[kartheekp-ms]

This PR adds the nowarn to repo's Directory.Build.props file - conditioned for full product source-build. It does not affect any other build target.

Thank you for the contribution. However, I don't see any changes to the repo's Directory.Build.props file in the proposed changes. Could you please update the PR description to accurately reflect the proposed changes?

[NikolaMilosavljevic]

This PR adds the nowarn to repo's Directory.Build.props file - conditioned for full product source-build. It does not affect any other build target.

Thank you for the contribution. However, I don't see any changes to the repo's Directory.Build.props file in the proposed changes. Could you please update the PR description to accurately reflect the proposed changes?

Thank you - will update the description. Implementation has changed based on PR discussion.

[kartheekp-ms]

The ironic thing is that we added these constructor overloads because roslyn CA code analyzers told us to! 🤦

Something else to note is that these constructors & ISerializable implementation is needed for cross-AppDomain support on .NET Framework, which NuGet still supports (largely due to Visual Studio). Having said that, NuGet doesn't use AppDomains, so we don't explicitly need cross-AppDomain support, but it goes to show the challenge of supporting .NET (Core) & .NET Framework at the same time. Depending on which TFM/LangVersion, you get contradictory analyzers 😭

I created a bug report dotnet/roslyn-analyzers#7026. I am also thinking to disable these analyzers from our code analysis ruleset file, remove [Serializable] attribute and constructor from the code as recommended in the dotnet/docs#34893 issue especially Microsoft strongly recommends against serialization libraries supporting the legacy serialization infrastructure ([Serializable] and ISerializable).