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

Check all dependencies for compatibility checking #2963

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

HebaruSan
Copy link
Member

Problems

The Compatible and Incompatible filters in GUI check dependencies, but only one level. For example, OuterPlanetsMod depends on Kopernicus depends on ModularFlightIntegrator; if the first two are compatible and the third isn't, then OuterPlanetsMod appears to be installable but will fail.

We do extra work when loading the GUI filters, since Registry.Available and Registry.Incompatible both loop over all modules and check their dependencies, and the results are not re-used. Similarly, various operations that refresh the mod list are slower than they need to be because we re-check all those same relationships over again even if they haven't changed.

Sometimes "Available" includes incompatible modules:

  • Registry.available_modules
  • Registry.AllAvailable
  • Registry.SetAllAvailable
  • Registry.HasAnyAvailable
  • Registry.AddAvailable
  • Registry.RemoveAvailable
  • Registry.GetAvailableMetadata

And sometimes it excludes them:

  • Registry.Available
  • ckan available

This is confusing.

Changes

Now Registry uses a new CompatibilitySorter class to track module compatibility, which has public properties Compatible and Incompatible and a CompatibleVersions property to represent which versions they're compatible with. When we need the compatible or incompatible mods, we check whether the compatible versions are the same as last time, and if so, we return the same lists as previously (which solves the problem of Registry.Available and Registry.Incompatible duplicating work), otherwise we create a new CompatibilitySorter to compute the new compatibilities.

CompatibilitySorter uses a fully recursive dependency checking algorithm, so modules will only be counted as compatible if all of their dependencies are compatible. It also uses the lists of compatible and incompatible mods to speed up the search as it goes, rather than re-checking over and over whether dependencies are compatible (now that it's recursive, we don't want to re-check all of Kopernicus's dependencies over and over for all of its many depending mods).

Now "Available" means all modules, and anything that excludes incompatible mods is renamed to "Compatible". AllAvailable is renamed to AvailableByIdentifier since it doesn't return "all" of anything. ckan available retains its old name so users don't have to switch to a new command.

Fixes #1105.
Fixes #1646.
Fixes #2231.
Fixes #2849.

@HebaruSan HebaruSan added Enhancement New features or functionality Core (ckan.dll) Issues affecting the core part of CKAN Pull request Registry Issues affecting the registry labels Jan 13, 2020
@HebaruSan HebaruSan added the In progress We're still working on this label Jan 13, 2020
@HebaruSan

This comment has been minimized.

@HebaruSan HebaruSan removed the In progress We're still working on this label Jan 13, 2020
@DasSkelett

This comment has been minimized.

@HebaruSan
Copy link
Member Author

I wasn't sure about those. In many places we pass null for the version to skip those compatibility checks, which makes them even more ambiguous. I could be persuaded if you strongly think they should be renamed, otherwise I would be comfortable leaving them alone.

@DasSkelett
Copy link
Member

DasSkelett commented Jan 15, 2020

Okay, let's leave those alone.
I still have a few suggestions, opening a PR to your branch...
Edit: Done in HebaruSan#7

@DasSkelett

This comment has been minimized.

@HebaruSan
Copy link
Member Author

Right, this hasn't been rebased; both PRs were tripped up by something else, apparently some third party changes in net-core. Do we have enough information to submit an issue to https://github.com/dotnet/core/issues ?

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the core build fail can be investigated separately.
Edit: Maybe just remove the .travis.yml change before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality Registry Issues affecting the registry
Projects
None yet
2 participants