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

Add public api analyzers for ObjectModel and dependencies #3042

Merged
merged 4 commits into from
Sep 8, 2021

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Sep 8, 2021

Description

Add public API analyzers for ObjectModel and it's dependencies to avoid changing public apis that we shipped.

Adding only to ObjectModel, because de definitions need to be duplicated for every TFM. Once/if this is merged, we can add it to all projects. Even those where changing the public api is not critical because they are used only by us.

Easiest way to introduce changes is by using the drop down TFM selector in VS, and fixing it for every TFM.

Related #2362

@nohwnd
Copy link
Member Author

nohwnd commented Sep 8, 2021

@Haplois we don't normally build for net6.0 only for source build, which now fails on the server. I think it's okay if I just don't analyze for that tfm?

@Sanan07
Copy link
Contributor

Sanan07 commented Sep 8, 2021

What does this analyzer do? It checks when we add some new public API and checks if we have it in that txt file?
So then we need manually add new API in txt file?

@nohwnd
Copy link
Member Author

nohwnd commented Sep 8, 2021

@Sanan07 For any public API it will check the shipped and unshipped files. If it does not find it, then RSxxx build error occurs and you will get the option to add the api using the normal lightbulb quick fix. The annoying thing for now is that you need to apply the fix for every TFM that it applies to. You can copy the line that was generated into Unshipped.txt into the other files, or select each tfm in VS and apply the same fix.

This can also be done for all occurrences of the error, per TFM. But sadly not for all TFMs at the same time. This will get better hopefully when the above fix is merged, because then we can have a central file with the common api, and separate files with TFM specific API. So you only need to change it in one file when it applies to all TFMs.

Additionally I will add a check to GH to require approval when PR changes any of the shipped and unshipped files. And before release, or someone in the release cycle we should review the apis that we would newly ship.

@nohwnd nohwnd enabled auto-merge (squash) September 8, 2021 14:27
@nohwnd nohwnd merged commit c14cf70 into microsoft:main Sep 8, 2021
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.

2 participants