Rewrite RGBFIX in Rust

[Oneirical]

This is an introductory project for me as a way to familiarize myself with the port of a lower level C++ program into Rust. I have 4 months of Rust experience, please do not expect senior-level quality.

How it works

Flags and arguments are taken in using clap's derive feature, editing a CLIOptions struct with all relevant data. The filename is sent to process_filename, which reads the file and sends it to process_file.

Then, each if statement checks if a relevant flag/argument was provided by the user using "if let Some" syntax, and overwrites bytes in the .gb file if necessary.

Questions & Weird Things

• I'm still not 100% sure when clap automatically parses strings for me, or when I need to do something special, like how I needed to add the clap_num crate to parse hexadecimal. It would be worth testing every flag with a dummy value to see if everything works.
• I am not sure if RGBFIX is supposed to accept multiple files or just a single one. There is this weird check repeated a few times which checks if the input file is the same as the output file, which, logically, should always succeed since input/output are one and the same?
• There is this assert repeated twice: "static_assert(0x10000 * BANK_SIZE <= SSIZE_MAX, "Max input file size too large for OS");". I have no idea how it could ever fail, so is it worth keeping around? Static asserts are not obvious to do in Rust, as far as I know.
• C++ code abuses integer overflow a couple of times in the original RGBFIX. I tried to use wrapping_add/sub/mul to mirror this, but I'm particularly doubtful of my wrapping_mul implementation. The rest should be fine.
• process_filename is weird. It checks if the filename is composed of nothing but a dash '-'? And if not, it passes the input file as both the input and output? Rust won't let me do that due to ownership rules, so I try to clone it with try_clone(), but this gives a new file descriptor to the clone, which makes them not truly equal. Super weird.
• I'm not fully certain if the code ACTUALLY edits the file or not. I imagined at first that a .gb file would crash my emulator before running it through RGBFIX, and become functional afterwards... But hello-world.gb from the tutorial works even without getting fixed. I wonder how I can actually debug and test my program if what it actually does is not obvious.

TODOs

• Add more help text.
• Fix cargo.toml to properly have this coexist with RGBASM.
• If the cartridge type is a TPP1 subtype, the C++ code calls upon a convoluted "tryReadSlice" function with a switch statement that parses a string char-by-char. I have not fully understood yet how to to handle this in my Rust code.

[evie-calico]

I'm not fully certain if the code ACTUALLY edits the file or not. I imagined at first that a .gb file would crash my emulator before running it through RGBFIX, and become functional afterwards... But hello-world.gb from the tutorial works even without getting fixed. I wonder how I can actually debug and test my program if what it actually does is not obvious.

This script should help:

head -c 336 < /dev/urandom > random.gb
cp random.gb fixed.gb
rgbfix $* fixed.gb
xxd random.gb > random.txt
xxd fixed.gb > fixed.txt
diff random.txt fixed.txt

rm -f random.gb fixed.gb random.txt fixed.txt

Invoke as:

sh script.sh <rgbfix flags>

[Oneirical]

Thank you! This works perfectly and confirms to me that the file is indeed getting edited, and that the padding flag works. I will be using this for further testing.

[evie-calico]

Make sure your files end with an "empty" line—this way each line is newline-delimited.

[ISSOtm]

Thank you very much for this PR! The code quality seems quite good.

I haven't reviewed everything yet, only wrote some of my more major feedback.

[ISSOtm]

Have you tried using test/fix/test.sh instead?

[Oneirical]

I have obtained the test/fix directory but am encountering some problems.

What I did:

1. In test.sh, changed the RGBFIX variable to "cargo run --bin rgbfix" instead of "./rgbfix"
2. Deleted:

cp ../../{rgbfix,contrib/gbdiff.bash} "$tmpdir"
cd "$tmpdir" || exit
# Immediate expansion is the desired behavior.
# shellcheck disable=SC2064
trap "cd; rm -rf ${tmpdir@Q}" EXIT

And manually brought gbdiff.bash into the directory instead.

The problem:

This causes an output of a massive wall of mismatches and errors. Here is one selected at random:

tpp1-bad-minor.err piped mismatch!
test.sh: 60: our_rc: not found
test.sh: 60: 127: not found
test.sh: 61: [[: not found
test.sh: 66: rc: not found
test.sh: 66: our_rc: not found
test.sh: 67: [[: not found
test.sh: 38: tests++: not found
test.sh: 40: [[: not found
test.sh: 43: [[: not found
--- /home/limace/Bureau/rsgbds/gb_testing/rgbds/test/fix/tpp1-nonjap.err    2024-04-17 16:15:52.330842019 -0400
+++ -    2024-04-17 16:18:45.421775740 -0400
@@ -1,5 +1,7 @@
-warning: TPP1 overwrites region flag for its identification code, ignoring `-j`
-warning: Overwrote a non-zero byte in the cartridge type
-warning: Overwrote a non-zero byte in the TPP1 identification code
-warning: Overwrote a non-zero byte in the TPP1 revision number
-warning: Overwrote a non-zero byte in the TPP1 feature flags
+error: unexpected argument '-m' found
+
+  tip: to pass '-m' as a value, use '-- -m'
+
+Usage: cargo run [OPTIONS] [args]...
+
+For more information, try '--help'.

As you can see, the strangest thing about this is how it complains about not finding variables like "our_rc" even though they are in scope.
I only spent 30 minutes so far trying to understand this issue, so it is possible I am missing something obvious here.

[ISSOtm]

I would advise transplanting the generated binary into the RGBDS directory instead, as that should be overall less finicky? But if you got something that works regardless, then great.

I don't understand the sh errors, they kind of look like the test script is invoked with sh instead of bash. Are you invoking it as ./test.sh?

As for the mismatches, you're missing the suggested -- after --bin rgbfix ;)

[Oneirical]

I don't understand the sh errors, they kind of look like the test script is invoked with sh instead of bash

Correct, that was the mistake! Thank you kindly.

[ISSOtm]

By the way, I would like rsgbds to pass RGBDS' test suite unaltered (save for the different error reporting, of course—which may require some tweaks to the test harness as well). Thus this file wouldn't be of much use.

(The plan is to move the tests to cargo test afterwards, but that will be somewhat involved.)

[ISSOtm]

We're already pulling in codespan-reporting as an error reporting crate, I'd like all of RGBDS to be consistent about it.

(Preferably, all programs would pull in a common module, but I'm fine with the PR duplicating work for the sake of simplicity; I can perform the deduplication refactoring myself afterwards.)

[Oneirical]

Interesting. This change was Evie's pick, and she said she thinks codespans should not be used for error reporting, but I understand the need for consistency.

[ISSOtm]

Yeah, I agree it's arguably abuse of the crate... but I also think consistency, being user-facing, trumps that.

[ISSOtm]

For what it's worth, I have pushed a commit that switches the diagnostics backend to ariadne. It should have a much more practical API than codespan_reporting for rgbfix.

[ISSOtm]

process_filename is weird.

I can explain how it works: RGBFIX has two ways of working,

• The "traditional" one where it's passed filenames, and each of those files is modified in-place.
• And the newer "pipe" one, where it's passed a single dash (standard Unix parlance for "what I really want is stdin/stdout [depending on context]") and it reads from stdin and writes to stdout.

This should be able to be emulated in some way. I'm considering something like

enum InOut {
    File(File),
    Stdio,
}

(...feel free to come up with a better name >_>) And then you can impl Read for InOut, impl Write for InOut, and use the appropriate APIs; or use a custom API if that's more ergonomic; you're in a better place to make that decision than me right now ^^

[ISSOtm]

Turns out the patharg crate does exactly that. I should replace dash_stdio.rs with it, too.