Improve CMake build and port conversion tools to Linux #19
Conversation
|
Thanks, I'll take a look at this over the weekend probably. |
There was a problem hiding this comment.
I think this mostly looks good but I'd have two main requests for now:
First, can you split the .gitattributes and line endings commits to a separate PR so this can be rebased on top of that? Having 839 file changes in the combined diff is making this difficult to review.
Second, I don't think UnixCompat.h should exist. The intent of the shim header (despite the name...) is to quarantine all of the MSVCRT problems in a way that the tools code doesn't need to worry about it, so a Unix port of it should just use a separate source file (and _fseeki64 and _ftelli64 should just have new wrapper funcs) and keep the same header.
sprintf_s doesn't need to be implemented, just change the source files to use sprintf instead, all of the tools using sprintf_s now are already including the Common property sheet that disables the VS "insecure CRT" warnings.
I guess fopen_s is still needed for now because bin2gp isn't using shim library, but bin2gp isn't needed for the package build. I can fix that afterwards so it can be taken out.
|
Actually a correction on It should be using |
|
It's possible to exclude whitespace changes in the GitHub UI, and to view individual commit diffs or shift-click select the range of commits excluding the normalization commit, but it's a lot clumsier than I realized it would be. GitHub also refuses to completely hide files with whitespace-only diffs, so you get 817 separate "Whitespace-only changes." messages, unlike the Git CLI. I'll split those commits into a separate PR. That will get rid of the big scary
Here's your opportunity to bikeshed a better name than |
MakeTimestamp/MakeTimestamp.cpp
Outdated
| ts.SetMacEpochTime(currentTimeUnix + ts.kMacEpochToUTC); | ||
|
|
||
| ts.SetLocalYear(currentTimeStruct->tm_year + 1900); | ||
| ts.m_localMonth = currentTimeStruct->tm_mon; |
There was a problem hiding this comment.
kMacEpochToUTC should be subtracted from UTC time, not added. Also, local month needs to be +1 because it is months-since-January in struct tm
See GpSystemServices_POSIX::GetLocalDateTime
There was a problem hiding this comment.
Okay, it's really stupid that tm_mon is 0-based but tm_mday is 1-based. I didn't have much of a reference for the correct format of your timestamp files, so thanks for catching this one for me.
I don't think you're right about the sign error, though. Assuming the Mac timestamp counts the number of seconds since 1904-01-01 00:00 UTC, which is the only sane interpretation I can think of,
2024-08-06 09:00 UTC
= 1722934800 seconds since 1970-01-01 00:00 UTC
= 3805779600 seconds since 1904-01-01 00:00 UTC
(1970-01-01 00:00 UTC) - (1904-01-01 00:00 UTC) = 2082844800
3805779600 = 1722934800 + 2082844800
Subtracting 2082844800 as per GpSystemServices_POSIX::GetTime():
1722934800 - 2082844800 = -359910000
(1904-01-01 00:00 UTC) + (-359910000) = 1892-08-04 09:00 UTC
That clearly isn't right, so it must be the other way.
Maybe kMacEpochToUTC isn't the best name for a magic number of 2082844800 but either it needs to stay positive and every instance of converting Unix → Mac should add this constant, and subtract for the reverse conversion, or the sign of kMacEpochToUTC needs to be flipped and every instance of converting Unix → Mac should subtract it, and add for the reverse.
If you intended the number to be the Mac epoch represented as a Unix timestamp, it should be a negative number.
Unfortunately All file system calls in this use wchar_t on Windows so Unicode paths are properly supported. |
|
OK, I looked through these as best I can and I think that should be all of the issues. |
AppendFmt calls vsnprintf twice, first to check the size of the
formatted string, then to write it for real, but it used the same
va_list for both calls, so the second call got an invalid va_list after
the first call had already consumed all its arguments.
This is UB and at least on Linux makes the second call print garbage. I
presume Windows implements va_list differently such that this somehow
worked correctly, because on Linux, all of the dialog items get parsed
into invalid JSON due to this bug, with lines like this (note the
missing second array element and closing bracket):
"pos" : [ -55947262,
The BUILD_INTERFACE generator expression isn't actually useful as we don't use export(). CMAKE_CURRENT_SOURCE_DIR is also unnecessary as CMake treats relative paths as relative to the current source directory (or the current binary directory for outputs).
This is the counterpart to WindowsUnicodeToolShim.cpp for *nix systems.
Downgrades c++11-narrowing to a warning when compiling with Clang. A better solution would be to fix the error, but this at least brings Clang in line with the more permissive GCC, which treats this as a warning by default.
I don't think this actually gets hit when converting the base Glider PRO resources, but it's an obvious error that makes the condition always true.
Also includes <algorithm> to provide std::find(). Evidently something in Windows implicitly provides find(), but it was broken on Linux.
- Adds missing includes, removes some unused ones. - Removes unused Windows.h and shellapi.h includes. - Uses / as path separator. This is portable and should still work fine on Windows.
Also add stddef.h include to GpRenderedGlyphMetrics.h to make size_t visible. Does MSVC provide size_t by default, or via stdint.h? This prevents compilation and I can't see how this would have compiled on Windows otherwise.
Removes unused Windows-specific headers and adds arguments to specify source and output directories, which is necessary for out-of-tree builds, which are necessary to avoid name collisions on platforms where executables have no file extension (so, basically everything except Windows).
- Includes WindowsUnicodeToolShim.h. - Replaces sprintf_s() with std::ostringstream. - Replaces fopen_s() with fopen_utf8(). - Switches to the toolMain() wrapper, which is a no-op on except on Windows, where it should improve Unicode support, though I can't test that on my end. - Replaces \ with / in paths, which should still be Windows-compatible.
These are only used on Windows and the converted files have been committed to the repo, but this might be useful to someone.
This automatically cleans up artifacts specified as BYPRODUCTS after running the commands.
This is meant as an alias to the name of the main executable, for consistency with the Executable install component.
All the individual executables are marked EXCLUDE_FROM_ALL, but the added Tools custom target is marked ALL. The only visible effect of this is that ConvertColorCursors does not get built by default, since all the other tools are dependencies for the Resources or Tools targets, which are both built by default. Otherwise, the benefit is simply to keep the dependency tree clean. Only three targets are built by default: Aerofoil, its resources and the conversion tools. Everything else is built to satisfy dependencies.
Uses add_custom_command() to add rules for generating these directories, rather than creating them only when CMakeLists.txt runs. With this change, you can manually delete Packaged from the build directory without causing subsequent builds to fail.
This will make CMake rebuild resources if for instance the fonts are updated.
This allows intermediate build artifacts to be cleaned up without specifying them individually as BYPRODUCTS. When building a data file <name>, all files are first built under tmp/<name> before the target itself is moved from tmp/<name>/<name> to Packaged/<name>. add_house_file() could also be modified in this way, but it wouldn't reduce the boilerplate by much, so it's probably not worth it.
I thought I saw |
|
Oh also please let me know if you want a credits line in the about box. |
|
Thanks for the merge! Regarding the timestamps:
Sure, I have no objections to being mentioned. |
Firstly, thanks for your work on this port, Eric! I was pleased to discover John Calhoun had released Glider PRO's source and just as pleased to discover you had spent the effort to bring this piece of Macintosh history to modern systems.
This PR ports the house and resource conversion tools to Linux and improves the CMake build system to produce (and optionally install) a fully functional build of Aerofoil without the need to import the converted resources from the Windows build.
I've tried to do this in as portable a way as possible, and building on Mac may work without further modification, though I am unable to test this myself.
I've kept Windows-specific code untouched as best I can, but please let me know if I've somehow broken something in the Windows build.
Other notable changes
Some files have CRLF endings, some files have LF endings, some files have mixed line endings. Letting Git normalize line endings internally will avoid whitespace-only merge conflicts in the future.
master, but if someone runs into conflicts, add-X renormalizetogit mergeorgit rebaseto resolve the issue automatically.va_listingpr2gpa.cpp::AppendFmt()This one is actually a pretty major bug that the Windows version doesn't exhibit by sheer luck. On Linux, this causes the second call to
vsnprintf()to print garbage, resulting in invalid JSON.=instead of==ingpr2gpa.cpp::DecompressSound()This typo means the following "unknown format" else-branch can never be hit, but I don't think it causes issues in practice when converting Glider PRO's resources. Still, no reason not to fix this.
/as path separator in a couple of the conversion toolsWindows should have no problem with this whatsoever, but I'm making a note of this as I haven't tested Windows. If this does somehow break something, it will likely break it loudly, at least.
c++11-narrowingerrorThis is a warning by default in GCC, but a hard error in Clang, so I've added
-Wno-error=c++11-narrowingwhen compiling with Clang. At some point, someone could fix the cause of this warning as a more permanent fix. I might submit another PR squashing some warnings in the future.LINUX.txtwith new and more detailed instructionsRelated issues
Probably resolved, as building on Linux is pretty painless now and Aerofoil seems to run fine, though I haven't rigorously tested absolutely everything.
Resolved if Mac can successfully build the tools in this branch.