Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ApiCompat run to repo #17959
Add ApiCompat run to repo #17959
Changes from 2 commits
759d9ad
de05237
1f6b1cf
ffaad70
5e7b6ae
64e8ab7
09512a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.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 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
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.
This looks like a bug in the tool
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]
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.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.
We should look into this one
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.
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.
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.
@Pilchie does this example refer to your question? The tool picked up a diff between the
src
file and the correspondingmanual.cs
fileThere 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.
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.
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.
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.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 suspect we just messed up when updating Manual.cs files.