AdamsLair / duality

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

Add option to the package manager to allow prerelease versions #641

Open Barsonax opened 6 years ago

Barsonax commented 6 years ago

Summary

The current package manager does not support prerelease packages.

Analysis

Barsonax commented 5 years ago

I see the UI already passes the name and the version of the package to install so just adding a bool that will make the UI find prereleases packages should work. The prerelease switch should just be a temporary switch that doesn't need to be persisted anywhere. Visual studio works like this as well. All it does is enable you to see and select the prerelease versions to install.

I do think we will run into a usability issue here. Currently I see no way to specify what version to install other than manually editing PackageConfig.xml. It will always take the latest version. The only thing that a prerelease bool would change is that it takes the latest prerelease version instead of the latest stable version.

It might very well be that the latest prerelease version is broken and a user wants to use the version before that.

ilexp commented 5 years ago

I see the UI already passes the name and the version of the package to install so just adding a bool that will make the UI find prereleases packages should work. The prerelease switch should just be a temporary switch that doesn't need to be persisted anywhere. Visual studio works like this as well. All it does is enable you to see and select the prerelease versions to install.

Sounds good 👍

I do think we will run into a usability issue here. Currently I see no way to specify what version to install other than manually editing PackageConfig.xml. It will always take the latest version. The only thing that a prerelease bool would change is that it takes the latest prerelease version instead of the latest stable version.

It might very well be that the latest prerelease version is broken and a user wants to use the version before that.

That is true - we currently have no way in the UI to select a specific version for install or update. It's an issue, but not necessarily connected to this one. We could do the simple prerelease switch first, and add a more complex version selection later on, or do both in one go, whichever fits.

Barsonax commented 5 years ago

Got a semi working implementation here: https://github.com/Barsonax/duality/tree/feature/nuget_prereleases

Some nice omissions and bugs currently:

Barsonax commented 5 years ago

It seems the current cache setup doesn't like prerelease packages. I keep getting no matching package could be found because it only caches the latest version which causes it to think the prerelease package does not exist. Other than some dirty hacks I don't see a trivial way to fix this currently.

I think the current cache setup needs to be redesigned to support this. It shouldn't just cache the latest version but it should cache all versions of the package. This will also make it easier to add version selection support later on as I suspect we will run into this same problem.

ilexp commented 5 years ago

I think the current cache setup needs to be redesigned to support this. It shouldn't just cache the latest version but it should cache all versions of the package.

That might not work, since the local packages repo is how NuGet v2 determines when to install a package, or use one that is already installed, as well as which versions to unintall and install when moving from one dependency graph to the next. And Duality requires the correct install / uninstall events to fire from NuGet in order to copy package files in place or remove them again.

I'm not sure the current way even is a cache, or just the way NuGet v2 organizes its packages. My understanding currently leans towards the latter.

It seems the current cache setup doesn't like prerelease packages. I keep getting no matching package could be found because it only caches the latest version which causes it to think the prerelease package does not exist.

Where exactly does that error occur?

It doesn't sound like there should be a fundamental problem. If a prerelease package is used, that package should be considered the latest package - not sure why that should be any different from regular packages, from the NuGet side. Let's rule out that this could be a solvable problem.

Barsonax commented 5 years ago

It occurs in the PackageManager:

        private void InstallPackage(PackageName packageName, bool skipLicense)
        {
            NuGet.IPackage installPackage = this.cache.GetNuGetPackage(packageName);
            if (installPackage == null)
            {
                throw new InvalidOperationException(string.Format(
                    "Unable to install package '{0}', because no matching package could be found.",
                    packageName));
            }

this.cache.GetNuGetPackage(packageName) returns null causing it to fail. Not sure how I can fix that.

ilexp commented 5 years ago

Turns out, my PackageManager implementation knowledge has become a bit rusty, and I interpreted your description of a cache as the local package repository in the Source/Packages folder. However, PackageCache from the this.cache field is actually a Duality class to avoid repeated costly retrieval of information for the same package name (ID + version combo).

It is entirely possible that there is an incompatibility with prerelease versions, because it was implemented on the assumption that there were none. I'm assuming something in this method, or its call hierarchy could be the culprit. For example, we have an explicit filter against prerelease packages here.

Barsonax commented 5 years ago

Actually this also filters against prerelease packages.

I think the only place that actually needs filtering is retrieving the latest package which may give a different result depending if you include prerelease packages or not. After that we have a version so we don't care about prerelease flags anymore. Just install that version.

EDIT: Why are we using a loop here? We know the version we want so we could do everything server side. Will save alot of performance and make the code simpler. Same goes for the loop above this one.

EDIT2: I think the entire branch in that if can be replaced by

return this.repository.GetPackages().FirstOrDefault(x => x.Id == packageRef.Id && x.Version.Version == packageRef.Version);

EDIT3: The above code seems to be interpreted differently on the server and fails to find a package. Could still be cleaned up though.

Barsonax commented 5 years ago

Seems its going to be more work than anticipated. Might be better if we do this after upgrading nuget (or replace it with some other library/write our own idk where it will end).

ilexp commented 5 years ago

EDIT3: The above code seems to be interpreted differently on the server and fails to find a package. Could still be cleaned up though.

And as far as I recall that's exactly why the code look the way it does, see the comments. Since the statement is transformed into a server query, and the server does whatever, we can't simply optimize the LINQ statement and get the same results.

We can change manual filtering to allow finding prerelease packages, but should not alter overall structure too much. I remember that it has evolved through a few iterations to get the best success rate for throwing those queries against the server and handling its results.

Seems its going to be more work than anticipated. Might be better if we do this after upgrading nuget (or replace it with some other library/write our own idk where it will end).

Might make sense. Depends how much work that will turn out to be in the end, and how strongly we need the prerelease support. If I manage to find a few days worth of time, I can probably push through on the prerelease side, but not sure when that'll be.

Barsonax commented 5 years ago

And as far as I recall that's exactly why the code look the way it does, see the comments. Since the statement is transformed into a server query, and the server does whatever, we can't simply optimize the LINQ statement and get the same results.

I was actually talking about the other branch in the if for when the version is not null. The server doesn't seem to handle comparing on version very well which broke my one liner LINQ query.