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.

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

WIP: Implement a "Replaced by" relationship #1888

Closed
wants to merge 30 commits into from
Closed

WIP: Implement a "Replaced by" relationship #1888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
cc32d1f
Update spec
politas Sep 6, 2016
815e17b
Add replaced_by to Schema
politas Nov 3, 2017
a93d5d5
Add replaced_by to CkanModule and HasReplacement to IRegistryQuerier
politas Nov 3, 2017
1215425
Add Replaced_by to CLI list command
politas Nov 4, 2017
3275e62
Add Cmdline 'replace' command
politas Nov 4, 2017
86eed6a
Some fixes to clean up replace command
politas Nov 4, 2017
1b16514
Add Replace method to ModuleInstaller
politas Nov 4, 2017
233c885
Fix compile errors
politas Nov 4, 2017
a3d0192
Tidy-up minor issues
politas Nov 4, 2017
604518d
PLEASE HELPgit statusgit status I don't get this testing system
politas Nov 4, 2017
d2d2e03
Fix compile errors
politas Nov 4, 2017
05d8b35
Clarify Spec and fix typo
politas Nov 5, 2017
9e2b227
Remove non-existent files from Tests.csproj
politas Nov 5, 2017
740f806
Debugged CmdLine changes
politas Nov 5, 2017
139a05c
Reworked logic to address HebaruSan's comments
politas Nov 8, 2017
6c17370
bugstomping
politas Nov 8, 2017
9265498
comments and refactoring mostly
politas Nov 10, 2017
bee216b
Save registry space as per #2179
politas Nov 10, 2017
f221ccc
Unduplicate LoadCKANFromFile function
politas Nov 10, 2017
fd3fd49
Fix Tests.csproj merge fail
politas Nov 15, 2017
d67cf7b
Tidy Tests.csproj
politas Nov 15, 2017
9b4ed9f
Copy JSON serializer reference to output dir
HebaruSan Nov 17, 2017
bf170ac
Unduplicate GetReplacement, fix KspVersionCriteria.ToString()
politas Nov 18, 2017
838663d
Remove unused code
politas Nov 18, 2017
3b84325
Fix dialog on CLI replace command
politas Nov 29, 2017
8fc3bbd
CR/LF changes :(
politas Dec 23, 2017
955a0bd
Rebasing added back CheckMonoVersion() to main
politas Dec 23, 2017
33c48a3
Handle dependencies in core and CLI
politas Dec 23, 2017
c8ab009
Support replaced_by in ConsoleUI
HebaruSan Nov 9, 2017
e5f932a
Add image Resources via VisualStudio
politas Dec 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions CKAN.schema
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,25 @@
"items" : { "type" : "string" },
"uniqueItems" : true
},
"replaced_by" : {
"description" : "Optional pointer to mod that should be selected instead and treated as an update to this mod",
"type" : "object",
"properties" : {
HebaruSan marked this conversation as resolved.
Show resolved Hide resolved
"name" : {
"description" : "Identifier of the mod",
"$ref" : "#/definitions/identifier"
},
"version" : {
"description" : "Optional version",
"$ref" : "#/definitions/version"
},
"min_version" : {
"description" : "Optional minimum version",
"$ref" : "#/definitions/version"
}
},
"required" : [ "name" ]
},
"resources" : {
"description" : "Additional resources",
"type" : "object",
Expand Down
18 changes: 1 addition & 17 deletions Cmdline/Action/Install.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)
// Parse the JSON file.
try
{
CkanModule m = LoadCkanFromFile(ksp, filename);
CkanModule m = MainClass.LoadCkanFromFile(ksp, filename);
options.modules.Add($"{m.identifier}={m.version}");
}
catch (Kraken kraken)
Expand Down Expand Up @@ -257,21 +257,5 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)

return Exit.OK;
}

internal static CkanModule LoadCkanFromFile(CKAN.KSP current_instance, string ckan_file)
{
CkanModule module = CkanModule.FromFile(ckan_file);

// We'll need to make some registry changes to do this.
RegistryManager registry_manager = RegistryManager.Instance(current_instance);

// Remove this version of the module in the registry, if it exists.
registry_manager.registry.RemoveAvailable(module);

// Sneakily add our version in...
registry_manager.registry.AddAvailable(module);

return module;
}
}
}
38 changes: 32 additions & 6 deletions Cmdline/Action/List.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)
foreach (KeyValuePair<string, Version> mod in installed)
{
Version current_version = mod.Value;

string modInfo = string.Format("{0} {1}", mod.Key, mod.Value);
string bullet = "*";

if (current_version is ProvidesVersion)
Expand All @@ -61,26 +61,52 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)
else if (current_version is DllVersion)
{
// Autodetected dll
bullet = "-";
bullet = "A";
}
else
{
try
{
// Check if upgrades are available, and show appropriately.
log.DebugFormat("Check if upgrades are available for {0}", mod.Key);
CkanModule latest = registry.LatestAvailable(mod.Key, ksp.VersionCriteria());

log.InfoFormat("Latest {0} is {1}", mod.Key, latest);
CkanModule current = registry.GetInstalledVersion(mod.Key);

if (latest == null)
{
// Not compatible!
log.InfoFormat("Latest {0} is not compatible", mod.Key);
bullet = "X";
if ( current == null ) log.DebugFormat( " {0} installed version not found in registry", mod.Key);

//Check if mod is replaceable
if ( current.replaced_by != null )
{
ModuleReplacement replacement = registry.GetReplacement(mod.Key, ksp.VersionCriteria());
if ( replacement != null )
{
//Replaceable!
bullet = ">";
modInfo = string.Format("{0} > {1} {2}", modInfo, replacement.ReplaceWith.name, replacement.ReplaceWith.version);
}
}
}
else if (latest.version.IsEqualTo(current_version))
{
// Up to date
log.InfoFormat("Latest {0} is {1}", mod.Key, latest.version);
bullet = "-";
//Check if mod is replaceable
if ( current.replaced_by != null )
{
ModuleReplacement replacement = registry.GetReplacement(latest.identifier, ksp.VersionCriteria());
if ( replacement != null )
{
//Replaceable!
bullet = ">";
modInfo = string.Format("{0} > {1} {2}", modInfo, replacement.ReplaceWith.name, replacement.ReplaceWith.version);
}
}
}
else if (latest.version.IsGreaterThan(mod.Value))
{
Expand All @@ -95,7 +121,7 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)
}
}

user.RaiseMessage("{0} {1} {2}", bullet, mod.Key, mod.Value);
user.RaiseMessage("{0} {1}", bullet, modInfo);
}
}
else
Expand All @@ -107,7 +133,7 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)

if (!(options.porcelain) && exportFileType == null)
{
user.RaiseMessage("\r\nLegend: -: Up to date. X: Incompatible. ^: Upgradable. ?: Unknown. *: Broken. ");
user.RaiseMessage("\r\nLegend: -: Up to date. X: Incompatible. ^: Upgradable. >: Replaceable\r\n A: Autodetected. ?: Unknown. *: Broken. ");
// Broken mods are in a state that CKAN doesn't understand, and therefore can't handle automatically
}

Expand Down
172 changes: 172 additions & 0 deletions Cmdline/Action/Replace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using log4net;

namespace CKAN.CmdLine
{
public class Replace : ICommand
{
private static readonly ILog log = LogManager.GetLogger(typeof(Replace));

public IUser User { get; set; }

public Replace(IUser user)
{
User = user;
}


public int RunCommand(CKAN.KSP ksp, object raw_options)
Copy link
Member

Choose a reason for hiding this comment

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

What's the rationale for making a new command for this? From reading #904, it sounded like this would tie into the existing upgrade command as needed. If we're confident enough in the replacement mod to put it in the metadata, why would we want to trouble the user with knowing whether it's an upgrade or a replaced_by?

Copy link
Member Author

@politas politas Nov 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is sufficiently different that users should have to make a distinct choice to replace a mod rather than simply upgrading. We don't want users asking us why they have unknown mods in their list suddenly when they aren't dependencies. Also, the logic is quite distinct and it's way easier to implement as a separate command than increase the complexity of the existing command.

Copy link
Member

Choose a reason for hiding this comment

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

Good points. But if a user just wants to get the latest updates and doesn't particularly care if they're called XYZ or XYZReplaced, I could see it being an irritation to have to run multiple commands, every time.

What about using an optional flag to create a combination command? Something like:
ckan upgrade --all --with-replacements

{
ReplaceOptions options = (ReplaceOptions) raw_options;

if (options.ckan_file != null)
{
options.modules.Add(MainClass.LoadCkanFromFile(ksp, options.ckan_file).identifier);
}

if (options.modules.Count == 0 && ! options.replace_all)
{
// What? No mods specified?
User.RaiseMessage("Usage: ckan replace Mod [Mod2, ...]");
User.RaiseMessage(" or ckan replace --all");
return Exit.BADOPT;
}

// Prepare options. Can these all be done in the new() somehow?
var replace_ops = new RelationshipResolverOptions
{
with_all_suggests = options.with_all_suggests,
with_suggests = options.with_suggests,
with_recommends = !options.no_recommends,
allow_incompatible = options.allow_incompatible
};

var registry = RegistryManager.Instance(ksp).registry;
var to_replace = new List<ModuleReplacement>();

if (options.replace_all)
{
log.Debug("Running Replace all");
var installed = new Dictionary<string, Version>(registry.Installed());
Copy link
Member

Choose a reason for hiding this comment

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

Installed already returns Dictionary<string, Version>. Do we really need to make a copy of it? That will presumably cost CPU time. This should work:

var installed = registry.Installed();

Copy link
Member Author

@politas politas Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why it is like that. I copied the code from the Upgrade command, and that bit is unchangedThe comment on the last change to that code says "remove registry-related getters in KSP class to reduce cyclomatic complexity" - it was by @ayan4m1 in #1828
I note that ayan4m1 didn't introduce this particular new Dictionary<string, Version>, but he didn't change it, either. That may have been because his focus was elsewhere and this was simply on necessary adjustment among many, but perhaps he saw some rationale behind making this a copy rather than a reference? Perhaps we don't want changes we make to the registry in the process to mess up our processing?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough; it would make sense to reserve this for a separate project that addresses all commands at once, if needed.


foreach (KeyValuePair<string, Version> mod in installed)
{
Version current_version = mod.Value;

if ((current_version is ProvidesVersion) || (current_version is DllVersion))
{
continue;
}
else
{
try
{
log.DebugFormat("Testing {0} {1} for possible replacement", mod.Key, mod.Value);
// Check if replacement is available

ModuleReplacement replacement = registry.GetReplacement(mod.Key, ksp.VersionCriteria());
if (replacement != null)
{
// Replaceable
log.InfoFormat("Replacement {0} {1} found for {2} {3}",
replacement.ReplaceWith.identifier, replacement.ReplaceWith.version,
replacement.ToReplace.identifier, replacement.ToReplace.version);
to_replace.Add(replacement);
}
}
catch (ModuleNotFoundKraken)
{
log.InfoFormat("{0} is installed, but it or its replacement is not in the registry",
mod.Key);
}
}
}
}
else
{
foreach (string mod in options.modules)
Copy link
Member

Choose a reason for hiding this comment

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

Currently if I try to replace mods that aren't replaceable...

$ mono ckan.exe replace KerbalEngineerRedux

Replacing modules...


Done!

... there's no error message, it just says that it finished. Similarly, I can even give it completely invalid mod names:

$ mono ckan.exe replace ThisIsNotAValidMod

Replacing modules...


Done!

If I'm trying to replace a mod that actually is replaceable but I mistype the name or capitalize it wrong, I might think I have just performed the replacement when in fact I have done nothing. I think it would be good to raise errors here if mods the user specified are invalid or not replaceable.

Copy link
Member Author

Choose a reason for hiding this comment

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

All those sort of issues are logInfos. By default, the CLI only reports actual actions it takes.

Copy link
Member

Choose a reason for hiding this comment

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

No, the CLI reports errors when mods are missing:

$ mono ckan install ThisIsNotAValidMod
Module ThisIsNotAValidMod required but it is not listed in the index, or not available for your version of KSP.
If you're lucky, you can do a `ckan update` and try again.
Try `ckan install --no-recommends` to skip installation of recommended modules.

{
try
{
log.DebugFormat("Checking that {0} is installed", mod);
CkanModule modToReplace = registry.GetInstalledVersion(mod);
if ( modToReplace != null)
{
log.DebugFormat("Testing {0} {1} for possible replacement", modToReplace.identifier, modToReplace.version);
try
{
// Check if replacement is available
ModuleReplacement replacement = registry.GetReplacement(modToReplace.identifier, ksp.VersionCriteria());
if (replacement != null)
{
// Replaceable
log.InfoFormat("Replacement {0} {1} found for {2} {3}",
replacement.ReplaceWith.identifier, replacement.ReplaceWith.version,
replacement.ToReplace.identifier, replacement.ToReplace.version);
to_replace.Add(replacement);
}
if (modToReplace.replaced_by != null)
{
log.InfoFormat("Attempt to replace {0} failed, replacement {1} is not compatible",
mod, modToReplace.replaced_by.name);
}
else
{
log.InfoFormat("Mod {0} has no replacement defined for the current version {1}",
modToReplace.identifier, modToReplace.version);
}
}
catch (ModuleNotFoundKraken)
{
log.InfoFormat("{0} is installed, but its replacement {1} is not in the registry",
mod, modToReplace.replaced_by.name);
}
}
}
catch (ModuleNotFoundKraken kraken)
{
User.RaiseMessage("Module {0} not found", kraken.module);
}
}
}
if (to_replace.Count() != 0)
{
User.RaiseMessage("\r\nReplacing modules...\r\n");
foreach (ModuleReplacement r in to_replace)
{
User.RaiseMessage("Replacement {0} {1} found for {2} {3}",
r.ReplaceWith.identifier, r.ReplaceWith.version,
r.ToReplace.identifier, r.ToReplace.version);
}

bool ok = User.RaiseYesNoDialog("\r\nContinue?");

if (!ok)
{
User.RaiseMessage("Replacements canceled at user request.");
return Exit.ERROR;
}

// TODO: These instances all need to go.
try
{
ModuleInstaller.GetInstance(ksp, User).Replace(to_replace, replace_ops, new NetAsyncModulesDownloader(User));
User.RaiseMessage("\r\nDone!\r\n");
}
catch (DependencyNotSatisfiedKraken ex)
{
User.RaiseMessage("Dependencies not satisfied for replacement, {0} requires {1} {2} but it is not listed in the index, or not available for your version of KSP.", ex.parent, ex.module, ex.version);
}
}
else
{
User.RaiseMessage("No replacements found.");
return Exit.OK;
}

return Exit.OK;
}
}
}
20 changes: 2 additions & 18 deletions Cmdline/Action/Upgrade.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)
UpgradeOptions options = (UpgradeOptions) raw_options;

if (options.ckan_file != null)
{
options.modules.Add(LoadCkanFromFile(ksp, options.ckan_file).identifier);
{
options.modules.Add(MainClass.LoadCkanFromFile(ksp, options.ckan_file).identifier);
}

if (options.modules.Count == 0 && ! options.upgrade_all)
Expand Down Expand Up @@ -132,21 +132,5 @@ public int RunCommand(CKAN.KSP ksp, object raw_options)

return Exit.OK;
}

internal static CkanModule LoadCkanFromFile(CKAN.KSP current_instance, string ckan_file)
{
CkanModule module = CkanModule.FromFile(ckan_file);

// We'll need to make some registry changes to do this.
RegistryManager registry_manager = RegistryManager.Instance(current_instance);

// Remove this version of the module in the registry, if it exists.
registry_manager.registry.RemoveAvailable(module);

// Sneakily add our version in...
registry_manager.registry.AddAvailable(module);

return module;
}
}
}
9 changes: 5 additions & 4 deletions Cmdline/CKAN-cmdline.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@
<Reference Include="CommandLine, Version=1.9.71.2, Culture=neutral, PublicKeyToken=de6f01bd326f8c32, processorArchitecture=MSIL">
<HintPath>..\_build\lib\nuget\CommandLineParser.1.9.71\lib\net45\CommandLine.dll</HintPath>
</Reference>
<Reference Include="log4net, Version=2.0.8.0, Culture=neutral, PublicKeyToken=669e0ddf0bb1aa2a, processorArchitecture=MSIL">
<Reference Include="System" />
<Reference Include="System.Transactions" />
<Reference Include="log4net, Version=2.0.8.0, Culture=neutral, PublicKeyToken=669e0ddf0bb1aa2a">
<HintPath>..\_build\lib\nuget\log4net.2.0.8\lib\net45-full\log4net.dll</HintPath>
</Reference>
<Reference Include="Newtonsoft.Json, Version=10.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">
<Reference Include="Newtonsoft.Json, Version=10.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed">
<HintPath>..\_build\lib\nuget\Newtonsoft.Json.10.0.3\lib\net45\Newtonsoft.Json.dll</HintPath>
</Reference>
<Reference Include="System" />
<Reference Include="System.Transactions" />
</ItemGroup>
<ItemGroup>
<Compile Include="..\_build\meta\GlobalAssemblyVersionInfo.cs">
Expand All @@ -63,6 +63,7 @@
<Compile Include="Action\List.cs" />
<Compile Include="Action\Remove.cs" />
<Compile Include="Action\Repair.cs" />
<Compile Include="Action\Replace.cs" />
<Compile Include="Action\Repo.cs" />
<Compile Include="Action\Search.cs" />
<Compile Include="Action\Show.cs" />
Expand Down
Loading