Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make PackageManager More Reliable Across Various Package Configurations #703

Closed
ilexp opened this issue Dec 9, 2018 · 6 comments
Closed
Assignees
Labels
Bug It's broken and should be fixed Editor Area: Duality editor or support libraries PackageManager Area: Editor package management or its UI
Milestone

Comments

@ilexp
Copy link
Member

ilexp commented Dec 9, 2018

Summary

The current PackageManager implementation does not explicitly handle NuGet target framework folders, causing some packages to fail installation in Duality. The PackageManager should be extended to have a clear framework and version preference, leading to more reliable and predictable results for those packages.

How to reproduce

  • Attempting to install a package where all files are located in a target framework folder with a defined version number exceeding 4.0 will result in no files being copied to the Plugins folder.
  • Other cases may apply as well.

Workaround

  • For Duality specific packages, do not use NuGet target framework folders for the time being.
  • For non-Duality or shared packages, ensure that there is a net40 or lower target framework folder.

Analysis

  • The outlined problems are entirely caused by the PackageManager not evaluating target frameworks the way it should do. It is essentially a not implemented feature behaving like a bug. Implement this.
@ilexp ilexp added Bug It's broken and should be fixed Editor Area: Duality editor or support libraries Plugin Area: Misc. core / editor plugins labels Dec 9, 2018
@ilexp ilexp added this to the General milestone Dec 9, 2018
@ilexp ilexp self-assigned this Dec 9, 2018
@ilexp
Copy link
Member Author

ilexp commented Dec 9, 2018

Progress

  • Started working on this in the feature/package-framework-selection branch.
  • Extended the internal file mapping algorithm to actively select the target framework to use, as well as differentiate between framework dependent (lib) and framework independent (content, source) files.
  • Added more PackageManager tests to verify the correct behavior.
  • Patched an unrelated threading bug in PackageManager sometimes causing a License Acceptance dialog to be displayed on install for unrelated packages.

ToDo

  • Check if the newly added internal operation locks are a viable option for long background operations in UI updates blocking an install. If not, consider a different solution.
  • Thoroughly test the new installation behavior:
    • A fresh Duality install, but with the new binaries
    • Installing and uninstalling various sample packages, preferably with dependencies
    • Uninstalling an re-installing editor module packages
  • Merge to master
  • Merge to release
  • Post-release testing

@ilexp
Copy link
Member Author

ilexp commented Dec 9, 2018

Related: Issue #696

@Barsonax
Copy link
Member

Barsonax commented Dec 9, 2018

Hmm I was under the impression we needed to upgrade nuget in order to support targetframework folders but it seems you have a solution that can do this without upgrading nuget? Nice work!

@ilexp
Copy link
Member Author

ilexp commented Dec 9, 2018

Hmm I was under the impression we needed to upgrade nuget in order to support targetframework folders but it seems you have a solution that can do this without upgrading nuget? Nice work!

Limited support only, so you're correct when thinking about the full deal.

What we get is an explicit priority decision between "old" frameworks such as .NET Framework or PCLs profiles, as well as explicit handling of per-framework files within a package. However, new frameworks such as .NET Standard will only show up as "Unsupported, Version 0.0" and while it would be possible to do custom parsing of the lib folder to distinguish them, a NuGet upgrade would solve this properly.

Usually, a .NET Standard library would still be consumable, since as far as I recall the official recommendation is to multi-target most of them with both .NET Standard and the equivalent .NET Framework.

@ilexp
Copy link
Member Author

ilexp commented Dec 9, 2018

Progress

  • Tested the previously mentioned concurrency fix via internal operation locks and found that this introduces long freeze times in the PackageManager UI, since it now has to wait for lock release on the background worker thread a lot.
  • Reverted the concurrency fix and instead implemented another one that uses multiple internal pooled dependency walkers instead of a single one, avoiding the need of an exclusive lock that is held for long time periods.
  • Implemented support for multiple, framework-dependent package dependency sets, although tests showed not a single package that actually used them. Might be a NuGet v2.x issue, or the packages in question simply did not need them.
  • Implemented an optimization to avoid expanding dependencies of "NETStandard.Library", instead treating it as opaque.
  • Tested the new package installation behavior as described and found no issues.

ToDo

  • Merge to master
  • Merge to release
  • Post-release testing

@ilexp
Copy link
Member Author

ilexp commented Dec 9, 2018

Merged and released.

@ilexp ilexp closed this as completed Dec 9, 2018
@ilexp ilexp added PackageManager Area: Editor package management or its UI and removed Plugin Area: Misc. core / editor plugins labels Jul 14, 2019
@ilexp ilexp modified the milestones: General, C# / .NET Upgrade Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It's broken and should be fixed Editor Area: Duality editor or support libraries PackageManager Area: Editor package management or its UI
Development

No branches or pull requests

2 participants