Skip to content

Conversation

@ericstj
Copy link
Member

@ericstj ericstj commented Feb 12, 2025

This is with the 8.1.0 packages as a baseline. Could be that other problems exist if you select an older baseline.

CC @ViktorHofer

<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:System.ServiceModel.CallbackBehaviorAttribute</Target>
<Left>left</Left>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quirk in the reporting. When the assembly is missing from ref it seems like it misses getting the identifier correct. @ViktorHofer - probably a bug in APICompat's logging. Here's some sample output around this:

tePackage.targets(39,5): error CP0004: Assembly with name 'left' does not exist at System.ServiceModel.Duplex.
    C:\src\dotnet\wcf\.dotnet\sdk\10.0.100-alpha.1.24573.1\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.ApiCompat.ValidatePackage.targets(39,5): error CP0001: Type 'System.ServiceModel.CallbackBehaviorAttribute' exists on lib/netstandard2.0/System.ServiceModel.Duplex.dll but not on left

Probably we can rebase this PR on @mconnew which should fix these missing assemblies.

<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.ServiceModel.Federation.WSTrustTokenParameters.get_KeySize</Target>
<Left>lib/net8.0/System.ServiceModel.Federation.dll</Left>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is another bug in API compat tooling. WSTrustTokenParameters has the new keyword because when the class was first introduced, the base class didn't have a KeySize property. The base class did have a KeySize property exposed on NetFx and was brought back, so the definition in WSTrustTokenParameters needed the new keyword so that it didn't become binary incompatible. This hasn't changed single 6.1.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it does seem to be off:

CP0002: Member 'System.Nullable<int> System.ServiceModel.Federation.WSTrustTokenParameters.KeySize.get' exists on [Baseline] lib/net8.0/System.ServiceModel.Federation.dll but not on lib/netstandard2.0/System.ServiceModel.Federation.dll

This has to do with comparing the previous net8.0 asset with the new netstandard2.0 asset. You'll only face this issue across this one baseline comparison. Details here dotnet/sdk#46834.

<MinorVersion>1</MinorVersion>
<PatchVersion>1</PatchVersion>
<PackageValidationBaselineVersion>$(MajorVersion).$(MinorVersion).$([MSBuild]::Subtract($(PatchVersion), 1))</PackageValidationBaselineVersion>
<WcfAssemblyVersion>8.1.1.0</WcfAssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next version will be 8.1.2. It looks like @HongGit doesn't update this version number until we're ready to build, so the comparison should be with 8.1.1 and not 8.1.0.

<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.ServiceModel.Channels.HttpTransportBindingElement.get_Proxy</Target>
<Left>ref/netstandard2.0/System.ServiceModel.Http.dll</Left>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a genuine problem, this api is listed in the netstandard reference assembly but isn't public on .NET Framework, so should only be in our net8 reference assembly.

<Suppression>
<DiagnosticId>CP0008</DiagnosticId>
<Target>T:System.Web.Services.Configuration.XmlFormatExtensionAttribute</Target>
<Left>ref/netstandard2.0/System.Web.Services.Description.dll</Left>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericstj, do you know what's going on with these three? As far as I can tell, these are correct. The CP0008 error means a base interface was removed from the interface hierarchy from one of the compared sides. Is this caused by Attribute no longer implementing the interface _Attribute? Is this an API compat tool bug as the interface is defined on Attribute, which is external to these packages so isn't a difference between WCF Client packages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a closer look at this and the others. The logged messages give a bit more context - @ViktorHofer has improved human readability of the suppression files in later 10.0 releases.

A lot of times you still need to look at the metadata to understand what's going on. I'll have a review and add detail by mid-day tomorrow.

Copy link
Member Author

@ericstj ericstj Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's System.Runtime.InteropServices._Attribute

Type 'System.Web.Services.Configuration.XmlFormatExtensionAttribute' does not implement interface 'System.Runtime.InteropServices._Attribute' on ref/netstandard2.0/System.Web.Services.Description.dll but it does on lib/net462/System.Web.Services.Description.dll

The reason we flag it is because when folks use references (ref folder) we do a strict comparison, because we expect folks want to expose all public API in their references that are in the lib. They can then suppress those that differ.

This particular package has this structure:

lib\net462
lib\netstandard2.0
ref\netstandard2.0

As a result, when looking at the lib\net462 implementation and say "all it's public API should be exposed by it's reference" - the ref\netstandard2.0 one. We see that _Attribute is not there -- thus this error.

You're doing a couple things I'd advise against:

  1. If you use ref - you should really aim for 1/1 references with lib. Especially when it comes to netframework - to avoid the need to bring in the giant netstandard support facade set.
  2. Don't use ref if you don't have to. We've really shied away from using ref just because there are too many .NET Framework design time tools that don't work well with reference assemblies. We only use ref for our targeting packs anymore.

@mconnew
Copy link
Member

mconnew commented Feb 25, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mconnew mconnew force-pushed the enablePackageValidation branch from bb6adab to 4dcad61 Compare February 27, 2025 21:55
@mconnew
Copy link
Member

mconnew commented Feb 27, 2025

The package versioning has been updated so it's now validating the latest changes against the actual latest released 8.1.1 package so the set of compatibility problems has reduced. I think some of them also disappeared due to fixes being shipped to the api compat tooling and we're now using a later SDK release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants