-
Notifications
You must be signed in to change notification settings - Fork 289
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
Update Package Management to Latest Stable NuGet (4.x) #574
Comments
Seems like NuGet v3 adopted a micro-dependency model and the former NuGet.Core is no longer updated and locked to v2.x. This could get really ugly - according to the following links, there might be between 24 and 40 dependencies required to get this up an running.
Investigate further. If it turns out that there will actually be that many different assemblies required in the working directory, consider adding a prerequisite issue to find out if there are ways to keep the main directory clean. |
Implementation related to the above blog: https://github.com/Wyamio/Wyam/tree/1897149192a0cb1efe6378aa6185fbaab1eadf9d/src/core/Wyam.Configuration/NuGet Latest version: |
Did a quick test and found that there will be at least 15 individual assemblies required, plus 15 optional XML docs files. This is definitely too much to put into the main directory. Created issue #578 as a dependency for this. |
Closed issue #578 unresolved due to running into more problems than expected while implementing it, skewing its cost / gain ratio for the worse. Could be re-opened in isolation at some later point, but should no longer be considered a dependency of this issue. |
Immediate ToDo
|
Progress
Immediate ToDo
|
Progress
Immediate ToDo
|
Side note about NuGet packing support for NetStandard packages: http://blog.tdwright.co.uk/2017/11/20/the-perils-of-publishing-a-net-standard-library-to-nuget/ |
Progress
Immediate ToDo
|
Putting this up for grabs to indicate that I'd appreciate help on this issue. Its main work will also be nicely self-contained and currently takes place building a prototype in a separate repo, making collaboration a lot easier and less constrained. So if anyone's up for joining in, let me know 👍 |
I have a netstandard branch for pathfindax here. I havent published it to nuget yet though (and probably would have to fix some appveyor related stuff to do so). If you want to test with a real plugin I could publish it to nuget as a prerelease package but as it is now duality does not support prerelease packages. Might be a good moment to add this functionality (I mean this sounds like really simple to add right?). |
Is there any branch I can base my new branch on? Also I see alot of nuget nuget packages on nuget but not really sure what is old and what is the v4 version. Is this the correct version?https://www.nuget.org/packages/NuGet.Common Looking at the dependencies that one depends on NETStandard.Library (>= 1.6.0) which means we need to use the duality branch that uses netstandard I believe. EDIT: checked this and I get this error when I try to install: |
If you're going to look into using NuGet 4.X in Duality, I'd try out the basics in a standalone project first. Check out this one that I started earlier, it's a regular .Net Framework project. From what I could gather, the main required package is |
Since we've run into trouble with .NET Standard only packages via NuGet 2.14 in issue #573, I'm bumping this issue again with some fresh thought and brief research.
After letting this topic sink in for a while, and also moving forward with NuGet.Core integration in #703, #710 and #715, I think it makes sense to be more open towards lower level implementations here, as @Barsonax suggested before, and ditch the high level |
Yup upgrading to Nuget 2.14 only solved some errors but didn't actually made nuget understand how to resolve netstandard properly. What currently works is a package that targets both netstandard and netframework (before nuget 2.14 this resulted in a exception I believe). What does not work is a package targeting only netstandard. It will install the package but it will think there are no dependencies and thus it won't install any dependencies. So once more into the breach. |
Started working on a nuget v5.1 implementation for duality (yup thats the latest version currently) based on findings in https://martinbjorkstrom.com/posts/2018-09-19-revisiting-nuget-client-libraries The good part is that it correctly resolves dependencies as can be seen here: The branch can be found here https://github.com/ilexp/NuGet4xTest/tree/Barsonax Gonna take some work to get all needed functionality though |
Awesome 🙂 Already looks better than the previous approach, more commands and work, less interface plumbing and fake project implementations. How are the API docs on all those classes? |
Haven't checked the api docs yet, I took whats in the blog post and made it work for nuget v5 which was pretty easy. I also added a simple console logging class to help our debugging efforts (I couldn't find one in the library with intelisense but maybe there is one already). EDIT: From what I see those blog posts are the only documentation so iam guessing we might have to figure things out ourselves at some point. Atleast its a starting point. |
Progress
|
With the above progress I believe we have achieved the basic functionality one would need for implementing a package manager. Next goals:
@ilexp I already made this project https://github.com/ilexp/NuGet4xTest/blob/Barsonax/NuGet4XTest/NuGet4XTest.csproj a netstandard library. How do you want to integrate this into duality? Just add this project to the duality solution or only the source? |
Great work 👍
Just the source - for the time being, I'd prefer integrating the NuGet glue code directly in the internal area of package management. |
Ok Another thing. I think nuget now does alot of caching for you. Not sure what duality caches currently but that might be unnecessary. Basically everything you download will be cached in the global nuget packages folder. It even works without internet once they are cached there. |
No caching on the Duality side, except for keeping (wrapped) metadata in memory for fast traversal. The Only thing that might be crucial here are (NuGet) events for installed / uninstalled packages which currently only fire for actual changes to the |
Started working on integrating it into duality: https://github.com/Barsonax/duality/tree/feature/nugetUpgrade Running into alot of compiler errors due to nuget references all over the place so its gonna take a while. Also iam not sure how I can fix the PackageCache class as the API it uses no longer exists. The whole class could be removed though as nuget does alot of caching already and the query functionality can then be reimplemented elsewhere.
We can probably fire the needed events ourselves since we know when we are installing a package. EDIT: removed PackageCache. Will see if performance degrades or not and then decide to reimplement it or not. Less than 20 errors left now but probably some bug fixing has to be done as well. I EDIT2: I managed to avoid infesting the entire code base with async await by wrapping up some methods with |
🎉 🙂
Also watch out for wasteful re-creation of the Duality package info class all the time - might still be worth storing in a private dictionary somewhere. Not having the wrapped info unique might lead to a bug or two as well, not sure if there wasn't any code that relied on reference equality of the info type for the same identity. Could be wrong, but watch out for it.
Sounds good 👍 I think once this is merged safely, we might just switch to async / await for the isolated case of the package manager and its editor frontend right afterwards. It's not core code, so there's probably less to worry about, and it's a good use case to test the waters separately from all the rest. |
Running into a issue where So so far the only thing that works is searching for packages (which btw loads alot faster). Also had to comment out some code that probably is still needed but needs to be rewritten. |
The Duality package setup is considered the ground truth of what should be installed (deep dependencies implicit), and the package manager will install and uninstall packages in order to match that. This is what triggers package restore, but also allows to just edit the file (or pull a new one from version control for changes) and restart the editor to get a certain setup.
Let me know if you need more info, or someones head to bounce ideas off in any case. |
This is getting a bit messy as I cannot simply use PackageSetup directly because it uses alot of different types for the same nuget concepts:
So you end up converting between those all the time. Not pretty. Maybe you can take a look at https://github.com/Barsonax/duality/tree/feature/nugetUpgrade and see if iam doing it wrong or we need to clean this up? |
Not sure I understand what's wrong yet, please elaborate. The Duality package manager intentionally hides away NuGet behind a Duality-only API surface, so both editor and plugins don't depend on NuGet directly, which is part of what makes this update possible in the first place. There are "Duality side" concepts for package management that apply to Duality API for managing packages, and they're expressed through those types. So it's of course expected that some of those Duality types have similarities with NuGet types - but why would that be a problem..?
Did a quick branch compare, and it seems a bit off, as almost all of the changes seem to do a netstandard switch and project system changes, not NuGet updates. Since we're going to need that for the PR anyway, can you do an isolated branch that only contains the NuGet related changes? Probably easier to talk about too. |
I also implemented a packageconfig class myself because I expected that we needed this. However since package setup does this for you I believe I no longer need that class. However I also implemented some logic that uses my class that needs to be refactored/removed now. Basically I made a small design error and got frustrated yesterday 😅
I based it on my netstandard branch as these changes are not compatible with .NET4.5.2 and C# 4.0. Compare it with that branch should make it alot cleaner. Might need to upgrade to .NET 4.7.2 and C# 7.3 (this one should work without build changes?) in a separate branch to avoid a big mega PR. |
Ah, got it. We've all been there 😄 Feel free to ping me whenever you need info on existing stuff. My response times admittedly have increased recently, but I'll help where I can 👍
Sounds good, I'm on it. Will see if I can manage to pull this off today. |
Nice, this should make this PR and the netstandard PR a bit smaller. We do have to fix conflicts but that should be doable. Likely we will also get some conflicts regarding package reference changes (maybe we need to split that out to a separate PR as well?) EDIT: made a issue for this #736 |
Done, merged and released. Didn't touch your NuGet branch though, maybe check to see if you can re-base (or re-create + cherry pick?) that based on the new master. |
Cherrypicked the nuget changes from the nugetbranch so that branch is now based on master instead of netstandard |
Lately I have been thinking. What if we drop the integrated package manager in the duality editor and simply use the tools that already exist in visual studio: Pros:
Cons:
A good place for this would be the GameDebugger project. Ofcourse I think GameLauncher would be a better name. |
Closing this as this issue is no longer relevant since we went for the solution in #786 |
Summary
As a prerequisite of issue #573, a NuGet update will be required. Regardless of this requirement, updating to the latest NuGet for package management is overdue, considering that Duality is still using 2.12, while 4.x has been out for a while.
Analysis
.nuspec
files might need to be updated - or they might not. Investigate this.netstandard1.1
later on.netstandard1.1
nuget package and see what the spec says. Especially the framework paths for files and dependencies should be interesting.Publish Game
script copy blacklist, as well as potentially existing.gitignore
files and nightly builder binary package config..gitignore
file to the binary release package by default.The text was updated successfully, but these errors were encountered: