search

NuGet / Home

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

Package restore via NominateProjectAsync API leads to build fail in VS 16.8 Preview 3 #10035

Open sae42 opened 4 years ago

sae42 commented 4 years ago

Details about Problem

VS 16.8 Preview 3

NuGet version (5.8.0.6823):

dotnet.exe --version (if appropriate):

VS version (if appropriate): 16.8 Preview 3

OS version : win 10 1909

Worked before? Worked in all preview versions (inc preview 1/2). Only started after upgrade to 16.8. Preview 3.

Detailed repro steps so we can see the same problem

From what I can tell there is a new property ("targetAlias") which is added to the project.assets.json file. E.g. in 16.8: { .... "frameworks": { "netcoreapp3.1": { "targetAlias": "netcoreapp3.1", "imports": [ "net461", ... ],

whereas earlier versions omitted the 'targetAlias' property. When using the NominateProjectAsync API for my custom project (using IVsSolutionRestoreService2.NominateProjectAsync()) this property remains omitted. This leads to a build fail (with a somewhat confusing message - I.E. the project has defined netcoreapp3.1): C:\Program Files\dotnet\sdk\5.0.100-rc.1.20452.10\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution. targets(241,5): error NETSDK1005: Assets file '[path_to_project_folder]obj\project.assets.j son' doesn't have a target for 'netcoreapp3.1'. Ensure that restore has run and that you have included 'netcoreapp3.1' in the TargetFrameworks for your project. [path_to_projectj]

Note, you can repro this error using a simple .NET Core C# project. E.g. create a .NET Core C# console project in 16.7. Assuming you have 16.8 Preview 3 on the same machine, open a 16.8 preview 3 dev prompt, go to the project folder and just type msbuild (i.e. don't do a restore first). If you open the C# project in 16.8 preview 3 the project.assets.json file is updated to include 'targetAlias'

From what I can tell the issue stems from my custom project going through the LegacyPackageReferenceProject class in NuGet (C# runs through NetCorePackageReferenceProject). I am currently using the NominateProjectAsync that doesn't accept an IVsProjectRestoreInfo - but I've tried using IVsSolutionRestoreService.NominateProjectAsync() which does and while there appears to be some code that maps a 'TargetFramework' property to 'TargetAlias' this ultimately gets lost:

Around here I think

I'm not sure if the requirement to add this property is actually deemed essential - so I'm unclear whether the issue is actually in the NuGet side or perhaps the .NET SDK (the issue with an existing C# project failing to build is undesirable I would have thought) so apologies is this should have been raised in the SDK repo.

The only workaround I have is to execute a /t:restore via msbuild in a command prompt.

nkolev92 commented 4 years ago

@sae42

A bit of background on the change:

And I think now we arrive to the reason why your build is failing.

LegacyPackageReferenceProject in VS + the project being built by the SDK is something that worked accidentally. Unfortunately we couldn't have predicted your project breaking. We've kept most of the different between legacy & net core projects minimal despite one being able to multi target and the other not.

Can you change your custom project-system so that it goes through the nomination workflow & ends up with NetCorePackageReferenceProject?

sae42 commented 4 years ago

@nkolev92 The NetCorePackageReferenceProject is picked based on the project system being 'CPS' as far as I can tell. My custom project system is not CPS based. Responding as such via project capabilities would just lead to problems elsewhere. I can certainly use the NominateProjectAsync API that passes in the restore info, but as I mentioned above the properties set are not being honored when writing out the package spec.

Notwithstanding this particular issue, I think the problem of a C# .NET Core 16.7 project failing to build in 16.8 Pr3 without having first done a restore is also a hole that needs filling. So the question could come down to shouldn't the build be more tolerant of the missing 'TargetAlias' property?

nkolev92 commented 4 years ago

@nkolev92 The NetCorePackageReferenceProject is picked based on the project system being 'CPS' as far as I can tell. My custom project system is not CPS based. Responding as such via project capabilities would just lead to problems elsewhere. I can certainly use the NominateProjectAsync API that passes in the restore info, but as I mentioned above the properties set are not being honored when writing out the package spec.

We're gonna change that in https://github.com/NuGet/NuGet.Client/pull/3644.

The reason why the alias is not written out is due to the fact that legacy projects don't support multi targeting, and SDK projects do.

That's why it's important that SDK based projects are detected as NetCorePackageReferenceProject.

Notwithstanding this particular issue, I think the problem of a C# .NET Core 16.7 project failing to build in 16.8 Pr3 without having first done a restore is also a hole that needs filling. So the question could come down to shouldn't the build be more tolerant of the missing 'TargetAlias' property?

Unfortunately it seems like it used to work by accident. SDK projects need to go through that NetCorePackageReferenceProject (Soon CPSPackageReferenceProject) workflow.

An alias to NuGet implies potential multi targeting and given that legacy projects can't multi target, it's not a scenario that's guaranteed to work.

sae42 commented 4 years ago

@nkolev92 The NetCorePackageReferenceProject is picked based on the project system being 'CPS' as far as I can tell. My custom project system is not CPS based. Responding as such via project capabilities would just lead to problems elsewhere. I can certainly use the NominateProjectAsync API that passes in the restore info, but as I mentioned above the properties set are not being honored when writing out the package spec.

We're gonna change that in NuGet/NuGet.Client#3644.

The reason why the alias is not written out is due to the fact that legacy projects don't support multi targeting, and SDK projects do.

That's why it's important that SDK based projects are detected as NetCorePackageReferenceProject.

Notwithstanding this particular issue, I think the problem of a C# .NET Core 16.7 project failing to build in 16.8 Pr3 without having first done a restore is also a hole that needs filling. So the question could come down to shouldn't the build be more tolerant of the missing 'TargetAlias' property?

Unfortunately it seems like it used to work by accident.

I don't quite follow this. If I create a C# .NET Console app in VS 16.7, do a build - everything is good. This is a single target (e.g. netcoreapp3.1 in the csproj). Now open a 16.8 dev prompt and go to the project folder and execute msbuild.exe. You'll get a build fail. Are you saying that's expected/acceptable? Fine if so, but it would be good to clarify.

SDK projects need to go through that NetCorePackageReferenceProject (Soon CPSPackageReferenceProject) workflow.

An alias to NuGet implies potential multi targeting and given that legacy projects can't multi target, it's not a scenario that's guaranteed to work.

zivkan commented 4 years ago

I don't quite follow this. If I create a C# .NET Console app in VS 16.7, do a build - everything is good. This is a single target (e.g. netcoreapp3.1 in the csproj). Now open a 16.8 dev prompt and go to the project folder and execute msbuild.exe. You'll get a build fail. Are you saying that's expected/acceptable? Fine if so, but it would be good to clarify.

I don't follow your scenario. netcoreapp3.1 projects can only be SDK style, hence get loaded using the CPS "flow".

Nikolche's comments about something that worked by accident refers to non-SDK style project ("legacy" projects) allowing/working with nominations. NuGet's nomination API was created for SDK style projects. PackageReference in non-SDK ("legacy") projects do not use nominations when using the default project system.

Maybe I read through this issue too quickly and missed it, or maybe I just don't have enough background knowledge, but if we understand why you're calling NominateProject, we can suggest how to do it in a way that's compatible, and designed, for non-SDK style projects. If it's about restoring the project, maybe IVsPackageRestorer does what you want?

Unfortunately NuGet's docs for VS extensibility are very limited (I don't believe we've documented NominateProject at all). Frankly, I don't believe that we've designed NuGet's VS extensibility to be generic. I feel like we're very tied to specific VS project systems, making it very difficult, or just about impossible, for 3rd parties to create their own project system with NuGet support. But that's my personal opinion, and I'm certainly not knowledgeable about the finer details about how all of NuGet's VS extensibility works.

sae42 commented 4 years ago

@zivkan I definitely think there is some confusion here. I'll try to summarize from my perspective as a bunch of bullet points:

  1. The example above (C# .NET Core project) is to demonstrate a build fail when moving from 16.7 to 16.8. You should be able to repro. The reason I added that example is that it shows the build issue I'm getting without needing anything from me.

  2. The reason I'm getting the same issue is that the NuGet VS support wraps my custom project as a legacy type, rather than NetCore (soon to be CPS). That leads to a project.assets.json file aligned with 16.7 rather than 16.8 and the problem in 1).

  3. My non-CPS based project system is using SDK-Style projects for this scenario. A project system does not have to be CPS-based to handle SDK-Style MSBuild projects. My project system does not support multi-targeting, but I do support Package refs and targeting .NET Core.

  4. I get that the Nominate api is only really getting tested/used from CPS projects (or until recently I guess that's really just C# .NET Core projects). I was not aware that the Nominate apis should only be used from CPS projects though. I could just switch to running the MSBuild restore target to resolve the problem, but it would be a pity to not be able to continue to use the Nominate api inside VS. Back to point 1) above - if that problem was fixed, I suspect my issue would go away. It may be that the issue is more in the SDK build rather than the VS integration.

Hope that helps clarify things.

zivkan commented 4 years ago
  1. The example above (C# .NET Core project) is to demonstrate a build fail when moving from 16.7 to 16.8.

Ah, I understand now. In this example, you restored and built using the 16.7 tools, then you tried to build, without restore, using the 16.8 tools. Unfortunately yes, I would say this is expected.

On the command line, dotnet build restores automatically before build, it's just msbuild.exe that does not restore automatically. In VS, CPS based projects get restored after nomination (project load). But non-SDK ("legacy") projects only get restored on build, or if the user explicitly selects the restore command.

Using Visual Studio's built-in project systems I would expect this to work, no errors. Regardless of a CPS or "legacy" project, NuGet will always restore before building.

Does your custom project system support projects targeting netcoreapp3.1, despite not being a CPS project?

nkolev92 commented 4 years ago

@zivkan The problem is the alias addition. There's no aliases in LegacyPackageReference paths, and the SDK looks up things based on the alias.

So it has to go through the specific CPS/SDK PackageReference codepath.

I'll write-up a more detailed explanation later.

sae42 commented 4 years ago
  1. The example above (C# .NET Core project) is to demonstrate a build fail when moving from 16.7 to 16.8.

Ah, I understand now. In this example, you restored and built using the 16.7 tools, then you tried to build, without restore, using the 16.8 tools. Unfortunately yes, I would say this is expected.

Yep, that's exactly right.

On the command line, dotnet build restores automatically before build, it's just msbuild.exe that does not restore automatically. In VS, CPS based projects get restored after nomination (project load). But non-SDK ("legacy") projects only get restored on build, or if the user explicitly selects the restore command.

Right, so I'm trying to use Nominate* in a non-CPS project which doesn't update the assets file (the VS restore command didn't do much either).

Using Visual Studio's built-in project systems I would expect this to work, no errors. Regardless of a CPS or "legacy" project, NuGet will always restore before building.

Does your custom project system support projects targeting netcoreapp3.1, despite not being a CPS project?

So would I, CPS should really have nothing to do with this - that's just a boilerplate implementation of a VS project system, not whether that project system supports SDK-Style MSBuild, NuGet package refs or whatever. Legacy is not a great name, but whatever the non-CPS project variety is, it would be good if it generated an assets file compatible with 16.8. As far as I can tell that requires the assets file to have a TargetAlias property (irrespective of whether the project is multi-targeting or not).

zivkan commented 4 years ago

The problem is the alias addition. There's no aliases in LegacyPackageReference paths, and the SDK looks up things based on the alias. So it has to go through the specific CPS/SDK PackageReference codepath.

This is what I was trying to get at. I wasn't sure if @sae42 was using the netcoreapp3.1 project being built with 16.8, but a 16.7 assets file was a scenario that their custom project type actually attempts, or if they just found a different scenario that has a similar error message.

it would be good if it generated an assets file compatible with 16.8

The assets file hasn't changed schema often, so normally this isn't a problem, but net5 TFM changes required the new property.

However, given that the assets file is a temporary build artifact, I personally think it's reasonable for the build system to expect the file to have been generated by bundled version of NuGet, and not some older version of NuGet. When NuGet does a restore, it tries to be as "incremental" as possible, so it will not overwrite the assets file if it detects no changes, but in this case upgrading from 16.7 to 16.8 should require an update to the assets file, hence a restore is necessary.

Something that I either missed, or I think is missing from this issue, is to understand why your custom project system has been calling nominate in the past. VS's non-CPS .NET project system doesn't restore on project open, only on a pre-build event. Does your custom project system also restore correctly on build? Are you calling nominate because you want restore to happen earlier?

I'm wondering if we have one or more X-Y problems here. The original post says that calling nominate for projects that NuGet detects as LegacyPackageReferenceProject is no longer working. But I don't know what problem is trying to be solved by calling nominate, and therefore I don't know if trying to call nominate is the best solution (although for LegacyPackageReferenceProject, it was never designed to work this way).

sae42 commented 4 years ago

The problem is the alias addition. There's no aliases in LegacyPackageReference paths, and the SDK looks up things based on the alias. So it has to go through the specific CPS/SDK PackageReference codepath.

This is what I was trying to get at. I wasn't sure if @sae42 was using the netcoreapp3.1 project being built with 16.8, but a 16.7 assets file was a scenario that their custom project type actually attempts, or if they just found a different scenario that has a similar error message.

it would be good if it generated an assets file compatible with 16.8

The assets file hasn't changed schema often, so normally this isn't a problem, but net5 TFM changes required the new property.

However, given that the assets file is a temporary build artifact, I personally think it's reasonable for the build system to expect the file to have been generated by bundled version of NuGet, and not some older version of NuGet. When NuGet does a restore, it tries to be as "incremental" as possible, so it will not overwrite the assets file if it detects no changes, but in this case upgrading from 16.7 to 16.8 should require an update to the assets file, hence a restore is necessary.

Something that I either missed, or I think is missing from this issue, is to understand why your custom project system has been calling nominate in the past. VS's non-CPS .NET project system doesn't restore on project open, only on a pre-build event. Does your custom project system also restore correctly on build? Are you calling nominate because you want restore to happen earlier?

Yes, Nominate is called after project load since we require package refs to be available/resolved and usable for real-time syntax checking.

I'm wondering if we have one or more X-Y problems here. The original post says that calling nominate for projects that NuGet detects as LegacyPackageReferenceProject is no longer working. But I don't know what problem is trying to be solved by calling nominate, and therefore I don't know if trying to call nominate is the best solution (although for LegacyPackageReferenceProject, it was never designed to work this way).

From the sound of it, this worked by chance - 16.7 and earlier generated a usable assets file. I'm still unclear on the purpose of the 'TargetAlias' property or whether for example the build should be more tolerant of it being missing (e.g. can it be inferred/defaulted?). Hopefully @nkolev92 can include this in his explanation?

nkolev92 commented 4 years ago

Yes, Nominate is called after project load since we require package refs to be available/resolved and usable for real-time syntax checking.

@sae42 Can you please point to which exact nominate API is getting called? :D We got 3! :D The 3 are: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.SolutionRestoreManager.Interop/IVsSolutionRestoreService.cs, https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.SolutionRestoreManager.Interop/IVsSolutionRestoreService2.cs, https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.SolutionRestoreManager.Interop/IVsSolutionRestoreService3.cs, and 1 and 3 are fundamentally different from 2.

From the sound of it, this worked by chance - 16.7 and earlier generated a usable assets file. I'm still unclear on the purpose of the 'TargetAlias' property or whether for example the build should be more tolerant of it being missing (e.g. can it be inferred/defaulted?). Hopefully @nkolev92 can include this in his explanation?

Yep it was working by chance.

Rough description about the alias:

NuGet the SDK have a contract through the assets file. It's designed to support multi targeting.

Prior to NET5.0, at restore time, NuGet would read the TargetFramework and write the assets file with that specific to that. At build time, the SDK would read the TargetFrameworkMoniker (which always matched the TargetFramework) and discover the correct assets to build with for that framework.

In NET5.0, the TargetPlatformMoniker was added to the framework representation. To represent a framework that NuGet & SDK would use to restore force/build for, we'd need to read both TargetFrameworkMoniker & TargetPlatformMoniker, so the contract between NuGet & the SDK now is to determine the applicable assets based on the TargetFramework (which is the alias).

So that's why the new SDK won't work if the assets file doesn't have an alias.

e.g. can it be inferred/defaulted?

Yep, this is what I've been trying to work out. The property is meant to not be interpreted, https://github.com/dotnet/project-system/blob/master/docs/repo/coding-conventions.md#data.
So in order for it to work, it'd need to be the exact string value.

On the other hand, the LegacyPackageReferenceProject codepath, always does only 1 framework and it's frameworks are represented by TargetFrameworkMoniker, one property, so the lookup there is simpler.

Simply put:

LegacyPackageReferenceProject -> assets file without aliases, read by https://github.com/dotnet/nuget.buildtasks. NetCorePackageReferenceProject -> assets file with aliases due to multi targeting, read by https://github.com/dotnet/sdk.

sae42 commented 4 years ago

@nkolev92 I'm currently using https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.SolutionRestoreManager.Interop/IVsSolutionRestoreService2.cs, (Number 2 - The one without an IVsProjectRestoreInfo). I have also tried using https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.SolutionRestoreManager.Interop/IVsSolutionRestoreService2.cs, with a quick and dirty implementation of IVsProjectRestoreInfo as described in the original issue description since I hoped this might give a route out of this problem. While I get called for a bunch of properties, TargetFramework is of course not one of them. I'm happy to switch to this if it can be adapted to resolve this issue.

nkolev92 commented 3 years ago

@sae42

There's 2 things that need to work a certain way.

Does your project-system implement VSProject4 or something like that?

Can customers use the PM UI to add packages to your project types?

sae42 commented 3 years ago

@nkolev92

Yep, our project system implements VSProject4 in order to supply the package refs. The NuGet PM UI all works well. We've supported this for several years (back to packages.config around VS2015 I think). We support SDK-Style projects in the same project system since enabling support for .NET Core 2.1.

nkolev92 commented 3 years ago

I see. Unfortunately the SDK projects experience is tightly coupled with CPS and the usage of the NetCorePackageReferenceProject.

The way to avoid future problems would be to move to a CPS based project system, one that does nominations for SDK projects.

What are the things that your project-system provides that the default project system does?

cc @davkean as he owns the project-system PackageReference nomination code.

sae42 commented 3 years ago

I see. Unfortunately the SDK projects experience is tightly coupled with CPS and the usage of the NetCorePackageReferenceProject.

The way to avoid future problems would be to move to a CPS based project system, one that does nominations for SDK projects.

What are the things that your project-system provides that the default project system does?

@nkolev92 I'm not sure I understand the question. My project system is functionally equivalent to csproj.dll + support for .NET Core/SDK-Style MSBuild projects (e.g. the References node has been extended to support dependencies/package refs etc). Rewriting this to CPS is not trivial (though remains a long term aim).

cc @davkean as he owns the project-system PackageReference nomination code.

BlairMcc commented 3 years ago

I came here from Google and am unable contribute to this discussion, but I noticed this issue on publishing some of our netcoreapp3.1 APIs that are restored using NuGet.exe v5.6 and MSBuild (others that used dotnet cli were okay)

In the end I resolved the ones that were failing by using a --force in the publish command

nkolev92 commented 3 years ago

@nkolev92 I'm not sure I understand the question. My project system is functionally equivalent to csproj.dll + support for .NET Core/SDK-Style MSBuild projects (e.g. the References node has been extended to support dependencies/package refs etc). Rewriting this to CPS is not trivial (though remains a long term aim).

For better or worse, on NuGet side, the SDK projects are synonymous with the NetCorePackageReference type and the CPS based nomination approach.

The 2 PackageReference approaches are pretty similar and while we've spent a lot of time to make them as close as possible, they still heavily rely on going through the right components.

NuGet needs the inner build value of the TargetFramework property to add as the alias. The LegacyPackageReferenceProject can't do that because it doesn't know about a concept like that.