Upgraded nuget to 5.4 #740

Barsonax · 2019-08-05T17:38:28Z

Not ready to merge yet. Prerequisites:

Remove duplicate package.config logic (duality already has such logic).
Fix any remaining bugs

Barsonax · 2019-08-07T12:37:34Z

Source/Editor/DualityEditor/PackageManagement/PackageManager.cs

-				.LocalRepository
-				.GetPackages()
-				.ToArray();
+			IEnumerable<PackageIdentity> allInstalledPackages = this.manager.GetInstalledPackages();


GetInstalledPackages relies on reading out the package.config file to see what packages are installed however duality also has logic for this already and iam not sure how to merge these in a clean way. @ilexp

So I cannot simply remove that logic from the manager as that would break this code.

I think duality shouldn't have anything to do with reading out a package.config file and should just leave that to nuget and friends. Whenever it needs this info it should simply ask the manager for it.

Ah I think that a installed package does not mean that the package is registered in the package.config but that its actually restored to the source/packages folder.

However with the new nuget this doesn't happen anymore due to the global package folder and thus clashes a bit with this approach. Ofcourse one can still add this back in manually...

So I fixed the above problem by looking at what packages are actually restored but now I get this when applying the update

Ugh yet again duality is removing packages that shouldn't be removed. That package that the error is complaining about is actually installed but then later on removed by duality.

@ilexp idk how to fix this, why is duality modifying the packages folder in the first place? That is nugets reponsibility and not duality. I feel duality is doing way too much and that is now all getting in the way of this upgrade.

Only skimmed the above thread since I'm very low on time right now, but it seems like you might have gone off the wrong track somewhere, correct me if I misunderstood. Short version from what I gathered:

Nuget manages the packages folder (or global packages folder in v5.x)

That's already the case.

Nuget manages the packages.config

~~That's already the case as well.~~

Edit: If there is one, that is. Not sure, now that I think of it, I don't think a packages.config in the traditional .csproj sense is even used at all in the current implementation.

Duality manages the plugin and lib folders

And this one too!

The only way in which you could say that Duality handles the first two points is via the actual NuGet API. It's possible that this API no longer exists, or some of the concepts changed in newer NuGet, but it's not intention that Duality would do either of those - and as far as I remember, it shouldn't in the current implementation.

The outline of the current / old implementation that is probably related to some of the issues that arose:

Duality has a list of (Duality-only!) packages in PackageConfig.xml. This is a letter of intent which (Duality only!) packages there should be.

Duality sets up NuGet with the local Packages folder and asks it which packages are currently installed. Internally, I assume NuGet figures that out by checking folders and files.

Duality then asks Nuget to remove those that aren't on the list, and to install those that are, in order to sync whatever state the NuGet stuff is in with the intended state that the Duality config specifies. Dependencies and everything are handled by NuGet, Duality has no knowledge of packages that wouldn't show up in its own package manager.

To apply the package install to plugin and lib folders, Duality registers a NuGet event that fires when packages are installed or uninstalled, and uses the provided info to copy files as needed.

Side note: In the old NuGet at least, "install" meant the package is physically in the right place and extracted, not in some config list, and "uninstall" meant that it is physically removed from there.

To summarize, Duality has a list of its own packages it wants and uses NuGet API to make sure NuGet syncs that up, plus dependencies and whatever is needed. It also uses NuGet API to figure out where it put things and when they are added or removed to sync its own plugin / lib folder files.

Let me know if that helps, will try to check back soon-ish.

Duality then asks Nuget to remove those that aren't on the list, and to install those that are, in order to sync whatever state the NuGet stuff is in with the intended state that the Duality config specifies. Dependencies and everything are handled by NuGet, Duality has no knowledge of packages that wouldn't show up in its own package manager.

So this is causing it to break atm since its removing packages that duality packages depend on that shouldnt be removed. I don't know how to fix this.

Hmm, it shouldn't remove any non-Duality packages, and even if it removes a Duality package that later turns out to be a dependency of one that is needed, that should be fixed right after when it verifies that every expected package is installed. It's more of an edge case though that doesn't happen that often, so I'd guess it's some sort of a bug that the package is removed in the first place.

As soon as I manage to carve out some time, I'll have a look as well, see if I can help.

hmm maybe I broke something in that area then

What about just ignore cleaning the package folder btw? I don't believe visual studio did that anyway. Besides this behavior is simply not compatible with the global nuget packages folder (which is why I make a copy of it...). Would be much nicer not having to copy that package over and just grab it from the global nuget packages folder I think.

What about just ignore cleaning the package folder btw?

The reason it's doing that is not just cleaning the package folder, but a necessary step (for the current implementation anyway) to find out which actual files to remove from plugin and lib folders via the events that NuGet 2.x fires on uninstall, including dependencies.

For the current implementation, the packages folder isn't a side effect or cache we could just ignore, but NuGet's "working memory". From what I gather from your progress reports and comments, this is no longer the case anyway, so I assume some things on the implementation will change in that regard. What we still need is some automated uninstall of Duality packages (+ deps) that are no longer in the PackageConfig.xml despite being previously installed - and vice versa, an auto-install for those that are in there, but not installed yet.

Use cases:

Pull update from version control, run editor. It should match the local packages with what's checked in via PackageConfig.xml, i.e. account for added and removed ones.

User manually edits PackageConfig.xml, then starts the editor.

Something in the package manager goes really wrong and Duality crashes. A restart can bring packages back in order.

So, assuming the new NuGet doesn't have any equivalent to a local package folder / working memory, then the Duality package manager will need some way to find out

Which files to copy or remove when a package is installed / uninstalled. (You probably got that figured out already)

Which items in PackageConfig.xml are not currently present in the project and need an install.

Which packages that are currently present in the project are not in PackageConfig.xml and need an uninstall.

In other words, there needs to be a way to ask for the "status quo" of current package installs, so we can diff it with the "letter of intent" in the PackageConfig.xml to find out what needs to be done. Previously, that was achieved via NuGet API (this.manager.LocalRepository, which used the packages folder) and then encapsulated in Duality local setup data. Is there any equivalent in NuGet 5, which we can query for project "status quo"?

If there isn't, we could come up with something custom, like a second, internal package config representing the actual local setup, or similar.

Barsonax · 2019-08-09T19:26:18Z

Is there any equivalent in NuGet 5, which we can query for project "status quo"

If I understand correctly you want a list of all packages needed including all dependencies. Nuget provides a API for this and the PackageConfig.xml itself can be used as input for this. With some logic you can then figure out what packages to install and what to uninstall.

I believe the nuget class we need for this is called DependencyResource or similar.

ilexp · 2019-08-10T07:09:50Z

If I understand correctly you want a list of all packages needed including all dependencies.

Ah, no, not the needed ones - I might have explained it all a bit convoluted. I'll try again, in short.

This is what we have now:

PackageConfig.xml: Which Duality packages the project wants.
Source\Packages via NuGet API: Which (NuGet) packages are installed right now.
Duality triggers NuGet to un/install so the list of Duality packages matches in both.

What we need now, since the packages folder no longer seems to play that role in NuGet:

PackageConfig.xml: Which Duality packages the project wants.
???: Which (NuGet) packages are installed right now.
Duality triggers NuGet to un/install so the list of Duality packages matches in both.

That second ??? part is what I meant. Is there some sort of NuGet-managed package lock file or something else that tells you what is actually installed right now? If not, we could probably write our own in some bookkeeping file inside the Source\Packages folder.

Barsonax · 2019-08-10T07:51:30Z

Rebased on the latest master. Changes are now down from 39 to 9 files :)

Barsonax · 2019-08-10T07:57:05Z

???: Which (NuGet) packages are installed right now.

Isn't this something duality could provide based upon the files present in the plugin folder? Might need to split it up to prevent ambiguity between packages and dll's that were just copied there?

Not depending on nuget for this would make future updates easier so design wise this be would be preferable. Nuget doesn't have logic for this anyway so the only way is to add another layer in between.

ilexp · 2019-08-10T11:32:29Z

In that case, I'd probably go with an InstalledPackages.xml inside Source\Packages that gets updated immediately after an individual package install / uninstall is complete. That way, NuGet would be out of the equation for bookkeeping here, and we don't risk falling into ambiguity traps when checking plugin, lib and sample files. For a first iteration, behavior would also be equal to what we have right now, which is nice. Could also be an opportunity to clean up a bit of local package setup code.

I can look into this, provided I manage to carve out some time, fingers crossed 😄

Barsonax · 2019-08-12T20:32:01Z

Yup might be better that you look into this particular part.

In the current commit of this PR there is still the package config on the nuget side that is not needed anymore I think btw.

ilexp · 2019-09-15T10:09:32Z

Source/Editor/DualityEditor/PackageManagement/PackageManager.cs

+			var nuGetFramework = NuGetFramework.Parse("net472");
+			var settings = NuGet.Configuration.Settings.LoadDefaultSettings(root: null);
+			this.logger = new PackageManagerLogger();
+			this.manager = new NuGetTargetedPackageManager(nuGetFramework, this.logger, settings, this.env.RepositoryPath);


It seems like the package repository URLs from the Duality PackageSetup are currently ignored, which also breaks most of the package tests because they create a local repository to source from. How do we specify repository URLs with NuGet 5?

I believe this is possible and I think I saw the API somewhere but currently not at my pc to check this. Will check it later.

Barsonax · 2019-09-22T08:50:35Z

Source/Editor/DualityEditor/PackageManagement/Internal/NuGetTargetedPackageManager.cs

+			this._nuGetFramework = nuGetFramework;
+			this._packagePathResolver = new PackagePathResolver(packagePath);
+			var sourceRepositoryProvider = new SourceRepositoryProvider(this._settings, Repository.Provider.GetCoreV3());
+			this._repositories = sourceRepositoryProvider.GetRepositories().ToArray();


I believe we have to modify this to define custom nuget repositories

ilexp · 2019-09-29T11:51:56Z

Okay, so right now I'm the bottleneck for moving this forward, but I'll try to briefly summarize what I think needs to be done here before we can merge:

ToDo

Get the basics for unit testing of package management back up and running.
- Respect repository URLs specified in Duality PackageSetup, including local folder paths.
- Check out if packing dummy packages for testing still works, fix if necessary.
- Ensure the most basic install / uninstall tests succeed.
Sandbox NuGet settings used in Duality package management.
- Use a default, then modify local instance.
- Do not rely on system or user settings for NuGet to avoid interference in various dev setups.
- NuGet settings links for research:
Introduce a Duality InstalledPackages.xml in Source\Packages to reflect which (NuGet) packages are currently installed, to replace the previous "local repository" mechanic that was based simply on existing folder structures. (See above)
Gradually fix things until all tests are green again.
Do a full DualityEditor test locally.
- Fresh install from local repository
- Fresh install from remote repository
- Package restore, install, uninstall and update via PackageConfig.xml changes and restart.
- Package install, uninstall and update via package manager UI
- Test performance now that PackageCache is gone and re-introduce some equivalent if it's too bad.
Final cleanup, API and style tweaks.

I'll report back with progress and help requests as soon as I get to it.

Barsonax · 2019-11-22T14:34:54Z

@ilexp do you still have time to implement this? If not maybe we should make it clear that somebody else can take it over.

Not that I have much time lately though...

ilexp · 2019-12-15T09:10:51Z

@ilexp do you still have time to implement this? If not maybe we should make it clear that somebody else can take it over.

Good call - I'm honestly a bit overwhelmed with real life stuff lately and since I'm clearly the bottleneck here, I'd be happy to let someone take over, and try to help with advice.

… relative path being passed to packagepathresolver

Barsonax · 2020-01-25T19:50:50Z

Upgraded to nuget 5.4 so we stay up to date. Currently working on getting the unit tests working properly again. Turns out we still referenced the old nuget 2.14 in the unit tests (FIXED). Also its currently not using the test repository that the unit tests sets up which breaks many tests.

Barsonax · 2020-03-20T12:38:27Z

Seems like PackagePathResolver and PackageIdentity truncate the revision number. Meaning a version like 1.0.0.0 will end up as 1.0.0. Seems nuget doesnt want you to use 1.0.0.0? Needs some more research to be sure.

EDIT: nuget is normalizing the version numbers: https://docs.microsoft.com/en-us/nuget/concepts/package-versioning#normalized-version-numbers

Barsonax · 2020-04-19T09:54:45Z

Closing this as we have decided to take a different route in #786

This was referenced Aug 5, 2019

Change plugin projects to netstandard and sdk style csproj's #741

Merged

Migrated all projects to packagereference #739

Merged

Barsonax commented Aug 7, 2019

View reviewed changes

Barsonax mentioned this pull request Aug 10, 2019

Duality package structure and nuspec is wrong #743

Closed

ilexp added this to the C# / .NET Upgrade milestone Aug 10, 2019

ilexp self-assigned this Aug 13, 2019

ilexp reviewed Sep 15, 2019

View reviewed changes

Barsonax commented Sep 22, 2019

View reviewed changes

ilexp added Editor Area: Duality editor or support libraries PackageManager Area: Editor package management or its UI Task ToDo that's neither a Bug, nor a Feature Unit Tests Good candidate for adding more tests Work in Progress PR: Do not merge / review yet labels Oct 29, 2019

Barsonax added 4 commits January 25, 2020 19:39

Updated to nuget 5.2

e5e9ce1

readded localrepository

1854465

WIP

d6b12dc

nuget 5.4.0

c3f1cee

Barsonax changed the title ~~Upgraded nuget to 5.2~~ Upgraded nuget to 5.4 Jan 25, 2020

removed old nuget 2.14 from test project, fixed compile errors, fixed…

d1fc999

… relative path being passed to packagepathresolver

made sure the test repository is used for the unit tests

0523b65

Barsonax added 3 commits March 20, 2020 11:41

fix using

ee09eb9

fixed test dependency sets being created with a invalid target framework

085e26c

fixed restoring already restored package

e76664d

nameof

80c5924

Barsonax closed this Apr 19, 2020

Barsonax deleted the feature/nugetUpgrade branch April 28, 2020 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgraded nuget to 5.4 #740

Upgraded nuget to 5.4 #740

Barsonax commented Aug 5, 2019 •

edited

Loading

Barsonax Aug 7, 2019

Barsonax Aug 7, 2019 •

edited

Loading

Barsonax Aug 7, 2019 •

edited

Loading

Barsonax Aug 7, 2019 •

edited

Loading

Barsonax Aug 7, 2019

ilexp Aug 8, 2019 •

edited

Loading

Barsonax Aug 8, 2019

ilexp Aug 8, 2019

Barsonax Aug 8, 2019 •

edited

Loading

ilexp Aug 9, 2019 •

edited

Loading

Barsonax commented Aug 9, 2019 •

edited

Loading

ilexp commented Aug 10, 2019

Barsonax commented Aug 10, 2019

Barsonax commented Aug 10, 2019 •

edited

Loading

ilexp commented Aug 10, 2019

Barsonax commented Aug 12, 2019

ilexp Sep 15, 2019 •

edited

Loading

Barsonax Sep 15, 2019

Barsonax Sep 22, 2019

ilexp commented Sep 29, 2019

Barsonax commented Nov 22, 2019

ilexp commented Dec 15, 2019

Barsonax commented Jan 25, 2020

Barsonax commented Mar 20, 2020 •

edited

Loading

Barsonax commented Apr 19, 2020

Upgraded nuget to 5.4 #740

Upgraded nuget to 5.4 #740

Conversation

Barsonax commented Aug 5, 2019 • edited Loading

Barsonax Aug 7, 2019

Choose a reason for hiding this comment

Barsonax Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Barsonax Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Barsonax Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Barsonax Aug 7, 2019

Choose a reason for hiding this comment

ilexp Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Barsonax Aug 8, 2019

Choose a reason for hiding this comment

ilexp Aug 8, 2019

Choose a reason for hiding this comment

Barsonax Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

ilexp Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

Barsonax commented Aug 9, 2019 • edited Loading

ilexp commented Aug 10, 2019

Barsonax commented Aug 10, 2019

Barsonax commented Aug 10, 2019 • edited Loading

ilexp commented Aug 10, 2019

Barsonax commented Aug 12, 2019

ilexp Sep 15, 2019 • edited Loading

Choose a reason for hiding this comment

Barsonax Sep 15, 2019

Choose a reason for hiding this comment

Barsonax Sep 22, 2019

Choose a reason for hiding this comment

ilexp commented Sep 29, 2019

ToDo

Barsonax commented Nov 22, 2019

ilexp commented Dec 15, 2019

Barsonax commented Jan 25, 2020

Barsonax commented Mar 20, 2020 • edited Loading

Barsonax commented Apr 19, 2020

Barsonax commented Aug 5, 2019 •

edited

Loading

Barsonax Aug 7, 2019 •

edited

Loading

Barsonax Aug 7, 2019 •

edited

Loading

Barsonax Aug 7, 2019 •

edited

Loading

ilexp Aug 8, 2019 •

edited

Loading

Barsonax Aug 8, 2019 •

edited

Loading

ilexp Aug 9, 2019 •

edited

Loading

Barsonax commented Aug 9, 2019 •

edited

Loading

Barsonax commented Aug 10, 2019 •

edited

Loading

ilexp Sep 15, 2019 •

edited

Loading

Barsonax commented Mar 20, 2020 •

edited

Loading