-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ApiCompat: Add ValidateAssemblies msbuild task and add global tool #26616
ApiCompat: Add ValidateAssemblies msbuild task and add global tool #26616
Conversation
Hey @ViktorHofer sorry I didn't get a chance to look at this today. Will review tomorrow. |
src/Compatibility/Microsoft.DotNet.ApiCompatibility/Abstractions/IMetadataStreamProvider.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Runner/ApiCompatWorkItemTests.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Shared/RoslynResolver.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Task/Microsoft.DotNet.ApiCompat.Task.csproj
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs
Show resolved
Hide resolved
using System; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using Jab; |
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 you explain this new dependency? Is this what is getting us Dependency Injection? If so, do you think it is worth it to take on this new dependency on the sdk for an abstraction that strictly speaking doesn't need to be there? I agree it makes testing simpler and it decouples your APIs, but I wonder if it's worth the risk of adding an external dependency for this.
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.
Sure. Jab is a compile time source generator (AOT) which doesn't use any reflection and hence is a build time only dependency (it's marked as PrivateAssets="all"). Jab is already part of our source build and because of that doesn't introduce a prebuilt. The author is a previous .NET team member.
The source generator creates a composition file that just wraps the instantiation of the various types that are required by other types.
If so, do you think it is worth it to take on this new dependency on the sdk for an abstraction that strictly speaking doesn't need to be there? I agree it makes testing simpler and it decouples your APIs, but I wonder if it's worth the risk of adding an external dependency for this.
Yes, I think this component helps tremendously with managing dependencies inside ApiCompat without sacrificing runtime performance. I reviewed the generated code and there's nothing that stands out in terms of risk or perf penalty.
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 this component helps tremendously with managing dependencies inside ApiCompat
I'd be interested in getting more details around this.
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'd be interested in getting more details around this.
Basically the general advantages of using dependency injection. https://betterprogramming.pub/the-6-benefits-of-dependency-injection-7802b207ec69 summarizes them quite nicely.
Most importantly, the following paragraph:
Probably the main benefit of dependency injection is maintainability. If your classes are loosely coupled and follow the single responsibility principle — the natural result of using DI — then your code will be easier to maintain.
And this section nicely translates to the challenges with the previous implementation of the ApiCompatRunner which created dependencies just in time, which made it impossible to unit test the runner:
If you pass dependencies to classes, it’s quite simple to pass in a test double implementation. If dependencies are hard-coded, it’s impossible to create test doubles for those dependencies.
src/Compatibility/Microsoft.DotNet.ApiCompat.Shared/RegexStringTransformer.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Shared/RegexStringTransformer.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Shared/ValidateAssemblies.cs
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Shared/ValidateAssemblies.cs
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.PackageValidation/ApiCompatRunnerExtensions.cs
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs
Outdated
Show resolved
Hide resolved
src/Compatibility/Microsoft.DotNet.ApiCompatibility/ApiComparerFactory.cs
Show resolved
Hide resolved
using Microsoft.DotNet.ApiCompatibility.Abstractions; | ||
using Microsoft.DotNet.ApiCompatibility.Logging; | ||
|
||
namespace Microsoft.DotNet.ApiCompatibility.Runner |
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.
Was anything changed in these files other than the interface abstraction? It's hard when the diff shows the whole files as this essentially means everything in compat must be re-reviewed.
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.
The ones that I already touched on in previous meetings:
- Removed package specific log to make it usable outside of PackageValidation.
- Dependencies are moved into the constructor so that the type becomes unit testable (by mocking the external dependencies).
In addition to that:
- More diagnostics with low importance logged so that they appear in a binlog / high verbosity text log
- Strongly type work items and store them in a
HashSet<T>
instead of aDictionary<T,U>
.
src/Compatibility/Microsoft.DotNet.ApiCompat.Shared/RegexStringTransformer.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.DotNet.ApiCompat.Tests/RegexStringTransformerTests.cs
Show resolved
Hide resolved
...Tests/Microsoft.DotNet.ApiCompatibility.Tests/Microsoft.DotNet.ApiCompatibility.Tests.csproj
Show resolved
Hide resolved
I obviously didn't review line by line, but this looks good other than the comments I added. This being such a big refactoring, I would strongly suggest to test all your changes against a few dotnet repos that already consume package validation today in case you haven't done it, as well as try to call the new entry points (like the assemblyvalidation targets or the apicompat tool) on those repos and ensuring things are working. |
Fixes #18677
Please review by commits.
This change adds a global tool frontend that makes invoking ApiCompat's ValidateAssemblies and ValidatePackage functionality outside of msbuild possible. In addition to that, the ValidateAssemblies feature is now also exposed via an msbuild task.
The commits are grouped by usefulness to review:
dotnet/runtime#72464 consumes this change and demonstrates that there aren't any regressions in dotnet/runtime.
ValidateAssemblies
Features:
In discussions with @ericstj we agreed on that it doesn't make sense to expose the ValidateAssemblies targets in the SDK and only include them in the transport package for core stack consumption (i.e. dotnet/runtime, wcf, aspnetcore, etc.).
Differently than arcade's apicompat msbuild targets, these targets allow to run ApiCompat either in an inner or outer build, based on if the project multi-targets. This speeds up the overall time spent in ApiCompat as there's only a single invocation instead of n invocations for each inner build.
We don't want to enable the
ValidateAssemblies
task in dotnet/runtime until #23905 is implemented which @smasher164 and I are currently working on.