Potential incrementality issue in GenerateTargetFrameworkMonikerAttribute

[KirillOsenkov]

msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets

Lines 3667 to 3689 in 9af8ff2

	<Target Name="GenerateTargetFrameworkMonikerAttribute"
	BeforeTargets="BeforeCompile"
	DependsOnTargets="PrepareForBuild;GetReferenceAssemblyPaths"
	Inputs="$(MSBuildToolsPath)\Microsoft.Common.targets"
	Outputs="$(TargetFrameworkMonikerAssemblyAttributesPath)"
	Condition="'@(Compile)' != '' and '$(GenerateTargetFrameworkAttribute)' == 'true'">
	<!-- This is a file shared between projects so we have to take care to handle simultaneous writes (by ContinueOnError)
	and a race between clean from one project and build from another (by not adding to FilesWritten so it doesn't clean) -->
	<WriteLinesToFile
	File="$(TargetFrameworkMonikerAssemblyAttributesPath)"
	Lines="$(TargetFrameworkMonikerAssemblyAttributeText)"
	Overwrite="true"
	ContinueOnError="true"
	Condition="'@(Compile)' != '' and '$(TargetFrameworkMonikerAssemblyAttributeText)' != ''"
	/>
	<ItemGroup Condition="'@(Compile)' != '' and '$(TargetFrameworkMonikerAssemblyAttributeText)' != ''">
	<Compile Include="$(TargetFrameworkMonikerAssemblyAttributesPath)"/>
	<!-- Do not put in FileWrites: this is a file shared between projects in %temp%, and cleaning it would create a race between projects during rebuild -->
	</ItemGroup>
	</Target>

I think the problem is that the condition on the target doesn't match the condition on the WriteLinesToFile task. I think we should add and '$(TargetFrameworkMonikerAssemblyAttributeText)' != '' to the Condition on the Target.

Otherwise we get this:

The target runs (because the output file doesn't exist), but then the tasks are skipped because the text is empty, and so it doesn't write the file again.

It's benign, but would be nice to get it out of the way for build incrementality investigations (the less targets run in an incremental build the easier it is to see targets which are breaking incrementality and shouldn't be running)

[JaynieBai]

After added and '$(TargetFrameworkMonikerAssemblyAttributeText)' != '' to the Condition on the Target. The condition on the target is false all the time even though $(TargetFrameworkMonikerAssemblyAttributeText) is not empty. It seems target condition is evaluated in the Evalution time not the execution time.
Here is the binlog
msbuild9840.binlog.txt

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Target Name="FirstTarget" BeforeTargets="SecondTarget">
    <!-- Define a property inside the first target -->
    <PropertyGroup>
      <ConditionProperty>ConditionValue</ConditionProperty>
    </PropertyGroup>
    <Message Text="Executing FirstTarget" />
  </Target>  
  <Target Name="SecondTarget" Condition="'$(ConditionProperty)' == 'ConditionValue'" >
    <Message Text="Executing SecondTarget" />
  </Target>
</Project>

msbuild /t:SecondTarget doesn't output the secondtarget message.

[surayya-MS]

Thank you @JaynieBai !

I tried it out too and got the exact same result.

This is because $(TargetFrameworkMonikerAssemblyAttributeText) property is set inside another target _SetTargetFrameworkMonikerAttribute
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.VisualBasic.CurrentVersion.targets#L332
https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.CSharp.CurrentVersion.targets#L331

If we add and '$(TargetFrameworkMonikerAssemblyAttributeText)' != '' to the condition on GenerateTargetFrameworkMonikerAttribute target, then the condition is always false and target is never run (see Jenny's example).

Another solution could be to remove _SetTargetFrameworkMonikerAttribute and instead set the property outside of any target, then we can add and '$(TargetFrameworkMonikerAssemblyAttributeText)' != '' to the Condition on GenerateTargetFrameworkMonikerAttribute target. @KirillOsenkov what do you think?

[rainersigwald]

It seems target condition is evaluated in the Evalution time not the execution time.

This is not quite right--it's more complicated :( The docs at https://learn.microsoft.com/en-us/visualstudio/msbuild/target-build-order?view=vs-2022#determine-the-target-build-order are accurate, but you have to read them about 300 times before they sink in. I did, anyway . . .

What happens in your example is that the condition is evaluated between seeing the target for the first time (when the build starts, because you specified it as the entry-point target) and running the BeforeTargets, so the condition observes project state before FirstTarget runs.

[KirillOsenkov]

thanks Rainer! I wasn't looking forward to digging in deeper to understand this :)

I'm going to bow out if you excuse me :)

[rainersigwald]

@surayya-MS can you try:

1. Extract the current body of GenerateTargetFrameworkMonikerAttribute to a new target _WriteTargetFrameworkMonikerAttributeToFile (name arbitrary), including Inputs/Outputs
2. Make GenerateTargetFrameworkMonikerAttribute have DependsOnTargets="PrepareForBuild;GetReferenceAssemblyPaths;_WriteTargetFrameworkMonikerAttributeToFile"
3. Make _WriteTargetFrameworkMonikerAttributeToFile have Condition="'@(Compile)' != '' and '$(GenerateTargetFrameworkAttribute)' == 'true' and '$(TargetFrameworkMonikerAssemblyAttributeText)' != '' "

That should delay the evaluation of the condition to the point where it's already computed, so the target can be conditioned out rather than marked out of date.

[JaynieBai]

@surayya-MS can you try:

1. Extract the current body of GenerateTargetFrameworkMonikerAttribute to a new target _WriteTargetFrameworkMonikerAttributeToFile (name arbitrary), including Inputs/Outputs
2. Make GenerateTargetFrameworkMonikerAttribute have DependsOnTargets="PrepareForBuild;GetReferenceAssemblyPaths;_WriteTargetFrameworkMonikerAttributeToFile"
3. Make _WriteTargetFrameworkMonikerAttributeToFile have Condition="'@(Compile)' != '' and '$(GenerateTargetFrameworkAttribute)' == 'true' and '$(TargetFrameworkMonikerAssemblyAttributeText)' != '' "

That should delay the evaluation of the condition to the point where it's already computed, so the target can be conditioned out rather than marked out of date.

@rainersigwald I tested your suggestions in the PR https://github.com/dotnet/msbuild/pull/10934/files and the condition is still evaluated as false. Here is the log msbuild.binlog.txt
If I misunderstood anything, please let me know. Thank you.