-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support NuGet PackageReference properly in WPF temporary assembly #2701
Conversation
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.) Fixes: http://github.com/NuGet/Home/issues/5894 These changes react to _OriginalProjectName that is set by the new version of the temp assembly generator code. (see nuget change)
Just to clarify, is this for 15.5 or 15.6? |
|
||
<Message Text="MSBuildProjectFile is $(MSBuildProjectFile)" Condition="'$(MSBuildTargetsVerbose)' == 'true'" /> | ||
|
||
<GenerateTemporaryTargetAssembly2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we consume this Task in MSBuild but we don't define it in MSBuild? Isn't assuming that the nuget .targets defining this task a bit hazardous?
In particular, I think this sort of pattern bit us recently in the VS repo (Razzle) where we have the msbuild shipping .targets but not the nuget targets. It ended up breaking language services within VS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Will dig into this.
</GenerateTemporaryTargetAssembly2> | ||
|
||
<CreateItem Include="$(IntermediateOutputPath)$(TargetFileName)" > | ||
<Output TaskParameter="Include" ItemName="AssemblyForLocalTypeReference" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a fancy way to write:
<AssemblyForLocalTypeReference Include="$(IntermediateOutputPath)$(TargetFileName)" />
If so, why not use the simple syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal was to copy winfx work identically, so that we have identical behavior ... with very low risk -- except for what we need to change.
@AndyGerlicher - targeting 15.6, not 15.5. |
Is there any updates on this? |
@rrelyea we have been looking at older PRs on the MSBuild repo. Is this PR something that you still plan on working on? It seems like there are some left over work here that we should address before we take this. |
Team triage: closing. Let us know if you're interested in picking this up again. |
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.)
Fixes: http://github.com/NuGet/Home/issues/5894
These changes react to _OriginalProjectName that is set by the new version of the temp assembly generator code. (see nuget change)