-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Comments
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. |
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 For what it's worth, my AAE full roms path on my server is: '/mnt/user/Hyperspin/ROMS/AAE/ROMS' SCREENSHOTRAWINFO: [RomM][alembic.runtime.migration] Context impl SQLiteImpl. The above exception was the direct cause of the following exception: Traceback (most recent call last): |
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 |
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??? |
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 @zurdi15 might have some thoughts on this |
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 |
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. |
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 |
@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. |
Nice one guys! I think your onto a winner! |
Hey @thestraycat just wanted to let you know this is now available in |
@gantoine - Thanks i'll keep an eye out for it! |
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)
The text was updated successfully, but these errors were encountered: