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.

Sign up for GitHub

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

Jump to bottom

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

Open
Sergio0694 opened this issue Oct 4, 2024 · 6 comments · May be fixed by #44695
Open

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

Sergio0694 opened this issue Oct 4, 2024 · 6 comments · May be fixed by #44695
Assignees
@MiYanni
Labels
Area-NetSDK untriaged Request triage from a team member

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Oct 4, 2024 •
edited
Loading

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

image

Further technical details

  • Binlog (MSFT only)
  • VS 17.12 P3
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Oct 4, 2024
@marcpopMSFT
Copy link
Member

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

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

In general I think I prefer hooking BeforeCompile as @Sergio0694 proposes. (whoops!: #11635 (comment))

@Sergio0694
Copy link
Contributor Author

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

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

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 Sergio0694 linked a pull request Nov 6, 2024 that will close this issue
Open
@Sergio0694
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MiYanni MiYanni

Labels
Area-NetSDK untriaged Request triage from a team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Run 'AddImplicitDefineConstants' on 'BeforeCompile' Sergio0694/sdk
4 participants
@rainersigwald @Sergio0694 @marcpopMSFT @MiYanni