dotnet / project-system

The .NET Project System for Visual Studio
MIT License
969 stars 387 forks source link

Package restore should be using NuGetTargetMoniker if its exists over TargetFrameworkMoniker #4854

Closed davkean closed 5 years ago

davkean commented 5 years ago

For all projects except UWP, NuGetTargetMoniker is equivalent to TargetFrameworkMoniker. However, in UWP, NuGetTargetMoniker represents the TargetPlatformMoniker: https://github.com/dotnet/sdk/blob/bb764dea3b6b7173522688fffe0f48640d714833/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets#L46.

This is what gets passed to NuGet in command-line restore.

In 16.1 we started using TargetFrameworkMoniker vs TargetFramework so that we can support projects that are not-Microsoft.NET.SDK based, this broke folks using MSBuild.Extras. We should change this to report NuGetTargetMoniker if it exists followed by TargetFrameworkMoniker.

MSiccDev commented 5 years ago

Any ideas to work around this until it gets fixed and rolled out? This completely blocks me from working on my current project.

1Cor125 commented 5 years ago

Also blocked on this.

davkean commented 5 years ago

I'm not aware of a workaround at the moment, for those with paid subscriptions can access previous versions of VS via: https://my.visualstudio.com/Downloads?q=Visual%20Studio%202019.

davkean commented 5 years ago

We are planning on shipping an update to 16.1 soon with a fix for this.

davkean commented 5 years ago

This fix will appear in 16.1.2.

dotMorten commented 5 years ago

Thank you @davkean for being awesome as usual 👍

WonyoungChoi commented 5 years ago

@davkean I'd like to know more about why you use the TargetFrameworkMoniker instead of the TargetFramework. In fact, this change is affecting the Tizen.NET projects. Tizen.NET projects typically have the following csproj:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>tizen40</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Tizen.NET" Version="4.0.0" />
  </ItemGroup>
</Project>

Of course, Microsoft.NET.Sdk doesn't handle "tizen TFM.", So TargetFrameworkIdentifier is defined in the build/props of Tizen.NET nuget package.

However, from 16.1.0 this method does not work. Because the TargetFrameworkMoniker is "_,Version=4.0" in this case. With following exception:

System.AggregateException: One or more errors occurred. ---> NuGet.Frameworks.FrameworkException: Invalid framework identifier ''.
   at NuGet.Frameworks.NuGetFramework.GetShortFolderName(IFrameworkNameProvider mappings)
   at NuGet.SolutionRestoreManager.VsSolutionRestoreService.<>c.<ToPackageSpec>b__11_0(TargetFrameworkInformation tfi)
   at System.Linq.Enumerable.WhereSelectArrayIterator`2.MoveNext()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
   at NuGet.SolutionRestoreManager.VsSolutionRestoreService.ToPackageSpec(ProjectNames projectNames, IEnumerable TargetFrameworks, String originalTargetFrameworkstr, String msbuildProjectExtensionsPath)
   at NuGet.SolutionRestoreManager.VsSolutionRestoreService.ToDependencyGraphSpec(ProjectNames projectNames, IVsProjectRestoreInfo projectRestoreInfo, IVsProjectRestoreInfo2 projectRestoreInfo2)
   at NuGet.SolutionRestoreManager.VsSolutionRestoreService.NominateProjectAsync(String projectUniqueName, IVsProjectRestoreInfo projectRestoreInfo, IVsProjectRestoreInfo2 projectRestoreInfo2, CancellationToken token)
   at NuGet.SolutionRestoreManager.VsSolutionRestoreService.NominateProjectAsync(String projectUniqueName, IVsProjectRestoreInfo2 projectRestoreInfo, CancellationToken token)
   at Microsoft.VisualStudio.ProjectSystem.VS.PackageRestore.PackageRestoreInitiator.PackageRestoreInitiatorInstance.<NominateProjectRestoreAsync>d__12.MoveNext()

Our approach was a little strange, so we have a plan to change the Tizen.NET.Sdk to a custom sdk style. However, it takes time to modify all Tizen.NET projects and sample applications.

davkean commented 5 years ago

Similar to Configuration/Platform properties, TargetFramework is a user controlled concept, and simply a user shortcut for writing the TargetFrameworkIdentifier and TargetFrameworkVersion properties which are the real handshake between components. A 1st party/3rd party provider of a "Target Framework" is supposed to take the <TargetFramework> and automatically convert it to those properties, which is then consumed by other components such as NuGet restore and project-to-project references. No component is supposed to consume TargetFramework as a rare value. Users are allowed to call this "foobar" and as long as TargetFrameworkIdentifier/TargetFrameworkVersion is set, this is supposed to work.

To support project types other those that use Microsoft.NET.Sdk, we moved to using TargetFrameworkMoniker (and now NuGetTargetMoniker) which is supposed to be functionality equivalent to TargetFramework. That is how we designed it.

It appears that Tizen is attempting to set the TargetFrameworkIdentifier after package restore (package props contains):

  <PropertyGroup>
    <TargetFrameworkIdentifier>Tizen</TargetFrameworkIdentifier>
    <_IsNETCoreOrNETStandard>true</_IsNETCoreOrNETStandard>
  </PropertyGroup>

You can see how that above project is already problematic before the above change when you open a project before package restore has finished, and the design-time build fails with the following:

Severity    Code    Description Project File    Line    Suppression State
Error   MSB4018 The "ResolvePackageAssets" task failed unexpectedly.
NuGet.Frameworks.FrameworkException: Invalid framework identifier ''.
   at NuGet.Frameworks.NuGetFramework.GetShortFolderName(IFrameworkNameProvider mappings)
   at Microsoft.NET.Build.Tasks.LockFileExtensions.GetTargetAndThrowIfNotFound(LockFile lockFile, NuGetFramework framework, String runtime)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheWriter..ctor(ResolvePackageAssets task, Stream stream)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheReader.CreateReaderFromDisk(ResolvePackageAssets task, Byte[] settingsHash)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheReader..ctor(ResolvePackageAssets task)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.ReadItemGroups()
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.ExecuteCore()
   at Microsoft.NET.Build.Tasks.TaskBase.Execute()
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext() ConsoleApp423   C:\Program Files\dotnet\sdk\2.1.507\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets    198 

There will be other fallout around this too - P2P's validation will broken, and anything that relies on TargetFrameworkIdentiifer to be set before package restore has finished will get the incorrect result/fail. This is the entire reason that SDKs were invented; you can't set critical information needed for a project in a package if you want VS to function correctly.

It sounds like we need to get a thread on the correct way to resolve this. Who have you worked with in the past? cc @nguerrera

WonyoungChoi commented 5 years ago

@davkean Thank you for your kind explanation. I don't think we need another thread for this issue. We will change our package to custom sdk style such as MSBuild.Sdk.Extras.

tomh4 commented 5 years ago

@WonyoungChoi any ideas how to get things working for Tizen again? Im stuck at this issue

WonyoungChoi commented 5 years ago

@tomh4 In workaround solution, you can add following lines to your .csproj file.

  <PropertyGroup>
    <TargetFrameworkIdentifier>Tizen</TargetFrameworkIdentifier>
  </PropertyGroup>

We are preparing the next version of Tizen.NET.Sdk as a SDK-style. After released, you can re-write your csproj file as follows:

<Project Sdk="Tizen.NET.Sdk/x.x.x">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>tizen40</TargetFramework>
  </PropertyGroup>

</Project>
jalbertSyncroTech commented 4 years ago

We're seeing this again in the latest VS 16.5.4 release.

There's a corresponding report in MSBuild.Sdk.Extras https://github.com/novotnyllc/MSBuildSdkExtras/issues/166#issuecomment-612650318

jalbertSyncroTech commented 4 years ago

@davkean Any updates on this?