Add ApiCompat run to repo #17959

wtgodbe · 2019-12-19T01:43:42Z

Adds an APICompat run between our ref assemblies (Contract) and the corresponding implementation assemblies. CC @dougbu @JunTaoLuo @Pilchie

Part of #17957. We could also add a run in servicing branches that compares against the previous patch build, but that will be a little more custom (and would require us to restore all the packages we shipped in the last patch)

Currently I only have this running on windows

wtgodbe · 2019-12-19T01:46:09Z

759d9ad adds the functionality, de05237 represents the set of errors logged. We can have the run either write to these baseline files, or fail when a new error appears that wasn't previously in the baseline files. There are a few real errors there we should look at and fix first (we probably don't need to care about CannotChangeAttribute)

CC @ericstj

wtgodbe · 2019-12-19T01:46:48Z

src/Components/Components/src/ApiCompatBaseline.txt

+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Components.LayoutAttribute' changed from '[AttributeUsageAttribute(4, AllowMultiple=false, Inherited=true)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class, AllowMultiple=false, Inherited=true)]' in the implementation.
+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Components.ParameterAttribute' changed from '[AttributeUsageAttribute(128, AllowMultiple=false, Inherited=true)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Property, AllowMultiple=false, Inherited=true)]' in the implementation.
+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Components.RouteAttribute' changed from '[AttributeUsageAttribute(4, AllowMultiple=true, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class, AllowMultiple=true, Inherited=false)]' in the implementation.
+ParameterModifiersCannotChange : Custom modifiers on parameter 'renderBatch' on method 'Microsoft.AspNetCore.Components.RenderTree.Renderer.UpdateDisplayAsync(Microsoft.AspNetCore.Components.RenderTree.RenderBatch)' are 'System.Runtime.InteropServices.InAttribute' in the implementation but 'System.Runtime.InteropServices.InAttribute' in the contract.


This looks like a bug in the tool

[In] is defined in a number of different assemblies. Perhaps something in how we build ref/ projects is very slightly different?

In any case, suggest a centralized exclusion for the [In] != [In] case.

wtgodbe · 2019-12-19T01:47:13Z

src/Http/Routing/src/ApiCompatBaseline.txt

@@ -0,0 +1,5 @@
+Compat issues with assembly Microsoft.AspNetCore.Routing:
+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Routing.HostAttribute' changed from '[AttributeUsageAttribute(68, AllowMultiple=false, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple=false, Inherited=false)]' in the implementation.
+MembersMustExist : Member 'Microsoft.AspNetCore.Routing.Matching.CandidateState.Values.set(Microsoft.AspNetCore.Routing.RouteValueDictionary)' does not exist in the implementation but it does exist in the contract.


We should look into this one

Values.Set is internal in the implementation:
https://github.com/aspnet/AspNetCore/blob/138e801e342af561c8f12c8bfd2591d47129db82/src/Http/Routing/src/Matching/CandidateState.cs#L53
But public in the ref assembly:
https://github.com/aspnet/AspNetCore/blob/138e801e342af561c8f12c8bfd2591d47129db82/src/Http/Routing/ref/Microsoft.AspNetCore.Routing.Manual.cs#L129
That seems like a bug in the ref assembly. It should be internal there too and use InternalsVisibleTo.

@Pilchie does this example refer to your question? The tool picked up a diff between the src file and the corresponding manual.cs file

This is not conclusive, since it was public on at least one side. I'd want to see a case where it reports a mismatch in something that is internal on both sides.

It's entirely possible we messed a few things up when creating and updating *.Manual.cs files e.g. did the all-important internal keyword get deleted accidentally? Suggest checking GenAPI output w/ --all because that tool uses the same base infrastructure as ApiCompat.

I suspect we just messed up when updating Manual.cs files.

wtgodbe · 2019-12-19T01:48:01Z

src/Servers/Kestrel/Core/src/ApiCompatBaseline.txt

@@ -0,0 +1,4 @@
+Compat issues with assembly Microsoft.AspNetCore.Server.Kestrel.Core:
+ParameterModifiersCannotChange : Custom modifiers on parameter 'buffer' on method 'Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser<TRequestHandler>.Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.IHttpParser<TRequestHandler>.ParseRequestLine(TRequestHandler, System.Buffers.ReadOnlySequence<System.Byte>, System.SequencePosition, System.SequencePosition)' are 'System.Runtime.InteropServices.InAttribute' in the implementation but 'System.Runtime.InteropServices.InAttribute' in the contract.


These look like bugs in the tool

Pilchie

Thanks, it's great to have this (apart from those bugs in the tool of course). However, I'm not confident that it actually protects us all the problems we might face. Specifically, if the internal surface area that is duplicated into our Manual.cs files changes, will we get an error so that we know to update the Manual.cs as well as the original file, or does the tool only look at public surface area?

wtgodbe · 2019-12-19T21:20:28Z

The tool just compares the two assemblies - I believe it looks at internal surface area as well as public, and would therefore tell us if we added something to a src file without updating the manual.cs files, but I'm not 100% sure - @ericstj can you confirm?

wtgodbe · 2019-12-20T19:48:14Z

Hmm, this approach won't work. APICompat depends on the ref assembly being built before the implementation assembly, which isn't always the case for us. Will try adding another pass thru all the projects post-build.

Directory.Build.targets

dougbu · 2019-12-21T01:37:33Z

src/Antiforgery/src/ApiCompatBaseline.txt

@@ -0,0 +1 @@
+Total Issues: 0


Can we avoid checking in all of these tiny files? E.g. include one Total Issues: 0 default file plus those containing issues we actually want to ignore.

I think we can just exclude these files if there are no issues
https://github.com/dotnet/arcade/blob/767154ac5bb8715ab1357c63f346244de6d4aa6b/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L56
It seems like if a baseline does not exist, it will simply not be passed to apicompat.

Note that there's no corresponding option to output a file only when there are errors since when the option is set, the baseline file is always written: https://github.com/dotnet/arcade/blob/767154ac5bb8715ab1357c63f346244de6d4aa6b/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L108

src/Components/Components/src/ApiCompatBaseline.txt

dougbu · 2019-12-21T01:43:17Z

src/Http/Routing/src/ApiCompatBaseline.txt

@@ -0,0 +1,5 @@
+Compat issues with assembly Microsoft.AspNetCore.Routing:
+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Routing.HostAttribute' changed from '[AttributeUsageAttribute(68, AllowMultiple=false, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple=false, Inherited=false)]' in the implementation.
+MembersMustExist : Member 'Microsoft.AspNetCore.Routing.Matching.CandidateState.Values.set(Microsoft.AspNetCore.Routing.RouteValueDictionary)' does not exist in the implementation but it does exist in the contract.


It's entirely possible we messed a few things up when creating and updating *.Manual.cs files e.g. did the all-important internal keyword get deleted accidentally? Suggest checking GenAPI output w/ --all because that tool uses the same base infrastructure as ApiCompat.

dougbu · 2019-12-21T01:54:52Z

src/Components/Components/src/ApiCompatBaseline.txt

+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Components.LayoutAttribute' changed from '[AttributeUsageAttribute(4, AllowMultiple=false, Inherited=true)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class, AllowMultiple=false, Inherited=true)]' in the implementation.
+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Components.ParameterAttribute' changed from '[AttributeUsageAttribute(128, AllowMultiple=false, Inherited=true)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Property, AllowMultiple=false, Inherited=true)]' in the implementation.
+CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Microsoft.AspNetCore.Components.RouteAttribute' changed from '[AttributeUsageAttribute(4, AllowMultiple=true, Inherited=false)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class, AllowMultiple=true, Inherited=false)]' in the implementation.
+ParameterModifiersCannotChange : Custom modifiers on parameter 'renderBatch' on method 'Microsoft.AspNetCore.Components.RenderTree.Renderer.UpdateDisplayAsync(Microsoft.AspNetCore.Components.RenderTree.RenderBatch)' are 'System.Runtime.InteropServices.InAttribute' in the implementation but 'System.Runtime.InteropServices.InAttribute' in the contract.


[In] is defined in a number of different assemblies. Perhaps something in how we build ref/ projects is very slightly different?

In any case, suggest a centralized exclusion for the [In] != [In] case.

wtgodbe · 2019-12-26T21:55:51Z

Maybe I can make it work during the normal pass, these are the projects complaining about the ref assembly not being available when building the impl:

Microsoft.AspNetCore.Cryptography.Internal.csproj
Microsoft.AspNetCore.Metadata.csproj
Microsoft.AspNetCore.Http.Features.csproj
Microsoft.AspNetCore.DataProtection.Abstractions.csproj
Microsoft.AspNetCore.Components.csproj

Looking for what these have in common. They all have netstandard2.0 configurations & IsAspNetCoreApp=true, but so does https://github.com/aspnet/AspNetCore/blob/release/3.1/src/Components/Web/src/Microsoft.AspNetCore.Components.Web.csproj, which doesn't hit this problem.

wtgodbe · 2019-12-26T22:12:17Z

https://github.com/aspnet/AspNetCore/blob/release/3.1/src/Security/Authorization/Core/src/Microsoft.AspNetCore.Authorization.csproj doesn't hit the error, and looks almost exactly the same as https://github.com/aspnet/AspNetCore/blob/release/3.1/src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj, which does

wtgodbe · 2019-12-26T22:29:04Z

Oh, some of this might be because projects in the Components dir output their artifacts directly into the source tree...

  Microsoft.AspNetCore.Components -> F:\workspace\_work\1\s\src\Components\Components\ref\bin\Release\netcoreapp3.1\Microsoft.AspNetCore.Components.dll

But others like Http.Features don't do that, and still get the same issue. Very confusing. Still looking around.

dougbu · 2019-12-27T03:45:05Z

@wtgodbe the ref/ projects are currently built before the corresponding src/ projects only when targeting netcoreapp3.0. To relax this and ensure consistent ref/ before src/ builds, remove the second part of the condition at https://github.com/aspnet/AspNetCore/blob/release/3.1/eng/targets/ResolveReferences.targets#L258. This will add the @(ProjectReference) more often.

Note: Do not change how $(_CompileTfmUsingReferenceAssemblies) is set. Other uses of that property avoid a number of build problems -- as discussed offline.

- allows ApiCompat to run as expected

- !important! only run ApiCompat in inner builds - get ref/ assembly path from the relevant project - not all projects write into artifacts/... - make incompatibilities errors (unless overridden) nits: - use `$(HasReferenceAssembly)` short form

dougbu · 2020-01-08T19:47:25Z

@wtgodbe the broken *.Manual.cs file is still broken and I haven't addressed all comments above.

wtgodbe · 2020-01-08T22:20:31Z

The same set of projects is still broken, but now with:

F:\workspace_work\1\s.packages\microsoft.dotnet.apicompat\1.0.0-beta.19607.3\build\Microsoft.DotNet.ApiCompat.targets(49,5): error : ResolvedMatchingContract item must be specified to run API compat. [F:\workspace_work\1\s\src\Components\Components\src\Microsoft.AspNetCore.Components.csproj]

Will look in a bit

- ensure `ResolveCustomReferences` target runs consistently - `ResolveAssemblyReferences` didn't actually drive execution and it runs too late anyhow - remove redundant `$(_GenAPICmd)` setting

dougbu · 2020-01-10T19:26:59Z

🆙📅 to include two more fixes, one a nit the other important

analogrelay · 2020-02-13T18:47:56Z

I'm trying to clear up the state of outstanding servicing PRs. Whats the situation with this? Just holding waiting to fix it it up and merge? Is this infrastructure-only (sounds like it)? If so, we can slap the tell-mode label on it.

dougbu · 2020-02-13T20:01:46Z

@anurse to do this right requires Arcade changes from dotnet/arcade#4585 and I haven't quite got that working yet (thought it's close). Moved the PR to 3.1.x because it's not going to be ready for 3.1.3.

wtgodbe added 2 commits December 18, 2019 17:26

Add ApiCompat run

759d9ad

Add baseline files

de05237

wtgodbe requested review from analogrelay, BrennanConroy, dougbu, halter73, jkotalik, SteveSandersonMS, Tratcher and a team as code owners December 19, 2019 01:43

wtgodbe commented Dec 19, 2019

View reviewed changes

wtgodbe added this to the 3.1.2 milestone Dec 19, 2019

wtgodbe changed the title ~~Api compat~~ Add ApiCompat run to repo Dec 19, 2019

Only run ApiCompat on windows

1f6b1cf

Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Dec 19, 2019

Pilchie approved these changes Dec 19, 2019

View reviewed changes

dougbu reviewed Dec 21, 2019

View reviewed changes

dougbu added 3 commits January 5, 2020 21:54

Ensure ref/ projects are always built before corresponding src/ projects

ffaad70

- allows ApiCompat to run as expected

!fixup! Remove unused $(RefOutDirName) property

64e8ab7

!fixup! add two quick fixes

09512a1

- ensure `ResolveCustomReferences` target runs consistently - `ResolveAssemblyReferences` didn't actually drive execution and it runs too late anyhow - remove redundant `$(_GenAPICmd)` setting

wtgodbe mentioned this pull request Jan 10, 2020

Fix setter for CandidateState.Values in reference assembly #18278

Merged

wtgodbe modified the milestones: 3.1.2, 3.1.3 Jan 15, 2020

dougbu modified the milestones: 3.1.3, 3.1.x Feb 13, 2020

wtgodbe closed this Feb 14, 2020

wtgodbe deleted the ApiCompat branch February 14, 2020 18:19

dougbu removed this from the 3.1.x milestone Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ApiCompat run to repo #17959

Add ApiCompat run to repo #17959

wtgodbe commented Dec 19, 2019 •

edited

Loading

wtgodbe commented Dec 19, 2019

wtgodbe Dec 19, 2019

dougbu Dec 21, 2019

wtgodbe Dec 19, 2019

Tratcher Dec 19, 2019

wtgodbe Dec 19, 2019

Pilchie Dec 20, 2019

dougbu Dec 21, 2019

JunTaoLuo Dec 26, 2019

wtgodbe Dec 19, 2019

Pilchie left a comment

wtgodbe commented Dec 19, 2019

wtgodbe commented Dec 20, 2019

dougbu Dec 21, 2019

JunTaoLuo Dec 26, 2019

dougbu Dec 21, 2019

dougbu Dec 21, 2019

wtgodbe commented Dec 26, 2019 •

edited

Loading

wtgodbe commented Dec 26, 2019

wtgodbe commented Dec 26, 2019

dougbu commented Dec 27, 2019

dougbu commented Jan 8, 2020

wtgodbe commented Jan 8, 2020

dougbu commented Jan 10, 2020

analogrelay commented Feb 13, 2020 •

edited

Loading

dougbu commented Feb 13, 2020

		@@ -0,0 +1,4 @@
		Compat issues with assembly Microsoft.AspNetCore.Server.Kestrel.Core:
		ParameterModifiersCannotChange : Custom modifiers on parameter 'buffer' on method 'Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpParser<TRequestHandler>.Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.IHttpParser<TRequestHandler>.ParseRequestLine(TRequestHandler, System.Buffers.ReadOnlySequence<System.Byte>, System.SequencePosition, System.SequencePosition)' are 'System.Runtime.InteropServices.InAttribute' in the implementation but 'System.Runtime.InteropServices.InAttribute' in the contract.

Add ApiCompat run to repo #17959

Add ApiCompat run to repo #17959

Conversation

wtgodbe commented Dec 19, 2019 • edited Loading

wtgodbe commented Dec 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie left a comment

Choose a reason for hiding this comment

wtgodbe commented Dec 19, 2019

wtgodbe commented Dec 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgodbe commented Dec 26, 2019 • edited Loading

wtgodbe commented Dec 26, 2019

wtgodbe commented Dec 26, 2019

dougbu commented Dec 27, 2019

dougbu commented Jan 8, 2020

wtgodbe commented Jan 8, 2020

dougbu commented Jan 10, 2020

analogrelay commented Feb 13, 2020 • edited Loading

dougbu commented Feb 13, 2020

wtgodbe commented Dec 19, 2019 •

edited

Loading

wtgodbe commented Dec 26, 2019 •

edited

Loading

analogrelay commented Feb 13, 2020 •

edited

Loading