Add initial version of ApiCompatibility

[safern]

This is the initial commit for the ApiCompatibility object model to achieve shipping tooling to help libraries validate binary and compile compatibility while developing new versions of their libraries.

I expect some things to change as I add more rules, this is first version of the design and as we move forward I will add optimizations as I find them and as we start to measure performance.

Also, there are still open questions on the diagnostic IDs and other behavior like settings and error baselining for the user that needs to be figured out, currently discussed at: dotnet/designs#196 and dotnet/designs#177

The plan is to first start with an MSBuild Task built on top of this object model and then expand it to other tools as needed/requested like for example a roslyn analyzer if possible.

I plan on moving diagnostic messages and exceptions to a resx file so that they are localized in a subsequent PR.

FYI: @weshaggard @dougbu @danmoseley @terrajobst

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[wli3]

What is the overall CI build+test time before vs after, local build time before vs after, make one line change rerun test time before vs after

[ericstj]

Looks similar to https://github.com/dotnet/runtime/blob/597186e83e3a1972f35c6a601fdbc889cac84b30/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/PathAssemblyResolver.cs#L23

Public key and version are important. If we don't constrain the load, then we need to diff them to catch things like mismatched assembly identities or lower versions in assemblies that are expected to be compatible.

[safern]

The reason why I didn't use System.Reflection.MetadataLoadContext to do this (that was my initial approach) is because we would need to provide the corelib path, however I think it makes sense to do it that way since users need to say "I want you to resolve references", in order for this code to be executed and if resolving references is enabled, then the search paths should contain where the corelib is?

[safern]

Will open a follow up issue.

[ericstj]

I didn’t mean that you had to use MLC but just consider how it solved the similar problem. Note that we actually had a bug here and had to patch MLC because it didn’t handle retargetable assemblies.

[ericstj]

I suspect you'll need to run on .NETFramework in the context of the desktop msbuild and may want to explicitly target it. Follow the pattern used by other SDK assemblies.

[safern]

It seems like other SDK assemblies target net472 and/or net6.0. However I did find a source generator that targets netstandard2.0. So if you have the preference of targeting net472 and netstandard2.0 or netcoreapp3.1 I can do that. Whatever your preference is.

[ericstj]

It’s not my preference that matters, you’ll need to consider the implications.

For source generators we have a host that ensures shims are present along with any necessary bindingRedirects. In general analyzers/source generators don’t need to carry dependencies since they typically just rely on the compiler platform assemblies.

MSBuild tasks on the other hand don’t get much from the host other than the MSBuild interfaces and need to carry their dependencies. Our versioning policy for some dotnet/runtime assemblies will result in versioning of netframework assemblies and not netstandard nor netcore. As a result we may introduce the need for more bindingRedirects if we don’t target netframework. As you know bindingRedirects are difficult to deal with in MSBuild tasks (we should avoid AssemblyRrsolve hack in production). I believe the SDK assemblies take special care to avoid them.

It’s fine to defer until it’s a problem if you want. I was expecting there was an established pattern in the SDK.

[safern]

Thanks for elaborating. It seems like the pattern they have or the way the avoid this is they target net472;net6.0 (or whatever the current tfm is), so I think we could safely target net472;netcoreapp3.1 here? Or would we need to include a netstandard2.0 asset as well?

[ericstj]

This seems like it might have some overhead when we have lots of rules. I wonder if there is a better pattern to use here? Might be something to think about / measure. Events/handlers seem like a similar problem, though they are a bit harder to use 🤔

[safern]

Yes, this is something I would like to revisit afterwards and improve. Maybe we can have rules hook up to specific events, like MemberMapperRuleEvent, so that we don't iterate on all the rules for a MemberMapper and just the rules that need to run on it?

[safern]

I chatted with @ericstj offline, I will open a follow up issue so that we can make progress on this but this is something that I should investigate.

[safern]

What is the overall CI build+test time before vs after, local build time before vs after, make one line change rerun test time before vs after

Local Machine Windows

Before build.cmd time

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:01:38.06

After build.cmd time

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:01:40.99

Before build.cmd -test time

Time Elapsed 00:47:40.96

After build.cmd -test time

Time Elapsed 00:50:07.74

Note that these times (for both after and before) varied in between multiple runs. However if I run dotnet test on my test project it takes only 9-12 secs per tfm to run the tests.

[safern]

I addressed all PR feedback except the one I'm going to follow up as separate issues. Let me know if there is something else blocking this PR.

[danmoseley]

Will this regress dev inner loop perf (which we're working hard to drive down)? Or is it opt in or release config only etc?

[safern]

Will this regress dev inner loop perf (which we're working hard to drive down)? Or is it opt in or release config only etc?

Yes I expect this to regress the inner loop perf (I don't know by how much I will get the numbers further down the road), however we can leave the APICompat run that happens at build be opt-in and the validation that happens only when you create a nuget package be opt-out.

[Anipik]

Awesome work. left few comments.

[ericstj]

Yes I expect this to regress the inner loop perf (I don't know by how much I will get the numbers further down the road), however we can leave the APICompat run that happens at build be opt-in and the validation that happens only when you create a nuget package be opt-out.

I don’t think we consider pack to be part of inner loop so I don’t think this will impact inner dev loop (build/test).

Running APICompat on build isn’t something I expect most customers to opt into since they don’t seperate their reference and implementation. We do in dotnet/runtime, but there I would hope we can make improvements over the existing out-of-process CCI based implementation perf.

[ericstj]

Take note of this scenario: https://github.com/dotnet/arcade/blob/3dd26504b456b68c2e3edd18ebf159d0b5486135/src/Microsoft.Cci.Extensions/Filters/PublicOnlyCciFilter.cs#L55-L66

And this one: https://github.com/dotnet/arcade/blob/3dd26504b456b68c2e3edd18ebf159d0b5486135/src/Microsoft.Cci.Extensions/Filters/PublicOnlyCciFilter.cs#L82-L93

Maybe those are other filters but they did matter for our existing API compat and GenAPI analysis.

[safern]

Thanks @ericstj. Will open an issue for this one as well. Filtering needs to be completed as I'm also missing filtering out attributes based on settings (I guess that will be another filter) but I will take note of these scenarios!

[danmoseley]

I don’t think we consider pack to be part of inner loop so I don’t think this will impact inner dev loop (build/test).
Running APICompat on build isn’t something I expect most customers to opt into since they don’t seperate their reference and implementation. We do in dotnet/runtime, but there I would hope we can make improvements over the existing out-of-process CCI based implementation perf.

Yeah, I was just wondering about regular builds. Sounds good then.

[safern]

Thanks for the feedback. Addressed all comments. Left unresolved the ones I'm going to open issues for.

[dsplaisted]

There is one recently added test (GivenNoPackageVersionItCanInstallLatestVersionOfPackage) failing on one leg (Ubuntu). I'm merging this PR anyway since it's time sensitive (and the failure is happening in other PRs).

[safern]

Thanks, @dsplaisted