Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Following up on API baseline check and documentation #30221

Merged
merged 18 commits into from
Feb 25, 2021
3 changes: 3 additions & 0 deletions .azure/pipelines/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ stages:
steps:
- powershell: ./eng/scripts/CodeCheck.ps1 -ci $(_InternalRuntimeDownloadArgs)
displayName: Run eng/scripts/CodeCheck.ps1
env:
PULLREQUEST_SOURCEBRANCH: $(System.PullRequest.SourceBranch)
PULLREQUEST_TARGETBRANCH: $(System.PullRequest.TargetBranch)
JunTaoLuo marked this conversation as resolved.
Show resolved Hide resolved
artifacts:
- name: Code_Check_Logs
path: artifacts/log/
Expand Down
22 changes: 19 additions & 3 deletions docs/APIBaselines.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,24 @@ This document contains information regarding API baseline files and how to work

When creating a new implementation (i.e. src) project, it's necessary to manually add API baseline files since API baseline are enabled by default. If the project is a non-shipping or test only project, add `<AddPublicApiAnalyzers>false</AddPublicApiAnalyzers>` to the project to disable these checks. To add baseline files to the project:

1. Copy eng\PublicAPI.empty.txt to the project directory and rename it to PublicAPI.Shipped.txt
1. Copy eng\PublicAPI.empty.txt to the project directory and rename it to PublicAPI.Unshipped.txt
1. `cp .\eng\PublicAPI.empty.txt {new folder}\PublicAPI.Shipped.txt`
1. `cp .\eng\PublicAPI.empty.txt {new folder}\PublicAPI.Unshipped.txt`
1. Update AspNetCore.sln and relevant `*.slnf` file to include the new project
1. `{directory containing relevant *.slnf}\startvs.cmd`
1. F6 # or whatever your favourite build gesture is
1. Click on a RS0016 (or whatever) error
1. Right click in editor on underscored symbol or go straight to the “quick fix” icon to its left. Control-. also works.
1. Choose “Add Blah to public API” / “Fix all occurrences in … Solution”
1. Click Apply
1. F6 # again to see if the fixer missed anything or if other RS00xx errors show up (not uncommon)
JunTaoLuo marked this conversation as resolved.
Show resolved Hide resolved
1. Suppress or fix other problems as needed but please suppress (if suppressing) using attributes and not globally or with `#pragma`s because attributes make the justification obvious e.g. for common errors that can't be fixed
`[SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "Required to maintain compatibility")]`
or
`[SuppressMessage("ApiDesign", "RS0027:Public API with optional parameter(s) should have the most parameters amongst its public overloads.", Justification = "Required to maintain compatibility")]`

More information available in
- https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md
- https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/Microsoft.CodeAnalysis.PublicApiAnalyzers.md
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding a note to start at step (4) when making changes in projects with existing API baselines.

Copy link
Member

Choose a reason for hiding this comment

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

We probably need at least a note about the *REMOVED* prefix whether or not the fixer will handle that automatically. Doubt other syntax notes are interesting to most people.

Speaking of special cases 😺 @BrennanConroy did the fixer handle most / all of the PublicAPI.Unshipped.txt changes in #29219 without manual changes, aside from any [SuppressMessage(...)] attributes you needed❔

Copy link
Member

Choose a reason for hiding this comment

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

No, I gave up on the fixer and did it all manually

Copy link
Member

Choose a reason for hiding this comment

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

Wow, I remember the fixer doing a pretty good job even when fixing all instances of a problem in an entire (albeit filtered) solution. What went wrong e.g. is there an issue or three we should report in https://github.com/dotnet/roslyn-analyzers❔

Copy link
Member

Choose a reason for hiding this comment

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

It seemed inconsistent about which file it edited, so I couldn't just blindly accept whatever changes it wanted to make.

Copy link
Member

@dougbu dougbu Feb 18, 2021

Choose a reason for hiding this comment

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

Ah, for example does it edit PublicAPI.Shipped.txt files rather than add *REMOVED* lines in PublicAPI.Unshipped.txt❔

@sharwell and @mavasani is that a known issue❔ Could the fixer be fixed to always leave PublicAPI.Shipped.txt alone❔ (Well, except if you add a gesture for use after a release -- where the files should be merged.)


## PublicAPI.Shipped.txt

Expand Down Expand Up @@ -47,4 +63,4 @@ Microsoft.AspNetCore.DataProtection.Infrastructure.IApplicationDiscriminator.Dis

## Updating baselines after major releases

TODO
This will be performed by the build team using scripts at https://github.com/dotnet/roslyn/tree/master/scripts/PublicApi. The process moves the content of PublicAPI.Unshipped.txt into PublicAPI.Shipped.txt
9 changes: 6 additions & 3 deletions eng/scripts/CodeCheck.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,18 @@ try {
}
}

Write-Host "Checking for changes to API baseline files"
$targetBranch = $env:PULLREQUEST_TARGETBRANCH
$sourceBranch = $env:PULLREQUEST_SOURCEBRANCH
JunTaoLuo marked this conversation as resolved.
Show resolved Hide resolved

# Retrieve the set of changed files compared to main
$commitSha = git rev-parse HEAD
$changedFilesFromMain = git --no-pager diff origin/main...$commitSha --ignore-space-change --name-only --diff-filter=ar
Write-Host "Checking for changes to API baseline files $targetBranch...$sourceBranch"

$changedFilesFromMain = git --no-pager diff $targetBranch...$sourceBranch --ignore-space-change --name-only --diff-filter=ar
$changedAPIBaselines = [System.Collections.Generic.List[string]]::new()

if ($changedFilesFromMain) {
foreach ($file in $changedFilesFromMain) {
# Check for changes in Shipped in dev branches and both Shipped and Unshipped in servicing branches
if ($file -like '*PublicAPI.Shipped.txt') {
$changedAPIBaselines.Add($file)
}
Expand Down
2 changes: 2 additions & 0 deletions src/Caching/SqlServer/src/PublicAPI.Shipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,5 @@ Microsoft.Extensions.DependencyInjection.SqlServerCachingServicesExtensions
~Microsoft.Extensions.Caching.SqlServer.SqlServerCacheOptions.TableName.get -> string
~Microsoft.Extensions.Caching.SqlServer.SqlServerCacheOptions.TableName.set -> void
~static Microsoft.Extensions.DependencyInjection.SqlServerCachingServicesExtensions.AddDistributedSqlServerCache(this Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action<Microsoft.Extensions.Caching.SqlServer.SqlServerCacheOptions> setupAction) -> Microsoft.Extensions.DependencyInjection.IServiceCollection

TEST
JunTaoLuo marked this conversation as resolved.
Show resolved Hide resolved