'AddImplicitDefineConstants' runs too late, and defines are missing when 'XamlPreCompile' runs, causing build failures

[Sergio0694]

Describe the bug

WinUI 3 and UWP apps (perhaps WPF/MAUI ones too?) use the XamlPreCompile task defined in the .NET SDK, which creates the intermediate .dll necessary for XAML compilation. The AddImplicitDefineConstants target runs before 'CoreCompile', but this seems to be too late and after 'XamlPreCompile' has already run. This causes the intermediate XAML build to not see the _OR_GREATER defines, meaning it will fail when those are used to exclude incompatible code, and resulting in very weird errors for someone looking at the IDE (which will show the code as being completely fine).

I hit this while migrating the Microsoft Store to .NET 9.

Can we make this target use BeforeTargets="BeforeCompile" or something, to address this?

To Reproduce

Create a WinUI 3 or UWP .NET 9 app with the following code behind:

#if !NET8_0_OR_GREATER
#error GenerateNETCompatibleDefineConstants didn't run 
#endif

Expected result

Should build as expected and match the behavior in the IDE.

Actual result

Further technical details

• Binlog (MSFT only)
• VS 17.12 P3

[marcpopMSFT]

@rainersigwald Would it be less risky to have XamlPreCompile add DependsOnTarget="AddImplicitDefineConstants"? I'm not sure the side effects in msbuild ordering of doing that. Both Targets are set to runtime BeforeTargets CoreCompile today and I want to ensure they run in the right order.

[rainersigwald]

Hmm. XamlPreCompile already DependsOn $(CoreCompileDependsOn) -- should AddImplicitDefineConstants be part of that list?

In general I think I prefer hooking BeforeCompile as @Sergio0694 proposes. (whoops!: https://github.com/dotnet/sdk/pull/11635#discussion_r428620254)

[Sergio0694]

Just for my own understanding, what's the difference in practice between:

• Adding AddImplicitDefineConstants to the DependsOnTargets set for XamlPreCompile
• Changing BeforeTargets for AddImplicitDefineConstants to BeforeCompile

I'm not too familiar with all the specific nuance there to have a strong preference here. Whichever you think is best 😅

cc. @manodasanW @jlaanstra thoughts?

[Sergio0694]

Hey @rainersigwald! Just circling back on this, should I go ahead and make a PR applying your suggestion then? 🙂

"In general I think I prefer hooking BeforeCompile"

I mean this.

[rainersigwald]

I think that makes sense.

Just for my own understanding, what's the difference in practice between:

• Adding AddImplicitDefineConstants to the DependsOnTargets set for XamlPreCompile
• Changing BeforeTargets for AddImplicitDefineConstants to BeforeCompile

I'm not too familiar with all the specific nuance there to have a strong preference here. Whichever you think is best 😅

It's very complicated and there's not usually a really clear answer. Most of the core targets use DependsOnTargets because that was the only option available in MSBuild v1. It has some advantages: implicit ordering in the expression (why, for example, BeforeBuild comes before CoreBuild comes before AfterBuild: they're defined that way in BuildDependsOn), ignorability, every doc knows this. And it has some disadvantages: ignorability, accidental overwrite-instead-of-append, and the biggie requires the target to have a DependsOn with a property which keeps it from being useful for many extension cases.

BeforeTargets is defined on the target being injected and can thus hook any target. It doesn't define order (if you hook two targets to the same target good luck figuring out which goes first) and it's also not ignorable.

In general I prefer BeforeTargets/AfterTargets hooks unless there's a strong reason to prefer something else. I don't think that's the case here so I default to BeforeTargets="BeforeCompile".

[Sergio0694]

Thank you so much for the additional context! I opened #44695 with the suggested change 🙂

[jlaanstra]

I think that makes sense.

Just for my own understanding, what's the difference in practice between:

• Adding AddImplicitDefineConstants to the DependsOnTargets set for XamlPreCompile
• Changing BeforeTargets for AddImplicitDefineConstants to BeforeCompile

I'm not too familiar with all the specific nuance there to have a strong preference here. Whichever you think is best 😅

It's very complicated and there's not usually a really clear answer. Most of the core targets use DependsOnTargets because that was the only option available in MSBuild v1. It has some advantages: implicit ordering in the expression (why, for example, BeforeBuild comes before CoreBuild comes before AfterBuild: they're defined that way in BuildDependsOn), ignorability, every doc knows this. And it has some disadvantages: ignorability, accidental overwrite-instead-of-append, and the biggie requires the target to have a DependsOn with a property which keeps it from being useful for many extension cases.

BeforeTargets is defined on the target being injected and can thus hook any target. It doesn't define order (if you hook two targets to the same target good luck figuring out which goes first) and it's also not ignorable.

In general I prefer BeforeTargets/AfterTargets hooks unless there's a strong reason to prefer something else. I don't think that's the case here so I default to BeforeTargets="BeforeCompile".

@rainersigwald @Sergio0694 I don't see how BeforeCompile is guaranteed to run before XamlPreCompile target, so I don't understand how this fix works?

XamlPreCompile is injected into PrepareResourcesDependsOn and so runs before PrepareResources. BeforeCompile is part of CompileDependsOn and so runs before Compile. However in CoreBuildDependsOn, PrepareResources is sequenced before Compile, which suggests that BeforeCompile would still run after XamlPreCompile and I have binlogs that suggest that's indeed happening :).

[rainersigwald]

That analysis makes sense to me. We can switch to AfterTargets="PrepareForBuild" (after to give other customizations a place to change things before it), or add the DependsOnTargets; I lean to the former but am fine with the latter given the history here.