search

alucryd / oxyromon

Rusty ROM OrgaNizer
Other
121 stars 13 forks source link

Roms not being verified properly #2

Closed muggsyd closed 4 years ago

muggsyd commented 4 years ago

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 commented 4 years ago

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

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 commented 4 years ago

Hi, Unfortunately I blew away my testing docker image last night. So I am attempting to recreate the steps today

I'm using Alpine Linux. I've cloned your github repository and an cargo test and it is failing (I needed to install some packages 1st) at the last step

error[E0658]: procedural macros cannot be expanded to expressions

--> src/database.rs:57:5

|

57 | / sqlx::query!(

58 | | "

59 | | UPDATE systems

60 | | SET name = ?, description = ?, version = ?

... |

66 | | id,

67 | | )

| |_____^

|

= note: see issue #54727 https://github.com/rust-lang/rust/issues/54727 for more information

= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

It was attempting to build 0.4.0 beta so that's good. I'm also building a debian unstable image to see if I can't progress further with that and a newer build of Rust / cargo

When building on a new Debian Unstable install I get the following error

Compiling oxyromon v0.4.0-beta.1 (/roms/build/oxyromon)

error: environment variable CARGO_BIN_NAME not defined

--> src/main.rs:52:28

|

52 | let matches = App::new(env!("CARGO_BIN_NAME"))

| ^^^^^^^^^^^^^^^^^^^^^^

|

= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

error: could not compile oxyromon.

To learn more, run the command again with --verbose. I will try and do some more testing later tonight once I have finished work

On Thu, 3 Sep 2020 at 00:38, Maxime Gauduin notifications@github.com wrote:

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alucryd/oxyromon/issues/2#issuecomment-685780280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQ2XWFMLYFDR36U4DC6B7NTSDZKGZANCNFSM4QSOB2RA .

alucryd commented 4 years ago

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 commented 4 years ago

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

alucryd commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

That's brilliant. I built the new code and I can confirm it does work as expected now. My roms are now correctly identified and moved to their new location

I have a couple of issues with dats that have headers that contain extra fields, and how it manages dats with subfolders etc (not no-intro or redump so out of scope) but they are not supported by oxyromon

I will definitely try and keep up with the developments as I am super keen on a cli rom manager I can use in docker on my nas rather than trying to manage on my laptop or via a VM. As I have a working environment it will be a lot easier for me now

On Fri, 4 Sep 2020 at 22:10, Maxime Gauduin notifications@github.com wrote:

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alucryd/oxyromon/issues/2#issuecomment-687103841, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQ2XWFO4H4DPFKZ46SGSOLTSEDKJVANCNFSM4QSOB2RA .