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

Multi-match 'find', allow 'as' for Ships and GameData #3064

Merged
merged 2 commits into from
Jun 11, 2020

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 31, 2020

Background

CKAN supports multiple properties for specifying which files to install (only one is allowed per modue):

  • install[].file - A path to a file or folder, starting from the ZIP root
  • install[].find - A partial path to a folder, or file if install[].find_matches_files is true
  • install[].find_regexp - A regular expression matching a folder, or file if install[].find_matches_files is true

The install[].as property can be used to install a folder under a different name.

Motivation

The implementation of find and find_regexp is kind of a hack; it relies on converting them to a file value based on the contents of a ZIP file. This creates a number of problems and limitations, detailed under the next section heading. (It has also created bugs historically, see #2120 for one of my earliest fixes.)

ModuleInstaller has some static functions that require a ModuleInstallDescriptor parameter (or one of its properties, related to the previous paragraph), which is a classic indication that encapsulation has been violated by putting those functions in the wrong class.

As long as we're doing a pre-release for v1.28.0 anyway, why not rip-and-replace another big core piece of functionality?

Problems and limitations

  • find_regexp is only allowed to match once, so setting it to "\\.craft$" will only install one file even if there are several, and which one is chosen is unpredictable (and the same is true but less conspicuous for find)
  • as arbitrarily isn't allowed for Ships or GameData, despite this being useful (e.g., you can put a Ships folder into Ships/VAB)
  • The dependence of find and find_regexp on a specific ZIP means that when the ZIP changes, the validity of the metadata can change along with it (see Fix CST100Starliner install NetKAN#7945 for a netkan that was valid until it wasn't)
  • find and find_regexp arbitrarily only match (containing) folders unless you set install[].find_matches_files to true
  • ModuleInstallDescriptor.DefaultInstallStanza requires a ZipFile parameter to work, because it is implemented in terms of find (which should be nobody else's business) and therefore needs to call ConvertFindToFile, which needs the ZIP

Changes

  • ModuleInstallDescriptor.ConvertFindToFile is deleted 😂; find and find_regexp are now supported as first-class concepts alongside file, all of which now flow into a single shared compiled Regex object initialized according to their particular quirks
    • ModuleInstallDescriptor.DefaultInstallStanza no longer needs a ZIP file to figure itself out
    • There may be a small performance benefit as we no longer generate new Regex objects on the fly in several spots
    • Assorted error messages are updated to be less confusing
  • The ModuleInstaller.FindInstallableFiles that takes a ModuleInstallDescriptor param is now a method of ModuleInstallDescriptor
    • ModuleInstallDescriptor.IsWanted is now private because it is an implementation detail of this class
  • ModuleInstaller.TransformOutputName is now a method of ModuleInstallDescriptor, rewritten to no longer work only with file
    • A source folder of GameData or Ships can now be renamed in-passing via install[].as, just like any other folder
    • A source folder of Missions will be removed from the path if install[].as isn't specified, just like GameData or Ships
  • find_matches_files no longer does anything; files and folders are both always matched (I can try to restore this if we need it, but it just gets in the way from what I've seen)
  • Tests are updated to reflect the above changes

Now this will work:

    "install": [ {
        "file": "Ships",
        "install_to": "Ships",
        "as": "VAB"
    } ]

And this will match all craft files:

    "install": [ {
        "find_regexp": "\\.craft$",
        "install_to": "Ships/VAB"
    } ]

@HebaruSan HebaruSan added Bug Something is not working as intended Enhancement New features or functionality Core (ckan.dll) Issues affecting the core part of CKAN Pull request labels May 31, 2020
@HebaruSan HebaruSan requested a review from DasSkelett May 31, 2020 00:46
@DasSkelett
Copy link
Member

With find and find_regexp now matching everything they... match, instead of only the first one they encounter, there are now cases where the installation fails hard, if there are two or more matching files, or folders containing files with the same names:


About to install...

 * SpaceX Launch Vehicles 6.2 (cached)

Oh no!

It looks like you're trying to install a mod which is already installed,
or which conflicts with another mod which is already installed.

As a safety feature, the CKAN will *never* overwrite or alter a file
that it did not install itself.

If you wish to install SpaceXLaunchVehicles 6.2 via the CKAN,
then please manually uninstall the mod which owns:

Ships/VAB/F9 Demo.craft

and try again.
Your GameData has been returned to its original state.
Error during installation!
If the above message indicates a download error, please try again. Otherwise, please open a issue for us to investigate.
If you suspect a metadata problem: https://github.com/KSP-CKAN/NetKAN/issues/new/choose
If you suspect a bug in the client: https://github.com/KSP-CKAN/CKAN/issues/new/choose

This is of course always a result of wrong metadata (see KSP-CKAN/NetKAN#7919 (comment) for this case), and thus probably a good thing so we get reports about it.

But I think we should display a different error message in this case. For example, we could try to scan for duplicate files before installation.
This check might also be handy in netkan.exe .

@HebaruSan
Copy link
Member Author

This check might also be handy in netkan.exe .

Good idea, if netkan.exe prints a warning, then we can catch and fix all of these before the new client is released.

@HebaruSan
Copy link
Member Author

$ netkan.exe NetKAN/SpaceXLaunchVehicles.netkan 
75510 [1] FATAL CKAN.NetKAN.Program (null) - Multiple files attempted to install to:
Ships/VAB
Ships/VAB/F9 Demo.craft
Ships/VAB/SpaceX Crew Dragon F9 Stack.craft
Ships/VAB/SpaceX Crew Dragon FH Stack.craft
Ships/VAB/SpaceX Crew Dragon.craft
Ships/VAB/SpaceX Falcon 1.craft
Ships/VAB/SpaceX Falcon 9 1_0.craft
Ships/VAB/SpaceX Falcon 9 1_1.craft
Ships/VAB/SpaceX Falcon 9 Block 5.craft
Ships/VAB/SpaceX Falcon Heavy.craft
Ships/VAB/SpaceX Starlink F9 Stack.craft

@HebaruSan HebaruSan removed the In progress We're still working on this label Jun 7, 2020
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.

While I can't 100% certify this has no bugs, it does seem to work, and I hope we are going to find possible unexpected behaviours in the pre-release 😄

@HebaruSan
Copy link
Member Author

Oh, I think we'll find unexpected behaviors in the next netkan pass... 🤞

@HebaruSan HebaruSan merged commit cfb5881 into KSP-CKAN:master Jun 11, 2020
@HebaruSan HebaruSan deleted the fix/convert-find-file branch June 11, 2020 18:00
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 Enhancement New features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants