AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 288 forks source link

Nuget packages with multiple targetframeworks not working properly #709

Closed Barsonax closed 5 years ago

Barsonax commented 5 years ago

Summary

When installing a plugin which has multiple targetframeworks in the package/metadata/dependencies node duality can pick the wrong framework and then complain about not being able to resolve dependencies. This can happen when it picks the .NETStandard1.1 framework for instance. A example of a nuspec with multiple targetframeworks can be found below:

    <dependencies>
      <group targetFramework=".NETFramework4.5.2" />
      <group targetFramework=".NETStandard1.1">
        <dependency id="NETStandard.Library" version="1.6.1" exclude="Build,Analyzers" />
      </group>
    </dependencies>

If you try to install such a package you will get a similar error to the following:

[Edit] Msg: Installing package 'Singularity.Duality.core 1.0.0.0'...
[Edit] Msg:   Attempting to resolve dependency 'Singularity (≥ 1.0.0)'.
[Edit] Msg:   Attempting to resolve dependency 'NETStandard.Library (≥ 1.6.1)'.
[Edit] ERR: An error occurred while processing Package 'Singularity.Duality.core', Version 1.0.0.0: InvalidOperationException: Unable to resolve dependency 'NETStandard.Library (≥ 1.6.1)'.
            CallStack:
               at NuGet.InstallWalker.OnDependencyResolveError(PackageDependency dependency)
               at NuGet.PackageWalker.Walk(IPackage package)
               at NuGet.PackageWalker.Walk(IPackage package)
               at NuGet.InstallWalker.ResolveOperations(IPackage package)
               at NuGet.PackageManager.Execute(IPackage package, IPackageOperationResolver resolver)
               at NuGet.PackageManager.InstallPackage(IPackage package, Boolean ignoreDependencies, Boolean allowPrereleaseVersions)
               at Duality.Editor.PackageManagement.PackageManager.InstallPackage(PackageName packageName, Boolean skipLicense)
               at Duality.Editor.Plugins.PackageManagerFrontend.PackageViewDialog.<>c.<InstallPackage>b__24_1(PackageOperationData d) in C:\projects\duality\Source\Plugins\EditorModules\PackageManagerFrontend\Modules\PackageViewDialog.cs:line 338
               at Duality.Editor.Plugins.PackageManagerFrontend.PackageViewDialog.<PackageOperationThread>d__58.MoveNext() in C:\projects\duality\Source\Plugins\EditorModules\PackageManagerFrontend\Modules\PackageViewDialog.cs:line 761

The problem seems to be that the packagemanager doesn't know what framework it should be looking for so instead of picking the .NETFramework4.5.2 it picks the .NETStandard1.1. I confirmed this by decompiling the InstallPackage method in Nuget.Core:

    public virtual void InstallPackage(IPackage package, bool ignoreDependencies, bool allowPrereleaseVersions)
    {
      FrameworkName targetFramework = (FrameworkName) null;
      bool ignoreDependencies1 = ignoreDependencies;
      bool allowPrereleaseVersions1 = allowPrereleaseVersions;
      this.InstallPackage(package, targetFramework, ignoreDependencies1, allowPrereleaseVersions1, false);
    }

Luckily this method is virtual and as such we can override it:

    public class NugetManager : NuGet.PackageManager
    {
        public NugetManager(IPackageRepository sourceRepository, string path) : base(sourceRepository, path)
        {
        }

        public NugetManager(IPackageRepository sourceRepository, IPackagePathResolver pathResolver, IFileSystem fileSystem) : base(sourceRepository, pathResolver, fileSystem)
        {
        }

        public NugetManager(IPackageRepository sourceRepository, IPackagePathResolver pathResolver, IFileSystem fileSystem, IPackageRepository localRepository) : base(sourceRepository, pathResolver, fileSystem, localRepository)
        {
        }

        public override void InstallPackage(IPackage package, bool ignoreDependencies, bool allowPrereleaseVersions)
        {
            var targetFramework = new FrameworkName(".NETFramework", new Version(4,5,2));
            this.InstallPackage(package, targetFramework, ignoreDependencies, allowPrereleaseVersions, false);
        }
    }

Then replace the nuget packagemanager with our own derived class here

With the above modification duality succeeds in installing the package. So I think the solution is to derive from NuGet.PackageManager and override the methods in order to pass in the correct targetframework.

Note there is a overload of InstallPackage which does accept a targetframework but its protected:

protected void InstallPackage(IPackage package, FrameworkName targetFramework, bool ignoreDependencies, bool allowPrereleaseVersions, bool ignoreWalkInfo = false);

How to reproduce

  1. First create a folder and copy the test nupkg files to it. Make sure the test package has multiple targetframeworks defined (I used .NETFramework4.5.2 and .NETStandard1.1). If one of the test packages has dependencies you need to include them all: image
  2. Make a clean build of duality
  3. Go to Build\Output and open up PackageConfig.xml to make sure the RepositoryUrl points to the folder we created in step 1.
  4. Start duality and install one of the testpackages. You should get a popup with a error.

Workaround

ilexp commented 5 years ago

Great writeup and analysis 👍

I'm flagging this as up for grabs, since you already proposed a fix that looks self-contained enough for a collaborative PR, maybe with a few iterations extra due to the importance of package management.

Barsonax commented 5 years ago

Unfortunately for some reason the Update method is not virtual and also needs the targetframework. Thus the final solution will contain some less desirable code such as hiding methods with new.

Ugh it could have been so clean...

Barsonax commented 5 years ago

Made a PR: #710

ilexp commented 5 years ago

Fixed by @Barsonax in #710. Release in progress. Closing this.