pokemon-speedrunning / gambatte-speedrun

Fork of https://github.com/sinamas/gambatte with Pokemon speedrunning-related changes.
GNU General Public License v2.0
94 stars 27 forks source link

Fix Windows crashes with Qt 5.11+, support MSVC, add automated build testing #56

Closed entrpntr closed 3 years ago

entrpntr commented 4 years ago

Fixes #17. See #46 for details on the fix for Qt 5.11+ crashes on Windows. All commits are related to fixing/minimizing builds with recent Qt releases, so I've annoyingly decided to combine the PRs.

This PR introduces some complications in exchange for sanity and smaller, automated release builds. The .github/build.yml file introduces build testing on pushes and pull requests. As of now, the following build processes would be attempted on each push/PR (artifacts uploaded are the main binaries we have interest in distributing):

  1. Ubuntu 18.04 (shared library and executable, no artifacts uploaded)
  2. macOS 10.15 (shared library and executable, .dylib and .dmg uploaded)
  3. MinGW 64-bit (shared library only, .dll uploaded)
  4. MSVC 2019 32-bit (executable only, .exe uploaded)

The current build.yml file references Qt builds which are uploaded as releases in the newly created gsr-qt-builds repo. For Windows, MSVC builds seemed to minimize the .exe a good deal more than MinGW, in addition to building faster. The macOS build is also much more reasonable in size with the custom Qt build. You can look at this run to see what tests are run and what artifacts are built. These findings are based on initial stabs at building Qt from source; we may learn more down the road that could change our approach.

Earlier iterations of the build.yml file did not use the gsr-qt-builds releases, and instead only built using the more widely available Qt libraries. That approach resulted in longer build times and larger executables, but wouldn't require as much attention to be given to the gsr-qt-builds repo or the build.yml file. Ideally the test suite could be added to the workflow, but it requires the bootroms, making any solution on the GitHub runners sketchy.

There is definitely more that can be done still:

Trying to figure everything out at once would likely drag this out a few more weeks, and I can't give it much more attention in the immediate future, so I'm leaving this as-is as a draft for now. Feel free to add more commits on top of this (maintainers should have push access to this branch in my repo) if you identify needed changes, and mark it ready for review whenever you think it can be merged.

entrpntr commented 4 years ago

I actually should double check the macOS build stuff before declaring this ready, since macdeployqt doesn't really do anything with statically built Qt. I think the static build was still the best approach for Mac, but the .dmg can be created in other ways, or we could elect to just upload the .app. In any event, I'll test some things and look over everything else one more time before calling this complete on my end.

entrpntr commented 4 years ago

On second look, the current Qt binaries are fine. macdeployqt on an app built with dynamic Qt binaries will add the Qt frameworks/libraries to the executable in the .app, but apparently it also strips some stuff from the executable as well. With the statically built Qt, macdeployqt doesn't add anything, but it does strip over 2 MB off the size of the executable, so might as well keep it around.

I did add 1 commit to better document some of the choices made in the build.yml file. Also updated the original comment for the PR, including a note that this only builds the PSR versions currently.