Skip to content

CKAN tests mod version interdependencies incorrectly #1693

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.

Sign up for GitHub

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

Jump to bottom
Closed
Starstrider42 opened this issue May 1, 2016 · 5 comments · Fixed by #2660
Closed

CKAN tests mod version interdependencies incorrectly #1693

Starstrider42 opened this issue May 1, 2016 · 5 comments · Fixed by #2660
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc. ★☆☆

Comments

@Starstrider42
Copy link
Contributor

Bug report as requested by @plague006 on KSP-CKAN/NetKAN#3841.

CKAN Version: v1.16.1-0-g2e91715 (beta)
Operating System: Windows 10 KP
The issue you are experiencing: The Custom Asteroids config file .ckans each depend on a minimum version of Custom Asteroids, but have a much broader range of KSP versions. Currently the dependencies are:

The intended behavior is that Pops-Stock-Inner v1.3.0 gets installed if you have Custom Asteroids v1.3.0, and v1.2.0 gets installed if you have any older version.

However, trying to install Custom Asteroids for KSP 1.0.5 (Custom Asteroids v1.2.0), which requires Pops-Stock-Inner as a dependency, causes CKAN to report a conflict between Pops-Stock-Inner v1.3.0 and Custom Asteroids v1.2.0. There is no option to download Pops-Stock-Inner v1.2.0, even though it is compatible with both KSP 1.0.5 and Custom Asteroids v1.2.0 and would satisfy Custom Asteroids's config dependency.

How to recreate this issue: (I will shortly be modifying CustomAsteroids-Pops-*.netkan to work around this issue, so the original example can't be used. The issue should be reproducible with any mod that depends on a newer version of a currently installed mod.)
@mheguy mheguy added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN ★☆☆ labels May 1, 2016
mheguy added a commit to KSP-CKAN/CKAN-meta that referenced this issue May 1, 2016
@mheguy
Match ksp_version compatibility to work around KSP-CKAN/CKAN#1693
9989f92
mheguy pushed a commit to KSP-CKAN/NetKAN that referenced this issue May 1, 2016
@Starstrider42
Workaround for KSP-CKAN/CKAN#1693 (#3844)
cee7c42 
* Adding changes that only build correctly after a v1.3 release.

* Noted compatibility with ART and DOS.

Note: ART is not currently on CKAN; I'm guessing the mod identifier would match the folder name.

* Added KSP-AVC as a suggested install.

* Updated conflict list

List based on case-by-case analysis of 44 planet packs. Non-CKAN packs (Boris System, Chani Planet Pack, Jungis Planet Pack, Stock-Alike Solar System) listed for forward compatibility, using a best guess at their eventual CKAN identifier.

* Corrected identifier for ART.

* Need spec 1.2+ to use supports.

* Temporary workaround for KSP-CKAN/CKAN#1693.

Closes #3841.

* Workaround permanent for compatibility with old clients.
mheguy added a commit to KSP-CKAN/CKAN-meta that referenced this issue May 1, 2016
@mheguy
Match ksp_version compatibility to work around KSP-CKAN/CKAN#1693
edecff8
mheguy pushed a commit to KSP-CKAN/CKAN-meta that referenced this issue May 1, 2016
Match ksp_version compatibility to work around KSP-CKAN/CKAN#1693 (#1119

Verified

This commit was signed with the committer’s verified signature.
politas Myk
GPG key ID: 47938AAEAD85818F
Verified
Learn about vigilant mode
2e2a859 
)
@HebaruSan HebaruSan added the Relationships Issues affecting depends, recommends, etc. label Feb 26, 2018
@HebaruSan HebaruSan mentioned this issue Jan 21, 2019
Merged
@HebaruSan
Copy link
Member

Possible alternate title: "Reverse dependencies are checked too late"
... since the relationship resolver uses CA's dependency (only) to pick a version of Inner, then later checks Inner's dependency, finds that it's not satisfied, and aborts the whole install. It would need to check Inner's dependency while trying to satisfy CA's original dependency, then fall back to other versions of Inner if it's not met (something I don't think we ever do currently).

@Starstrider42
Copy link
Contributor Author

No objections to the renaming. I wrote the original title in a behavior-oriented way because I don't know anything about CKAN's internals.

Sorry, something went wrong.

@HebaruSan
Copy link
Member

Well for now this is just my notes-to-self, so I can pick up where I left off in the future. I could be mistaken about any of this.

This is the line where the RR effectively narrows us down to just one version of a mod, without checking reverse dependencies:

CKAN/Core/Relationships/RelationshipResolver.cs

Lines 407 to 408 in c49489b

List<CkanModule> candidates = registry.LatestAvailableWithProvides(dep_name, kspversion, descriptor)
.Where(mod=>descriptor1.WithinBounds(mod.version) && MightBeInstallable(mod)).ToList();

We would need something like:

  • Registry.AllAvailableWithProvides instead of Latest, which RR would then filter by reverse dependencies; or
  • LatestAvailableWithProvides needs to gain the ability to filter based on reverse dependencies, including an IEnumerable<CkanModule> input parameter indicating the current change plan

I guess that second option sounds more reasonable.

Sorry, something went wrong.

@HebaruSan HebaruSan mentioned this issue Jan 22, 2019
Merged
@HebaruSan
Copy link
Member

@Starstrider42, I think I've got this figured out as part of #2660. These are the relevant tests, if you wouldn't mind double checking that I've understood the problem accurately:

CKAN/Tests/Core/Relationships/RelationshipResolver.cs

Lines 726 to 807 in d48fad4

[Test]
public void Constructor_ReverseDependencyDoesntMatchLatest_ChoosesOlderVersion()
{
// Arrange
CkanModule depender = CkanModule.FromJson(@"{
""identifier"": ""depender"",
""version"": ""1.0"",
""download"": ""https://kerbalstuff.com/mod/269/Dogecoin%20Flag/download/1.01"",
""depends"": [ { ""name"": ""dependency"" } ]
}");
CkanModule olderDependency = CkanModule.FromJson(@"{
""identifier"": ""dependency"",
""version"": ""1.0"",
""download"": ""https://kerbalstuff.com/mod/269/Dogecoin%20Flag/download/1.01"",
""depends"": [ {
""name"": ""depender"",
""min_version"": ""1.0""
} ]
}");
CkanModule newerDependency = CkanModule.FromJson(@"{
""identifier"": ""dependency"",
""version"": ""2.0"",
""download"": ""https://kerbalstuff.com/mod/269/Dogecoin%20Flag/download/1.01"",
""depends"": [ {
""name"": ""depender"",
""min_version"": ""2.0""
} ]
}");
AddToRegistry(olderDependency, newerDependency, depender);
// Act
RelationshipResolver rr = new RelationshipResolver(
new CkanModule[] { depender }, null,
options, registry, null
);
// Assert
CollectionAssert.Contains( rr.ModList(), olderDependency);
CollectionAssert.DoesNotContain(rr.ModList(), newerDependency);
}
[Test]
public void Constructor_ReverseDependencyConflictsLatest_ChoosesOlderVersion()
{
// Arrange
CkanModule depender = CkanModule.FromJson(@"{
""identifier"": ""depender"",
""version"": ""1.0"",
""download"": ""https://kerbalstuff.com/mod/269/Dogecoin%20Flag/download/1.01"",
""depends"": [ { ""name"": ""dependency"" } ]
}");
CkanModule olderDependency = CkanModule.FromJson(@"{
""identifier"": ""dependency"",
""version"": ""1.0"",
""download"": ""https://kerbalstuff.com/mod/269/Dogecoin%20Flag/download/1.01""
}");
CkanModule newerDependency = CkanModule.FromJson(@"{
""identifier"": ""dependency"",
""version"": ""2.0"",
""download"": ""https://kerbalstuff.com/mod/269/Dogecoin%20Flag/download/1.01"",
""conflicts"": [ {
""name"": ""depender""
} ]
}");
AddToRegistry(olderDependency, newerDependency, depender);
// Act
RelationshipResolver rr = new RelationshipResolver(
new CkanModule[] { depender }, null,
options, registry, null
);
// Assert
CollectionAssert.Contains( rr.ModList(), olderDependency);
CollectionAssert.DoesNotContain(rr.ModList(), newerDependency);
}

@Starstrider42
Copy link
Contributor Author

I think so? You've removed the KSP versioning aspect of the original issue by forcing a specific version of depender, but I think that's reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support depends on any_of lists HebaruSan/CKAN
3 participants
@HebaruSan @Starstrider42 @mheguy