Support NuGet PackageReference properly in WPF temporary assembly #1786
Conversation
…ling .g.targets and .g.props to be properly included, despite the project file name change. We're copying GenerateTemporaryTargetAssembly class from PresentationBuildTasks.dll and creating a GenerateTemporaryTargetAssembly2 class. The temp proj file will no longer be: <randfilename>.tmp_proj it will now be: <randfilename>.<originalProjectExtension> And we'll set _OriginalProjectName msbuild property so that microsoft.common.props/targets can find the <origProjectName>.g.props/targets. There are a set of changes necessary with msbuild as well.
| /// ResolveAssemblyReference (RAR) again. | ||
| /// | ||
| /// </summary> | ||
| public sealed class GenerateTemporaryTargetAssembly2 : Task |
There was a problem hiding this comment.
This Task wasn't previously OSS, was it? Do we have the necessary approvals to make it so?
There was a problem hiding this comment.
Good point. Yes, will have to deal with license side of this.
| // | ||
|
|
||
| // GetRandomFileName( ) could return any possible file name and extension, but | ||
| // some file externsion has special meaning in MSBUILD system, such as a ".sln" |
There was a problem hiding this comment.
typo: externsion
(even if you copied it from src)
There was a problem hiding this comment.
Fixing in next iteration. Yes, was copied.
| // Add AssemblyName and IntermediateOutputPath to the global property list | ||
| globalProperties[intermediateOutputPathPropertyName] = IntermediateOutputPath; | ||
| globalProperties[assemblyNamePropertyName] = AssemblyName; | ||
| globalProperties[originalProjectNamePropertyName] = Path.GetFileNameWithoutExtension(CurrentProject); |
There was a problem hiding this comment.
I'm surprised to see these are all the global properties that are set. There could be any number of other global properties that were passed to the outer build that could result in this project experiencing a build break when they are dropped. If that's prior behavior of the original task, then I guess we can Won't Fix this as one of the Evils of the WPF inner-build design.
There was a problem hiding this comment.
globalProperties were used for intermediateOutputPathPropertyName, assemblyNamePropertyName. I added originalProjectNamePropertyName
Resolved won't fix due to evil design.
| try | ||
| { | ||
| // Delete the random project file from disk. | ||
| File.Delete(randProjPath); |
There was a problem hiding this comment.
wish list: based on an environment variable, skip the step of deleting the random project. so many times I've wished I could inspect and build this file myself when debugging why the inner build fails when the outer build would succeed.
There was a problem hiding this comment.
Adding code to see if an environment variable of the name "WPF_DoNotDeleteTemporaryProject" has a non-empty/non-null value.
| // | ||
| // Remove specific items from project file. Every item should be under an ItemGroup. | ||
| // | ||
| private void RemoveItemsByName(XmlDocument xmlProjectDoc, string sItemName) |
There was a problem hiding this comment.
There is a lot I'd like to critique about this method, but I'll forbear since I suspect you just copied it and don't want to change it.
There was a problem hiding this comment.
yes, copied this method.
| XmlDocument xmlProjectDoc = null; | ||
|
|
||
| xmlProjectDoc = new XmlDocument(); | ||
| xmlProjectDoc.Load(CurrentProject); |
There was a problem hiding this comment.
Why are we loading this as an XmlDocument instead of using the MSBuild construction model? If this is as it was and you're just copying it, then fine. Otherwise if it's new code, IMO we should fix this.
There was a problem hiding this comment.
Just copying the code with very minor changes.
* Enable WPF_DoNotDeleteTemporaryProject EnvVar switch to allow an advanced user to keep it around to help debug failures.
…/nuget.client into dev-rrelyea-wpftempprojfix
ghost
removed
the
|
just checking in on the status of this? |
|
Currently no plans to ship within NuGet. The fix will be shipped along-side MsBuild 15.6 and net472. |
|
That's extremely unfortunate that there's no way to get this out-of-band. |
Support NuGet PackageReference properly in WPF temporary assembly by enabling .g.targets and .g.props to be properly included, despite the project file name change.
(Right now, just shopping the approach around, and looking for feedback. Need to coordinate with WPF team as well.)
Bug
Fixes: NuGet/Home#5894
Regression: No
If Regression then when did it last work: Never w/ project.json or PackageReference
If Regression then how are we preventing it in future: n/a
Fix
Details:
We're copying GenerateTemporaryTargetAssembly class from PresentationBuildTasks.dll and creating a GenerateTemporaryTargetAssembly2 class.
The temp proj file will no longer be:
.tmp_proj
it will now be:
.
And we'll set _OriginalProjectName msbuild property so that microsoft.common.props/targets can find the .g.props/targets.
There are a set of changes necessary with msbuild as well. (will edit to add link once I create that PR)
Testing/Validation
Tests Added: No
Reason for not adding tests: Will do before final PR
Validation done: made frankenbuild, and tried Nerdbank.GitVersioning