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

Potential incrementality issue in GenerateTargetFrameworkMonikerAttribute #9840

Open
KirillOsenkov opened this issue Mar 7, 2024 · 5 comments · May be fixed by #9925
Open

Potential incrementality issue in GenerateTargetFrameworkMonikerAttribute #9840

KirillOsenkov opened this issue Mar 7, 2024 · 5 comments · May be fixed by #9925
Assignees
Labels
Priority:2 Work that is important, but not critical for the release triaged

Comments

@KirillOsenkov
Copy link
Member

<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:
image

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)

@AR-May AR-May added Priority:2 Work that is important, but not critical for the release triaged labels Mar 12, 2024
@JaynieBai
Copy link
Member

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
image

<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
Copy link
Member

surayya-MS commented Apr 16, 2024

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:2 Work that is important, but not critical for the release triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants