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

[MSBuildReferences.projitems] Allow consumers to opt into Microsoft.Build package. #220

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Nov 17, 2023

Projects that import MSBuildReferences.projitems receive a <PackageReference> to Microsoft.Build. However, many consumers are netstandard2.0 and this package does not have that framework, resulting in the following warnings:

Warning NU1701 Package 'Microsoft.Build 17.3.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project.
Warning NU1701 Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project.	

Most consumers do not actually need this package, so we can make it optional to prevent the warnings and extra dependencies in those projects.

Initially, this is done by making the package reference opt-in, with:

<_IncludeMicrosoftBuildPackage>true</_IncludeMicrosoftBuildPackage>

However this will be a "breaking" change to an unknown amount of consumers. If we would like to avoid this, we can invert the change to allow projects to "opt-out" instead.

@jpobst jpobst marked this pull request as ready for review November 17, 2023 00:09
@@ -10,7 +10,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Build" Version="$(MSBuildPackageReferenceVersion)" />
<PackageReference Include="Microsoft.Build" Version="$(MSBuildPackageReferenceVersion)" Condition=" '$(_IncludeMicrosoftBuildPackage)' == 'true' " />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be applied to all Microsoft.Build.* PackageReferences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add different properties for each PackageReference if we have a use case for them. The use case we are currently looking at is we want to add a reference to all of the Microsoft.Build.* packages except this one.

@jonpryor
Copy link
Member

After some discussion, my preference is to not introduce $(_IncludeMicrosoftBuildPackage) and instead make the Microsoft.Build package reference conditional on $(TargetFramework) not being netstandard. This would likewise fix the NU1701 warnings, as Microsoft.Build would no longer be includerd in the builds of .NETStandard projects.

The one "exception" to this is Xamarin.Android.Build.Tasks.csproj, which still needs the project reference. We would just need to add an "explicit" <ProjectReference Include="Microsoft.Build" /> to Xamarin.Android.Build.Tasks.csproj, which will also emit the NU1701 warning, but that's unavoidable.

@jpobst
Copy link
Contributor Author

jpobst commented Nov 29, 2023

After some discussion, my preference is to not introduce $(_IncludeMicrosoftBuildPackage) and instead make the Microsoft.Build package reference conditional on $(TargetFramework) not being netstandard.

After thinking some more about this, I think the original $(_IncludeMicrosoftBuildPackage) idea makes more sense. It allows us to key this <PackageReference> on whether the consuming project needs the reference or not, rather than the consuming project's target framework.

The property allows us cleaner understanding of the intent rather than a hidden side-effect of target framework.

It's also cleaner to add the package to a project targeting both netstandard and something else:
https://github.com/xamarin/xamarin-android-tools/blob/08a69900df23e9f95057f3cacae72bd40d640a27/src/Microsoft.Android.Build.BaseTasks/Microsoft.Android.Build.BaseTasks.csproj#L6

@jpobst jpobst marked this pull request as draft November 29, 2023 19:14
@jpobst
Copy link
Contributor Author

jpobst commented Nov 29, 2023

Test XA bump: dotnet/android#8545

Fixes 226 CI reported warnings (390 -> 164).

@jpobst jpobst marked this pull request as ready for review November 29, 2023 23:44
@jonpryor jonpryor merged commit 4889bf0 into main Nov 30, 2023
4 checks passed
@jonpryor jonpryor deleted the optional-microsoft-build branch November 30, 2023 16:13
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Dec 2, 2023
Changes: dotnet/android-tools@8d38281...4889bf0

  * dotnet/android-tools@4889bf0: [MSBuildReferences.projitems] Require opt-in to use `Microsoft.Build` (dotnet/android-tools#220)
  * dotnet/android-tools@21de3d7: [build] update $(MSBuildPackageReferenceVersion) to 17.6.3 (dotnet/android-tools#221)
  * dotnet/android-tools@08a6990: Bump android-sdk NDK version to 26.1.10909125
  * dotnet/android-tools@6ae1f2a: Bump android-sdk build-tool version to 34.0.0
  * dotnet/android-tools@184b6b3: Bump android-sdk cmdline-tools to version 11.0
  * dotnet/android-tools@1365e33: Bump android-sdk platforms-tools to version 34.0.5

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
jpobst added a commit to dotnet/android that referenced this pull request Dec 5, 2023
Bump `xamarin-android-tools` from `bde49e6` to `4889bf0`.

With dotnet/android-tools#220, we now need to set `$(_IncludeMicrosoftBuildPackage)` for projects that need a `Microsoft.Build` reference.

This bump fixes 226 CI reported warnings (390 -> 164).
jpobst added a commit to dotnet/android that referenced this pull request Dec 6, 2023
Context: dotnet/android-tools#220
Context: #8545

We added the ability for consumers of MSBuild NuGet packages to not add the `Microsoft.Build` package to their project.  One project that currently still requires `Microsoft.Build` is `Xamarin.Android.BootstrapTasks`.  

However the only thing that uses `Microsoft.Build` is `TimingLogger` which produced a build performance report for our old Jenkins build system.  As we no longer use Jenkins or this code, removing it allows us to remove the `Microsoft.Build` dependency.

Removes warnings:

```
C:\a\_work\1\s\build-tools\Xamarin.Android.Tools.BootstrapTasks\Xamarin.Android.Tools.BootstrapTasks.csproj : warning NU1701: Package 'Microsoft.Build 17.8.3' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project. [C:\a\_work\1\s\Xamarin.Android.BootstrapTasks.sln]
C:\a\_work\1\s\build-tools\Xamarin.Android.Tools.BootstrapTasks\Xamarin.Android.Tools.BootstrapTasks.csproj : warning NU1701: Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project. [C:\a\_work\1\s\Xamarin.Android.BootstrapTasks.sln]
```

This reduces the number of CI reported warnings from 166 to 56.
pjcollins pushed a commit that referenced this pull request Oct 23, 2024
…#220)

Projects that import `MSBuildReferences.projitems` receive a
`@(PackageReference)` to the `Microsoft.Build` NuGet package.
However, many consumers have `$(TargetFramework)`=netstandard2.0, and
the `Microsoft.Build` package does not have support netstandard2.0,
resulting in the following warnings:

	Warning NU1701 Package 'Microsoft.Build 17.3.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project.
	Warning NU1701 Package 'Microsoft.IO.Redist 6.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project.	

Most consumers do not actually need this package, so we can make it
opt-in to prevent the warnings and extra dependencies in those
projects.  Packages opt-in to referencing the `Microsoft.Build`
package by setting the `$(_IncludeMicrosoftBuildPackage)` MSBuild
property to true:

	<_IncludeMicrosoftBuildPackage>true</_IncludeMicrosoftBuildPackage>

Note: this will be a "breaking" change to an unknown amount of consumers.
We're crossing our fingers that this won't break too much.

Enabling this change fixes 226 CI warnings in xamarin-android.
pjcollins added a commit to dotnet/android that referenced this pull request Dec 4, 2024
With dotnet/android-tools#220, we now need to
set `$(_IncludeMicrosoftBuildPackage)` for projects that need a
`Microsoft.Build` reference.

Changes: dotnet/android-tools@d50747c...f53106a

  * dotnet/android-tools@f53106a: Bump to dotnet/msbuild@10fbfbf2
  * dotnet/android-tools@a2e146d: Bump to dotnet/msbuild@33de0b22
  * dotnet/android-tools@1818d59: Bump LibZipSharp and Xamarin.Build.AsyncTask
  * dotnet/android-tools@75f79e7: Bump LibZipSharp to 3.1.1
  * dotnet/android-tools@b1f795e: [MSBuildReferences.projitems] Require opt-in to use `Microsoft.Build`

Changes: xamarin/monodroid@b753d75...99627fe

  * xamarin/monodroid@99627fe: [release/8.0.4xx] Bump android-sdk-installer, androidtools
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