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] Unescape PresentationBuildTasks.dll paths containing special characters #2487

Merged

Conversation

vatsan-madhavan
Copy link
Member

@vatsan-madhavan vatsan-madhavan commented Jan 30, 2020

Fixes #2415

Description

.NET Core 3.1 SDK installed in paths containing special characters like '( fails to build WPF projects. This affects the default WOW64 installation - i.e., x86 MSI installation on x64 system, that typically installs under %SystemDrive%\Program Files (x86)\dotnet

The builds fail with the following error:

c:\Program Files (x86)\dotnet\sdk\3.1.101\Sdks\Microsoft.NET.Sdk.WindowsDesktop\targets\Microsoft.WinFX.targets(225,9): 
error MSB4062: The "Microsoft.Build.Tasks.Windows.MarkupCompilePass1" task could not be loaded from
the assembly c:\Program Files %28x86%29\dotnet\sdk\3.1.101\Sdks\Microsoft.NET.Sdk.WindowsDesktop\tools\netcoreapp2.1\PresentationBuildTasks.dll. 
Could not load file or assembly 'c:\Program Files %28x86%29\dotnet\sdk\3.1.101\Sdks\Microsoft.NET.Sdk.WindowsDesktop\tools\netcoreapp2.1\PresentationBuildTasks.dll'.
The system cannot find the path specified. Confirm that the <UsingTask> declaration is correct, that the assembly
and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. 

When the SDK is installed in a path containing special characters, for e.g., under C:\Program Files (x86)\dotnet (the special characters here being {'(', ')'}), calling an MSBuild property function like $([System.IO.Path]::GetFullPath()) on $(MSBuildThisFileDirectory) will result in MSBuild escaping any special characters that are returned.

Now, this is what the documentation says about using property functions:

String values returned from property functions have special characters escaped. If you want the value to be treated as though it was put directly in the project file, use $([MSBuild]::Unescape()) to unescape the special characters.
...
In static property functions, you can use any static method or property of these system classes:

System.Byte
...
System.IO.Path
...
Microsoft.Build.Utilities.ToolLocationHelper

When such an escaping happens, characters like ( get translated to%28 etc. When UsingTask encounters a value of $(_PresentationBuildTasksAssembly) containing encoded characters, it fails.

We must unescape these characters by calling $([MSBuild]::Unescape()) to ensure that UsingTask will not fail.

Customer Impact

Developers who install x86 SDK on x64 OS are not able to build WPF apps using 3.1.x SDK.

Regression

This was a regression introduced by #2075, [release/3.1] Use correct PresentationBuildTasks.dll for VS and MSBuild builds.

We did not catch this during development and testing early on since testing was done using SDK's installed under %appdata%, %temp% etc., which typically didn't have special characters that are escaped by MSBuild.

We are still trying to understand how we failed to catch this in end-to-end testing. The current theory is that the x86 SDK was likely tested directly on an x86 OS installation where this problem would not have manifested itself (since the default install location would have been %SystemDrive%\Program Files, and would not have involved WOW64).

We are taking steps to ensure that this scenario is covered in testing.

Risk

Low.

  • The change is being tested by building a corpus of WPF applications on different configurations (x86 SDK on x86 OS, x86 SDK on x64 OS, x64 SDK on x64 OS) x (simple installation path, installation paths with special chars).
  • The fix is simple and easy to review for correctness.

@ghost ghost requested a review from rladuca January 30, 2020 22:59
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jan 30, 2020
@ghost ghost requested a review from SamBent January 30, 2020 22:59
@vatsan-madhavan
Copy link
Member Author

/cc @richaverma1

@vatsan-madhavan vatsan-madhavan self-assigned this Jan 30, 2020
@vatsan-madhavan vatsan-madhavan added this to the 3.1.3 milestone Jan 30, 2020
@vatsan-madhavan
Copy link
Member Author

Wait for branch to open before merging.

@vatsan-madhavan vatsan-madhavan added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 7, 2020
When the SDK is installed in a path containing special characters, for e.g., under `C:\Program Files (x86)\dotnet` (the special characters here being `{'(', ')'}`), calling an MSBuild property function like `$([System.IO.Path]::GetFullPath())` on `$(MSBuildThisFileDirectory)` will result in MSBuild escaping any special characters that are returned.

Now, this is what the [documentation ](https://docs.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2019)says about using property functions:

>String values returned from property functions have[ special characters](https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2019) escaped. If you want the value to be treated as though it was put directly in the project file, use `$([MSBuild]::Unescape())` to unescape the special characters.
>...
>In static property functions, you can use any static method or property of these system classes:
> ```
> System.Byte
>...
> System.IO.Path
>...
> Microsoft.Build.Utilities.ToolLocationHelper
> ```

When such an escaping happens, characters like `(` get translated to`%26` etc. When `UsingTask` encounters a value of `$(PresentationBuildTasksAssembly)` containing encoded characters, it fails.

We must unescape these characters by calling `$([MSBuild]::Unescape())` to ensure that `UsingTask` will not fail.
@vatsan-madhavan vatsan-madhavan force-pushed the windowsdesktop-tasks-complexpath-fixup branch from fe2f46e to 9b2d40d Compare February 13, 2020 19:26
@vatsan-madhavan vatsan-madhavan added auto_merge bot-command and removed * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) labels Feb 13, 2020
@ghost
Copy link

ghost commented Feb 13, 2020

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.

@vatsan-madhavan
Copy link
Member Author

The branch is open for 3.1.3 and branding updates are merged (#2572). Removing NO MERGE and scheduling for auto-merge.

@ghost ghost merged commit abc56b3 into dotnet:release/3.1 Feb 13, 2020
@vatsan-madhavan vatsan-madhavan deleted the windowsdesktop-tasks-complexpath-fixup branch February 13, 2020 21:38
@ElDuderinoBerlin
Copy link

Uninstall the x86-SDK of NETCore 3.1 on Windows x64.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 13, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto_merge bot-command 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.

5 participants