Correct build transitive targets location

[RussKie]

Resolves #4772

Microsoft Reviewers: Open in CodeFlow

[joperezr]

While this change does fix the issue, I think that the root cause that should be fixed is the logic that is adding the _._ placeholders to the package. This was originally intended to be used in order to be able to emit an error when users consumed the packages targeting unsupported TFMs (like netcoreapp3.1, and net5.0) but that is no longer required since the support for those TFMs was dropped from the package in the 8.0 timeframe when we stopped cross-compiling for them. At that time, we needed a place holder file on the minimum supported TFM so that the error wouldn't be printed for net6.0+.

That is what was causing this issue, since that minimum supported TFM is now net8.0, meaning in net8.0 we are emitting the placeholder, causing our net6.0 props and targets to no longer get imported. My suggestion is to disable the logic putting the placeholders all together for Core, and only keeping it for net462 which is the only place where we want it. That way, we will solve this issue, but we would also make it so that any future packages that want to add their own props and targets won't come into this pitfall. Does that make sense?

[joperezr]

We should do more cleanup in general for that target as it was designed for a very different purpose than what it is doing now, but essentially I'm suggesting that perhaps just deleting

extensions/eng/MSBuild/Packaging.targets

Lines 67 to 75 in 3b57f78

	<!-- Add the placeholder file to the supported target framework buildTransitive folder, if it's empty.
	Because this target uses Item batching, it will run more than once so adding the placeholder to None which would cause a warning.
	In order to workaround it, we know this item will contain duplicates, so we will use Distinct() and add it in a target that runs
	right after this. -->
	<NoneWithDuplicates Include="$(PlaceholderFile)"
	PackagePath="$(_NETStandardCompatErrorPlaceholderFilePackagePath)\"
	Pack="true"
	Condition="'@(_PackageBuildFile)' == '' or
	!@(_PackageBuildFile->AnyHaveMetadataValue('PackagePathWithoutFilename', '$(_NETStandardCompatErrorPlaceholderFilePackagePath)'))" />

And

extensions/eng/MSBuild/Packaging.targets

Lines 79 to 85 in 3b57f78

	<!-- Add the de-duped placeholder file to the package. -->
	<Target Name="_AddPlaceholderFileToNone"
	AfterTargets="_AddNETStandardCompatErrorFileForPackaging">
	<ItemGroup>
	<None Include="@(NoneWithDuplicates->Distinct())" />
	</ItemGroup>
	</Target>

will also fix this issue, and it will also be a step closer to the right direction we want. It would also future proof so that we don't have to go through the same issue each year when we change the minimum supported TFM.

[joperezr]

However, for .NET Framework we only provide a single file, which doesn't remove the analyzers:

It does, it's just not in the package :). Setting that property DisableMicrosoftExtensionsLoggingSourceGenerator to true will remove the generator whenever an application got it via the Logging.Abstractions NuGet package, which for net462, that is always the case. For .NET Core though, you could also get the generator via the aspnetcore ref pack, which would then not have the target specified to disable it which is why for net6.0 (or net8.0 after your change) we have a target that does this.

[RussKie]

We should do more cleanup in general for that target as it was designed for a very different purpose than what it is doing now, but essentially I'm suggesting that perhaps just deleting

extensions/eng/MSBuild/Packaging.targets

Lines 67 to 75 in 3b57f78

	<!-- Add the placeholder file to the supported target framework buildTransitive folder, if it's empty.
	Because this target uses Item batching, it will run more than once so adding the placeholder to None which would cause a warning.
	In order to workaround it, we know this item will contain duplicates, so we will use Distinct() and add it in a target that runs
	right after this. -->
	<NoneWithDuplicates Include="$(PlaceholderFile)"
	PackagePath="$(_NETStandardCompatErrorPlaceholderFilePackagePath)\"
	Pack="true"
	Condition="'@(_PackageBuildFile)' == '' or
	!@(_PackageBuildFile->AnyHaveMetadataValue('PackagePathWithoutFilename', '$(_NETStandardCompatErrorPlaceholderFilePackagePath)'))" />

And

extensions/eng/MSBuild/Packaging.targets

Lines 79 to 85 in 3b57f78

	<!-- Add the de-duped placeholder file to the package. -->
	<Target Name="_AddPlaceholderFileToNone"
	AfterTargets="_AddNETStandardCompatErrorFileForPackaging">
	<ItemGroup>
	<None Include="@(NoneWithDuplicates->Distinct())" />
	</ItemGroup>
	</Target>

will also fix this issue, and it will also be a step closer to the right direction we want. It would also future proof so that we don't have to go through the same issue each year when we change the minimum supported TFM.

I had a look but I don't think we can remove this infra without making substantial manual work it automates. It essentially generates the "not supported" warnings for all packages that we ship. If we remove this infra we'll have to create all these files manually, and that'd be a significant overhead.

I added some comments that would hopefully help removing the confusion I had. The newly added target we'll alert us when we bump the $(MinimumSupportedTfmForPackaging), which will be once in two years event.

[RussKie]

I'm merging this to unblock the build of the dotnet/extensions-samples repo

[joperezr]

That's fine, can we still log an issue to track the cleanup of that infrastructure then?

[xakep139]

That's fine, can we still log an issue to track the cleanup of that infrastructure then?

Filed #4834. Feel free to update the description there