Opened 9 months ago

Closed 7 months ago

#10878 closed defect (fixed)

WIN32: Disabled engine files are included as resources

Reported by: sluicebox Owned by: sluicebox
Priority: normal Component: Port: Win32
Keywords: has-pull-request Cc:
Game:

Description

Windows builds are including data files for disabled engines as embedded resources. It's not clear to me that the scheme of excluding disabled engine files, introduced 8 years ago, has ever worked. That's plausible since it wouldn't have broken anything, it would have just potentially added extra bytes to the exe. This applies to both mingw and visual studio builds.

The root cause is simple: STATIC_PLUGIN, which plugins.h defines as 1, is never defined for the resource compiler.

Engine data files are included in scummvm.rc as such:

#if ENABLE_HUGO       == STATIC_PLUGIN
hugo.dat               FILE    "dists/engine-data/hugo.dat"
#endif

The goal is to only include a file if the engine is enabled AND as a static plugin. (DYNAMIC_PLUGIN = 2)

The ENABLE_* identifiers are correctly being passed to the resource compiler but there is nothing that informs the resource compiler of STATIC_PLUGIN. Since STATIC_PLUGIN is undefined, the resource compiler evaluates each of these preprocessor statements as true regardless of ENABLE_*, causing all files to always be included. You can easily confirm this by replacing ENABLE_HUGO with a made-up identifier and inspecting the resulting exe, it will contain hugo.dat. (I sent away for the HUGO II: WHODUNNIT hint book in the early 90s because I was stuck at the chasm for months. YOU JUST WALK ACROSS IT?! Permanently turned me off to bananas. But I did learn the word chasm. Thanks England.)

There are of course endless ways to inform a component about a simple definition, so I defer to someone with more seniority as to which the most scummvmy way is, but I have some observations to assist. STATIC_PLUGIN is defined in plugins.h. Assuming you don't want to do anything crass like repeat the definition, the best thing to do is probably factor the *_PLUGIN definitions out to a very small and simple .h file so that scummvm.rc can #include it, similar to internal_version.h. In a perfect world you could just #include plugins.h in scummvm.rc, but the resource compiler isn't a c++ compiler, and you'll get into trouble. Microsoft's resource compiler is guaranteed to accept #includes of .c and .h files, ignoring everything but preprocessor macros, and indeed that ball of wax works at the moment (https://blogs.msdn.microsoft.com/oldnewthing/20171004-00/?p=97126), but mingw gets into trouble because it doesn't have its own dedicated hack-fest, it just hands the work off to gcc's preprocessor, and the world of c++ that plugins.h drags in breaks down in my mingw environment. Factoring out the plugin definitions into a simple .h will make life simple for both resource compilers and we won't have to worry about some unexpected c++ indirectly breaking it in the future.

I don't know what the timetable is for a 2.1 release but there are a number of in-progress engines with data files and if this isn't resolved before then the 2.1 exe will end up containing them.

Huge thanks to SupSuper for helping in getting to the bottom of this and walking me though getting a mingw environment setup like the child that I am. Windows masochists of the world, unite!

[ Fair warning: if this isn't addressed I will belligerently submit my own PR that will make everyone mad =) ]

Change History (3)

comment:1 by sluicebox, 8 months ago

Keywords: has-pull-request added

comment:2 by Filippos Karapetis <bluegr@…>, 8 months ago

In e67b8501:

WIN32: Fix embedding disabled engine files

Fix Windows builds (msvc and mingw) including data files from disabled
engines as embedded resources in executable. Bug #10878

comment:3 by bluegr, 7 months ago

Owner: set to sluicebox
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.