dosbox-x: Update to version 2026.05.02, fix checkver & autoupdate#17675
dosbox-x: Update to version 2026.05.02, fix checkver & autoupdate#17675Vinfall wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 6/8 reviews remaining, refill in 12 minutes and 59 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bucket/dosbox-x.json (2)
33-33: Optional rewording of the ROMnotes.Per the author's request for alternative wording, a slightly more neutral and grammatically tightened version below clarifies the workflow without sounding prescriptive, and pluralizes "ROM" for consistency with the four ROM filenames documented in the PR description.
✏️ Suggested wording
- "notes": "To use dumped ROM, place them inside $persist_dir and reinstall. This only needs to be done once.", + "notes": "To load dumped ROMs (e.g. bios.rom, FONT.ROM, itf.rom, sound.rom) for improved accuracy, place them in $persist_dir and reinstall DOSBox-X. This only needs to be done once.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/dosbox-x.json` at line 33, The "notes" string in bucket/dosbox-x.json is worded awkwardly and uses "ROM" singular; update the "notes" field to a clearer, more neutral phrasing that pluralizes "ROM" (e.g., "To use dumped ROMs, place them inside $persist_dir and reinstall; this only needs to be done once.") so the workflow is grammatically tight and matches the multiple ROM filenames referenced elsewhere.
24-24: Backup file is overwritten on every reinstall.Each reinstall unconditionally replaces
dosbox-x.conf.oldwith the currentdosbox-x.conf, so any prior backup is lost. If the intent is to preserve the user's last-known-good config across multiple reinstalls, consider only writing the backup when one does not already exist (or timestamping it). If single-rolling-backup is intentional, this can be ignored.♻️ Optional change
- "pre_install": "if (Test-Path \"$persist_dir\\dosbox-x.conf\") { Copy-Item \"$persist_dir\\dosbox-x.conf\" \"$persist_dir\\dosbox-x.conf.old\" }", + "pre_install": "if ((Test-Path \"$persist_dir\\dosbox-x.conf\") -and -not (Test-Path \"$persist_dir\\dosbox-x.conf.old\")) { Copy-Item \"$persist_dir\\dosbox-x.conf\" \"$persist_dir\\dosbox-x.conf.old\" }",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/dosbox-x.json` at line 24, The pre_install script currently always overwrites dosbox-x.conf.old; change the logic in the "pre_install" step so it only creates a backup when one does not already exist (or alternately create a timestamped backup). Locate the "pre_install" entry that references Test-Path "$persist_dir\\dosbox-x.conf" and adjust it to check for the absence of "$persist_dir\\dosbox-x.conf.old" before running Copy-Item (or implement timestamping of the backup filename) so prior backups are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/dosbox-x.json`:
- Line 25: The post_install currently checks for the existence of "bios.rom"
before copying ROMs, which prevents copying other valid ROMs (e.g., FONT.ROM)
when bios.rom is absent; update the post_install logic to test for the presence
of any ROM files in the persist_dir (e.g., check if the wildcard
"$persist_dir\\*.rom" matches files) and only then run the Copy-Item
"$persist_dir\\*.rom" "$dir\\" command so ROM copying is gated on the wildcard
result rather than the specific "bios.rom" filename; modify the shell expression
around post_install to reference persist_dir, the "*.rom" glob, and dir
accordingly.
- Line 5: The JSON metadata currently sets the "license" field to
"GPL-2.0-only", which misrepresents the upstream COPYING clause; update the
"license" field value in the dosbox-x metadata (the "license" key) to
"GPL-2.0-or-later" so the SPDX identifier reflects the upstream "(at your
option) any later version" grant.
---
Nitpick comments:
In `@bucket/dosbox-x.json`:
- Line 33: The "notes" string in bucket/dosbox-x.json is worded awkwardly and
uses "ROM" singular; update the "notes" field to a clearer, more neutral
phrasing that pluralizes "ROM" (e.g., "To use dumped ROMs, place them inside
$persist_dir and reinstall; this only needs to be done once.") so the workflow
is grammatically tight and matches the multiple ROM filenames referenced
elsewhere.
- Line 24: The pre_install script currently always overwrites dosbox-x.conf.old;
change the logic in the "pre_install" step so it only creates a backup when one
does not already exist (or alternately create a timestamped backup). Locate the
"pre_install" entry that references Test-Path "$persist_dir\\dosbox-x.conf" and
adjust it to check for the absence of "$persist_dir\\dosbox-x.conf.old" before
running Copy-Item (or implement timestamping of the backup filename) so prior
backups are preserved.
🪄 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: 500ad12d-79c8-4df7-844d-87db30b85405
📒 Files selected for processing (1)
bucket/dosbox-x.json
Vinfall
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bucket/dosbox-x.json (2)
23-25: Persist + pre/post_install handling is sound; one small consideration on the rotation.
persist: "dosbox-x.conf"plus the wildcard-gatedpost_installCopy-Item "$persist_dir\*.rom" "$dir\"is correct (Test-Pathwith wildcards is supported andCopy-Itemoverwrites by default).Minor note on
pre_install: becausedosbox-x.confis persisted, the file at$persist_dir\dosbox-x.confis the same file the user is editing across upgrades, so the rotation todosbox-x.conf.oldends up overwriting the previous.oldon every install. That is fine as a single-slot snapshot, but it might surprise users who expect a longer history. If a single rolling backup is the intent, the current code is good as-is — feel free to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/dosbox-x.json` around lines 23 - 25, The current pre_install rotates the persisted dosbox-x.conf into a single-slot backup dosbox-x.conf.old which overwrites the previous backup each install; if you want multi-slot history instead of a single rolling backup, change the pre_install logic that references "persist" and the pre_install Copy-Item target so it preserves previous backups (for example, append a timestamp or incremented suffix to the backup filename instead of always using dosbox-x.conf.old); leave post_install's wildcard Copy-Item behavior as-is if you want the default overwrite semantics.
33-33: Optional: tighten thenoteswording.The author explicitly invited rewording. The current sentence works, but
place them inside $persist_dirreads a bit ambiguously (singular/plural agreement) and "reinstall" is a strong word for what is actuallyscoop reset/scoop update. A small tweak:✏️ Suggested wording
- "notes": "To use dumped ROM, place them inside $persist_dir and reinstall. This only needs to be done once.", + "notes": "To use dumped ROM files (e.g. bios.rom, FONT.ROM, itf.rom, sound.rom), place them in $persist_dir and run `scoop reset dosbox-x` (or reinstall). This only needs to be done once.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/dosbox-x.json` at line 33, Update the "notes" string in the JSON (the notes field in dosbox-x.json) to tighten wording: make plurality consistent (e.g., "ROMs" if plural), clarify the destination (e.g., "place ROMs in $persist_dir"), and replace the vague/strong verb "reinstall" with a precise action such as running a scoop reset or scoop update command (or otherwise restoring the app via scoop). Keep the sentence short and explicit about it only needing to be done once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bucket/dosbox-x.json`:
- Around line 23-25: The current pre_install rotates the persisted dosbox-x.conf
into a single-slot backup dosbox-x.conf.old which overwrites the previous backup
each install; if you want multi-slot history instead of a single rolling backup,
change the pre_install logic that references "persist" and the pre_install
Copy-Item target so it preserves previous backups (for example, append a
timestamp or incremented suffix to the backup filename instead of always using
dosbox-x.conf.old); leave post_install's wildcard Copy-Item behavior as-is if
you want the default overwrite semantics.
- Line 33: Update the "notes" string in the JSON (the notes field in
dosbox-x.json) to tighten wording: make plurality consistent (e.g., "ROMs" if
plural), clarify the destination (e.g., "place ROMs in $persist_dir"), and
replace the vague/strong verb "reinstall" with a precise action such as running
a scoop reset or scoop update command (or otherwise restoring the app via
scoop). Keep the sentence short and explicit about it only needing to be done
once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db87c9e0-8564-4659-b99c-61c850a2c76d
📒 Files selected for processing (1)
bucket/dosbox-x.json
z-Fng
changed the title
Vinfall
force-pushed
the
dosbox-x-fix
branch
3 times, most recently
from
51a12dd to
9d13f05
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bucket/dosbox-x.json (1)
33-33: 💤 Low valueNit: plural "ROMs" in the notes string.
"ROM" should be "ROMs" since the workflow covers multiple ROM files (
bios.rom,FONT.ROM, etc.).✏️ Proposed fix
- "notes": "To use dumped ROM, place them inside $persist_dir and reinstall. This only needs to be done once.", + "notes": "To use dumped ROMs, place them inside $persist_dir and reinstall. This only needs to be done once.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/dosbox-x.json` at line 33, Update the "notes" string in the JSON object (the "notes" property in dosbox-x.json) to use the plural "ROMs" instead of "ROM" so it reads e.g. "To use dumped ROMs, place them inside $persist_dir and reinstall. This only needs to be done once." — locate the "notes" key in the dosbox-x.json content and change the word "ROM" to "ROMs".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bucket/dosbox-x.json`:
- Line 33: Update the "notes" string in the JSON object (the "notes" property in
dosbox-x.json) to use the plural "ROMs" instead of "ROM" so it reads e.g. "To
use dumped ROMs, place them inside $persist_dir and reinstall. This only needs
to be done once." — locate the "notes" key in the dosbox-x.json content and
change the word "ROM" to "ROMs".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c028b70d-8415-4f88-bf24-20a35fb6039c
📒 Files selected for processing (1)
bucket/dosbox-x.json
Vinfall
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bucket/dosbox-x.json`:
- Line 33: Update the "notes" JSON value for the key "notes" in
bucket/dosbox-x.json to use the plural "ROMs" instead of "ROM" (the current
string: "To use dumped ROM, place them inside $persist_dir and reinstall. This
only needs to be done once."); change only the word "ROM" to "ROMs" so it
correctly references the multiple files (bios.rom, FONT.ROM, itf.rom,
sound.rom).
🪄 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: e1e210c7-f6b8-4f4c-bc44-e3b8b1c1f79e
📒 Files selected for processing (1)
bucket/dosbox-x.json
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
|
Rebased onto 2026.05.02, edited title and description to match your edit. @z-Fng, anything I can do to have this merged? I can remove ROM restoration if you think it's too aggressive. |
1 participant
Update to 2026.05.02
Fix persist as DOSBox-X does not use
dosbox.confbutdosbox-x.conf. Now pre_install would backup old config todosbox-x.conf.old.DOSBox and DOSBox-X supports loading dumped ROMs (namely
bios.rom,FONT.ROM,itf.romandsound.rom) for better accuracy. A post_install script with accompanied notes is added regarding this.Use conventional PR title:
<manifest-name[@version]|chore>: <general summary of the pull request>I have read the Contributing Guide
Free feel to edit if you want to revert any of these.
By the way, I think ROM loading notice is a bit invasive. If you can think up a better approach, please let me know!