VS Auto-Restore does not run CollectPackageReferences for non-SDK project files

[Cellivar]

Details about Problem

NuGet product used: VS UI

VS version (if appropriate): 16.7.6

OS version Win10 10.0.19041

dotnet --version: 3.1.403

Detailed repro steps so we can see the same problem

1. Clone/download this repo: https://github.com/Cellivar/test-nuget-restore-problem
2. Open the Solution
3. Right-click the solution -> Restore NuGet Packages

There is no new folder in the project directory.

4. Run `dotnet restore ./WebApplication1.csproj

A folder ThisDirectoryShouldBeCreatedOnNugetRestore appears in the project directory.

---

The important pieces of the csproj file are that

1. It is a non-SDK project (and cannot be converted to SDK format).
2. It uses PackageReference instead of the config format.
3. It has a target that runs before CollectPackageReferences :

  <Target Name="CreateFolderOnRestore" BeforeTargets="CollectPackageReferences">
    <MakeDir Directories="ThisDirectoryShouldBeCreatedOnNugetRestore"/>
  </Target>

CollectPackageReferences was added to address this issue back in #4634 and I can confirm that this same setup works correctly for SDK-style projects. That is, from Visual Studio the nuget restore operations will run this target. It is only the non-SDK style ASP.NET projects that are not executing the targets.

Background

To promote local development workflows we're dynamically replacing the versions of packages that are pulled into the project on the fly. If a package is referenced in a project file and a temporary prerelease version of that package appears in a magical directory on a developer's machine, the version reference will automatically be replaced (via a target set to run before CollectPackageReferences) with a reference to the local version instead of the standard version.

This works excellent, as initial clone + build works fine using the standard package versions. When the developer clones and builds a dependency of the project they just automatically start using it with no additional effort.

Or at least that was the case until we ran into this problem with non-SDK projects, of which we have ~200 representing around 1.2m LoC. They can't be converted anytime soon. The non-SDK projects do not execute the target (and I can't seem to find any way to force them to) which results in only the standard versions of the packages being pulled down. This can result in odd behavior and package downgrade errors.

I'm open to using workarounds if a fix is not on the near-term roadmap.

[rrelyea]

CollectPackageReferences is an extensibility point that I believe we are trying to wean people from, for perf reasons: see https://github.com/NuGet/docs.microsoft.com-nuget/issues/1941
[update: my info CollectPackageReferences was out of date...we eventually ran that target again with our RestoreUseStaticGraphEvaluation work. (at one point we didn't, but by the time it shipped it did.]

We are working on a central version feature in nuget that may help your scenario?
https://github.com/NuGet/Home/wiki/Centrally-managing-NuGet-package-versions

[Cellivar]

I definitely understand the performance side of things, restore is noticeably affected.

What we're trying to accomplish includes an aspect of what the central package file attempts to accomplish. We have a 'default version' that we want projects in the same repo to use. This is built as a static list of <PackageReference Update="Pkg.Id".../> elements in a targets file. This aspect of the non-SDK projects works as expected: Regardless of the version set in the csproj it will take the version from the targets file.

The second component is dynamically reading the list of packages in a local repo directory and generating override PackageReferences for those available packages. This involves generating an ItemGroup out of the files and using regex to break their name into ID and Version fields, something I can't do outside of a target and haven't found a good Evaluation-phase replacement for.

If I could either

1. Dynamically generate the list of PackageReferences at evaluation time.
2. Run nuget CLI prior to the build to get the proper restore behavior.
3. Find some other target that is not CollectPackageReferences that VS will still execute, even if it's undocumented/unsupported/flaky but will still work.

then I can proceed with this project for all of our internal repositories. I'd very much like to not force our developers to interact outside of Visual Studio just to support non-SDK projects.

[rrelyea]

@Cellivar - i updated my first reply...with correction about the CollectPackageReference target.
@nkolev92 - CollectPackageReferences in VS for non-sdk projects? thoughts?
[/cc @rconard @cristinamanum - for the version feedback/scenarios]

[nkolev92]

It is unlikely to get solved.
The csproj.dll project system doesn't run targets right now. It doesn't even evaluate conditions.

See relevant issue: dotnet/project-system#4175.

I think this is a won't fix at this point.

[Cellivar]

I was afraid that might be the result.

The only reason we have non-SDK projects at this point in time is the remaining ASP.NET projects in our codebase. Some are relatively small and will likely be converted in the near future to ASP.NET Core. Other represent a significant fraction of our total LoC and will not be converted for the foreseeable future. I see that davkean commented on the long standing issue that it's something being looked at.

Is there any centralized list of caveats and unsupported features for non-SDK projects that we could more easily reference? It's frustrating to have to find this information via GitHub issues or StackOverflow posts.

[nkolev92]

Personally, I'm not aware whether there's a mega document summarizing the differences and caveats, but the folks over at the project-system repo understand that better, so I'd defer to them about any such doc.

As far as NuGet is concerned, CollectPackageReferences is not even something that's currently documented.
The target existence does deviate from the goal to keep restore info evaluation based, as @rrelyea mentioned, but that's what we have right now.