Support restoring packages.config with restore task #2917
Conversation
This enable msbuild /t:Restore to restore projects with packages.config as well as PackageReference.
| @@ -435,6 +435,14 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <!-- If this is not a PackageReference project check if project.json or projectName.project.json exists. --> | |||
| <RestoreProjectStyle Condition=" '$(RestoreProjectStyle)' == '' AND '$(_CurrentProjectJsonPath)' != '' ">ProjectJson</RestoreProjectStyle> | |||
| <!-- This project is either a packages.config project or one that does not use NuGet at all. --> | |||
| <RestoreProjectStyle Condition=" '$(RestoreProjectStyle)' == '' AND | |||
| ('$(MSBuildProjectExtension)' == '.csproj' | |||
There was a problem hiding this comment.
Let me know what you think of this list, is it too greedy or am I missing any?
| var v2RestoreResult = await PerformNuGetV2RestoreAsync(log, dgFile); | ||
| restoreSummaries.Add(v2RestoreResult); | ||
|
|
||
| // TODO: Message if no packages needed to be restored? |
There was a problem hiding this comment.
Some resources are only in nuget.exe, should I move them or replicate them?
| @@ -242,5 +314,243 @@ public void Dispose() | |||
| { | |||
| _cts.Dispose(); | |||
| } | |||
|
|
|||
| #if IS_DESKTOP | |||
| private async Task<RestoreSummary> PerformNuGetV2RestoreAsync(Common.ILogger log, DependencyGraphSpec dgFile) | |||
There was a problem hiding this comment.
I ended up copy/pasting most of this method. Another option is to move it to NuGet.PackageManagement so its shared between nuget.exe and RestoreTask. But it would have to be public so I wasn't sure what you'd prefer
| var nuGetPackageManager = new NuGetPackageManager(sourceRepositoryProvider, settings, repositoryPath); | ||
|
|
||
| // TODO: different default? Allow user to specify? | ||
| var packageSaveMode = Packaging.PackageSaveMode.Defaultv2; |
There was a problem hiding this comment.
You can't specify a PackageSaveMode when using the RestoreTask, should I add a parameter? And what should the default be?
| cacheContext.NoCache = RestoreNoCache; | ||
|
|
||
| // TODO: Direct download? | ||
| // //cacheContext.DirectDownload = DirectDownload; |
There was a problem hiding this comment.
There's no DirectDownload parameter to RestoreTask so I figured I'd not allow it for this scenario. Just making sure you agree.
| } | ||
|
|
||
| #if IS_DESKTOP | ||
| public class CommandLineSourceRepositoryProvider : ISourceRepositoryProvider |
There was a problem hiding this comment.
This is another copy/paste that I could just move to NuGet.PackageManagement instead. Let me know your preference
|
|
||
| namespace NuGet.CommandLine | ||
| namespace NuGet.ProjectManagement |
There was a problem hiding this comment.
I moved this class from nuget.exe to NuGet.PackageManagement because it is already public, is that ok?
|
I will write unit tests once some details are figured out. Also, here's some sample output: |
|
I believe there's a discussion to be had about whether we want this into the product before reviewing this PR :) |
|
Hi @jeffkl! I've created a corresponding issue over at NuGet/Home#8506 and I'm gonna be helping make sure we get all your questions answered and get this landed -- the NuGet team has decided this is a good change to make that addresses previous concerns about backporting P.C. support, and this is baaaasically what I'll be working on for the next couple of weeks! Thanks a lot for this PR! It's a HUGE help in getting things across the board. I haven't started reviewing it yet (I've got a P0 on my plate right now), but I'll do that soon, and then help come up with a good testing strategy, since my understanding is this isn't a very well tested part of NuGet (I'm new here, so please bear with me while I get up to speed!). I look forward to working with you~ |
|
@zkat awesome, thanks for taking this on. Feel free to schedule a meeting with me to discuss it. I can't wait to get this change in. |
|
Closing in favor of #3071 |
This enable msbuild /t:Restore to restore projects with packages.config as well as PackageReference.
There are two project types that still don't support
PackageReference, Service Fabric (.sfproj) and Visual C++ (.vcxproj). When you have a code base with one or more of these project types, you must either restore twice (once with msbuild and once with nuget.exe) or restore with nuget.exe against a Visual Studio solution file. It does not work well for large code bases to do either of those.It is fully understood that
packages.configis deprecated, however there are large code bases that cannot move away from.vcxprojor.sfproj. Until those project types supportPackageReference, there needs to be a way to restore them.Another point is that everything works as expected in Visual Studio. Packages are restored for all projects regardless of whether or not they are using
packages.configorPackageReference. So this really just brings parity back.I've only made this work for .NET Framework and the feature is disabled for .NET Core. The only scenario that won't work as expected is a
.csprojstill usingpackages.config(the other project types,.vcxprojand.sfprojdo not build in .NET Core anyway). But I don't want to enable restoringpackages.configfor scenarios where people could just migrate toPackageReference.