NuGet/Home

PackageReference broken in WPF projects due to tmp_proj not importing Package-supplied build authoring

AArnott opened this issue Β· 61 comments

I don't know how I lost track of it when reporting it here, but PackageReference is broken for WPF projects.

WPF has an inner build via a generated randomname.tmp_proj project file, where it compiles the source code among other things, I think as part of compiling the BAML. The problem is this random name that is assigned to this synthesized project file makes the imports for the generated ProjectName.csproj.nuget.g.props and .targets files not work because the $(MSBuildProjectName) macros doesn't evaluate to ProjectName, but to randomname, and thus none of the imports are found.

Here is one of the nuget imports in question:

  <Import Project="$(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props" Condition="'$(ImportProjectExtensionProps)' == 'true' and exists('$(MSBuildProjectExtensionsPath)')">

And the build error is reported not from my own WPF project file, but from a synthesized one like this: "C:\Users\username\source\repos\WpfApp3\WpfApp3\5gejw5ia.tmp_proj"

One can get away with this if the PackageReference doesn't point to a package that actually matters to the compilation. But if that package reference adds msbuild imports that are important, you can end up with a build error or a runtime error down the road.

Here are the repro steps for one such case:

  1. Create a new WPF app
  2. Make sure VS is set to use PackageReference
  3. Install-Package Nerdbank.GitVersioning
  4. In the MainWindow.xaml.cs file, add this line of code to MainWindow(): string s = ThisAssembly.AssemblyConfiguration;
  5. Build the project.

This should compile fine, and in fact the main project would, but the tmp_proj fails to compile because in fact the MSBuild target that generates ThisAssembly doesn't get imported from the PackageReference for the tmp_proj.
Note similar repro steps but using a C# console project works perfectly well -- because there is no tmp_proj involved that scrambles the $(MSBuildProjectName).

This bug repros on all versions of NuGet that I have tested since PackageReference support was first added. I am using 15.4 Preview right now when I see this.

I'm hobbling along with this workaround for now:

At the top of my project file:

  <PropertyGroup>
    <!-- workaround for https://github.com/NuGet/Home/issues/5894 -->
    <OriginalProjectName>IntellisenseImpl</OriginalProjectName>
    <RootMSBuildProjectExtensionsPath>$(RepoRoot)obj\$(BuildSubPath)$(OriginalProjectName)\</RootMSBuildProjectExtensionsPath>
  </PropertyGroup>
  <Import Project="$(RootMSBuildProjectExtensionsPath)$(OriginalProjectName).*.props"
          Condition=" '$(MSBuildProjectName)' != '$(OriginalProjectName)' and '$(ImportProjectExtensionProps)' != 'false' and exists('$(RootMSBuildProjectExtensionsPath)')" />

and this at the bottom of my project file:

  <Import Project="$(RootMSBuildProjectExtensionsPath)$(OriginalProjectName).*.targets"
          Condition=" '$(MSBuildProjectName)' != '$(OriginalProjectName)' and '$(ImportProjectExtensionProps)' == 'true' and exists('$(RootMSBuildProjectExtensionsPath)')" />

Note this is not equivalent to the originally designed behavior because the imports aren't in the same spot during msbuild evaluation as if the bug didn't exist, and it's quite tedious to hand-add this to each affected file and customize the OriginalProjectName which must be hard-coded. It made it particularly difficult to invent this workaround that I can't see the tmp_proj content nor what it actually imports since (I think) it is synthesized and executed all within an MSBuild task.

@AArnott
I can repro and I'm trying to figure out a NuGet side fix.
The root cause is in the generation of the tmp_proj, but if a fix was shipped on WPF side the adoption would be significantly slower.

On a different note,
I'm having trouble building the project with your package in legacy packref because of duplicate assembly attributes.

Severity	Code	Description	Project	File	Line	Suppression State

Error	CS0579	Duplicate 'System.Reflection.AssemblyVersionAttribute' attribute	ConsoleApp9	C:\Users\nikolev.REDMOND\Documents\Visual Studio 2017\Projects\ConsoleApp9\ConsoleApp9\obj\Debug\ConsoleApp9.Version.cs	11	Active
Error	CS0579	Duplicate 'System.Reflection.AssemblyFileVersionAttribute' attribute	ConsoleApp9	C:\Users\nikolev.REDMOND\Documents\Visual Studio 2017\Projects\ConsoleApp9\ConsoleApp9\obj\Debug\ConsoleApp9.Version.cs	12	Active

Are you aware of this issue?

The duplicate assembly attributes build break is due to dotnet/sdk#1142. After package restore, do a rebuild (or delete the generated .cs files under the project's obj folder) to get going again.

Thanks for looking into this. Yes, WPF's tmp_proj is bad on many levels but "fixing" that will not likely happen quickly. So the best NuGet can do is workaround it.

met with WPF team engineer yesterday to discuss fixing PresentationBuildTasks.dll -- based on my PR http://github.com/nuget/nuget.client/pull/1786 -- will update when I understand timeframe of fix into WPF. We'll then do the fix in microsoft.common.* in msbuild.

Note that this appears to be affecting using MSBuild.Sdk.Extras to add WPF support since the targets aren't getting added to the tmp_proj builds.

The workaround @AArnott mentioned works here as well, but it's really nasty.

Ping, any updates?

wjk commented

@onovotny See the very bottom of this file: https://github.com/Microsoft/dotnet-framework-early-access/blob/master/release-notes/build-3052/dotnet-build-3052-changes.md. Once .NET Framework 4.7.2 is released and installed, this issue should go away.

@wjk Is there any more details on how this is fixed? Also, how about a workaround that doesn't rely on a .NET Fx fix? Is there anything that can be done out-of-band? Any ETA on 4.7.2 as a release?

@onovotny
The fix is in NET 472, which should ship alongside RS4/Visual Studio Update 6.

The PR on msbuild side that still needs to be merged is here.

Currently, there's no plans for down-level support.

@nkolev92 Does 4.7.2 always get installed with 15.6? I.e., if I have 15.6, will this always build ok? Just trying to understand how to ensure that builds are always successful here. What about people on RS3 or lower?

Also, any idea when that PR will be merged to msbuild?

@onovotny
I can't really answer your first question definitively. AFAIK there's normally an option in the installer to install .NET 4.7.2, so I'm pretty sure it doesn't just install when updating to 15.6.

.NET 4.7.2 can also be installed in RS3 and lower Windows builds, so any machine with .NET 4.7.2 + 15.6 should work.

@nkolev92 From what I've seen before, VS may not default to including/installing 4.7.2, so there's no way to be sure. That can be problematic for broken builds here as well as for build agents.

I understand that it can get painful, that decision was a longer discussion, that @rrelyea has more insights on.

There's a lot of risk in shipping via NuGet since it's basically overwriting what the build tasks that ship with the .NET full framework itself do.

@nkolev92 fair enough, but can any of the proposed changes be an opt-in out-of-band fix? I.e., something I can ship as part of my MSBuild.Sdk.Extras for people to opt-in to?

Basically, what was the fix NuGet was going to do and can I use that fix in my project directly?

If you overwrite enough targets yes you should be able to fix it.
And no, 15.6 will not require net472.

For some reason I thought this was going to be fixed in 15.6, but it evidently isn't, as this repro project demonstrates.
Any idea when it will be fixed?

jviau commented

I found a simpler workaround. If your original project and assembly name are identical, you can use $(AssemblyName) in the .tmp_proj to import the nuget props & targets. You can then put these imports in Directory.Build.props/targets and condition them based on '$(MSBuildProjectExtension)' == '.tmp_proj'.

Example:

<Import Project="$(BaseIntermediateOutputPath)$(AssemblyName).*.props" Condition="'$(MSBuildProjectExtension)' == '.tmp_proj'" />

Just to note, my Directory.Build.props manually sets AssemblyName to MSBuildProjectName and sets all the output paths early on. I find the SDK sets them too late, breaking some nuget packages like the VSSDK build tools.

The fix is in net472, you need to install that on your build box. It fixes the WPF build task to use the original file extension and location so props/targets are included.

I have the fix where instead of .tmp_proj, a _tmpprj.csproj is produced. But evidently that doesn't guarantee a complete fix, because my nuget imports are still not impacting the inner project.

jviau commented

I also have the newer WPF targets, and is indeed not fixed. This is because the nuget targets are imported like so:

<Import Project="$(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props" Condition="'$(ImportProjectExtensionProps)' == 'true' and exists('$(MSBuildProjectExtensionsPath)')" /> 

https://github.com/Microsoft/msbuild/blob/8e015cc049b5f3f9cbd9e10ee30ae86998d934d5/src/Tasks/Microsoft.Common.props#L66

This will not work as it will be searching for {OriginalName}_{hash}_wpftmp.*.props, which does not include {OriginalName}.g.nuget.props.

@AArnott Can you share your fix for *.tmp_proj? Can we put them in MSBuild.Sdk.Extras?

No, because my fix requires that the xml itself hard-code the original file name of the project file.
However, if we assume the presence of the WPF change that changes *.tmp_proj with *_tmpprj.csproj, we might be able to come up with some crafty MSBuild syntax to recreate the original project name. In that case, I think it's possible.

This is in PresentationBuildTasks. So, We might need a Custom Task for that. NuGet had a temporary fix but they ran into license issues, right? So, they put the fix into .NET 4.7.2.

Let's see If we can use that workaround!

wjk commented

@AArnott @onovotny I have discovered something that may be of use here. Disassembling PresentationBuildTasks.dll (yuck, I know...) reveals this:

presentationbuildtasks

This dump reveals that we now need to use the $(_TargetAssemblyProjectName) variable to find the correct NuGet imports. I understand how Microsoft didn't use this by default in the SDK, because the SDK doesn't officially support WPF; that's our job. Has anyone tried to use that variable in MSBuild.Sdk.Extras? I'm always happy to submit a PR.

I already did in my recent commits, both by property and by overriding this task. Have to ask MSBuild team for legality!

It'll be included in PR MSBuild.Sdk.Extras

I think that $(_TargetAssemblyProjectName) property was added recently by Rob. But I guess somehow it didn't get tunneled through the .targets yet. Nice find.

We're hitting this as well, so I'm subscribing to this thread to get notified when there is a solution available.

I am also having this issue.

For me, it looks like $(MSBuildProjectName) is being set to the random string that is being generated for the temporary file.

We have props files that are code generated using things like this:

<MyPropertiesProject>c:\src\objects\codegen\MSBuildSupport\$(MSBuildProjectName)\$(Configuration)\My_Project.props</MyPropertiesNIProject>

Then, when I try to build, this is the error I am getting:

C:\src\objects\codegen\My_Common.props(40,3): error MSB4019: The imported project "c:\src\objects\codegen\MSBuildSupport\jr0jnttk\Debug\My_Project.props" was not found. 
Confirm that the path in the <Import> declaration is correct, and that the file exists on disk. [C:\src\source\MyProject\jr0jnttk.tmp_proj]

So for some reason its setting $(MSBuildProjectName) to jr0jnttk

Another side effect of the generated proj file is that it is losing metadata set on references (code path goes through ShimReferencePathsWhenCommonTargetsDoesNotUnderstandReferenceAssemblies target in Microsoft.Common.CurrentVersion.targets. As a result, workaround for embedding interop types does not work (it correctly sets metadata that gets discarded).

I was able to workaround it with similar one:

  <Target Name="EmbeddingBuilderDLLInteropTypes2" AfterTargets="ShimReferencePathsWhenCommonTargetsDoesNotUnderstandReferenceAssemblies" BeforeTargets="CoreCompile">
    <ItemGroup>
      <ReferencePathWithRefAssemblies Condition=" '%(FileName)' == 'my-interop-assembly' ">
        <EmbedInteropTypes>true</EmbedInteropTypes>
      </ReferencePathWithRefAssemblies>
    </ItemGroup>
  </Target>

I wonder if we need a separate issue for that or this one will do?

@ivan-danilov

Your scenario has the same root cause as this issue, so this one is enough.

This seems to be fixed for me, starting with MAT 4.0.6913.0
Anyone else seeing this as fixed with the latest MAT?

@dlbeeman This doesn't have to do with MAT specifically. If MAT somehow worked around it, that's fine. But we need a fix to the underlying system so all the build influencing packages get picked up by tmp_proj.

This is pretty nasty when trying to utilize Directory.Build.props to do something like... say sign your Release builds using properties from an internal NuGet package. Your output just doesn't run, since the AssemblyOriginatorKeyFile property is blank for the temporary project, the temporary Wpf assembly is unsigned, while your final output assembly is signed because everything was properly imported.

FWIW, this is my current workaround, which only works because the _TargetAssemblyProjectName is the same as the folder name:

<Import Project="$(_TargetAssemblyProjectName)\$(IntermediateOutputPath)\..\$(_TargetAssemblyProjectName)$(MSBuildProjectExtension).nuget.g.props" Condition="$(MSBuildProjectName.EndsWith('_wpftmp'))" />

@mletterle Migrate to an SDK-Style project (using Microsoft.NET.Sdk.WindowsDesktop) and see if the issue still persists.

@mletterle Migrate to an SDK-Style project (using Microsoft.NET.Sdk.WindowsDesktop) and see if the issue still persists.

Sorry that's not going to help - see dotnet/wpf#1056 (comment) for an almost-but-not-quite-successful attempt to make this work in WindowsDesktop SDK projects. /cc @ryalanms

@mletterle Migrate to an SDK-Style project (using Microsoft.NET.Sdk.WindowsDesktop) and see if the issue still persists.

This was found during the migration to an SDK-Style project :P

@vatsan-madhavan The problem is with the temporary project generation. So, Is it possible to come up with a different solution without having to rely on temporary project to generate the assembly?

@Nirmal4G that’s going to require a redesign of markup compilation.

@vatsan-madhavan Does the UAP tools use the same technique (as it's also XAML)? If not, How are they doing that?

@vatsan-madhavan Does the UAP tools use the same technique (as it's also XAML)? If not, How are they doing that?

WPF and UAP builds process XAML very differently as they use different binary formats in the output.

WPF uses BAML which is much more a compiled representation than UAP which uses XBF which is basically just serialized XAML.

IMO a redesign is long overdue. But if that's not likely to get funded, we ought to be able to make a tactful small change to the tmp_proj itself so that it imports the same package refs that its outer project does.

Yes I'm guessing when markup compilation was designed for the first time Roslyn wasn't around so you had to resort to compiling an assembly first and then reflecting over it. With Roslyn I'm guessing one could avoid calling Csc twice and save on performance and fragility that have been so problematic over the years.

We should really consider investing in it at some point.

I don't think Roslyn should be an assumed requirement in the redesign. WPF is used by non-Roslyn languages too. If the reason for the inner build is that reflection over types is required, I hope that can still be done within the main project by just adding more targets.

/cc @aortiz-msft, @karann-msft

At the min we should push for the change @AArnott is recommeding. It's something that was considered a while back, but now that .NET Core WPF is always PackageReference, it's more important.

Change would be somewhere here: https://github.com/dotnet/wpf/blob/master/src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/GenerateTemporaryTargetAssembly.cs

If the pre-compilation is only needed for just reflection (and intellisense) then I would go with @KirillOsenkov's proposal, using Roslyn to do just that and more. This could provide the best build performance and better visual experiences across any tools and IDEs that uses Roslyn and LSP.

The approach @AArnott is proposing has problems. That’s the approach taken by dotnet/wpf#1056, which had to be ultimately abandoned for reasons discussed in the PR. In short, we cannot run NuGet restore on the temp project safely and risk hitting the network once the build process (for the main project) has started.

And he also right in suggesting that we cannot rely on Roslyn for a redesign - that would indeed hamper use by non-Roslyn languages.

And he also right in suggesting that we cannot rely on Roslyn for a redesign - that would indeed hamper use by non-Roslyn languages.

I'm not aware of this. What are the other languages?

@vatsan-madhavan I wasn't proposing that we run package restore on the tmp_proj. Restore should only happen on the original project file. What I'm proposing is simply that the tmp_proj import the same props and targets from PackageReference items that the original project does. Does that change your assessment of my proposal?

@Nirmal4G: VS is an open platform. There are several languages including 3rd party languages that allow you to develop WPF apps. To redesign WPF's build system to rely on Roslyn would cut off these other languages that have built up their own customer base. I don't know the languages off hand to enumerate to you, but I've encountered them on occasion, and it's how we designed it. So IMO we shouldn't abandon the customers who built on our platform.

Note that my proposal does not exclude non Roslyn languages. There can be API that inspects the source and returns the types, namespaces and other info needed. It could be implemented by Roslyn for csproj and vbproj, the F# compiler for F# and whoever else for their language as needed. Build would just call a target that is implemented differently. When I said Roslyn I meant compiler API that understands source, so we don’t have to use reflection.

Given most compilers don't have an API like Roslyn's, and Roslyn was very expensive to create, I think this would have to be an 'opt in' feature so that we don't break everyone else in the meantime.

But honestly I’d be curious to see any data about projects using markup compilation that are not C#. Even anecdotal.

Even if we just fix it for C# it’ll be a vast improvement for the vast majority of projects.

we shouldn't abandon the customers who built on our platform.

We are not! Previously, the PresentationBuildTasks was closed source. But now is not the case. So when we do improve it for roslyn based languages, others would see it and implement something similar for their own toolchain.

What I'm proposing is simply that the tmp_proj import the same props and targets from PackageReference items that the original project does. Does that change your assessment of my proposal?

We did consider this one briefly.

*.nuget.g.props aren't imported by the project - they are imported by Microsoft.Common.props like this:

  <Import Project="$(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props" Condition="'$(ImportProjectExtensionProps)' == 'true' and exists('$(MSBuildProjectExtensionsPath)')" />

Importing $(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props into the tmp_proj would not work as-is - that happens anyway today and doesn't work because the values of these properties change for each project.

We'd have to import $(MSBuildOriginalProjectExtensionsPath)$(MSBuildOriginalProjectFile).*.props or something like that. It requires computing the right values for the original projects paths (like MSBuildProjectExtensionsPath) first when building the tmp_proj.

These values are relatively straightforward to infer (but still in the realm of guesswork) in uncustomized build environments; in customized build environments, it gets harder to get these right, and the solution approach starts looking less-than solid. Besides, PBT already has weaknesses in reliably inferring $(ObjDir) type paths even for the projects being currently built, esp. when those paths have been redirected to custom locations outside the cone of the project directory.

@vatsan-madhavan

The nuget.g.props and nuget.g.targets do not have any project specific mentions by design.

For example this is a nuget.g.props for a project:

<?xml version="1.0" encoding="utf-8" standalone="no"?>
<Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <RestoreSuccess Condition=" '$(RestoreSuccess)' == '' ">True</RestoreSuccess>
    <RestoreTool Condition=" '$(RestoreTool)' == '' ">NuGet</RestoreTool>
    <ProjectAssetsFile Condition=" '$(ProjectAssetsFile)' == '' ">$(MSBuildThisFileDirectory)project.assets.json</ProjectAssetsFile>
    <NuGetPackageRoot Condition=" '$(NuGetPackageRoot)' == '' ">$(UserProfile)\.nuget\packages\</NuGetPackageRoot>
    <NuGetPackageFolders Condition=" '$(NuGetPackageFolders)' == '' ">C:\Users\nikolev.REDMOND\.nuget\packages\;C:\Microsoft\Xamarin\NuGet\;C:\Program Files\dotnet\sdk\NuGetFallbackFolder</NuGetPackageFolders>
    <NuGetProjectStyle Condition=" '$(NuGetProjectStyle)' == '' ">PackageReference</NuGetProjectStyle>
    <NuGetToolVersion Condition=" '$(NuGetToolVersion)' == '' ">5.5.0</NuGetToolVersion>
  </PropertyGroup>
  <PropertyGroup>
    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
  </PropertyGroup>
</Project>

Just making a copy of the targets with the temp project name and later deleting it should do it.
The assets file is already used in the additional compilation step so just ensuring the props/targets get imported is enough.

A while back I tested this approach with WPF .NET Framework and it worked.

It's hacky, but it works.

Just found out this is affecting our source only packages too.
We use some source only nuget packages, we need them as source only for license and obfuscation matters, the point is: In wpf projects the linked files are not linked because nuget targets are not included in tmp_project.

The right issue to track for .NET Core WPF is dotnet/wpf#810.

Closing per above.

Maybe i missed something here, but what about the people using .Net Framework (not .Net Core or .Net 5), WPF, and the new project system together? Is dotnet/wpf#810 for this as well? Or is the new project system not officially supported with .Net Framework?

FYI (thanks to @mletterle), the workaround I needed for this (Visual Studio 2017 targeting .NET 4.6.2 with WPF, trying to use PackageReference) was to add this to the top of the project file:

  <Import Project="$(IntermediateOutputPath)..\..\$(_TargetAssemblyProjectName)$(MSBuildProjectExtension).nuget.g.props" Condition="$(MSBuildProjectName.EndsWith('_wpftmp'))" />

and also a similar one referencing the .targets file to the bottom of the file. This is probably not universally robust.