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

Fix 404/500 issues on tools/scummvm for both systems and images #2

Merged
merged 4 commits into from Mar 10, 2022

Conversation

pkegg
Copy link
Collaborator

@pkegg pkegg commented Mar 8, 2022

Issues Fixed

  • tools and scummvm systems show 404
  • scummvm and tools have images in a non-standard location and also gives 404
  • Dates on certain tools scripts have dates which issue a 500
  • Roms in gamelist.xml that don't exist on the file system are shown (and they really shouldn't show up)
  • Rom not found with non-ascii file names (causing 'duplicates' before previous issue with hiding files not found in gamelist fixed)
  • 'Editing' a rom that's not in gamedata gives 500. entry wouldn't work as it didn't create a new UUID. (fix is to do the same as 'upload' and create a new XML element with UUID)

Root Cause and Logic to Address Issues

  • Tools and scummvm have non-standard locations for their ‘roms_folder’.
    • Incorrect assumption in code: The main roms folder (/storage/roms) + system name can be used to find the roms directory for a given system
    • Fix logic: look up system folder in es_systems.cfg. NOTE: a simplification possible by using this lookup - can use 'name' of system to identify it (no need for 'folder')
  • Tools and scummvm have non-standard locations for their ‘images’
    • Incorrect assumption in code: The system roms folder + “images/”+ image name can be used to find an image
    • Fix logic: use path from gamedata.xml and don’t derive images path
      • Also fix upload to create 'images' folder if it does not exist (needed for scummvm)
        - Fix image logic to not issue 500 error on file size if file doesn't exit (gamedata.xml references missing file). I just ran into this fixing image upload and seemed like a good idea.
  • Tools provides dates like ‘2020’ which are not parsed
    • Fix logic: if a date cannot be parsed for formatting, just display it.
  • Rom not found with non-ascii file. It appears that the text cleanup processing with ftfy fix_text was causing the utf-8 normalization differences with the rom name. This post-processing didn't matter for the cleaned up text per say (it looks fine), but when using the file it appeared to not exist unless it's an exact binary match.
  • Fix logic avoid calling the find_normalized methods on the rom name.

Fix Overview

Most of the above fixes are simple to implement. However, getting the system roms directory from es_systems.cfg is not always obvious as it is often needed on a per game basis in contexts where es_systems.cfg has not been parsed. Parsing it for each game (for example) would be a huge overhead.

The proposed solution is to lazily cache the systems.cfg object in memory and use it across all APIs. This is the most controversial part of this PR as there is no existing pattern to follow for this in the code. A few thoughts as to why this is ok:

  • the file is read only and the in memory object is not updated so it should be ok to use concurrently.
  • In the event the server were to be multithreaded, each thread would just cache es_systems.cfg so it should work without additional logic.

NOTE on memory: Caching the es_systems.cfg object does potentially use a bit more memory, but it should be relatively small. If this were to be an issue, parsing and keeping a dictionary with only the system paths would be a good option.

Testing

  • Ensure tools system does not display a 404
    • Ensure each script inside tools shows up w/o a 404/500.
    • Ensure that the 'images' for tools show up properly in UI
    • Ensure that scripts from tools can be launched with 'launch' button.
    • Ensure that 'Release Date' shows up on 351Files as 2021
  • Ensure that screenshots system does not display a 404 (might make sense to hide/enhance in the future)
  • Ensure that scummvm system does not display a 404
    • Ensure scummvm game can be launched via launch button.
  • Ensure existing systems which have been scraped work properly
    • Ensure launch works
    • Ensure download rom works
    • Ensure download save state works
    • Ensure screen cap of save state works
    • Ensure edit rom works
    • Ensure upload rom works

Quick memory usage check:

Nothing fancy - just check total memory used reported by top before and after killing python server.

  • Without fixes after clicking around in systems/games/edit/etc: 210.4 - 194.4 = 16MB
  • With fixes after clicking around into systems/games/edit/etc, etc: 209.5 - 194.0 = 15.5MB
  • I kind of expect that memory usage isn't actually better and my testing was just not exact enough, but it at least shows that memory usage isn't way more or anything.

**Issues Fixed:**
- tools and scummvm systems show 404
- scummvm and tools have images in a non-standard location and also gives 404
- Dates on certain tools scripts have dates which issue a 500

**Root Cause and Logic to Address Issues**:
- Tools and scummvm have non-standard locations for their ‘roms_folder’.
	- **Incorrect assumption in code**: The main roms folder (/storage/roms) + system name can be used to find the roms directory for a given system
	- **Fix logic**: look up system folder in es_systems.cfg.  NOTE: a simplification possible by using this lookup - can use 'name' of system to identify it (no need for 'folder')
- Tools and scummvm  have non-standard locations for their ‘images’
	- **Bad assumption in code**: The system roms folder + “images/”+ image name can be used to find an image
	- **Fix logic**: use path from gamedata.xml and don’t derive images path
          - Also fix upload to create 'images' folder if it does not exist
          - Fix image logic to not issue 500 on file size if file doesn't exit (gamedata.xml references missing file)
- Tools provides dates like ‘2020’ which are not parsed
	- **Fix logic**: if a date cannot be parsed for formatting, just display it.

**Fix Overview**
Most of the above fixes are simple to implement.  However, getting the system roms directory from es_systems.cfg is not always obvious as it is often needed on a per game basis in contexts where es_systems.cfg has not been parsed. Parsing it for each game (for example) would be a huge overhead.

The proposed solution is to lazily cache the systems.cfg object in memory and use it across all APIs.  This is the most controversial part of this PR as there is no existing pattern to follow for this in the code.  A few thoughts as to why this is ok:
- the file is read only and the in memory object is not updated so it should be ok to use concurrently.
- In the event the server were to be multithreaded, each thread would just cache es_systems.cfg so it should work without additional logic.

NOTE on memory: Caching the es_systems.cfg object does potentially use a bit more memory, but it should be relatively small.  If this were to be an issue, parsing and keeping a dictionary with only the system paths would be a good option.
…le are not shown

2. Ensure that the rom name is never sent to ftfy 'fix_text'.  This is because fix_text changes the utf-8 'normalization' which can break loading of files with non-ascii characters such as: 'Pokémon' as it is converted to a visually equivalent, but not binary equivalent value.
…es not have a value in gamelist.xml (similar to how it's done on upload)

Also create the new <game> element on a new line in the XML (make it slightly prettier)
@dhwz dhwz merged commit a375824 into AmberELEC:main Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants