exe2iso: rewrite on top of the ISO9660 builder#2036
Open
nicolasnoble wants to merge 1 commit into
Open
Conversation
exe2iso predates the supportpsx ISO9660 authoring API and hand-rolled a minimal, non-conformant disc image just barely good enough for the BIOS to boot. Now that the builder exists, use it: the output is a proper ISO9660 filesystem with a single PSX.EXE;1 in the root, valid EDC/ECC on every sector, and the PLAYSTATION system identifier in the PVD. This drops the options the builder makes meaningless: -regen (EDC/ECC is always computed now), -offset (a real filesystem places the file), and the undocumented -data. Padding stays, but now defaults on with a -nopad opt-out, since writing to an actual disc is the case that wants it. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
📝 WalkthroughWalkthrough
Changesexe2iso refactor to ISO9660Builder
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/exe2iso/exe2iso.cc`:
- Around line 51-62: The condition at line 51 combines the help flag check with
usage error checks, causing the help message to exit with -1 (failure) instead
of 0 (success). Refactor the logic to check asksForHelp separately first: if
true, print the help message and return 0, then check the remaining conditions
(hasExactlyOneInput and hasOutput) separately and only return -1 if those
validations fail. This ensures that requesting help with the -h flag exits
successfully while actual usage errors still indicate failure.
In `@tools/exe2iso/README.md`:
- Around line 10-12: The markdown linter rule MD058 (blanks-around-tables)
requires a blank line before table content. In the README.md file, add a blank
line between the usage example line starting with "Usage: exe2iso input.ps-exe"
and the table header row that starts with "| Argument". This blank line
separation is necessary to satisfy the markdownlint requirements for proper
table formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 540893e4-69a4-41be-aae5-10a7573a42f8
📒 Files selected for processing (3)
tools/README.mdtools/exe2iso/README.mdtools/exe2iso/exe2iso.cc
Comment on lines
+51
to
62
| if (asksForHelp || !hasExactlyOneInput || !hasOutput) { | ||
| fmt::print(R"( | ||
| Usage: {} input.ps-exe [-offset value] [-pad] [-regen] [-license file] -o output.bin | ||
| Usage: {} input.ps-exe [-license file] [-nopad] -o output.bin | ||
| input.ps-exe mandatory: specify the input ps-exe file. | ||
| -o output.bin mandatory: name of the output file. | ||
| -offset value optional: move the exe data by value sectors. | ||
| -data filename optional: insert this file into the iso after the exe. | ||
| -pad optional: pads the iso with 150 blank sectors. | ||
| -regen optional: generates proper ECC/EDC. | ||
| -license file optional: use this license file. | ||
| -nopad optional: don't append {} trailing blank sectors. | ||
| -h displays this help information and exit. | ||
| )", | ||
| argv[0]); | ||
| argv[0], c_trailingPaddingSectors); | ||
| return -1; | ||
| } |
Contributor
There was a problem hiding this comment.
Return success when -h is requested.
Line 51 currently routes help requests and invalid-usage errors through the same branch, so -h exits with -1. That makes help look like a failure in scripts/CI.
🔧 Suggested fix
- if (asksForHelp || !hasExactlyOneInput || !hasOutput) {
+ if (asksForHelp) {
+ fmt::print(R"(
+Usage: {} input.ps-exe [-license file] [-nopad] -o output.bin
+ input.ps-exe mandatory: specify the input ps-exe file.
+ -o output.bin mandatory: name of the output file.
+ -license file optional: use this license file.
+ -nopad optional: don't append {} trailing blank sectors.
+ -h displays this help information and exit.
+)",
+ argv[0], c_trailingPaddingSectors);
+ return 0;
+ }
+
+ if (!hasExactlyOneInput || !hasOutput) {
fmt::print(R"(
Usage: {} input.ps-exe [-license file] [-nopad] -o output.bin
input.ps-exe mandatory: specify the input ps-exe file.
@@
)",
argv[0], c_trailingPaddingSectors);
return -1;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/exe2iso/exe2iso.cc` around lines 51 - 62, The condition at line 51
combines the help flag check with usage error checks, causing the help message
to exit with -1 (failure) instead of 0 (success). Refactor the logic to check
asksForHelp separately first: if true, print the help message and return 0, then
check the remaining conditions (hasExactlyOneInput and hasOutput) separately and
only return -1 if those validations fail. This ensures that requesting help with
the -h flag exits successfully while actual usage errors still indicate failure.
Comment on lines
+10
to
12
| Usage: exe2iso input.ps-exe [-license file] [-nopad] -o output.bin | ||
| | Argument | Type | Description | | ||
| |-|-|-| |
Contributor
There was a problem hiding this comment.
Add a blank line before the table to satisfy markdownlint.
Line 11 triggers MD058 (blanks-around-tables). Add an empty line between the usage line and the table header.
📝 Suggested fix
Usage: exe2iso input.ps-exe [-license file] [-nopad] -o output.bin
+
| Argument | Type | Description |
|-|-|-|📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Usage: exe2iso input.ps-exe [-license file] [-nopad] -o output.bin | |
| | Argument | Type | Description | | |
| |-|-|-| | |
| Usage: exe2iso input.ps-exe [-license file] [-nopad] -o output.bin | |
| | Argument | Type | Description | | |
| |-|-|-| |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 11-11: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/exe2iso/README.md` around lines 10 - 12, The markdown linter rule MD058
(blanks-around-tables) requires a blank line before table content. In the
README.md file, add a blank line between the usage example line starting with
"Usage: exe2iso input.ps-exe" and the table header row that starts with "|
Argument". This blank line separation is necessary to satisfy the markdownlint
requirements for proper table formatting.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
exe2iso predates the supportpsx ISO9660 authoring API and hand-rolled a minimal, non-conformant disc image just barely good enough for the BIOS to boot. Now that the builder exists, use it: the output is a proper ISO9660 filesystem with a single PSX.EXE;1 in the root, valid EDC/ECC on every sector, and the PLAYSTATION system identifier in the PVD.
This drops the options the builder makes meaningless: -regen (EDC/ECC is always computed now), -offset (a real filesystem places the file), and the undocumented -data. Padding stays, but now defaults on with a -nopad opt-out, since writing to an actual disc is the case that wants it.