Skip to content

Fix macos bugs with Warcraft II installer extraction - #781

Open
TomAnthony wants to merge 5 commits into
Wargus:masterfrom
TomAnthony:fix/macos-gog-installer-extraction
Open

Fix macos bugs with Warcraft II installer extraction#781
TomAnthony wants to merge 5 commits into
Wargus:masterfrom
TomAnthony:fix/macos-gog-installer-extraction

Conversation

@TomAnthony

Copy link
Copy Markdown

This is a bunch of fixes for getting Wargus running on macos, including:

  • Find innoextract even when installed by homebrew/ports etc. so the Wargus binary launched as a GUI works
  • Fix incomplete file extraction caused by quotes handling on mac
  • Fix bug with extraction when not using expansion CD

In case it matters: Claude did the actual code work, with me driving and testing.

Wargus now launches direct from the GUI, where you can select an .exe installation file and it correctly extracts and launches. Tested on macos 26 on ARM.

I haven't got access to Windows, but I think the changes should be safe there but should be tested.

TomAnthony and others added 5 commits June 28, 2026 16:09
runCommand() passed the caller-supplied argv straight to execvp(), but
execvp uses argv[0] as the program-name slot and starts option parsing at
argv[1]. To match the Windows overload, callers pass only the arguments
(no program name), so the first real argument was silently swallowed:
`innoextract -i <file>` ran as `innoextract <file>`, turning a harmless
info probe into a full extraction. Prepend the program name to the child
argv so all caller-supplied arguments survive.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
argv[2] was set to tmpp.string().c_str(), but tmpp.string() returns a
temporary std::string that is destroyed at the end of the statement,
leaving argv[2] dangling. With a long (heap-allocated) temp path this is a
genuine use-after-free, so innoextract intermittently received a garbage
-d argument and the extraction failed. Hold the path in a local string for
the duration of the runCommand() call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On macOS extractor_tool is used as a filesystem path: fs::path(...) when
locating the bundled scripts/contrib directories and when building the
osascript extraction command. The quote characters baked in for the POSIX
shell made those paths invalid, so extraction aborted with a "could not
discover scripts path" error -- and because that message contains a quote,
tinyfiledialogs replaced it with "INVALID MESSAGE WITH QUOTES", hiding the
real cause. Only quote on the non-macOS POSIX path, which concatenates the
extractor directly into a shell command.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The contrib copy (scripts, UI graphics, shaders, ...) was skipped whenever
force == 2. That value is used both by the "extract more" expansion
recursion -- which must not re-copy, since copy_dir() wipes its target
before copying and the expansion pass does not re-extract the base data --
and by a data file passed on the command line. The latter can be a first
extraction, which still needs the contrib dirs or the engine cannot start
(it fails to load e.g. scripts/stratagus.lua and the UI graphics).
Distinguish the two cases by datafileCstr: the expansion recursion passes
none.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A Finder-launched app inherits the minimal launchd PATH, which excludes
Homebrew and MacPorts, so a bare "innoextract" was not found even when it
was installed and the user could only extract from a terminal. Resolve it
explicitly: next to our own executable (so it can be bundled in future),
then the common package-manager locations, finally falling back to a bare
name so execvp still searches PATH when run from a shell.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@Jarod42 Jarod42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good to be.
Untested though.

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

Successfully merging this pull request may close these issues.

2 participants