Update Microsoft.NET.Compatibility.Common.targets

[ViktorHofer]

Fixes #24943

Make sure that the BuildProjectReferences property is set to false when the NoBuild=true property is set, to avoid the ResolveReferences target to build project references when it shouldn't.

Before this change, a non cross-targeting build avoided to use an MSBuild task to gather the project's references but as NuGet's Pack target itself already doesn't differentiate between if it's running as part of a cross targeting or a non-cross targeting build and always calls into an MSBuild task, this optimization isn't necessary in package validation. Instead, this change brings package validation in sync with how NuGet's Pack target invokes inner build projects to make sure that additional project evaluations are avoided.

As an alternative, an additional target could have been added that sets BuildProjectReferences to false like GenAPI is doing but it feels wrong to me to diverge in behavior between NuGet and package validation as the latter already depends on the former in execution.

[ViktorHofer]

/azp run dotnet-sdk-public-ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ericstj]

Previously we used _ComputeTargetFrameworkItems which, though private, has a lot of other dependencies in this repo. Why did we need to change to _GetTargetFrameworksOutput and is the NuGet team OK with that?

[ViktorHofer]

Even though we depend on a NuGet internal here, we should be able to catch any change to it by adding a test that utilizes this code path. NuGet directly inserts into dotnet/sdk so that would be caught as part of the insertion. Of course we want to keep the noise low but I wouldn't expect this to change often.

Why did we need to change to _GetTargetFrameworksOutput

As discussed offline, we intentionally want to walk the same code path as NuGet does during packaging to keep our logic in sync. The change that requires this target is the MSBuild task that now batches on the TargetFrameworks property.

and is the NuGet team OK with that?

@dotnet/nuget-team are you ok with us relying on this private _GetTargetFrameworksOutput target?

[nkolev92]

This functionality ships with every new SDK version right? There's a world where someone installs a higher version of the nuget.build.tasks.pack package that has tweaked that target and it might break you.

We haven't changed that target in 5 years, so I don't expect a lot of movement there either.
The standard "internal" target disclosure applies, but it should be reasonably easy react to any changes.

[joperezr]

This functionality ships with every new SDK version right?

Yes and no. This does ship inside the SDK, but it also ships as a standalone package that someone can reference in order to pick a different version of this functionality (similar to how the Roslyn compiler ships in the SDK but you can also opt to use a different version). I'm assuming that fact doesn't change much your stance given that as you point out, this target hasn't been changed in 5 years?

[ViktorHofer]

IMO we don't want to advertise referencing a new version of the Compatibility tooling via a Package Reference. Roslyn or the linker both ship toolset packages and don't advertise them as they are only meant for internal consumption of our core stack repositories.

[joperezr]

I wasn't suggesting advertising them. My point was more that because we have that shipping vehicle, it does mean that we could run into additional cases of SDK and PackageValidation coming from two different builds/branches/releases, so I was wondering if that might aggravate @nkolev92 concern above since he specifically asked if the shipping vehicle for this was only the SDK.

[ViktorHofer]

What I find interesting is that the Microsoft.DotNet.Compatibility package ships publicly. I thought we wanted to treat it as a transport only package but it shipped/ships to nuget.org. We could change that if we would like to.

[ViktorHofer]

It sounds like @nkolev92 is fine that we depend on NuGet's internal target, especially as it didn't change much and as we can react to any change. The fact that the Compatibility package could be referenced manually stands but I'm willing to take the risk that customers could be broken if NuGet changes its implementation, given that referencing the package and not using the feature as part of the SDK isn't a supported scenario (beyond experimentation).

FWIW I could also see a world where the sdk/msbuild would expose their current internal target that NuGet and others could then depend on to get the required inner build information.

[nkolev92]

, given that referencing the package and not using the feature as part of the SDK isn't a supported scenario (beyond experimentation).

Make sure you document that :)

[ericstj]

By making this an MSBuild call below will that result in an extra evaluation for a non-cross-targeting project?

[ViktorHofer]

I provided the reasoning for this change in the top post 🔝

[joperezr]

The changes look good to me as long as NuGet team is ok with us depending on this private target. Of course, ideally we would want to have them promote this to a non-private target so that we have a higher confidence of not getting randomly broken by this.