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

Fix installation of metapackages on cmdline #3362

Merged
merged 1 commit into from
May 13, 2021

Conversation

DasSkelett
Copy link
Member

@DasSkelett DasSkelett commented May 10, 2021

Problem

When trying to install metapackages from cmdline, using ckan.exe -c metapackage.ckan, it doesn't actually install any mods:

$ mono ckan.exe install --instance "Fake-1.11.2" -c asdf.ckan 
About to install:


Continue? [Y/n] 
Updating registry
Committing filesystem changes
Rescanning GameData
Done!

whereas the GUI installs them correctly.

Cause

In InstallList(List<string> modules, ...), we are running the RelationshipResolver for the identifier list, to get back a list of CkanModule to install. Then we filter out all modules installed as dependencies, to make sure the resolver running in InstallList(ICollection<CkanModule> modules, ...) can re-detect them as dependencies, so we can mark them as auto-installed.

public void InstallList(List<string> modules, RelationshipResolverOptions options, RegistryManager registry_manager, ref HashSet<string> possibleConfigOnlyDirs, IDownloader downloader = null)
{
var resolver = new RelationshipResolver(modules, null, options, registry_manager.registry, ksp.VersionCriteria());
// Only pass the CkanModules of the parameters, so we can tell which are auto
InstallList(resolver.ModList().Where(m => resolver.ReasonFor(m) is SelectionReason.UserRequested).ToList(), options, registry_manager, ref possibleConfigOnlyDirs, downloader);
}

The problem with metapackages is, that the first RR filters them out of the mod list (we don't want to actually install the metapackage after all). Thus the list of mods passed to InstallList(ICollection<CkanModule> modules, ...) is empty.

Changes

Now we also keep modules whose selection reason is "I'm a dependency|recommendation|suggestion of a metapackage" in the list passed to InstallList(ICollection<CkanModule> modules, ...) .
The InstallList(List<string> modules, ...) method is only used in the CmdLine; GUI sends the metapackage as CkanModule to InstallList(ICollection<CkanModule> modules, ...), and the ConsoleUI pretty much only deals in CkanModules (and doesn't have a "Install from .ckan" feature).

InstallList() is now also returning early if there are no mods to install, no need to go through the whole transaction and registry saving shebang.

I was considering two other solutions:
b) make CmdLine.Install.RunCommand()) call the other InstallList() passing a list of CkanModule instead of identifier+version combos. This might or might not have introduced other issues causing installation of outdated modules (I think we moved to a list of strings some time ago deliberately?)
c) make RelationshipResolver.Resolve() mark dependencies of metapackages as SelectionReason.UserRequested, not Depends. Might have had other side effects as well.

@DasSkelett DasSkelett added Bug Something is not working as intended Cmdline Issues affecting the command line Pull request labels May 10, 2021
@DasSkelett DasSkelett requested a review from HebaruSan May 10, 2021 14:46
@HebaruSan
Copy link
Member

HebaruSan commented May 10, 2021

Will this work for a metapackage that uses recommends instead of depends?

What about a metapackage with suggests, when the user passes --with-suggests and/or --with-all-suggests?

Maybe we should check reason.Parent?.IsMetapackage regardless of the reason type.

- Fix installation of metapackages on cmdline
- Don't run through install flow if there's no mod to install
@DasSkelett DasSkelett force-pushed the fix/cmdline-metapackages branch from eec22f2 to 2584615 Compare May 13, 2021 13:14
@DasSkelett
Copy link
Member Author

Good point, changed it to keep all relationships of metapackages.

Copy link
Member

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

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

@HebaruSan HebaruSan merged commit 5a2cd8e into KSP-CKAN:master May 13, 2021
@DasSkelett DasSkelett deleted the fix/cmdline-metapackages branch May 13, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Cmdline Issues affecting the command line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants