shinyquagsire23 / OpenJKDF2

A cross-platform reimplementation of JKDF2 in C
Other
493 stars 39 forks source link

Add version info and internal assembly manifest to Windows executable #252

Closed JakeSmarter closed 1 year ago

JakeSmarter commented 1 year ago

This PR is basically about bringing OpenJKDF2 to the modern age on Windows. We can do this by adding version information and a manifest to binaries. So far, OpenJKDF2 is running legacy system code paths, meaning no better than Windows Vista by default.

However, adding version info to the Windows executable in this project as it is right now proves a little bit tricky. Looking at the build system you seem to intend to use the distpkg_version.sh and increment_version.py scripts for managing releases.

Generally speaking, it is not the best idea to hard code version data in source code. Usually, it is better to manage and increment version data in or through the build system instead of modifying source code files on every release (with custom automation scripts). Source code should have no more than placeholders or macros where version data should be placed.

CMake uses the project() command’s VERSION argument for this purpose. This is where version data should either be stored (and incremented) from release to release or be passed to on every build. Using CMake’s project() command would also have the benefit that you could easily propagate version data to all kinds of tools (and files) in the build process. Additionally, since many major IDEs, like Visual Studio and Xcode, support managing projects with the CMake build system generator, you could potentially manage releases and versioning via your favorite GUI too. Not that I insist on using an (or any particular) IDE. It would be just an added bonus to those who want.

Furthermore, basically all modern operating systems provide an infrastructure for storing and handling version information of binaries. So, there is not actually much need for redundant version data stored in global variables in binaries, like in version.c. Besides, many of these global variables are not ever accessed in OpenJKDF2’s code. And, because external code could potentially access globals anytime at runtime they are not dropped by the compiler either. So, it is probably going to be best to migrate the code to use platform native version infrastructure (don’t worry, I am volunteering to do this :wink:).

There is also another issue that came to my attention while looking at the code in version.c. Usually, you do not want to ever define quoted or stringified values in C pre-processor macros. If anything you want to quote or stringify macros on use with the # macro parameter operator.

Long story short; how would you like to manage future releases? In other words, how would you like to do version bumps in the future? I can update the increment_version.py script to modify CMakeLists.txt where necessary and things should basically continue to work like they do now but I am not sure this is the best way going forward. Generally speaking, I would like to and also would recommend to you to get rid of custom scripts because AFAICT there is actually nothing in the scripts that could not be handled by CMake directly. What do you think?

shinyquagsire23 commented 1 year ago

Yeah so basically my process for releases currently is like

./distpkg_version.sh v0.8.19

and then it builds zips for Win64+macOS and preps the Flatpak repo. Then the only manual things are writing the release notes uploading the zips, and opening the Flatpak PR.

There's also distpkg_hotfix.sh which is basically just, if the zip I just built is a complete dud and crashes on startup or something breaking, it keeps the release version but updates the commit hash.

As far as getting rid of version.c goes, I'm hesitant only because there are a few niche platforms where I know platform versioning is basically nonexistent (ie, I'm curious about porting to DSi or Switch, and sometimes reading the actual binary file is not well supported there). So the ideal medium is probably just going to be, CMake holds versioning and generates that file, defines are moved to symbols so that commit bumps don't force a full rebuild.

Getting rid of the per-platform build scripts for stuff like Valve GNS has been on my TODO list, but the last time I tried to integrate GNS into CMake directly I wasted about a solid two days trying to make it work lol.

One question actually, because I'm confused on the wording: would this PR kill Windows 7 support? If it were up to me I'd only support Win10, but I know there's definitely Win7 diehards running OpenJKDF2 so that's kinda my unofficial minver. Which kills me inside bc running unsupported Windows versions is an awful idea that I hate to even slightly endorse, but this ~is a game that ran on Win95 so it's kinda part of the territory I feel like lol. Honestly what I might have to do is make a 32-bit build with a WinXP minver so that 64-bit can be less constrained.

shinyquagsire23 commented 1 year ago

I did a quick test to see how well the CMake versioning fits into my old workflow and it's only a few lines of changes: https://github.com/shinyquagsire23/OpenJKDF2/commit/0f0977384045a915100e72686917f4c6668a0e53

JakeSmarter commented 1 year ago

Thank you for taking your time to explain your release process. It definitely improves my understanding of the project and what to do next.

As far as getting rid of version.c goes, I'm hesitant only because there are a few niche platforms where I know platform versioning is basically nonexistent (ie, I'm curious about porting to DSi or Switch, and sometimes reading the actual binary file is not well supported there).

I see.

So the ideal medium is probably just going to be, CMake holds versioning and generates that file, defines are moved to symbols so that commit bumps don't force a full rebuild.

Yeah, sounds reasonable; we could exclude these globals with defines on all platforms that have built-in versioning support, like Windows, OSX, Linux, Android, BSD and the like (basically all PE and ELF based systems). I have not had any contact with DSi, Switch, PS4, or PS5 binaries but I am pretty sure that at least Switch, PS4, and PS5 are all ELF based.

would this PR kill Windows 7 support?

No, on the contrary. Windows 7 users would finally get OpenJKDF2 to use genuine Windows 7 code paths. Already, you should be able to run the AMD64/x64 flavor on as far back as Windows XP Professional x64 Edition SP2.

Honestly what I might have to do is make a 32-bit build with a WinXP minver so that 64-bit can be less constrained.

I would love to see that happen or do it myself because backwards compatibility is dear to my :heart: too. :grin: Honestly, I love seeing modern code run on older hardware, sometimes even better than the original:exclamation: But, regardless of that imho caring for 32‑bit releases often enough results in better software for everybody, simply because you usually find corner cases, pitfalls, and bugs you would not find otherwise supporting only one architecture (or machine word width).

Anyhow, I am having trouble building the MinGW64 target. After calling ./build_win64.sh I get this:

[  0%] Generating ~/src/GitHub/OpenJKDF2/src/globals.h
/bin/sh: line 1: cog: command not found

Any ideas?

JakeSmarter commented 1 year ago

Oh btw, can you somehow configure those build actions on GitHub so that one can download the build results (I think GitHub calls them “artifacts”)? Thanks!

This way, for example, we could ask @dgrdsv to test this build if the changes in this PR do anything for him with regards to #248.

JakeSmarter commented 1 year ago

Anyhow, I am having trouble building the MinGW64 target. After calling ./build_win64.sh I get this:

[  0%] Generating ~/src/GitHub/OpenJKDF2/src/globals.h
/bin/sh: line 1: cog: command not found

Ah, got that part more or less figured out. I have installed the cog and cogapp Python packages but whenever I call cog I get this error:

Traceback (most recent call last):
  File "~/.local/bin/cog", line 5, in <module>
    from cogapp import main
ModuleNotFoundError: No module named 'cogapp'

:disappointed:

JakeSmarter commented 1 year ago

:tired_face: Alright, I have figured out that when you install cogapp per user you have to set the PYTHONUSERBASE environment variable, PYTHONPATH will not do the trick. :angry: I wonder, why would any sane person expect that? This is why Python is and remains a script kiddy tool. Why have just one environment variable for dependency resolution like all professional and sane competing runtimes have when you can have 7? And, to make life even more miserable for your users, lets not mention all of them in the help message but let people dig through the online documentation! :exploding_head: The amount of stupidity Python developers are pursuing is just staggering.

:thinking: Anyhow, now I am getting this error, which confuses me because it builds flawlessly on GitHub’s build action…

[  0%] Building CXX object CMakeFiles/openjkdf2-64.dir/src/Platform/Networking/GNS/stdComm_GNS.cpp.obj
In file included from ~/src/GitHub/OpenJKDF2/src/Platform/Networking/GNS/stdComm_GNS.cpp:9:
~/src/GitHub/OpenJKDF2/src/SDL2_helper.h:49: warning: "GL_R8" redefined
   49 | #define GL_R8 GL_RED
      | 
In file included from ~/src/GitHub/OpenJKDF2/src/SDL2_helper.h:43,
                 from ~/src/GitHub/OpenJKDF2/src/Platform/Networking/GNS/stdComm_GNS.cpp:9:
~/src/GitHub/OpenJKDF2/3rdparty/glew/include/GL/glew.h:7454: note: this is the location of the previous definition
 7454 | #define GL_R8 0x8229
      | 
~/src/GitHub/OpenJKDF2/src/Platform/Networking/GNS/stdComm_GNS.cpp: In function ‘char* get_ip_str(const sockaddr*, char*, size_t)’:
~/src/GitHub/OpenJKDF2/src/Platform/Networking/GNS/stdComm_GNS.cpp:1307:13: error: ‘inet_ntop’ was not declared in this scope; did you mean ‘inet_ntoa’?
 1307 |             inet_ntop(AF_INET, &(((struct sockaddr_in *)sa)->sin_addr),
      |             ^~~~~~~~~
      |             inet_ntoa
shinyquagsire23 commented 1 year ago

For cog, I think it may be better to replace it with python3 -m cogapp or something along those lines, I'll have to double-check if that works. I've also run into annoying PATH bugs in macOS because there's like 5 possible runtime dirs. Honestly though the only sane way to deal w/ Python deps is to never use the apt packages and only have the one user dir, which is ~usually the default.

Oh btw, can you somehow configure those build actions on GitHub so that one can download the build results (I think GitHub calls them “artifacts”)? Thanks!

I had it set up ~briefly but kinda opted out of it, because the GitHub runners don't include Valve GNS I believe (or at least one of them didn't, it takes forever to build on one core). Might not be a terrible idea though for Win64 tests.

inet_ntop

Apparently that can happen with an outdated MinGW/Winsock2? The runners use Ubuntu 20.04 so it's not crazy new, but I've tried like once on 18.04 and it had a lot of trouble with it. It's got both IPv4+IPv6 in there so it definitely trips up on sockets junk bc there's zero consistency for those headers between *nix and everything else.

JakeSmarter commented 1 year ago

For cog, I think it may be better to replace it with python3 -m cogapp

Nah, I do not think so. I have tried this manually just to see what happens: still ModuleNotFoundError. In the end it is just that the Python runtime does not implement searching for modules in certain user directories by default. Maybe this is configurable (perhaps in /etc?) but this is not how things should work. You either support user and system module installations and then implement default search directories or you do nothing. Python developers are just a breed of its own…

Apparently that can happen with an outdated MinGW/Winsock2? The runners use Ubuntu 20.04 so it's not crazy new, but I've tried like once on 18.04 and it had a lot of trouble with it.

:thinking: Hmm, I should be fairly recent because I am using Flathub’s org.freedesktop.Sdk.Extension.mingw-w64 runtime, which figures as version 10.0.0 and the compiler is version 11.2.0. I have rebased my branch on your latest changes in hopes of perhaps having missed something but I still get the same error. I will have to investigate more later.

[…] GitHub runners don't include Valve GNS […]

I don’t know, does Valve GNS really have to be rebuilt on every OpenJKDF2 build?

Might not be a terrible idea though for Win64 tests.

In my humble experience this does come in handy, especially when you want to engage users in testing.

JakeSmarter commented 1 year ago

Phew, :relieved: I have finally got it compiled.

The reason for the inet_ntop() hiccup turns out to be the fact that the Winsock2 headers’ backward version compatibility has been moved forward to no earlier than Windows Vista (_WIN32_WINNT=0x0600). I have been specifying Windows Server 2003 (_WIN32_WINNT=0x0502) because this was the first x64/AMD64 release of Windows. So, things should have built fine actually because already the initial release of Windows XP was IPv6 capable. But, for some reason all of the Winsock2 headers have been forcibly version forwarded. Now, I am not sure who actually did it. Was it Microsoft or the MinGW devs? Microsoft’s online documentation says Windows Vista is required on all of Winsock2, which in fact is not or was not. Hmm, it might have been a policy move by Microsoft to finally end third party app development for Windows XP and MinGW just followed suit.

Anyway, we can bump the minimum Windows SDK headers version to 0x0600 (Windows Vista), if you are okay with that? However, folks should still be able to run OpenJKDF2 on Windows Server 2003 and Windows XP Professional x64 Edition (the earliest x64/AMD64 versions of Windows) because as far as I can see there are no calls into truly Windows Vista or later functions. It is just that the headers have been version fenced off while the binary code remains unaltered.

So, I am almost done. Thank you for your patience.

JakeSmarter commented 1 year ago
[100%] Linking CXX executable openjkdf2-64.exe
x86_64-w64-mingw32-g++: error: 0x10000,0x100000: No such file or directory
x86_64-w64-mingw32-g++: error: 0x1000000,0x10000000: No such file or directory
x86_64-w64-mingw32-g++: error: unrecognized command line option ‘--heap’; did you mean ‘--help’?
make[3]: *** [CMakeFiles/openjkdf2-64.dir/build.make:7785: openjkdf2-64.exe] Error 1
make[2]: *** [CMakeFiles/Makefile2:100: CMakeFiles/openjkdf2-64.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:107: CMakeFiles/openjkdf2-64.dir/rule] Error 2
make: *** [Makefile:124: openjkdf2-64] Error 2
x86_64-w64-mingw32-objdump: 'build_win64/openjkdf2-64.exe': No such file

FYI: Ah, the Win64 SDL2 GitHub workflow does not fail correctly. It’s still reported as “passed” :heavy_check_mark:.

shinyquagsire23 commented 1 year ago

Yeah bumping to Vista minimum (or even just 7) is totally fine, at some point for ✨completion✨ sake I'll make a 32-bit target that still runs on Win98 with like, OpenGL 2.x or something idk.

Win64 target fail

ah, that's no good lol.

Let me know when you're ready to review and merge btw

JakeSmarter commented 1 year ago

Sorry for the delay. :worried: I may have broken something because GNS does not build anymore.

[  0%] Linking CXX shared library ../../bin/libGameNetworkingSockets.dll
/usr/lib/sdk/mingw-w64/lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lprotobuf
collect2: error: ld returned 1 exit status

:thinking: So, I have tried adding link_directories() before GNS linking and set Protobuf_LIBRARIES_PATH to basically build_win64/build_protobuf/lib but to no avail. Do you have any ideas?

JakeSmarter commented 1 year ago

:sweat: Phew, got it. I was using the ~-dynamic~ option on ld which does not exist. The correct option is -Bdynamic. :partying_face: Yey, I can finally get productive!

JakeSmarter commented 1 year ago

:wave: Hi, I have not abandoned this project yet. On the contrary. I have been quite busy, as you may have seen from all the failed CI builds.

The biggest holdup for me until now was integrating GameNetworkingSockets into OpenJKDF2’s CMake build system. Well, lets just say that it may have been partially my fault either. Guided by—in hindsight—rather over optimistic enthusiasm, I have initially tried to build the latest versions of protobuf (v3.22.3) and GameNetwokringSockets (v1.4.1), which per se is not a bad idea. However, only after weeks I have found out that Google developers have introduced some breaking changes since protobuf v3.22.0. Additionally, what did not help either is the fact that GameNetworkigSockets is of abysmal :man_facepalming: code quality, just like every other ValveSoftware product :disappointed:, with the grisly designed and patched up Steam at the forefront. But anyway, I have settled on protobuf v3.21.12 and GameNetwokringSockets v1.4.1. From here on it is probably going to be rather downhill.:crossed_fingers:

Oh, do not worry (yet) about the sort of broken CI builds. I will get them to work. :thinking: Though, I may be needing some help with the MacOS build.

shinyquagsire23 commented 1 year ago

Yeah GNS and Protobuf are both horrendous in their own ways lol, but it does at least solve enough issues to make it worthwhile to have as the network backend. macOS I should be able to handle tho.

JakeSmarter commented 1 year ago

Yeah GNS and Protobuf are both horrendous in their own ways

However, imho protobuf seems to be at least managed and developed by people with knowledge and skills. GNS currently just lacks proper CMake support. You can really see that when you look at the GNS CMake scripts. Interestingly enough, Valve Software even acknowledged their poor CMake integration and made a shout out with the v1.4.0 release about this. :roll_eyes: Yeah, I am not doing a multi‑million dollar company’s work for free.

it does at least solve enough issues to make it worthwhile to have as the network backend

Speaking of that, does not Wine have an open source implementation of DirectPlay? I will have to investigate that. Does GNS integrate with Steam’s match making infrastructure? Because if it does then it is surely worth the effort but if it does not, while effectively being nothing more than DirectPlay, then I am not so sure.

JakeSmarter commented 1 year ago

Speaking of that, does not Wine have an open source implementation of DirectPlay?

:mag_right: at Wine’s DirectPlay implementation, one could be inclined to say that it does but it is very difficult to assess its completeness. :thinking: And then it probably also has depenedencies on the Windows registry, Winsock, and other stuff, so never mind…

JakeSmarter commented 1 year ago

I am almost done with moving all dependencies into CMake.

However, is there a special reason why you have included curl sources directly into the project sources instead of linking to it statically? I am assuming curl is for the auto‑update feature, mainly on Windows? Does it even work? Why not leave this to the package manager? Besides, it currently causes DLL exports on the final executable.

Because I am not sure what to do with curl, I am probably going to leave it for now. Though honestly, in the foreseeable future I would like to get rid of curl in favor of MSIX. There is really no need for implementing an auto‑update feature separately. Better rely on existing infrastructure.

shinyquagsire23 commented 1 year ago

at the moment the reason for curl sources is just that MinGW doesn't really ~have package management on Linux (afaik), and macOS I'm kinda already doing this hack where I use some brew packages and copy them into the app bundle, so something like curl where it implements every feature will mean a lot of extra copying. On Linux though I think I did just leave it to the package manager.

The auto-updates work correctly on macOS and Windows, on Linux it's handled via Flatpak/package managers. It's straightforward enough though imo, just zips and JSON (which I needed both if I wanted JKGM+MoTS secret level auto-extraction). I'd rather keep it in place honestly, it might also be useful for other mod management stuff later maybe. It was also one of the easier third-party dependencies to add in. The only kinda dicey thing is the certs, but apparently that's sorta the de-facto way if handling it on not-Linux (if projects like SuperTuxKart are to be trusted lol)

JakeSmarter commented 1 year ago

It's straightforward enough though imo, just zips and JSON (which I needed both if I wanted JKGM+MoTS secret level auto-extraction).

So you do handle privilege elevation via UAC when you have to update binaries where Users group accounts have FILE_GENERIC_READ | FILE_GENERIC_EXECUTE permissions only, or do you just ignore available updates for those accounts completely? Because, you know, even though openjkdf2.exe can be used as a drop‑in replacement for JK.EXE it may very well be controlled by Administrators group accounts only.

The only kinda dicey thing is the certs…

Yeah, this is another problem which makes me feel quite uncomfortable, and may admins probably too.

it might also be useful for other mod management stuff later maybe

Actually, MSIX works with mods and plug‑ins wonderfully. :wink: And, you would have solved mod versioning and deployment (file permissions stuff) in one go too! :smiley: Really, there is no need to reinvent the wheel in this space.

JakeSmarter commented 1 year ago

:thinking: The workflow build should work but because the mingw-w64 package on Ubuntu 22.04 (LTS) is basically helplessly outdated the build fails. More specifically, it would suffice to update the mingw-w64-x86-64-dev package to version 10.0.0, which does exist for Ubuntu 23.04. However, I do not know how to do this properly. I was hoping to find a MinGW-w64 PPA with the latest versions for LTS but there is nothing. So, I am afraid that Ubuntu 22.04 is stuck on version 8.0.0 of mingw-w64 for ever. At least for now, GitHub does not seem to support any later Ubuntu version than 22.04 either. Do you know how to upgrade mingw-w64?

This issue has not been apparent to me so far because I have been building against Flathub’s org.freedesktop.Sdk.Extension.mingw-w64//22.08 version 11.0.0 (!) runtime. Hence, another way forward could be to transition the Windows and Linux workflows to building against Flathub’s org.freedesktop.Sdk. runtimes. Personally, I would probably favour this because Flathub’s org.freedesktop.Platform runtime releases are regularly updated with the latest dependencies during their lifecycle* and so far you have been releasing OpenJKDF2 for Linux via Flathub only anyway. So, there is not much reason to let the workflow build against Ubuntu.

JakeSmarter commented 1 year ago

https://github.com/shinyquagsire23/OpenJKDF2/pull/252/files#diff-2230bd2e2d3e357254016786859b5db825ab0b004f9c962fe319fce4d50a4f2dR29-R52

For now, lets just download and install updated packages from the pool. Later, we can build on Ubuntu 23.04 or something when it becomes available on GitHub. Unfortunately, I do not know how to download the latest packages from the pool automatically.

shinyquagsire23 commented 1 year ago

Probably going to be pulling this in soon if I can (and fixing up macOS support while I'm at it), I ran into a really annoying hitch w/ macOS where I'm having to pull in way more dylibs than I'd like for SDL2_mixer, and I've been overdue on fixing the lack of static linking

shinyquagsire23 commented 1 year ago

ok so I have an initial working macOS build on test_win_version_info, for the most part everything ported over fine.

The only changes I made were to

I'll keep chipping away on test_win_version_info and then merge it manually probably, I ran into a really nasty portability issue on macOS where SDL2 is importing a ton of garbage and I think I'm ready to do things correctly and prune down dependencies + statically link as much as I can. I think the remaining items are more or less just actually getting CMake to use the new built projects instead of all the old tangled dependencies w/ Homebrew and whatnot.

shinyquagsire23 commented 1 year ago

The changes are merged now. I made sure everything was fixed up for SDL_mixer and it's working on all the main platforms. MSVC required some interesting changes to ExternalProject_Add, specifically BUILD_BYPRODUCTS. Otherwise ninja wouldn't work correctly.

The HiDPI changes also seemed to expose an interesting bug on Windows with MinGW. For some reason, SSAA has very little effect on performance, the window size greatly affects the FPS even if the game itself is rendering tiny resolutions. For some reason, this doesn't happen on MSVC, so I'm going to ~try and sort it out before the first release with HiDPI fixes.

The bug isn't specifically from the HiDPI change though, it actually was present on v0.8.19 and previous releases.

JakeSmarter commented 1 year ago

Sorry for the late response. I was quite busy lately, so I have not had the time to finish the PR nor test it thoroughly enough on Windows. So, I am a bit surprised you went ahead anyway because you may have not made yourself a favor by merging prematurely. There was quite some stuff I wanted to iron out before merging, not only for Windows but also for Linux.

When it comes to HiDPI and SSAA, I am not sure this issue is actually exclusive to Windows because rendering behaves quite strangely in general on Linux either. Maybe this phenomenon just becomes more prominent on Windows, especially on HiDPI desktops. I get really low FPS numbers on high resolution viewports, even on modern machines. 8k viewports should still deliver FPS numbers in the hundreds.

Besides, HUD scaling is broken either. Integer HUD scaling factors (including 1.0) do not actually translate to integer multiples of the HUD bitmaps.

Just for the sake of completeness, please note that both things existed before the PR and it does not affect either of the issues above.

prune down dependencies

:+1: This is always a good idea.

statically link as much as I can

:thinking: Sounds easy but in my experience unfortunately this usually works detrimentally on dependency security and stability. You have to maintain any dependency fixes yourself.

shinyquagsire23 commented 1 year ago

Yeah the FPS performance vs resolution scaling is definitely off, haven't had time to look at it. It seems to be CPU-related though because it's much worse with -g, and I ended up moving MinGW to -O2 because of that.

I mostly ended up merging though because that macOS dependency hell was really nasty (like 20 audio/codec/midi libraries), so it kinda forced my hand a bit. But the way I kinda see it is like, even with some things being janky (ie Linux should probably use system libraries for libpng), on net it's an improvement.

With regard to dependency security/stability: The good news is that SDL2 is the one dependency I'm actually worried about from the standpoint of 'an OS update can break this easily', but statically linked SDL2 has a 'backdoor' where it can dynamically load a newer SDL2. Everything else is like, yeah libpng gets CVEs but OpenJKDF2 isn't fetching remote PNGs, and you'd need a mod messing with the COG VM to break ASLR. The user-facing stability issues will be mostly in SDL2 I think.