Switch to PublicApiAnalyzers #7018

rainersigwald · 2021-11-05T18:40:03Z

Abandon GenAPI for the new hotness in public API change tracking, which

Is a Roslyn analyzer.
Works cross-platform.
Has clear messages.
Actually tracks the API, instead of making us do it manually on review.

Based off the vs17.0 branch so I didn't accidentally canonize new API.

This is the modern replacement for GenAPI that Roslyn itself has been using for a while, and it makes clearer errors and works cross-platform.

This is also public, shipping API.

with <Target Name="CreatePublicAPI" BeforeTargets="BeforeBuild"> <MakeDir Directories="@(AdditionalFiles->'%(RootDir)%(Directory)')" /> <Touch Files="@(AdditionalFiles)" AlwaysCreate="true" /> </Target>

Get-ChildItem -Recurse PublicAPI.Shipped.txt | % {Remove-Item $_} Get-ChildItem -Recurse PublicAPI.Unshipped.txt | % {Rename-Item $_ PublicAPI.Shipped.txt -Force ; New-Item $_ -ItemType file}

The vast majority of our API is not nullable aware so disable this.

The generic parameter was correct but this avoids this C# error: error CS1658: Type parameter declaration must be an identifier not a type. See also error CS0081.

The new PublicAPI.Shipped.txt stuff should do the trick.

rainersigwald · 2021-11-05T18:40:29Z

@ladipro, I'd especially like your review on this because of the StringTools API-tracking stuff.

jeffkl

GenAPI has served us well but I'm excited to see us adopt more evolved solution!

rainersigwald · 2021-11-05T19:36:33Z

The merge I just pushed will demonstrate what the errors look like in CI (that's totally why I'm doing it, nothing to do with laziness).

rainersigwald · 2021-11-05T19:55:22Z

Not too bad IMO:

https://dev.azure.com/dnceng/public/_build/results?buildId=1456841&view=results

Now I'll push a better merge.

rainersigwald · 2021-11-05T20:32:22Z

Now I'll push a better merge.

Let my failure to do this be a WARNING and a CAVEAT to this: I used "fix all in project" and it only did the first TF of that project. You can instead use "fix all in solution" to get everything (per dotnet/roslyn-analyzers#4954 (comment)).

benvillalobos · 2021-11-05T21:04:36Z

src/Directory.Build.targets

    <GenAPIAssemblyName>$(AssemblyName)</GenAPIAssemblyName>
    <GenAPIAssemblyName Condition="'$(GenAPIAssemblyName)' == ''">$(MSBuildProjectName)</GenAPIAssemblyName>
    <GenAPIShortFrameworkIdentifier Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">net</GenAPIShortFrameworkIdentifier>
    <GenAPIShortFrameworkIdentifier Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'">netstandard</GenAPIShortFrameworkIdentifier>
    <GenAPIShortFrameworkIdentifier Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp'">netstandard</GenAPIShortFrameworkIdentifier>
-    <GenAPITargetPath>$(RepoRoot)ref\$(GenAPIAssemblyName)\$(GenAPIShortFrameworkIdentifier)\$(GenAPIAssemblyName).cs</GenAPITargetPath>


I noticed GenAPITargetPath wasn't used before. Was it getting picked up somewhere else in the build, or was this an unused property?

It was used in the GenAPI package's targets somewhere.

Got it, and we're not using them anymore so should we get rid of some of these variables? Or proactively rename them to match the analyzer we're moving to.

Consider this a nit though.

This is used only in Debug and the difference is only the one thing so I don't think it's worth having separate Debug/Release APIs.

ladipro

Looks great, thank you! What is supposed to go into the Unshipped files? The documentation just says they must exist; no details given.

rainersigwald · 2021-11-08T15:15:25Z

What is supposed to go into the Unshipped files?

Lol, yeah, we should write up some docs. The PublicApiAnalyzers process goes like this:

The analyzer keeps the PublicAPI.*.txt files updated.
New API surface goes into PublicAPI.Unshipped.txt.
At release time, we must manually promote the Unshipped public API to Shipped

The idea is to distinguish between new public API surface in a given release (can still be changed if we figure out a better API shape, because we don't have to maintain compat with daily builds) and shipped public API surface (which we shouldn't change because someone might be using it).

I'll write up a doc.

Primarily to give a place to mention what we now need to do for PublicApiAnalyzers.

This allows us to delete the `ref` folder entirely. This was an intermediate state of D.B.targets: ``` <PropertyGroup Condition="'$(GenerateReferenceAssemblySource)' == 'true'"> <GenAPIAssemblyName>$(AssemblyName)</GenAPIAssemblyName> <GenAPIAssemblyName Condition="'$(GenAPIAssemblyName)' == ''">$(MSBuildProjectName)</GenAPIAssemblyName> <GenAPIShortFrameworkIdentifier Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">net</GenAPIShortFrameworkIdentifier> <GenAPIShortFrameworkIdentifier Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'">netstandard</GenAPIShortFrameworkIdentifier> <GenAPIShortFrameworkIdentifier Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp'">netstandard</GenAPIShortFrameworkIdentifier> <PublicApiTfm Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">net</PublicApiTfm> <PublicApiTfm Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework' and $([MSBuild]::GetTargetFrameworkVersion('$(TargetFramework)')) == '3.5'">net35</PublicApiTfm> <PublicApiTfm Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETStandard'">netstandard</PublicApiTfm> <PublicApiTfm Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETCoreApp'">netstandard</PublicApiTfm> <GenAPIFolderPath>$(RepoRoot)ref\$(GenAPIAssemblyName)\$(GenAPIShortFrameworkIdentifier)\</GenAPIFolderPath> </PropertyGroup> <ItemGroup Condition="'$(GenerateReferenceAssemblySource)' == 'true'">  <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" PrivateAssets="all" /> <AdditionalFiles Include="PublicAPI/$(PublicApiTfm)/PublicAPI.Shipped.txt" /> <AdditionalFiles Include="PublicAPI/$(PublicApiTfm)/PublicAPI.Unshipped.txt" /> </ItemGroup> <Target Name="MovePublicAPI" BeforeTargets="BeforeBuild" Condition="'$(GenerateReferenceAssemblySource)' == 'true'"> <MakeDir Directories="$(MSBuildProjectDirectory)/PublicAPI/$(PublicApiTfm)" /> <Copy SourceFiles="$(GenAPIFolderPath)PublicAPI.Shipped.txt;$(GenAPIFolderPath)PublicAPI.Unshipped.txt" DestinationFolder="$(MSBuildProjectDirectory)/PublicAPI/$(PublicApiTfm)" /> </Target> ```

Conflicts: ref/Microsoft.Build/net/Microsoft.Build.cs ref/Microsoft.Build/netstandard/Microsoft.Build.cs

rainersigwald added 10 commits November 5, 2021 13:29

Adopt Microsoft.CodeAnalysis.PublicApiAnalyzers

cec7356

This is the modern replacement for GenAPI that Roslyn itself has been using for a while, and it makes clearer errors and works cross-platform.

GenerateReferenceAssemblySource for StringTools

c63588b

This is also public, shipping API.

Create PublicAPI files

260122e

with <Target Name="CreatePublicAPI" BeforeTargets="BeforeBuild"> <MakeDir Directories="@(AdditionalFiles->'%(RootDir)%(Directory)')" /> <Touch Files="@(AdditionalFiles)" AlwaysCreate="true" /> </Target>

Fix all RS0016 in solution via VS

32978be

Promote current API surface to Shipped

07387d2

Get-ChildItem -Recurse PublicAPI.Shipped.txt | % {Remove-Item $_} Get-ChildItem -Recurse PublicAPI.Unshipped.txt | % {Rename-Item $_ PublicAPI.Shipped.txt -Force ; New-Item $_ -ItemType file}

Turn on nullability for StringTools API

0f3070e

Disable RS0037 'annotate nullability in API'

99f6c4d

The vast majority of our API is not nullable aware so disable this.

Annotate GetOrCreateEntry's cacheHit param

4cf2513

Genericize 'see' references in doc text

1c0b20d

The generic parameter was correct but this avoids this C# error: error CS1658: Type parameter declaration must be an identifier not a type. See also error CS0081.

Remove GenAPI

d6aeafc

The new PublicAPI.Shipped.txt stuff should do the trick.

rainersigwald added the Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. label Nov 5, 2021

rainersigwald added this to the MSBuild 17.1 milestone Nov 5, 2021

rainersigwald requested review from benvillalobos and ladipro November 5, 2021 18:40

jeffkl approved these changes Nov 5, 2021

View reviewed changes

rainersigwald force-pushed the publicapianalyzers branch from 7fb4b92 to 3faffd6 Compare November 5, 2021 19:55

Merge remote-tracking branch 'upstream/main' into publicapianalyzers

2f47dc4

rainersigwald force-pushed the publicapianalyzers branch from 3faffd6 to 2f47dc4 Compare November 5, 2021 20:29

benvillalobos approved these changes Nov 5, 2021

View reviewed changes

rainersigwald added 2 commits November 5, 2021 16:23

fixup! Remove GenAPI

c42fa30

Remove WaitForDebugger from public API

030958c

This is used only in Debug and the difference is only the one thing so I don't think it's worth having separate Debug/Release APIs.

ladipro approved these changes Nov 8, 2021

View reviewed changes

Forgind added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Nov 8, 2021

Add "release" doc

89cd715

Primarily to give a place to mention what we now need to do for PublicApiAnalyzers.

rainersigwald added 2 commits November 10, 2021 14:53

Merge remote-tracking branch 'upstream/main' into publicapianalyzers

d0a27a0

Conflicts: ref/Microsoft.Build/net/Microsoft.Build.cs ref/Microsoft.Build/netstandard/Microsoft.Build.cs

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Nov 17, 2021

rainersigwald merged commit 2127450 into dotnet:main Nov 22, 2021

rainersigwald deleted the publicapianalyzers branch November 22, 2021 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to PublicApiAnalyzers #7018

Switch to PublicApiAnalyzers #7018

rainersigwald commented Nov 5, 2021 •

edited

Loading

rainersigwald commented Nov 5, 2021

jeffkl left a comment

rainersigwald commented Nov 5, 2021

rainersigwald commented Nov 5, 2021 •

edited

Loading

rainersigwald commented Nov 5, 2021

benvillalobos Nov 5, 2021

rainersigwald Nov 5, 2021

benvillalobos Nov 5, 2021

ladipro left a comment

rainersigwald commented Nov 8, 2021

Switch to PublicApiAnalyzers #7018

Switch to PublicApiAnalyzers #7018

Conversation

rainersigwald commented Nov 5, 2021 • edited Loading

rainersigwald commented Nov 5, 2021

jeffkl left a comment

Choose a reason for hiding this comment

rainersigwald commented Nov 5, 2021

rainersigwald commented Nov 5, 2021 • edited Loading

rainersigwald commented Nov 5, 2021

benvillalobos Nov 5, 2021

Choose a reason for hiding this comment

rainersigwald Nov 5, 2021

Choose a reason for hiding this comment

benvillalobos Nov 5, 2021

Choose a reason for hiding this comment

ladipro left a comment

Choose a reason for hiding this comment

rainersigwald commented Nov 8, 2021

rainersigwald commented Nov 5, 2021 •

edited

Loading

rainersigwald commented Nov 5, 2021 •

edited

Loading