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

Roms not being verified properly #2

Closed
muggsyd opened this issue Sep 2, 2020 · 8 comments
Closed

Roms not being verified properly #2

muggsyd opened this issue Sep 2, 2020 · 8 comments

Comments

@muggsyd
Copy link

muggsyd commented Sep 2, 2020

I have a complete set of NoIntro Sega Master System roms verified by RomVault using the nointro dat
System: Sega - Master System - Mark III
Version: 20200429-062328
Games: 586

When I use this with Oxyromon It only verifies approx 14 of them and says the rest do not match
Also, when the new folder structure is created the new zip file also includes the .sms name
So for Aerial Assault (USA).sms the new zip file is called Aerial Assault (USA).sms.zip

it also does not appear to validate the Test roms either except for the following
Test Game (Asia).rom
Test Game (Japan).rom
Test Game (USA, Europe) (Beta).rom

I believe I am using version 0.3.0 however when I do a --version it does show oxyromon 0.1.0

output from cargo install -f oxyromon

Replacing /usr/local/cargo/bin/oxyromon
Replaced package oxyromon v0.3.0 with oxyromon v0.3.0 (executable oxyromon)
root@rust1:~/Emulation# oxyromon --version
oxyromon 0.1.0

Let me know if you need anything else. I am really keen on a rom manager I can use in a docker container :)

@alucryd
Copy link
Owner

alucryd commented Sep 2, 2020

Hi there, thanks for taking the time to report this.

  • I just fixed the cli version, that was hardcoded, the git HEAD is now it's pulling the info directly from Cargo.toml.
  • The archive naming is intended, I should probably clarify that in the readme. Basically archives with only one file take the rom name and append the archive extension, while archives with multiple files take the game name instead. It is to better identify what's actually inside the archive, but I could of course change this behavior based on a config option in the future.
  • As for the matching errors, it is possible that the archives lack the embedded CRCs that I use to match against the database. It is supposed to just extract the files when they are missing, but as I don't possess such archives and don't even know how to generate some, it's never been tested.

Would you mind cloning the git HEAD and run the unit tests (cargo test), see if you can get any useful information as to why even the test games aren't recognized? Note that you'll need all third-party executables in your path to do that, namely 7z, chdman and maxcso, you can just ignore the chd and cso tests if you don't have the last 2.

Beware that I'm in the middle of a major rewrite of the database layer, and I haven't verified that everything is working yet, but all current tests should pass. This should not be used in production yet though, I still need to write tests for sorting and conversion.

What version of 7z do you have installed? Could you email me one of the archive that isn't properly recognized? Some roms may have some kind of header that should be ignored, although to my knowledge the only systems that do this are NES and FDS.

@muggsyd
Copy link
Author

muggsyd commented Sep 2, 2020 via email

@alucryd
Copy link
Owner

alucryd commented Sep 3, 2020

Yeah, error[E0658] means rust is too old, this particular feature went stable not long ago.
As for the CARGO_BIN_NAME variable, it should be exposed at build time: https://doc.rust-lang.org/cargo/reference/environment-variables.html

I can reproduce on alpine using the repo cargo and rust, and it refuses to install rustup for some obscure reason. The latest git HEAD requires rust 1.46, alpine still has 1.44. You may try an arch linux docker instead, we have the latest version in our repos, or you can use rustup on the debian testing image, hopefully it should get you the latest toolchain.

@alucryd
Copy link
Owner

alucryd commented Sep 3, 2020

Oh, rustup is only available on x84_64 for alpine, that's why I can't download it on the RPI :/

@alucryd
Copy link
Owner

alucryd commented Sep 3, 2020

I was able to build in an x86_64 alpine docker, you'll need to install rustup, gcc, musl-dev, p7zip, and openssl-dev, then call rustup-init to download the latest toolchain. Cargo will be located in $HOME/.cargo/bin.

All tests pass here, except import_cso and import_chd because I don't have chdman and maxcso in the PATH.

@muggsyd
Copy link
Author

muggsyd commented Sep 4, 2020

OK, I have made some more progress.
I needed to install 0.3.0 to have the database created for me. If I just installed 0.4.0-beta.1 it would complain about i missing. Once 0.3.0 had been installed and I successfully ran an import-dat. Once that was successful I was able to remove rust and cargo from alpine and use rustup to install a newer cargo and rust. Now I can successfully install 0.4.0-beta.1

I am still experiencing issues with the roms not being picked up correctly though. I have attached an archive that contains the nointro dat I am using, and 2 folders. Fail means oxyromon did not correctly verify the file. Success means they were successfully processed

All roms should be verified correctly as they came from my RomVault collection
oxyromon-testing_0.4.0-beta.1.zip

@alucryd
Copy link
Owner

alucryd commented Sep 4, 2020

Thanks! I fixed the database not being created with the latest beta, and also fixed a case issue when matching CRCs against the database. With the latest HEAD I was able to import the roms from your failed folder.

I now have tests covering the sort-roms subcommand, and quickly tested on my collection without issue. I probably don't have all combinations covered yet but at least there are no obvious regressions compared to 0.3.0.

I will be tackling convert-roms and purge-roms tests over the weekend and hopefully release a stable 0.4.0 version early next week.

@muggsyd
Copy link
Author

muggsyd commented Sep 5, 2020 via email

@muggsyd muggsyd closed this as completed Sep 6, 2020
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

No branches or pull requests

2 participants