Explicitly import NuGet.targets #2663
Conversation
NuGet.targets was added as a hard dependency in dotnet#2595, this change will explicitly import it in Microsoft.Common.CurrentVersion.targets.
| <!-- Import NuGet.targets (required for GetReferenceNearestTargetFrameworkTask and used for Restore functionality) --> | ||
| <PropertyGroup> | ||
| <NuGetRestoreTargets Condition="'$(NuGetRestoreTargets)'=='' and '$([MSBuild]::IsRunningFromVisualStudio())'=='true'">$(MSBuildToolsPath32)\..\..\..\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets</NuGetRestoreTargets> | ||
| <NuGetRestoreTargets Condition="'$(NuGetRestoreTargets)'==''">$(MSBuildToolsPath)\NuGet.targets</NuGetRestoreTargets> |
There was a problem hiding this comment.
Is the location being changed from $(MSBuildExtensionsPath)\NuGet.targets ? (https://github.com/Microsoft/msbuild/blob/master//targets/bootstrapDependencies/Core/15.0/Microsoft.Common.targets/ImportAfter/Microsoft.NuGet.ImportAfter.targets#L15)
There was a problem hiding this comment.
Good catch, thanks! This shouldn't be needed at all anymore. This file was just because that was the way we imported NuGet.targets and on .NET Core we didn't have the file locally to copy it. Now that the import is explicit this file isn't needed.
There was a problem hiding this comment.
And to the location, it should be either in the VS install location (when IsRunningFromVisualStudio is true) or next to MSBuild in the .NET Core case ($(MSBuildToolsPath)). It should be the case that MSBuildToolsPath and MSBuildExtensionsPath are the same, but not necessarily. This work ok with mono?
There was a problem hiding this comment.
We deploy to MSBuildExtensionsPath. And that differs from MSBuildToolsPath on Mono. If the two are the same for netcore, then we can change it to use MSBuildExtensionsPath, I guess?
There was a problem hiding this comment.
Is it possible to change? Prefer to not use MSBuildExtensionsPath. Since they're fully required now, it's not really an extension. More like Roslyn is:
https://github.com/Microsoft/msbuild/blob/master/src/MSBuild/app.config#L101
The checked in file was just to get tests working not for any deployment strategy.
There was a problem hiding this comment.
re:change, sure, I'll do that. I am wondering if I should be removing it from MSBuildExtensionsPath or have it in both locations (it doesn't import anything, currently!).
| :: warning MSB4011: Imported target already imported. This will happen when Microsoft.NuGet.ImportAfter.targets installed on the local | ||
| :: machine still has the import to NuGet.targets which is now in Microsoft.Common.CurrentVersion.targets. This can be | ||
| :: removed when we want to require VS15.5+ to build locally. | ||
| SET _NOWARN=MSB3277;MSB3026;MSB3073;AL1053;MSB4011 |
There was a problem hiding this comment.
You will need to add the code to cibuild.sh as well
There was a problem hiding this comment.
Or not, on cross plat I guess we won't get a double import warning?
NuGet.targets was added as a hard dependency in #2595, this change will
explicitly import it in Microsoft.Common.CurrentVersion.targets.
NuGet change: dotnet/NuGet.BuildTasks#44