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

SDK-style WPF Projects targeting net472 are broken because ImportFrameworkWinFXTargets is false #4515

Closed
KirillOsenkov opened this issue May 13, 2021 · 11 comments · Fixed by #4630
Assignees
Labels
Bug Product bug (most likely)
Milestone

Comments

@KirillOsenkov
Copy link
Member

This PR has regressed building WPF projects that use the SDK-style:
#2976

By default ImportFrameworkWinFXTargets was true, but now changes to false, resulting in MarkupCompile not running.

FYI @Nirmal4G

@KirillOsenkov
Copy link
Member Author

so just upgrading the SDK from 5 to 6 will break the build in a very confusing way

@Nirmal4G
Copy link
Contributor

Nirmal4G commented May 14, 2021

Even if it is set as false, the .NET SDK should reset it to true based on the TargetFramework. Let me take a look…

@KirillOsenkov
Copy link
Member Author

It wasn't set to anything in my codebase, and setting it to true in my Directory.Build.props fixed it.

Just create a new SDK-style .csproj, target net472 only, add some WPF references (PresentationCore, PresentationFramework, System.Xaml and WindowsBase), create a new Application and Window in Main() and build.

@KirillOsenkov
Copy link
Member Author

This is the line that sets it to false (whereas otherwise it is true for me):
https://github.com/dotnet/wpf/pull/2976/files#diff-5827fafdcece80f275068609904789952b21a8e75ae7b3fcb6b0fd39b3d0d0b5R171

@vatsan-madhavan
Copy link
Member

IIRC WPF SDK was using the SDK’s WinFx props/Targets (it has a props too), PresentationBuildTasks etc even when building NetFx targets. That has been the behavior since sometime in 3.x. NetFx targets do not use NetFx’s copy of targets or tasks.

Not sure if the regression implicates this or not but figured I’d put this out there to help with the investigation.

See #2075

@Nirmal4G
Copy link
Contributor

In .NET SDK's BeforeCommon.targets These Lines 49-56 reset it to true when targeting .NETFramework but doesn't do that! since, we set this in props in the Windows Desktop SDK. So, the solution here is, either remove the setting from the WindowsDesktop.props which may break projects or move the setting to Sdk.props and update the Main SDK logic to handle this case.

I recommend we move the setting to Sdk.props to preserve it's intended behavior—as it was meant be used only when using the Windows Desktop SDK directly. My PR was supposed to be merged before the v5 refactoring but was delayed. Meanwhile, I didn't know the logic was being updated in the Main SDK during that time.

I can go ahead and open a PR on what the team recommends.

@KirillOsenkov
Copy link
Member Author

No worries at all, it’s hard not to break MSBuild and we do it all the time. Welcome to the club! And thanks for looking into this.

@vatsan-madhavan
Copy link
Member

@dsplaisted fyi

@dsplaisted
Copy link
Member

Yeah, it made sense to set this to false in WindowsDesktop.props when that was only imported if the project was explicitly using Microsoft.NET.Sdk.WindowsDesktop. But now we always import that props file.

You could move it to the WindowsDesktop Sdk.props file, I think that would work in both situations.

@ryalanms ryalanms added Bug Product bug (most likely) and removed Requires WPF team triage Untriaged labels May 24, 2021
@ryalanms ryalanms added this to the 6.0.0 milestone May 24, 2021
@ryalanms ryalanms self-assigned this May 24, 2021
@ryalanms
Copy link
Member

ryalanms commented Jun 3, 2021

We need to fix this. @Nirmal4G: Are you going to create a PR or should someone from the WPF team get it? Thanks.

@Nirmal4G
Copy link
Contributor

Nirmal4G commented Jun 4, 2021

For now, Moving the logic to the SDK.props should work but a long-term fix should be using the SDK's WinFX targets instead of using the inbox targets and tasks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Product bug (most likely)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants