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

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

Closed
AArnott opened this issue Sep 15, 2017 · 61 comments
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:Restore Partner:DotNet Priority:2 Issues for the current backlog. Resolution:External This issue appears to be External to nuget Style:PackageReference
Milestone

Comments

@AArnott
Copy link
Contributor

AArnott commented Sep 15, 2017

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.

@AArnott AArnott changed the title PackageReference broken in WPF projects due to tmp_proj not importing them PackageReference broken in WPF projects due to tmp_proj not importing Package-supplied build authoring Sep 15, 2017
@AArnott
Copy link
Contributor Author

AArnott commented Sep 15, 2017

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.

@nkolev92
Copy link
Member

nkolev92 commented Oct 5, 2017

@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?

@AArnott
Copy link
Contributor Author

AArnott commented Oct 5, 2017

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.

@rrelyea
Copy link
Contributor

rrelyea commented Nov 29, 2017

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.

@clairernovotny
Copy link

clairernovotny commented Dec 12, 2017

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.

@clairernovotny
Copy link

clairernovotny commented Feb 5, 2018

Ping, any updates?

@wjk
Copy link

wjk commented Feb 5, 2018

@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.

@clairernovotny
Copy link

@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?

@nkolev92
Copy link
Member

nkolev92 commented Feb 5, 2018

@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.

@clairernovotny
Copy link

@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?

@clairernovotny
Copy link

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

@nkolev92
Copy link
Member

nkolev92 commented Feb 5, 2018

@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.

@clairernovotny
Copy link

@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.

@nkolev92 nkolev92 added Triage:NeedsTriageDiscussion and removed Priority:2 Issues for the current backlog. labels Feb 12, 2020
@Nirmal4G
Copy link

Nirmal4G commented Feb 13, 2020

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.

@vatsan-madhavan
Copy link

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.

@Nirmal4G
Copy link

Nirmal4G commented Feb 13, 2020

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?

@AArnott
Copy link
Contributor Author

AArnott commented Feb 13, 2020

@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.

@KirillOsenkov
Copy link

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.

@AArnott
Copy link
Contributor Author

AArnott commented Feb 13, 2020

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.

@KirillOsenkov
Copy link

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.

@Nirmal4G
Copy link

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.

@vatsan-madhavan
Copy link

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.

@nkolev92
Copy link
Member

@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.

@aortiz-msft aortiz-msft added Priority:2 Issues for the current backlog. and removed Triage:NeedsTriageDiscussion labels Mar 12, 2020
@TrabacchinLuigi
Copy link

TrabacchinLuigi commented Mar 24, 2020

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.

@nkolev92 nkolev92 added Partner:DotNet Resolution:BlockedByExternal Progress on this task is blocked by an external issue. When that issue is completed this can proceed Resolution:External This issue appears to be External to nuget and removed Resolution:BlockedByExternal Progress on this task is blocked by an external issue. When that issue is completed this can proceed labels May 29, 2020
@nkolev92
Copy link
Member

nkolev92 commented Jun 1, 2020

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

@aortiz-msft
Copy link
Contributor

Closing per above.

@felixhirt
Copy link

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?

@uecasm
Copy link

uecasm commented Jul 1, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:Restore Partner:DotNet Priority:2 Issues for the current backlog. Resolution:External This issue appears to be External to nuget Style:PackageReference
Projects
None yet
Development

No branches or pull requests