NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 253 forks source link

NuGet.exe restore does not import Directory.Build.Targets/Props #6734

Open AArnott opened 6 years ago

AArnott commented 6 years ago

Details about Problem

NuGet product used (NuGet.exe | VS UI | Package Manager Console | dotnet.exe): nuget.exe

NuGet version (x.x.x.xxx): 4.6.1

MSBuild version (if appropriate): v15.7.108.30652

Worked before? If so, with which NuGet version: Never tried.

Detailed repro steps so we can see the same problem

git clone https://devdiv.visualstudio.com/DefaultCollection/Personal/_git/VS-Platform.andarno 
cd VS-Platform.andarno 
git co nugetIssue6734
cd src\Identity\ASALExtension
nuget restore
msbuild

Expected

The build succeeds

Actual

The build fails because newtonsoft.json.dll isn't referenced in the Microsoft.VisualStudio.ASALExtension.UnitTests project.

Now try:

MSBuild /restore

The build succeeds!

A close inspection of project.assets.json for the Microsoft.VisualStudio.ASALExtension.UnitTests project reveals that when nuget restore is used, no packages referenced by Microsoft.VisualStudio.ASALExtension are transitive to its unit test project over the P2P reference. But when using MSBuild /restore, packages do traverse the project reference.

This started happening when I moved PackageReference items' Version metadata away from the project that references the package and into a common src_build\packageversions.targets file that uses <PackageReference Update="pkgid" Version="1.2.3" /> as a means to easily control the versions of packages referenced across the repo.

AArnott commented 6 years ago

I've confirmed the problem comes from the fact that my Directory.Build.targets file isn't even imported during the nuget restore command. I confirmed this by introducing xml syntax errors, and nuget restore didn't report this, but failed in the same way. Since this .targets file is responsible for defining the package versions, all the other failures make sense.

So the question is: why does nuget restore fail to import Directory.Build.targets where MSBuild /restore does it right?

I once filed a bug last year (https://github.com/Microsoft/msbuild/issues/1991) for failing to import Directory.Build.targets under some circumstances, but that bug was fixed. I wonder if this is somehow related to having an old version of something that includes that bug?

AArnott commented 6 years ago

It seems like the bug may be in nuget.targets. Using detailed verbosity on nuget restore, I realized that nuget.exe was invoking MSBuild with a temporary generated .targets file as the project. I found this from Microsoft.Common.targets:

  <PropertyGroup Condition="'$(ImportDirectoryBuildTargets)' == 'true' and '$(DirectoryBuildTargetsPath)' == ''">
    <_DirectoryBuildTargetsFile Condition="'$(_DirectoryBuildTargetsFile)' == ''">Directory.Build.targets</_DirectoryBuildTargetsFile>
    <_DirectoryBuildTargetsBasePath Condition="'$(_DirectoryBuildTargetsBasePath)' == ''">$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildProjectDirectory), '$(_DirectoryBuildTargetsFile)'))</_DirectoryBuildTargetsBasePath>
    <DirectoryBuildTargetsPath Condition="'$(_DirectoryBuildTargetsBasePath)' != '' and '$(_DirectoryBuildTargetsFile)' != ''">$([System.IO.Path]::Combine('$(_DirectoryBuildTargetsBasePath)', '$(_DirectoryBuildTargetsFile)'))</DirectoryBuildTargetsPath>
  </PropertyGroup>

This depends on $(MSBuildProjectDirectory), but that won't be the actual project directory when the project being built is a temporary file generated in %TEMP%\NuGetScratch. If the project files listed in that temporary file were actually invoked using the MSBuild task I think things might be OK, and I'm not sure how it does do it, but I considered it might just be importing these files or otherwise evaluating them in an inappropriate context, so I add this XML to one of my Directory.Build.props files:

    <!-- Workaround https://github.com/NuGet/Home/issues/6734 -->
    <_DirectoryBuildTargetsFile Condition="'$(_DirectoryBuildTargetsFile)' == ''">Directory.Build.targets</_DirectoryBuildTargetsFile>
    <_DirectoryBuildTargetsBasePath>$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), '$(_DirectoryBuildTargetsFile)'))</_DirectoryBuildTargetsBasePath>

This seemed to largely alleviate the issue. In this workaround, I'm forcing MSBuild to find the Directory.Build.targets file at the directory where it actually exists, instead of potentially searching up from %TEMP%\NuGetScratch for it. This seems to have eliminated the nuget restore failures/warnings about missing package versions. The build still fails for me for what may be unrelated causes to this bug, which I'm working through.

This workaround is quite hacky though. Please find a fix.

AArnott commented 6 years ago

I don't yet know fully why, since it's clear that my .targets file is being imported with my workaround, but nuget restore still doesn't work right. There may be more than one underlying issue.

nkolev92 commented 6 years ago

When building the dependency graph we call msbuild from nuget.exe with a targets file to workaround issues with escaping the parameters correctly.

The graph is also built for the whole set of projects(solution potentially), so of the top of my head, not sure how to fix it.

We'll need to dig some more.

AArnott commented 6 years ago

Thanks, @nkolev92. Can you confirm whether your NuGet.targets file invokes the MSBuild task on each of the projects, or if something else happens? Because if you're executing MSBuild for each project from your nuget.targets file, I would have expected this to work.

geraldcsoftware commented 5 years ago

Anybody found a solution to this? I faced a similar issue where nuget restore doesnt work but MsBuild restore does.