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

Check grandparent+ directories for find and find_regexp #2120

Merged
merged 2 commits into from
Oct 3, 2017

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Sep 28, 2017

ZIP files can contain entries for directories, but it's optional. Currently, CKAN behaves differently depending on whether these entries are present, which is probably bad since it may not be controllable by or even apparent to the user.

find and find_regexp currently loop through each entry in the ZIP and check whether they match against the search pattern. If file matching is turned on, the names of file entries are checked, otherwise their immediate parent folders are checked. If there are multiple matches, the shortest one is used.

This algorithm only works in two cases:

  1. The directory being searched for is present as a directory entry
  2. The directory being searched for does not have a directory entry but is the immediate parent directory of a file

It fails for an important third case:

  • The directory being searched for does not have a directory entry but is a grandparent, great-grandparent, etc. directory of one of the other entries

For example, take a look at PlanetCerillion (many files omitted for brevity):

https://github.com/KSP-CKAN/NetKAN/blob/f6a054d20ca962da0f11bb5e2abf47616996b3a1/NetKAN/PlanetCerillion.netkan#L3-L7

  Length      Date    Time    Name
---------  ---------- -----   ----
        0  2017-09-23 13:57   Planet_Cerillion/Cache/
   706632  2017-09-23 13:57   Planet_Cerillion/Cache/1-Cerillion.bin
   706632  2017-09-23 13:57   Planet_Cerillion/Cache/2-Mavis.bin
        0  2017-09-23 11:57   Planet_Cerillion/Configs/
    69702  2017-09-23 14:08   Planet_Cerillion/Configs/atmo.cfg
    23211  2017-09-23 13:27   Planet_Cerillion/Configs/Cerillion_1.cfg
        0  2017-09-23 13:22   Planet_Cerillion/Data/
    17970  2017-07-12 11:18   Planet_Cerillion/Data/Arch.obj
    94598  2017-04-16 12:58   Planet_Cerillion/Data/Artifact1.obj
        0  2017-09-23 11:55   Planet_Cerillion/Data/Cerillion/
  8388608  2017-07-13 11:35   Planet_Cerillion/Data/Cerillion/inscatter.half
     8192  2017-07-13 11:35   Planet_Cerillion/Data/Cerillion/irradiance.half
        0  2017-09-23 13:22   Planet_Cerillion/Data/Disparia/
  8388608  2017-09-23 13:11   Planet_Cerillion/Data/Disparia/inscatter.half
     8192  2017-09-23 13:11   Planet_Cerillion/Data/Disparia/irradiance.half
        0  2017-09-23 13:53   Planet_Cerillion/PluginData/
  8388736  2016-03-29 22:28   Planet_Cerillion/PluginData/Cerillion_biome.dds
  4113460  2016-03-27 15:41   Planet_Cerillion/PluginData/Cerillion_color.png
      317  2017-09-23 14:29   License.txt
      701  2017-04-11 17:47   README.txt

The find clause is trying to find the Planet_Cerillion directory, but it is not present as a directory entry, and all the files are contained within sub-subfolders rather than directly under Planet_Cerillion. This causes "Could not find Planet_Cerillion entry in zipfile to install".

With the attached code changes, the algorithm will check all grandparent, great-grandparent, etc. directories. This fixes the Planet_Cerillion case above, as well as the problem found in KSP-CKAN/NetKAN#5841 and probably others.

@politas politas merged commit a2dd48b into KSP-CKAN:master Oct 3, 2017
@politas
Copy link
Member

politas commented Oct 3, 2017

That looks great. Any chance you could put together a test case to make sure this doesn't get broken in future?

@HebaruSan
Copy link
Member Author

HebaruSan commented Oct 3, 2017

Good idea. I don't know much about creating tests, though. How do I run the current tests locally? I thought I saw them at the end of the build process, but submitting this PR dispelled that notion.

UPDATE: Looks like it's nunit-console Tests/bin/Debug/CKAN.Tests.dll, however that's failing for me currently:

Errors and Failures:
1) Test Error : Tests.NetKAN.Sources.Spacedock.SpacedockApiTests.GetsModCorrectly
   CKAN.Kraken : Could not get the mod from SD, reason: Mod not found..
  at CKAN.NetKAN.Sources.Spacedock.SpacedockApi.GetMod (System.Int32 modId) [0x00043] in <0f08e65d79cf45928efe78c668c7e3be>:0 
  at Tests.NetKAN.Sources.Spacedock.SpacedockApiTests.GetsModCorrectly () [0x00012] in <85a37031cbf24fed9f89fdd9a7266a11>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00038] in <8f2c484307284b51944a1a13a14c0266>:0 

Am I missing something?

UPDATE II: If I skip that test with --exclude=Online, it finishes the remaining ones with no failures. Hopefully that won't cause problems with a new test for this fix...

UPDATE III: Oops, Tests/bin/Debug was over a year old, presumably from when I first cloned this repo and before you moved everything into _build. Using _build/out/CKAN.Tests/Debug/bin/CKAN.Tests.dll instead, I discover that my nunit-console is out of date:

NUnit-Console version 2.6.4.0
...
NUnit.Core.UnsupportedFrameworkException: Skipped loading assembly CKAN.Tests because it references an unsupported version of the nunit.framework, 3.6.1.0

2.6.4.0 is the newest version for the most recent Ubuntu release, but I saw a rumor that it's also available via NuGet. Wish me luck!

UPDATE IV: I finally succeeded in running the current tests (and my new one as well, though the test data is not complete yet). Added a section to the wiki to hopefully make it easier for the next person (and myself, next time).

@politas
Copy link
Member

politas commented Oct 4, 2017

You can run the tests with ./build test

@HebaruSan
Copy link
Member Author

Thanks, that's much simpler! Wiki updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants