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 Build Acceleration for known incompatible packages #9214

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Aug 2, 2023

Contributes to #9106

While rare, it's possible for a NuGet package to include .props and/or .targets files that customise the build in a way that's not compatible with Build Acceleration. Ideally such packages would also set AccelerateBuildsInVisualStudio to false, however that's not always an option.

To address this situation, this change introduces the ability to specify NuGet packages that, when present in a project, will disable Build Acceleration.

We add one known package (Microsoft.VSSDK.BuildTools, for VSIX projects) to our design-time .targets file, but SDKs and customers may add their own items too.

For example, if MyPackage is known to be incompatible with Build Acceleration, adding a BuildAccelerationIncompatiblePackage item to your Directory.Build.props will automatically cause Build Acceleration to be disabled for any project that references that MyPackage:

<Project>
  <PropertyGroup>
    <AccelerateBuildsInVisualStudio>true</AccelerateBuildsInVisualStudio>
  </PropertyGroup>
  <ItemGroup>
    <BuildAccelerationIncompatiblePackage Include="MyPackage" />
  </ItemGroup>
</Project>

When a project is found to have an incompatible project reference, the FUTDC log displays a message to that effect, such as:

Build acceleration has been disabled for this project due to known incompatible NuGet package reference(s) 'Microsoft.VSSDK.BuildTools'. See https://aka.ms/vs-build-acceleration.

Microsoft Reviewers: Open in CodeFlow

While rare, it's possible for a NuGet package to include `.props` and/or `.targets` files that customise the build in a way that's not compatible with Build Acceleration. Ideally such packages would also set `AccelerateBuildsInVisualStudio` to `false`, however that's not always an option.

To address this situation, this change introduces the ability to specify NuGet packages that, when present in a project, will disable Build Acceleration.

We add one known package (`Microsoft.VSSDK.BuildTools`, for VSIX projects) to our design-time `.targets` file, but SDKs and customers may add their own items too.

For example, if `MyPackage` is known to be incompatible with Build Acceleration, adding a `BuildAccelerationIncompatiblePackage` item  to your `Directory.Build.props` will automatically cause Build Acceleration to be disabled for any project that references that `MyPackage`:

```xml
<Project>
  <PropertyGroup>
    <AccelerateBuildsInVisualStudio>true</AccelerateBuildsInVisualStudio>
  </PropertyGroup>
  <ItemGroup>
    <BuildAccelerationIncompatiblePackage Include="MyPackage" />
  </ItemGroup>
</Project>
```

When a project is found to have an incompatible project reference, the FUTDC log displays a message to that effect, such as:

> Build acceleration has been disabled for this project due to known incompatible NuGet package reference(s) 'Microsoft.VSSDK.BuildTools'. See https://aka.ms/vs-build-acceleration.
@drewnoakes drewnoakes added the Feature-Build-Acceleration Build Acceleration can skip calls to MSBuild within Visual Studio label Aug 2, 2023
@drewnoakes drewnoakes added this to the 17.8 milestone Aug 2, 2023
@drewnoakes drewnoakes requested a review from a team as a code owner August 2, 2023 11:14
@drewnoakes drewnoakes mentioned this pull request Aug 2, 2023
11 tasks
<!-- Disable Build Acceleration for VSIX projects -->
<AccelerateBuildsInVisualStudio Condition="'$(IsVsixProject)' == 'true'">false</AccelerateBuildsInVisualStudio>
Copy link
Member Author

Choose a reason for hiding this comment

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

This never really worked property. Turns out that IsVsixProject is our own property, not one defined in Microsoft.VSSDK.BuildTools's .props or .targets.

Comment on lines +499 to +503
<!-- Collect packages that are incompatible with Build Acceleration. -->
<PropertyGroup Condition="'$(CollectBuildAccelerationIncompatiblePackageDesignTimeDependsOn)' == ''">
<CollectBuildAccelerationIncompatiblePackageDesignTimeDependsOn></CollectBuildAccelerationIncompatiblePackageDesignTimeDependsOn>
</PropertyGroup>
<Target Name="CollectBuildAccelerationIncompatiblePackageDesignTime" DependsOnTargets="$(CollectBuildAccelerationIncompatiblePackageDesignTimeDependsOn)" Returns="@(BuildAccelerationIncompatiblePackage)" />
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows extenders to add more items in targets, should they need to. I expect most additions to BuildAccelerationIncompatiblePackage would be done via evaluation though.

Comment on lines +3 to +14
<Rule Name="BuildAccelerationIncompatiblePackage"
xmlns="http://schemas.microsoft.com/build/2009/properties">

<Rule.DataSource>
<DataSource HasConfigurationCondition="False"
MSBuildTarget="CollectBuildAccelerationIncompatiblePackageDesignTime"
Persistence="ProjectFile"
SourceOfDefaultValue="AfterContext"
SourceType="TargetResults" />
</Rule.DataSource>

</Rule>
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need any properties for these items — just the item spec.

private bool? _isBuildAccelerationEnabled;
private bool? _isBuildAccelerationEnabledInProject;
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed this, which increased the diff size here unfortunately. But the field no longer specified whether BA is enabled, only whether the AccelerateBuildsInVisualStudio property is present and with what value. We have added other ways of controlling whether BA is enabled (feature flag and incompatible package references) so the old name became misleading.

@@ -39,7 +39,8 @@ public sealed class BuildUpToDateCheckTests : BuildUpToDateCheckTestBase, IDispo
private bool _isTaskQueueEmpty = true;
private bool _isFastUpToDateCheckEnabledInSettings = true;
private bool _isBuildAccelerationEnabledInSettings;
private bool? _isBuildAccelerationEnabled;
private bool? _isBuildAccelerationEnabledInProject;
private bool? _expectedIsBuildAccelerationEnabled;
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this new field. Where we used to use _isBuildAccelerationEnabled to indicate whether we expected BA to ultimately be considered as enabled, we now need to take into account other inputs (as mentioned in my prior comment). This makes that explicit on the expectation side.

@kvenkatrajan kvenkatrajan merged commit 0492aff into dotnet:main Aug 2, 2023
5 checks passed
@drewnoakes drewnoakes deleted the build-acceleration-incompatible-packages branch August 3, 2023 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Build-Acceleration Build Acceleration can skip calls to MSBuild within Visual Studio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants