-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Fixup API baselines after merge #28253
Conversation
@JunTaoLuo did this get fixed elsewhere❔ Please fix the conflicts or close😃 |
Good start apart from the conflicts 😁 |
Restore Shipped files to match release/5.0 Update Unshipped to reflect changed APIs
ed291af
to
450e05c
Compare
Add codecheck test for modified baseline files Add documentation
|
||
## Updating baselines after major releases | ||
|
||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dougbu would you like to update this section since you did it for 5.0 and have more expreience 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 5.0, I only had to copy files around because all PublicAPI.Shipped.txt files were empty. Fortunately a couple of scripts are available e.g. in the Roslyn repo. dotnet/roslyn-analyzers#3226 covers centralizing that 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these modified automatically during the build? Is there a way to automatically populate them? If not how should devs update them? Would be worth calling out in the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visual Studio is the only reliable way to update the files at the moment. Editing them manually is strongly discouraged because the syntax is finicky.
@Pilchie is working with the Roslyn team to enable dotnet format
to handle this from the command line.
In the meantime, instructions for adding new implementation projects should mention
cp .\eng\PublicAPI.empty.txt {new folder}\PublicAPI.Shipped.txt
cp .\eng\PublicAPI.empty.txt {new folder}\PublicAPI.Unshipped.txt
- Update AspNetCore.sln and relevant
*.slnf
file to include the new project {directory containing relevant *.slnf}\startvs.cmd
- F6 # or whatever your favourite build gesture is
- Click on a RS0016 (or whatever) error
- Right click in editor on underscored symbol or go straight to the “quick fix” icon to its left . Control-. also works.
- Choose “Add Blah to public API” / “Fix all occurrences in … Solution”
- Click Apply
- F6 # yes, again to see if the fixer missed anything or if other RS00xx errors show up (not uncommon)
- 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
|
||
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to make the change now, but if there's a way to not hard-code the branch name that'd be preferable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how we can remove the hard coding to the default branch, is there a git command to do that? In any case, I figured the default branch name shouldn't be modified often and I think in those cases, we would scrub the script for usages of the name anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There be dragons @JunTaoLuo In particular,
- We need to enable a more drastic version of this in
release/5.0
, covering the PublicAPI.Unshipped.txt files too- We need to compare against
release/5.0
there
- We need to compare against
- Need to compare against our preview branches for 6.0 after each is split from
main
- This check will do nothing in rolling builds and should be suppressed unless
$(Build.Reason)
(in YAML) or$env:BUILD_REASON
containPullRequest
To get current branch in PR builds, can use
git config --show-current
displays the current branch name- same information should be available in
$(Build.SourceBranchName)
in YAML or$env:BUILD_SOURCEBRANCHNAME
- use above w/o
Name
/NAME
to get the full branch path but, with the name, should display the PR id
Need something like the GitHub REST API to get more information e.g. to display the base aka target branch
Invoke-WebRequest -Headers @{ Accept = "application/vnd.github.v3+json"} https://api.github.com/repos/dotnet/aspnetcore/pulls/{PR id} | `
ConvertFrom-Json |ForEach-Object `
base |ForEach-Object `
ref
For internal PRs, the commands are probably like
Invoke-WebRequest -Headers @{ Accept = "application/vnd.github.v3+json"} https://dev.azure.com/{blah}/{blah}/_git/dotnet-aspnetcore/_apis/git/pullrequests/{PR id}?api-version=5.0 |ForEach-Object `
targetRefName
But, I don't see documentation on how to authenticate nor control the preferred response type. (May respond w/ JSON in successful cases but I'm only getting 404s despite testing w/ an existing PR.)
@ilyas1974 anyone in your world with the AzDO REST API knowledge we need❔ Note we'd like to run this in CI, where we should have the right tokens available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suepct we'd be fine w/ a translation of https://docs.microsoft.com/en-us/rest/api/azure/devops/git/pull%20requests/get%20pull%20request%20by%20id?view=azure-devops-rest-5.0#security into English 😀
The codecheck failure is expected since this PR needs to fix and restore APIBaseline.Shipped.txt files. In these cases, the admins need to override the failure and merge. I'll follow up with improvements to the documentation separately. |
TODO: