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

Resolve virtual module dependencies in same order as non-virtual #3476

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Nov 13, 2021

Background

During the addition of RP1 Express (see KSP-CKAN/NetKAN#8701), we observed some unexpected quirks with how dependencies were resolved. We had to restructure the modules in that pull request to fit the peculiar way the relationship resolver works.

I think it's highly likely that the WildBlue play modes (see KSP-CKAN/NetKAN#8275) are also affected by these quirks, but I do not know of a specific example of this.

Non-virtual dependencies

Consider this set of mods:

Mod Provides Depends Conflicts
A B, C
B C1
C1 C C
C2 C C

When the user tries to install A:

  1. The relationship resolver first tries to satisfy A's dependency on B, which succeeds trivially by finding the real module B.
  2. Then B's dependencies are resolved, which pulls C1 into the change set.
  3. Finally, A's dependency on C is resolved, which is satisfied by C1 already in the change set, so everything is fine and the installation proceeds without the user having to make any choices.

Successful installation without unnecessary prompting is good, and this is probably how most people would expect it to work.

Problem

Virtual dependencies

Now suppose B is virtual and provided by two non-virtual modules B1 and B2, which each depend on their own preferred module satisfying C:

Mod Provides Depends Conflicts
A B, C
B1 B C1 B
B2 B C2 B
C1 C C
C2 C C

When the user tries to install A:

  1. The relationship resolver first tries to satisfy the dependency on B. It discovers that multiple modules can satisfy that dependency, so it throws TooManyModsProvideKraken to tell the UI to make the user choose.
  2. If the user chooses B1, the UI submits a new request to the relationship resolver for a change set containing A and B1.
  3. This time it finds that B1 is in the change set and can satisfy A's dependency on B, so TooManyModsProvideKraken is not thrown for B.
  4. However, instead of then resolving B1's dependencies, the relationship resolver next tries to satisfy A's dependency on C. Nothing in the current change set provides C, but there are multiple available modules that do, so it throws TooManyModsProvideKraken to make the user choose between C1 and C2.
  5. If the user picks C2, this results in a new resolution request for a change set containing A, B1, and C2.
  6. Again the resolver tries to satisfy A's dependency on B first, which B1 does, and then it tries to satisfy A's dependency on C, which C2 does.
  7. Only at this point does the resolver finally get around to looking at B1's dependencies, which results in the discovery that B1 must have C1 installed, but C1 conflicts with C2 which is already in the change set, so we have an unresolvable conflict and the resolution fails.

Note that the presence of a virtual dependency has caused the dependency tree to be resolved in a different order than in the previous scenario (in step 4). Since the user has been prompted to make an unnecessary choice and the installation ultimately failed, we can regard this as a bad outcome.

This is what happened with RP1 Express. The main module depends on a virtual module representing different levels of graphical detail. Originally, the main module was also supposed to depend on the rest of the core modules needed to install RP1, some of which are virtual and were intended to be overridden by the specific graphics level chosen by the user. However, this resulted in extra contradictory prompts to choose those virtual modules anyway, because the graphics level module was waiting for its turn at the end of the change set. To get it to work, we had to move all of the dependencies into the graphical modules themselves, turning the main module into a very small shell:

While this works, the original structure was preferable for maintenance as it factored out common functionality into the main module rather than duplicating it into three modules.

Semi-virtual dependencies and any_of

It is possible for this problem to sneak up on us. Suppose that, starting from the first set of relationships above, we later add a new module B2 providing B (MechJeb2-dev and CustomBarnKit-RO both do this):

Mod Provides Depends Conflicts
A B, C
B C1
B2 B C2 B
C1 C C
C2 C C

Now even though B itself is not virtual and was never intended to be virtual, the dependency of A on B is treated as virtual, and so B and/or B2's dependencies will be resolved after C, again resulting in potential unexpected conflicts.

The any_of property also works by throwing TooManyModsProvideKraken and submission of updated change sets, so it has the same problem, again for modules that were never planned to be virtual in themselves.

Changes

Now if we find that a dependency is resolved by a module already in the change set, we resolve that module's dependencies immediately rather than making it wait its turn. This makes the change set containing A and B1 automatically pull in C1 without prompting the user, as desired.

Incidental build stuff

Mono seems to have enabled HTTP to HTTPS redirection for their APT repo recently. Unfortunately their Docker images for Mono 6.6 and earlier don't have apt-transport-https installed and so they fail on apt-get update. Now we install that package so these images will work.

These changes were cherry-picked in 83f54bf.

@HebaruSan HebaruSan added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Pull request Relationships Issues affecting depends, recommends, etc. labels Nov 13, 2021
@HebaruSan HebaruSan requested a review from DasSkelett November 13, 2021 19:56
@HebaruSan

This comment has been minimized.

@HebaruSan HebaruSan force-pushed the fix/resolve-virtual-in-order branch 3 times, most recently from 032bd99 to 017be8e Compare November 14, 2021 15:01
@DasSkelett
Copy link
Member

I'm gonna cherry-pick the HTTPS CI fix commit and directly push it to master so it's present for other PRs, it might take a while until I can wrap my head around the resolver changes 😅

@HebaruSan HebaruSan force-pushed the fix/resolve-virtual-in-order branch from 017be8e to 8492d1a Compare November 17, 2021 15:34
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.

Okay I think I managed to understand the issue and how this fixes it.
I played around with re-ordering RP-1 dependencies, moving common ones to the RP1-Express-Install and I believe it improved the situation a bit.
If you move the RP-1-ExpressInstall-Graphics virtual dependency to the very bottom of RP1-Express-Install, it still allows you to pick a variant which conflicts with the RSSTextures module you choose earlier, but if it stays at the top it's a-okay.

@HebaruSan HebaruSan merged commit c76fef0 into KSP-CKAN:master Apr 22, 2022
@HebaruSan HebaruSan deleted the fix/resolve-virtual-in-order branch April 22, 2022 00:58
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 Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants