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

[release/3.1] Use correct PresentationBuildTasks.dll for VS and MSBuild builds #2075

Merged
1 commit merged into from
Oct 22, 2019

Conversation

vatsan-madhavan
Copy link
Member

@vatsan-madhavan vatsan-madhavan commented Oct 22, 2019

Port of #1999
Fixes #1998

Description

The PresentationBuildTasks.dll built out of the .NET Core codebase is not being used for msbuild based builds (i.e., when $(MSBuildRuntimeType)==Full) of WPF projects that use WindowsDesktop SDK. Instead, the PresentationBuildTasks.dll from GAC, i.e., the DLL that shipped with .NET Framework, is being used for builds instead.

This is because the first occurance of an UsingTask element that applies to a TaskName will always be used - it can not be overridden by subsequent UsingTask entries (this is unlike Property and Item behavior). See note in https://github.com/MicrosoftDocs/visualstudio-docs/blob/master/docs/msbuild/usingtask-element-msbuild.md immediate after the Syntax section (Note: The msbuild team added this note after identifying this behavior as part of investiaging this PresentationBuildTasks.dll issue).

This fix ensures that the UsingTask declarations supplied by the WindowsDesktop SDK precede those supplied by .NET Framework's copy of Microsoft.WinFX.targets by introducing a new .props file - Microsoft.WinFX.props - and moving a small number of Property and UsingTask declartions into it. Since .props are imported before targets, the UsingTask declarations supplied by WindowsDesktop SDK will thus take precedence.

Note that Pbt.props is used only for building this repo - it doesn't ship.

Customer Impact

Multi-targeted builds targeting .NET Framework will be produced incorrectly under some conditions, specifically when building using VS. There may be no problems initially due to the strong backwards compatibility between .NET Core and .NET Framework WPF assemblies - but errors in XAML/markup compilation may lead to hard-to-identify/debug problems.

Regression

Not a regression. This was a missed corner case not caught until now.

Risk

  • The changes are scoped and safe.
  • They have been tested by building a corpus of WPF samples
  • The fix has been in .NET 5 for 2 weeks now without any adverse reports.

@ghost ghost requested a review from rladuca October 22, 2019 01:50
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Oct 22, 2019
@ghost ghost requested a review from SamBent October 22, 2019 01:50
@vatsan-madhavan vatsan-madhavan added this to the 3.1 milestone Oct 22, 2019
@vatsan-madhavan vatsan-madhavan self-assigned this Oct 22, 2019
@vatsan-madhavan vatsan-madhavan changed the title Use correct PresentationBuildTasks.dll for VS and MSBuild builds Use correct PresentationBuildTasks.dll for VS and MSBuild builds [release/3.1] Oct 22, 2019
@vatsan-madhavan vatsan-madhavan changed the title Use correct PresentationBuildTasks.dll for VS and MSBuild builds [release/3.1] [release/3.1] Use correct PresentationBuildTasks.dll for VS and MSBuild builds Oct 22, 2019
@vatsan-madhavan vatsan-madhavan added ask-mode * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) Bug Product bug (most likely) labels Oct 22, 2019
@vatsan-madhavan vatsan-madhavan removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 22, 2019
… not being used for msbuild based builds (i.e., when `$(MSBuildRuntimeType)==Full`) of WPF projects that use WindowsDesktop SDK. Instead, the PresentationBuildTasks.dll from GAC, i.e., the DLL that shipped with .NET Framework, is being used for builds instead.

This is because the *first* occurance of an `UsingTask` element that applies to a `TaskName` will always be used - it can not be overridden by subsequent `UsingTask` entries (this is unlike `Property` and `Item` behavior). See note in https://github.com/MicrosoftDocs/visualstudio-docs/blob/master/docs/msbuild/usingtask-element-msbuild.md immediate after the **Syntax** section (Note: The msbuild team added this note after identifying this behavior as part of investiaging this PresentationBuildTasks.dll issue).

The fix ensures that the `UsingTask` declarations supplied by the WindowsDesktop SDK precede those supplied by .NET Framework's copy of `Microsoft.WinFX.targets` by introducing a new `.props` file - `Microsoft.WinFX.props` - and moving a small number of `Property` and `UsingTask` declartions into it. Since `.props` are imported before `targets`, the `UsingTask` declarations supplied by WindowsDesktop SDK will thus take precedence.
@vatsan-madhavan vatsan-madhavan force-pushed the pbt-msbuild-fixup-rel31 branch from d6a347e to 8f14852 Compare October 22, 2019 18:58
@vatsan-madhavan vatsan-madhavan added the auto_merge bot-command label Oct 22, 2019
@ghost
Copy link

ghost commented Oct 22, 2019

Hello @vatsan-madhavan!

Because this pull request has the auto_merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ask-mode auto_merge bot-command Bug Product bug (most likely) PR metadata: Label to tag PRs, to facilitate with triage Servicing-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants