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

[Bug] Case-sensitivity issue where SNES/ROMS fails but SNES/roms works. #356

Closed
thestraycat opened this issue Aug 31, 2023 · 12 comments · Fixed by #376
Closed

[Bug] Case-sensitivity issue where SNES/ROMS fails but SNES/roms works. #356

thestraycat opened this issue Aug 31, 2023 · 12 comments · Fixed by #376
Assignees
Labels
bug Something isn't working

Comments

@thestraycat
Copy link

thestraycat commented Aug 31, 2023

RomM version
Latest (v1.10)

Describe the bug
Romm seems to fail on case-sensitivty of the 'roms folder. It seems to fail on '/SNES/ROMS' but will process 'SNES/roms'

To Reproduce
Steps to reproduce the behavior:
Simply rename the roms folder from a platform to ROMS

Expected behavior
That romm can process folders regardless of case sensitivity (either roms or ROMS)

@thestraycat thestraycat added the bug Something isn't working label Aug 31, 2023
@gantoine
Copy link
Member

Can you post the output of docker logs when the error appears? And what exactly is the behaviour you're seeing?

I'm trying this on macos and it seems to work fine, but FWIW macos doesn't respect case sensitivity for folder names.

@thestraycat
Copy link
Author

thestraycat commented Sep 1, 2023

Sure.

As i had to rename my 'AAE/ROMS' folder to 'AAE/roms' it had already scanned some of the roms. So to re-create the issue i've just renamed the 'AAE/roms' folder back to 'AAE/ROMS' and told it to re-scan all roms. Restarted the container fresh and the logs below are from a fresh run.

My docker (on unraid) mapped paths are as below

/romm/config >> /mnt/cache/appdata/romm
/romm/resources >> /mnt/cache/appdata/romm/resources
/romm/logs >> /mnt/cache/appdata/romm/logs
/romm/database >> /mnt/cache/appdata/romm/database
/romm/library >> /mnt/user/Hyperspin/ROMS

For what it's worth, my AAE full roms path on my server is: '/mnt/user/Hyperspin/ROMS/AAE/ROMS'

SCREENSHOT

image

RAW

INFO: [RomM][alembic.runtime.migration] Context impl SQLiteImpl.
INFO: [RomM][alembic.runtime.migration] Will assume non-transactional DDL.
INFO: Uvicorn running on http://0.0.0.0:5000 (Press CTRL+C to quit)
INFO: Started parent process [19]
INFO: Started server process [22]
INFO: Waiting for application startup.
INFO: Application startup complete.
INFO: Started server process [21]
INFO: Waiting for application startup.
INFO: Application startup complete.
INFO: ('127.0.0.1', 60508) - "WebSocket /socket.io/?EIO=4&transport=websocket" [accepted]
INFO: connection open
WARNING: [RomM][2023-09-01 04:58:48] Twitch token invalid: fetching a new one...
WARNING: [RomM][2023-09-01 04:58:48] Twitch token invalid: fetching a new one...
INFO: [RomM][2023-09-01 04:58:49] Twitch token fetched!
INFO: [RomM][2023-09-01 04:58:49] Twitch token fetched!
INFO: connection closed
INFO: 192.168.1.167:0 - "GET /platforms HTTP/1.0" 200 OK
INFO: ('127.0.0.1', 56500) - "WebSocket /socket.io/?EIO=4&transport=websocket" [accepted]
INFO: connection open
INFO: [RomM][2023-09-01 05:00:01] 🔎 Scanning
INFO: [RomM][2023-09-01 05:00:25] · AAE
WARNING: [RomM][2023-09-01 05:00:25] AAE not found in IGDB
Task exception was never retrieved
future: <Task finished name='Task-19' coro=<AsyncServer._handle_event_internal() done, defined at /usr/local/lib/python3.10/dist-packages/socketio/asyncio_server.py:522> exception=Roms not found for platform AAE. Check RomM folder structure here: https://github.com/zurdi15/romm#-folder-structure>
Traceback (most recent call last):
File "/back/utils/fs.py", line 226, in get_roms
fs_single_roms: list[str] = list(os.walk(roms_file_path))[0][2]
IndexError: list index out of range

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/usr/local/lib/python3.10/dist-packages/socketio/asyncio_server.py", line 524, in _handle_event_internal
r = await server._trigger_event(data[0], namespace, sid, *data[1:])
File "/usr/local/lib/python3.10/dist-packages/socketio/asyncio_server.py", line 558, in _trigger_event
ret = await handler(*args)
File "/back/endpoints/scan.py", line 88, in scan_handler
await scan_platforms(platforms, complete_rescan)
File "/back/endpoints/scan.py", line 48, in scan_platforms
fs_roms = fs.get_roms(scanned_platform.fs_slug)
File "/back/utils/fs.py", line 228, in get_roms
raise RomsNotFoundException(p_slug) from exc
utils.exceptions.RomsNotFoundException: Roms not found for platform AAE. Check RomM folder structure here: https://github.com/zurdi15/romm#-folder-structure

@gantoine
Copy link
Member

gantoine commented Sep 1, 2023

Ok yeah this is definitely a linux vs macos thing, which is why I can't reproduce it. The code expects the library structure to be /roms/<platform> or /<platform>/roms, so ROMS is not supported. You'll have to change the name of the folder to roms in lowercase.

@thestraycat
Copy link
Author

would it not be better to change the code to be case-insensitive? As lots of people will run this on linux... whats really weird is that the 'platform' was fine in capitals For example this works:

AAE/roms

Based on the fact that AAE is in capitals and roms has to be lower case surely this needs to be added to the code base???

@gantoine
Copy link
Member

gantoine commented Sep 2, 2023

It's because the platform could be any slug (any string really), and we store it as-is when building the path to each ROM file. The roms folder however is hardcoded, and supporting both adds code complexity for something that's trivial to change in the filesystem.

@zurdi15 might have some thoughts on this

@zurdi15
Copy link
Member

zurdi15 commented Sep 3, 2023

Yeah I am with @gantoine . This could cause a lot of potential problems with how directories are managed in linux because you can have both roms and ROMS folders in the same node, so it can be problematic when detecting the RomM folder structre. I will test it anyway to see how it behaves but for now I suggest just to rename to roms

@thestraycat
Copy link
Author

thestraycat commented Sep 4, 2023

I hear you, but by renaming the folder from ROMS to roms could potentially break everybodies emulator setups when running on linux, in all due respect, i dont think anyone will want to jeorpardize their rocket-launcher roms paths (which connects the roms to the emulators and manages the link paths) to be compliant with a ROM manager, so you could be excluding a large group of people who would have otherwised used this. is there a reason that ROMM canot use a variable for the roms library that is configured at first startup? I'd be happy to rescan to keep my folder structure... I've got 145 folders to change it in. I mean it would be infinitely more flexible if it could... It's hard to bend your potential users to a rigid and strict path format for the sake of using a tool surely? I'd love to use it. But i can't be breaking everything to use it.

@gantoine
Copy link
Member

gantoine commented Sep 4, 2023

I can appreciate your reason for wanting to specify the roms folder name.

@zurdi15 Is this the extent of changes that need to happen to support this? I can't test it cause I'm on macos lol https://github.com/zurdi15/romm/tree/roms-folder-name-env

@zurdi15
Copy link
Member

zurdi15 commented Sep 4, 2023

@thestraycat I understand, make sense tho. I need to think about how should be the best UX for this in terms of not overload the docker-compose with small configurations, but the approach @gantoine made should work good enough.

I'll test it and let you know.

@thestraycat
Copy link
Author

Nice one guys! I think your onto a winner!

@gantoine
Copy link
Member

gantoine commented Sep 18, 2023

Hey @thestraycat just wanted to let you know this is now available in dev-2.0.0-rc.5, and will be included in the upcoming 2.0.0 release.

@thestraycat
Copy link
Author

@gantoine - Thanks i'll keep an eye out for it!

@zurdi15 zurdi15 mentioned this issue Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants