Skip to content

Add MSBuild task to ApiCompat - #8523

Merged
ViktorHofer merged 2 commits into
dotnet:mainfrom
ViktorHofer:ApiCompatTask
Mar 10, 2022
Merged

Add MSBuild task to ApiCompat#8523
ViktorHofer merged 2 commits into
dotnet:mainfrom
ViktorHofer:ApiCompatTask

Conversation

@ViktorHofer

@ViktorHofer ViktorHofer commented Mar 1, 2022

Copy link
Copy Markdown
Member

Add an MSBuild task for ApiCompat to avoid out-of-process invocations and keep the existing console application to not break existing developer workflows.

The changes result in dotnet/runtime libs building 10-15% faster (on my machine, Surface Book 2) as hundreds of out-of-process invocations of ApiCompat are now happening in-proc.

Time Elapsed 00:08:34.52 (before)
Time Elapsed 00:07:28.36 (after)

This PR shouldn't introduce a breaking change:

  • The arguments for the CLI invocations are exactly the same as before
  • The two removed properties aren't used by anyone _ApiCompatPath, _ApiCompatCommand (except for dotnet/runtime which I will update separately).

The diff is a bit annoying to read...

@ViktorHofer
ViktorHofer requested review from a team and ericstj March 1, 2022 15:26
@ViktorHofer ViktorHofer self-assigned this Mar 1, 2022
@ericstj

ericstj commented Mar 1, 2022

Copy link
Copy Markdown
Member

@ViktorHofer is this ready for review or did you want to first address the test failures? -- I didn't dig in yet to see if they're related. Let me know and I will have a look.

@@ -0,0 +1,350 @@
// Licensed to the .NET Foundation under one or more agreements.

@ViktorHofer ViktorHofer Mar 2, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the file changes here are just a move of code over from ApiCompatRunner.cs. The only noticeable change is the addition of the isMsBuildTask parameter.

@ViktorHofer

ViktorHofer commented Mar 2, 2022

Copy link
Copy Markdown
Member Author

@ericstj I just resolved the test failures. I had to introduce an additional assembly to make sure that msbuild types aren't resolved when running in a console to avoid dependencies on MSBuild.* assemblies on .NETCoreApp which ship as part of msbuild.

That said, there are still no breaking changes.

@ericstj

ericstj commented Mar 3, 2022

Copy link
Copy Markdown
Member

Not sure when I'll have a chance to look over this in detail; adding a couple reviewers. I see this API compat version as somewhat of a dead-end as we'd like to eventually move to dotnet/sdk#18677 so that we don't have to be chasing language features in our tools. We should keep our investments here minimal while maintaining its current functionality. I'm OK with this change to add a task but try to keep the risk super low.

And apply code clean-up suggested by VS.

Move core logic into Microsoft.DotNet.ApiCompat.Core
@ViktorHofer

Copy link
Copy Markdown
Member Author

@ericstj the change itself is trivial and I already tested it on dotnet/runtime without any observable behavior changes (except for better perf). When looking at the changes you will see that most of it is just a move. Here's the summary of the change in more detail:

  1. The code part that isn't console specific is moved from ApiCompatRunner.cs to Executor.cs.
  2. An ApiCompatTask.cs file is added which just accepts all the msbuild task arguments and invokes Executor.
  3. The remaining ApiCompatRunner.cs logic just handles the command line argument parsing and same as ApiCompatTask.cs invokes Executor.
  4. All the core logic is moved into a different assembly Microsoft.DotNet.ApiCompat.Core.dll as it uses MEF (CompositionHost) which enumerates all the types in it. We don't want the msbuild types to be resolved when running as a console as that would create a dependency on the MSBuild assemblies which for .NETCoreApp shouldn't be included in the package as they via the SDK with msbuild (under dotnet/sdk//).
  5. That means that the all the "frontend" logic is contained in Microsoft.DotNet.ApiCompat.dll and all the ApiCompat "backend" logic resides in Microsoft.DotNet.ApiCompat.Core.dll.
  6. The targets file is updated to use the ApiCompatTask (in-proc) instead of the Exec task (out-of-proc).

@ViktorHofer

Copy link
Copy Markdown
Member Author

Here's the comparison when building dotnet/runtime's lib subset with the existing Exec task that invokes ApiCompat.dll (Before) and with the ApiCompatTask (After). The result is impressive and I think speaks for itself. Take the Duration numbers with a grain of salt, the difference is actually more than 45 seconds on my machine but I forgot to warm up my machine before building dotnet/runtime with the ApiCompatTask changes.

Before After
image image

@ViktorHofer

ViktorHofer commented Mar 6, 2022

Copy link
Copy Markdown
Member Author

Me trying to motivate people to spend time reviewing this PR:

Let's assume that 200 individuals rebuild dotnet/runtime once per day. With this change they should be able to build the libraries part of the repository roughly 40s faster which means that ~130 minutes of build time is saved per day and resources can be spent elsewhere.

Another angle: CI builds will be able to build dotnet/runtime faster and a decent amount of money will be saved per day.

If you are reading this and your review is requested, consider spending a few minutes of your time so that time is given back to the team, partners and community members by building dotnet/runtime more efficiently

Thanks :)

@ViktorHofer

Copy link
Copy Markdown
Member Author

dotnet/runtime#66073 consumes these changes from my personal myget feed. You can see that no breaking changes are observable in dotnet/runtime :)

Comment thread src/Microsoft.DotNet.ApiCompat/src/ApiCompatRunner.cs Outdated
Comment thread src/Microsoft.DotNet.ApiCompat/src/ApiCompatTask.cs
Comment thread src/Microsoft.DotNet.ApiCompat/src/ApiCompatTask.cs Outdated
Comment thread src/Microsoft.DotNet.ApiCompat/src/ApiCompatTask.cs
Comment thread src/Microsoft.DotNet.ApiCompat/src/ApiCompatTask.cs Outdated
CustomErrorRegularExpression="^[a-zA-Z]+ :"
StandardOutputImportance="Low"
IgnoreExitCode="true">
<ApiCompatTask Contracts="@(ResolvedMatchingContract)"

@joperezr joperezr Mar 7, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be value in keeping a target that runs out-of-proc as an exe. I'm mainly thinking about potential consumers using a very old MSBuild or folks that hit issues when trying to move the the in-proc version. We wouldn't want those consumers to have to do work to continue using the latest APICompat since this APICompat is legacy, so instead we would want them to move to the PackageValidation version which is the one we want to invest on moving forward.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this PR is to switch from out-of-process to in-process without any breaking changes for all consumers of the current version of ApiCompat. Community members like @filipnavara already expressed their interest in benefiting from the change.

If someone uses a very old MSBuild version, then the .targets file couldn't be loaded anyway because the Project element doesn't have any of the boilerplate attributes or the xml declaration element that were required by MSBuild years ago.

If someone wants to use this outside of the MSBuild world then nothing changes as the console application continues to be invokable directly from the package.

Comment thread src/Microsoft.DotNet.ApiCompat/src/MSBuildTextWriter.cs
}

_writer.WriteLine("Total Issues: {0}", _totalDifferences);
if (!_isMSBuildTask)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not output this on the task output as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left an inline comment. As we are using TextWriter in the code path, there isn't a good way to log with different verbosity levels. As the msbuild logger always logs errors, it can't be used in this case without creating an entire abstraction that would also touch other projects like AsmDiff and CciExtensions. Note that this code path is only avoided when the task doesn't output to a baseline file which means when invoked directly.

@ViktorHofer

Copy link
Copy Markdown
Member Author

@joperezr thanks for the great feedback. Tests are now running for both the console and the msbuild version, comments are added where it makes sense and resolved a few behavior differences between the msbuild task and the console runner. Please find the changes in the recent commit. I also published a new version of ApiCompat to my private feed and will retrigger CI in the dotnet/runtime PR.

@joperezr joperezr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @ViktorHofer

@ViktorHofer
ViktorHofer merged commit 3d988e1 into dotnet:main Mar 10, 2022
@ViktorHofer
ViktorHofer deleted the ApiCompatTask branch March 10, 2022 07:30
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.

3 participants