Avoid duplicate 'WindowsAppRuntimeAutoInitializer.cpp' compilation

[mschofie]

Fixes #5395.

build/NuSpecs/WindowsAppSDK-Nuget-Native.AutoInitializer.targets attempts to add a ClCompile Item during the WindowsAppRuntimeAutoInitializer Target. In doing so it attempts to use the existing metadata by uttering %(PreprocessorDefinitions). This syntax is fine for ItemGroups outside of a Target, but within a Target, it causes the (implicit) task that creates the Item to be batched (documentation) - causing an Item with the Identity of WindowsAppRuntimeAutoInitializer.cpp to be added for every unique set of ClCompile.PreprocessorDefinitions. The duplicate ClCompile Items cause a cascade of subsequent errors.

The fix is to add the ClCompile in two steps:

1. Create the ClCompile without specifying any metadata.
2. Modify the metadata for the ClCompile, using a condition for '%(Identity)' == '$(WindowsAppRuntimeAutoInitializerPath)' to force a single task batch (for the Item we're looking for).

Thanks to @chwarr for help with this.

[chwarr]

LGTM

[Scottj1s]

@mschofie thanks for taking a stab at this. I don't think it's a complete fix, since we had to scrap a couple earlier attempts (#5685, #5782).

My concern is that any batching at all (whether PreprocessorDefinitions, AdditionalOptions, etc) just sets a trap for subsequent dynamic source additions.

Can we avoid batching entirely by applying macros to all sources after they've been dynamically added, and that supports Xaml generated sources, etc?
#5685 (comment)

[mschofie]

I don't think it's a complete fix, since we had to scrap a couple earlier attempts (#5685, #5782).

Thanks for linking to the earlier attempts. TBH, both of those attempts appear to have the same problem as the original bug, that my approach doesn't. For example, #5782 says:

Root Cause: When two targets in BeforeClCompileTargets both modify the same parameter (e.g., PreprocessorDefinitions), the second target to execute may cause duplicate entries to be added to the cl compile command due to parameter modification conflicts.

And that's not right - it's not that they're modifying the same parameter Item metadata, it's because they're creating multiple sets of Items when batched by %(PreprocessorDefinitions). Even if those fixes worked, they'd break if a consumer had ClCompile Items with different %(PreprocessorDefinitions). This PR uses the '%(Identity)' == '$(WindowsAppRuntimeAutoInitializerPath)' Condition to force a single Item batch.

Can we avoid batching entirely by applying macros to all sources after they've been dynamically added, and that supports Xaml generated sources, etc?
#5685 (comment)

I don't understand how that mitigates the batching. If there's two ClCompile Items with unique PreprocessorDefinitions metadata created during the evaluation phase, then you'd get two batches (each with one ClCompile Item in) if you were to access %(PreprocessorDefinitions) during an ItemGroup within a Target (without forcing a smaller batch first). Here's a trivial example that shows batching of 'static' Items:

<Project>
  <ItemGroup>
    <ClCompile Include="foo" PreprocessorDefinitions="FOO" />
    <ClCompile Include="bar" PreprocessorDefinitions="BAR" />
  </ItemGroup>

  <Target Name="Build">
    <Message Text="Batch: %(ClCompile.PreprocessorDefinitions) Items: %(ClCompile.Identity)" />
  </Target>
</Project>

If you're OK applying the preprocessor defines globally, then there's a much easier fix - simply decouple the ClCompile Item creation from setting the PreprocessorDefinitions metadata; i.e.

1. If any of the necessary properties are set; add the 'ClCompile' Item
2. Based on which are set, update the PreprocessorDefinitions in an ItemDefinitionGroup.

Property evaluation and ItemDefinitionGroup updates would have to happen outside of the 'Target', but I'm not sure that's a problem...? This is more 'order dependent' than this current PR, but we're in a .targets file, so I think it's OK. Thoughts? So, something like (I'm coding in an edit box; apologies for typos)

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <WindowsAppRuntimeAutoInitializerPath>$(MSBuildThisFileDirectory)..\..\include\WindowsAppRuntimeAutoInitializer.cpp</WindowsAppRuntimeAutoInitializerPath>
    <WindowsAppRuntimeAutoInitializerEnabled>false</WindowsAppRuntimeAutoInitializerEnabled>
    <WindowsAppRuntimeAutoInitializerEnabled Condition="'$(WindowsAppSdkBootstrapInitialize)'=='true'">true</WindowsAppRuntimeAutoInitializerEnabled>
    <WindowsAppRuntimeAutoInitializerEnabled Condition="'$(WindowsAppSdkDeploymentManagerInitialize)'=='true'">true</WindowsAppRuntimeAutoInitializerEnabled>
    <WindowsAppRuntimeAutoInitializerEnabled Condition="'$(WindowsAppSdkUndockedRegFreeWinRTInitialize)'=='true'">true</WindowsAppRuntimeAutoInitializerEnabled>
    <WindowsAppRuntimeAutoInitializerEnabled Condition="'$(WindowsAppSdkCompatibilityInitialize)'=='true'">true</WindowsAppRuntimeAutoInitializerEnabled>
  </PropertyGroup>

  <Target Name="WindowsAppRuntimeAutoInitializer" Condition=" '$(WindowsAppRuntimeAutoInitializerEnabled)'=='true' ">
    <ItemGroup>
      <ClCompile Include="$(WindowsAppRuntimeAutoInitializerPath)" />
    </ItemGroup>
  </Target>

  <ItemDefinitionGroup>
    <ClCompile>
      <PreprocessorDefinitions Condition="'$(WindowsAppSdkBootstrapInitialize)'=='true'">MICROSOFT_WINDOWSAPPSDK_AUTOINITIALIZE_BOOTSTRAP;%(PreprocessorDefinitions)</PreprocessorDefinitions>
      <PreprocessorDefinitions Condition="'$(WindowsAppSdkDeploymentManagerInitialize)'=='true'">MICROSOFT_WINDOWSAPPSDK_AUTOINITIALIZE_DEPLOYMENTMANAGER;%(PreprocessorDefinitions)</PreprocessorDefinitions>
      <PreprocessorDefinitions Condition="'$(WindowsAppSdkUndockedRegFreeWinRTInitialize)'=='true'">MICROSOFT_WINDOWSAPPSDK_AUTOINITIALIZE_UNDOCKEDREGFREEWINRT;%(PreprocessorDefinitions)</PreprocessorDefinitions>
      <PreprocessorDefinitions Condition="'$(WindowsAppSdkCompatibilityInitialize)'=='true'">MICROSOFT_WINDOWSAPPSDK_AUTOINITIALIZE_COMPATIBILITY;%(PreprocessorDefinitions)</PreprocessorDefinitions>
    </ClCompile>
  </ItemDefinitionGroup>
</Project>

[Scottj1s]

And that's not right - it's not that they're modifying the same parameter Item metadata,

Agreed - that's not a correct RCA

I don't understand how that mitigates the batching. If there's two ClCompile Items with unique PreprocessorDefinitions metadata

Right - I'm suggesting we eliminate batching via speciation of PreprocessorDefinitions (or any other %BatchingMetadata) and just apply metadata globally to all sources - pch, module, whatever. I had suggested doing that during execution after all sources were sure to have been ClCompile included. But I'm fine with doing it during evaluation phase, via ItemDefinitionGroup - that's very appealing.

[mschofie]

Sorry, I don't understand your suggestion. How would you add the PreprocessorDefines? You'd still need to utter %(PreprocessorDefines) in an ItemGroup in a Target, wouldn't you?

At the minute, the only two paths I see are:

1. Force a single item batch. What this PR does.
2. Use an ItemDefinitionGroup. Which becomes a little order dependent (i.e. the WindowsAppSdkBootstrapInitialize (and other) properties would have to be set before build/NuSpecs/WindowsAppSDK-Nuget-Native.AutoInitializer.targets was evaluated).

I'll update this PR with the ItemDefinitionGroup after I've buddy tested it, and we can see which approach folks like the best.

[Scottj1s]

You'd still need to utter %(PreprocessorDefines) in an ItemGroup in a Target, wouldn't you?

Yes, you would - but that's not the issue per se. Having different definitions of PreprocessorDefines is what causes the batching. If we apply the same value to all sources, we get no batching and consequently no duplicate source compilation.

<Target Name="AddFoo"  BeforeTargets="ClCompile" AfterTargets="$(BeforeClCompileTargets);$(ComputeCompileInputsTargets);MakeDirsForCl" > 
  <ItemGroup>
    <ClCompile Include="Foo.Cpp">
        <AnyMetadataExceptPreprocessorDefinitions>this_is_safe</>
    </ClCompile>
    <ClCompile>
        <!-- Now, add macros to all sources, immediately before ClCompile, so no batching concerns -->
        <PreprocessorDefinitions>%PreprocessorDefinitions);FOO_DEFINES</PreprocessorDefinitions>
    </ClCompile>
  </ItemGroup>
</Target> 

[mschofie]

Having different definitions of PreprocessorDefines is what causes the batching.

Indeed, and I don't think we can prevent different definitions of PreprocessorDefines, because customers can easily write valid vcxproj files with different definitions of PreprocessorDefines. A customer can write the following in their .vcxproj:

<Project>
<ItemGroup>
    <ClCompile Include="CustomerSource.cpp" />
    <ClCompile Include="MoreCustomerSource.cpp" PreprocessorDefines="%(PreprocessorDefines);ENABLE_FANCY_MODE"/>
</ItemGroup>
</Project>

And then batching will occur in an ItemGroup (in a Target) that uses %(PreprocessorDefines).

[Scottj1s]

Having different definitions of PreprocessorDefines is what causes the batching.

Indeed, and I don't think we can prevent different definitions of PreprocessorDefines, because customers can easily write valid vcxproj files with different definitions of PreprocessorDefines. A customer can write the following in their .vcxproj:

<Project>
<ItemGroup>
    <ClCompile Include="CustomerSource.cpp" />
    <ClCompile Include="MoreCustomerSource.cpp" PreprocessorDefines="%(PreprocessorDefines);ENABLE_FANCY_MODE"/>
</ItemGroup>
</Project>

And then batching will occur in an ItemGroup (in a Target) that uses %(PreprocessorDefines).

That's true - we can't avoid that. But we can at least avoid self-inflicted issues like this one, by applying PreprocessorDefinitions globally. The interesting thing about this issue is that it's the result of our two targets both including ClCompile items with specific PreprocessorDefinitions: AddUndockedRegFreeWinRTCppDefines and WindowsAppRuntimeAutoInitializer. It's actually the second target that loses, by including its source file into separate batches that the first target has already created. So I'm fine with any solution that avoids our own (re)batching of existing batches.

[mschofie]

I've updated this PR with an implementation that takes the ItemDefinitionGroup-approach. This is - IMHO - a tidier approach. The only downside is that if customers are setting any of

• WindowsAppSdkBootstrapInitialize
• WindowsAppSdkDeploymentManagerInitialize
• WindowsAppSdkUndockedRegFreeWinRTInitialize
• WindowsAppSdkCompatibilityInitialize

within a 'Target', then it won't be honored, they need to be set during evaluation. This seems like a reasonable expectation to me.

Thoughts?

[chwarr]

If we're going to rely on consuming projects setting these properties before importing the .targets file, this inclusion can be an evaluation-time conditional inclusion; no target needed.

[mschofie]

IIUC, the point of adding the 'ClCompile' in the Target is to 'hide it' from Solution Explorer. If the ClCompile is added outside of a Target (in a 'top level' ItemGroup) it shows in Solution Explorer.

[chwarr]

Looks like <Visible>false</Visible> metadata doesn't work anymore for cases like this. A target seems fine then.

[chwarr]

Minor: consider putting the IDG after the PropertyGroup and before the targets so that a top-to-bottom read more closely matches the evaluation order.

[chwarr]

Suggested change

	<WindowsAppRuntimeAutoInitializerEnabled>false</WindowsAppRuntimeAutoInitializerEnabled>
	<WindowsAppRuntimeAutoInitializerEnabled Condition=" '$(WindowsAppRuntimeAutoInitializerEnabled)' == '' ">false</WindowsAppRuntimeAutoInitializerEnabled>

In case a consuming project has already set this to try, a conditional assignment will avoid clobbering that value.

Consider moving the false assignment after all the other conditions so that it gets set to false as a last resort if not set to anything else first.

Up to you if you want to test against '' or != 'true'