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

disable api compat for design time build #32157

Closed
wants to merge 1 commit into from
Closed

disable api compat for design time build #32157

wants to merge 1 commit into from

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Feb 12, 2020

Related to #32146

Design time build is failing as it tries to run apicompat even for all the target frameworks. ApiCompat will still run for building inside the visual studio

@@ -12,6 +12,7 @@
<RuntimeGraph>$(LibrariesProjectRoot)\OSGroups.json</RuntimeGraph>
<BuildTargetFramework>netcoreapp5.0</BuildTargetFramework>
<AdditionalBuildTargetFrameworks Condition="'$(BuildingInsideVisualStudio)' == 'true'">$(AdditionalBuildTargetFrameworks);$(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-OSX;$(NetCoreAppCurrent)-Linux;$(NetCoreAppCurrent)-NetBSD;$(NetCoreAppCurrent)-FreeBSD</AdditionalBuildTargetFrameworks>
<RunApiCompat Condition="'$(DesignTimeBuild)' == 'true'">false</RunApiCompat>
Copy link
Member

@eerhardt eerhardt Feb 12, 2020

Choose a reason for hiding this comment

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

I was thinking a bit more about this. Why is ApiCompat hooked into $(TargetsTriggeredByCompilation)? That doesn't seem like the ideal place for it to be hooked into the build. I would have expected it to be an AfterTargets="Build" or AfterTargets="CoreBuild", something along those lines.

If we are going to keep it hooked into $(TargetsTriggeredByCompilation) and if we didn't want to depend on the $(DesignTimeBuild) property, it looks like a different property we could check is $(SkipCompilerExecution). This is the property that tells CoreCompile to not actually compile the C# code during these design-time builds. It is paired with $(ProvideCommandLineArgs), which tells the Csc task to just output the full csc.exe command line args, which VS uses. It seems reasonable to not run ApiCompat when $(SkipCompilerExecution) == true.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at https://github.com/dotnet/project-system/blob/master/docs/design-time-builds.md#determining-whether-a-target-is-running-in-a-design-time-build, it looks like depending on $(DesignTimeBuild) is acceptable.

QUESTION: Should this change also go into https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets#L21? Does it ever make sense to run ApiCompat during a DesignTimeBuild? Reading that project-system doc:

Targets that dynamically change references, source files or compilation options must run during design-time builds to avoid unexpected behavior in Visual Studio. In contrast, if a target does not contribute these items, then it should actively avoid running in these builds to ensure design-time builds are as fast as possible.

I don't think ApiCompat changes references, source files or compilation options, right? So it would seem best to default it to off during a DesignTimeBuild.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @eerhardt on that. Let's move the fix over to arcade and change the defaults in ApiCompat.targets to not run on design time builds. I would not merge this in to not add unnecessary complexity.

Copy link
Member

Choose a reason for hiding this comment

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

Here's my Build Logging window when ApiCompat is running during DesignTimeBuild:

image

And when I shut it off:

image

Notice that the time went from 2 seconds down to under 0.1 seconds for the netstandard2.0 design time build. If you had a few projects in a single solution, doing ApiCompat would be taking a lot of time.

@ViktorHofer
Copy link
Member

Superseded by dotnet/arcade#4829

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants