Added targetframeworks support to the duality package manager

[Barsonax]

TODO:

• [x] Get nuget to work properly with packages that contain multiple targetframeworks
• [x] Add targetframeworks support to the unit test mockups
• [x] Add unit tests to test packages with multiple frameworks

Possible improvements:

• [ ] The targetframework (.NET 4.5.2) is hardcoded.
• [ ] Maybe there is a cleaner way for doing this. Open to suggestions.
• [ ] Update Nuget.Core to 2.14 to also support targets like netstandard (could be done in a separate PR though). In my test I found no breaking changes.

Changes:

• Decompiled the nuget PackageManager and copied it over to our solution. Cleaned up the class afterwards and added the code we need for targetframework functionality. I didnt see another way to do this due to shortcomings in the code of nuget. Its 250 lines but might be cleaned up a bit more as we don't use everything.

[Barsonax]

When you install a package that triggers a update of another package nuget will internally use the update method. Since we didnt override this update method but hidden it it uses the update method without the targetframework and thus it fails to correctly install the dependency.

I don't think this can be fixed with our current approach. We would need to make a copy of the source of nuget to modify it to our needs.

Still amazes me why nuget makes only the install method virtual but not the update method and simply passes in a null targetframework...

[Barsonax]

Copied over the whole PackageManager from nuget and did the small modifications we needed to make it work.

Running into some issues regarding the unit tests with the nuget framework (again...). Apparently when creating the package for the unit tests it sets the targetframework to a invalid one even though we supply a correct one. This has to do with the fact that nuget uses IPackageMetadata instead of ManifestMetadata as the type. When it retrieves the metadata it doesnt simply return DependencySets but it uses IPackageMetadata.DependencySets which has this weird code:

    IEnumerable<PackageDependencySet> IPackageMetadata.DependencySets
    {
      get
      {
        if (this.DependencySets == null)
          return Enumerable.Empty<PackageDependencySet>();
        List<PackageDependencySet> list = this.DependencySets.Select<ManifestDependencySet, PackageDependencySet>(new Func<ManifestDependencySet, PackageDependencySet>(ManifestMetadata.CreatePackageDependencySet)).GroupBy<PackageDependencySet, FrameworkName>((Func<PackageDependencySet, FrameworkName>) (set => set.TargetFramework)).Select<IGrouping<FrameworkName, PackageDependencySet>, PackageDependencySet>((Func<IGrouping<FrameworkName, PackageDependencySet>, PackageDependencySet>) (group => new PackageDependencySet(group.Key, group.SelectMany<PackageDependencySet, PackageDependency>((Func<PackageDependencySet, IEnumerable<PackageDependency>>) (g => (IEnumerable<PackageDependency>) g.Dependencies))))).ToList<PackageDependencySet>();
        int index = list.FindIndex((Predicate<PackageDependencySet>) (set => set.TargetFramework == (FrameworkName) null));
        if (index > -1)
        {
          PackageDependencySet packageDependencySet = list[index];
          list.RemoveAt(index);
          list.Insert(0, packageDependencySet);
        }
        return (IEnumerable<PackageDependencySet>) list;
      }
    }

Which eventually calls into VersionUtility.ParseFrameworkName which simply fails to parse the framework even though we are passing in a correct one (actually tested this). In fact its even unable to parse even VersionUtility.DefaultTargetFramework....

Wouldnt have expected the nuget library to be that bugged.

EDIT: after some more digging around I found there is a VersionUtility.GetFrameworkString which solves the above problem. So I now have arrived at a situation where all the unit tests are using the net452 (unless explicitly stated otherwise) target and they all complete succesfully. Next step is actually adding new tests to specifically test multi framework packages.

[Barsonax]

Added some unit tests. Some things I noticed:

1. Nuget is still unable to properly parse .NetStandard targetframeworks from a nuspec. It will see it as VersionUtility.UnsupportedFrameworkName which is this new FrameworkName("Unsupported", new Version()).
2. Nuget is still unable to resolve a NetStandard dependency due to the above
3. Because of the above this fix does not enable duality to consume 'Netstandard' packages. It will make duality prefer the net452 target so a multi framework package that also targets net452 is now fine.

Possible fix for 1 and 2 might be updating nuget to atleast v2.12. This page says that version should support netstandard. Maybe we won't even need the current modifications?

EDIT: updated Nuget.Core to 2.14 (the latest version) as a test and now it does correctly parse netstandard as a target! With this change in theory all you have to do is change the targetframework you pass to our modified packagemanager to netstandard and it should then install the netstandard versions of the packages. We still need our custom NuGetTargetedPackageManager class though.

[ilexp]

Wow, wouldn't have expected that many additional issues popping up here. Glad to see you made it through regardless 👍

So I now have arrived at a situation where all the unit tests are using the net452 (unless explicitly stated otherwise) target and they all complete succesfully. Next step is actually adding new tests to specifically test multi framework packages.

Added some unit tests.

Means PR ready for the next review iteration?

Possible fix for 1 and 2 might be updating nuget to atleast v2.12. This page says that version should support netstandard. Maybe we won't even need the current modifications?

EDIT: updated Nuget.Core to 2.14 (the latest version) as a test and now it does correctly parse netstandard as a target! With this change in theory all you have to do is change the targetframework you pass to our modified packagemanager to netstandard and it should then install the netstandard versions of the packages. We still need our custom NuGetTargetedPackageManager class though.

Great find. We should tackle an internal NuGet update to 2.14 in a separate PR.

[Barsonax]

Wow, wouldn't have expected that many additional issues popping up here. Glad to see you made it through regardless 👍

Didn't expect the nuget library to have that many issues either. Not the best code I have seen, by a long shot.

Means PR ready for the next review iteration?

Aye, this monday I will go on vacation for a month though so if there is anything left to do at that point it either has to wait or someone else (probably you :P) would have to do it.

Great find. We should tackle an internal NuGet update to 2.14 in a separate PR.

Made the issue already: #711

[ilexp]

Aye, this monday I will go on vacation for a month though so if there is anything left to do at that point it either has to wait or someone else (probably you :P) would have to do it.

Noted, thanks for the heads up. Probably not enough time from my side to fast-track this together with you this weekend, so I'll either pick this up in the mean time, or prepare the next review for when you return. Probably not that time critical for now.

Made the issue already: #711

👍

[ilexp]

Started doing a few tweaks and fixes. As a side note, there are a lot of unrelated whitespace changes in this PR that make it hard to distinguish what actually changed - please try to avoid that kind of noise in future PRs.

One thing that remains an issue is unit test cases for .NET Standard target framework folders failing to set up properly, as .NET Standard is an unknown framework with the current NuGet version. This will be fixed in issue #711 - however, those unit tests need to remain operational until then.

As the issue was introduced by changing MockPackageSpec to work on parsed NuGet FrameworkName instead of a string identifier directly, my approach here would be to revert that change and operate based on string identifiers again. They are converted to string when talking to NuGet anyway, and I think there's no reason to have this soft dependency in there, especially for unit test cases that might also intentionally use custom or unsupported framework folders which are by definition unknown to NuGet.

Progress

• A few whitespace tweaks.
• Fixed Portable framework folder names in two cases where the conversion to FrameworkName-based mock specs accidentally switched them to .NET Framework 4.5.

ToDo

• Revert back to string-based framework folders and identifiers in mock package specs, and update their new framework-dependent dependencies accordingly. Consider not using DependencySet for that, as that's not a test-specific class where validating framework names makes more sense.
• Consider removing DependencySet, if it is no longer used after the above test change.
• Consider simplifying NuGetTargetedPackageManager by removing unused parts, if any.
• Test real-world package install with the new changes.
• Review code again.

[ilexp]

Progress

• Moved back to string-based target frameworks in MockPackageSpec as discussed above.
• Removed now-unused DependencySet class.
• Moved NuGetTargetedPackageManager to Internal subfolder and -namespace.

ToDo

• Consider simplifying NuGetTargetedPackageManager by removing unused parts, if any.
• Test real-world package install with the new changes.
• Review code again.

[Barsonax]

As a side note, there are a lot of unrelated whitespace changes in this PR that make it hard to distinguish what actually changed - please try to avoid that kind of noise in future PRs.

Iam not sure why I get these whitespace changes. Was the file not normalized yet in git or is this some setting in my IDE that causes this?

[ilexp]

Iam not sure why I get these whitespace changes. Was the file not normalized yet in git or is this some setting in my IDE that causes this?

Not sure how this happened, but if your IDE is doing this it should show up in the git diff before you commit. My guess would be some auto format setting?

[Barsonax]

Will have to check that when iam back home

[ilexp]

Tested self-install, package install and uninstall in a local setup, everything worked as expected. Tests are all green too, so no regressions expected. Merging this and triggering a binary release.