Add macOS support

[Thunderforge]

This project currently has Windows and Linux builds for each release. I'd like to see macOS releases as well so that I can run OpenTESArena on my preferred platform.

[afritz1]

You saw what I said on the OpenMW IRC, didn't you? 😉

The best way to get the ball rolling is to have a Mac user try and build the project with CMake, and then leave their findings here. There aren't very many dependencies -- just SDL2, OpenAL Soft, WildMIDI, and a C++11 compiler. WildMIDI is even optional for now if you don't want music (with the HAVE_WILDMIDI define, although that might change in the future). Instructions for building from source are here.

There might need to be some changes to the top-level CMakeLists.txt, which is fine. We'll probably also want to add Clang to the list of Travis CI builds in .travis.yml.

In order to have consistent macOS releases, we'd need someone on that operating system to maintain them, since I'm on Windows and @Ragora is on Linux.

[Thunderforge]

Yup, I did see you saying that nobody had asked for a Mac release, so I figured I'd do that.

I'm a complete newbie to C++ (I have plenty of Java and Python experience though) and am especially confused with dependency management (on Java, we just use Gradle or Maven to download and build everything with one command, and in a sandboxed mode too). That said, I decided to give it a try.

First, I installed CMake, OpenAL Soft, and SDL via Homebrew (WildMIDI wasn't available). I simply ran cmake -G "Unix Makefiles" .., and was able to build successfully (with a warning that I wouldn't have MIDI music). When I ran make after that, I got a number of errors like this:

~/OpenSource/OpenTESArena/components/archives/archive.cpp:61:19: error: 
      use of overloaded operator '-' is ambiguous (with operand types
      'std::streampos' (aka 'fpos<__mbstate_t>') and 'std::streamsize'
      (aka 'long'))
    return newPos - mStart;
           ~~~~~~ ^ ~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/string:483:37: note: 
      candidate function
    _LIBCPP_INLINE_VISIBILITY fpos  operator- (streamoff __off) const {f...
                                    ^
~/OpenSource/OpenTESArena/components/archives/archive.cpp:61:19: note: 
      built-in candidate operator-(long long, long)
    return newPos - mStart;
                  ^
~/OpenSource/OpenTESArena/components/archives/archive.cpp:61:19: note: 
      built-in candidate operator-(unsigned __int128, long)

Does this give you a clue as to what might be going wrong?

[afritz1]

Hmm, are you compiling in 32-bit mode by chance? We've gotten some warnings about other conversions between std::streampos and unsigned int before when compiling for 32-bit. Maybe try using return newPos - static_cast<std::streampos>(mStart); there instead, since it doesn't like trying to subtract a 32-bit signed integer from a 64-bit unsigned integer. It says in the standard that std::streamsize should never be negative, so that static cast should be safe.

[Thunderforge]

I don't think I'm compiling in 32-bit mode. Macs have been 64-bit for at least a decade (and Apple just announced that the next version of macOS will be the last to support 32-bit apps). I also tried adding properties to target macOS 10.12 (10.5 was the last to be available in 32-bit).

cmake \
-D CMAKE_OSX_DEPLOYMENT_TARGET="10.12" \
-D CMAKE_OSX_SYSROOT="macosx10.12" \
-D CMAKE_BUILD_TYPE=Release \
-G "Unix Makefiles" ..

Didn't work though. Before I start messing with the source doe, is there some CMAKE property I need to set to switch between 32-bit and 64-bit mode?

[afritz1]

I think this Stack Overflow question should answer your 32/64-bit question. The third bullet is for Mac, and I think /path/to/source/dir is just ..:
https://stackoverflow.com/questions/5334095/cmake-multiarchitecture-compilation

Unfortunately I'm not a CMake expert, so I'm just figuring these things out as we go. I use the CMake GUI on Windows.

Also, my previous suggestion for the static cast was wrong, but the next try would be return static_cast<long long>(newPos) - mStart;.

[Thunderforge]

Adding -D CMAKE_OSX_ARCHITECTURES=x86_64 didn't work, but the casting via static_cast<long long>(newPos) did work and I got past that issue.

Unfortunately, I came across a new set of errors involving SDL not being found.

In file included from /Users/Will/OpenSource/OpenTESArena/OpenTESArena/src/Entities/../Game/Game.h:6:
/Users/Will/OpenSource/OpenTESArena/OpenTESArena/src/Entities/../Game/InputManager.h:23:38: error: 
      unknown type name 'SDL_Keycode'
        bool keyPressed(const SDL_Event &e, SDL_Keycode keycode) const;
                                            ^

This is actually the same issue I came across for OpenMW. It looks like you are using the same FindSDL2.cmake file that they were using. I discovered that there was a bug report to create a better solution that would not require such a file. Perhaps that could be implemented here?

[afritz1]

I'm reading through that OpenMW post and I'm wondering why this issue hasn't already been run into by other Mac users before.

I don't think the problem is with finding SDL2main, because that only occurs at the linking stage. Your compiler error is from a missing type SDL_Keycode, but that is included in a file within SDL.h. Maybe CMake can find SDL.h, but Clang isn't finding the other SDL header files? I'm confused. CMake should tell you if it can't find a path.

Does this help at all? https://stackoverflow.com/questions/28016258/using-homebrew-installed-sdl2-with-xcode. I think you would give those results from sdl2-config to CMake through CFLAGS, CXXFLAGS, and/or LDFLAGS. It's just a shot in the dark though, sorry. We could take a look at sdl2-config.cmake if that's a better option in the long run.

[Thunderforge]

Turns out that I had SDL installed from a Pygame 1.9.1 download from years ago and the way it installed it was messing with this project. Once I deleted that, I successfully compiled OpenTESArena!

The next problem is a failure to read the options.txt file, which I'll start looking into tomorrow. I've copied it over as the instructions say, so I'll need to figure out if something else is going on.

[Game/Game.cpp(33)] Initializing (Platform: Mac OS X).
[Game/OptionsParser.cpp(37)] Reading "options/options.txt".
[Utilities/File.cpp(13)] Error: Could not open "options/options.txt".

In the mean time, should I create a PR for the castings that we did earlier to get the app to build on macOS?

[afritz1]

That's good news that you've compiled it!

I wonder why the executable wouldn't find the options folder in the same directory as it. Does macOS have a particular way of getting the current working directory? Maybe the std::ifstream in File.cpp needs to be in ASCII mode instead of binary? options.txt is a text file after all, but opening in binary mode shouldn't prevent it from working in the first place. Your folder should look something like this:

data
- ARENA
- eawpats (used with WildMIDI)
- text
- icon.ppm
options
- options.txt
TESArena.app (or whatever the Mac extension is for executables)
Shared libraries...

You could do a pull request if you'd like. I would also add a comment there explaining why it's being done, like // Type conversion is required for macOS + Clang compatibility..

[Ragora]

Only thing that really makes sense to me in the immediate sense is that the working directory for whatever reason isn't set correctly. Or permissions aren't set correctly to allow read access on the file, etc. Of course, this is assuming your install follows the folder structure in the post from @afritz1 above.

The error comes from here: https://github.com/afritz1/OpenTESArena/blob/opentesarena-0.4.0/OpenTESArena/src/Utilities/File.cpp#L13

Which is being raised upon the attempt of opening the file in the first place.

[Thunderforge]

I don't understand what I was doing wrong earlier, but when I built the project in CLion instead of just from my Mac command line, I was able to successfully run Arena on macOS!

I noticed an issue where the system mouse appeared above the game crosshairs, but otherwise the game worked without incident.

Right now, it's basically a Unix executable file. Mac apps (.app) are actually special folders that macOS treats as an executable. The idea is that you have one app that has a bunch of files bundled inside; this is where the Unix executable, the options and data folders, etc are stored. I took a look into how OpenMW creates their Mac app files via CMake. Once it's in that format, I think I'd consider macOS supported, but of course subject to any bug reports.

[afritz1]

Great! We can start thinking about adding macOS support to the list of operating systems once we figure out the necessary changes to the CMake code.

I'm not sure why the system mouse would be visible in-game. Maybe SDL2 doesn't behave the same on all platforms.

[Thunderforge]

Possibly, or alternatively macOS isn't willing to give it up. I figure I'll hold off on reporting Mac-specific issues until I make the changes to get it as a Mac app, and have instructions in the Readme so that people besides me can build it on that platform.

[Thunderforge]

Still wrestling with CMake bundling. I have most of the stuff working, including an icon on the Mac app.

The biggest issue that we're going to have to overcome is how paths are handled. Mac apps have a structure like this:

• TESArena.app
  • Contents
    • Info.plist [a Mac properties file]
    • Frameworks
      • [Bundled versions of the required libs, e.g. SDL2]
    • MacOS
      • [Compiled files, i.e. TESArena]
    • Resources
      • [All non-code resources, including icons, properties files, and graphics, e.g. the data folder and options folder]

As you can see, the data and options folders are supposed to be in a different directory from the compiled code. However, OpenTESArena is assuming that they are in the same directory. In order to get the app to run, I've had to modify, for instance, OptionsParser::PATH.

How should we go about handling this? My thought is maybe have a class that contains methods like getOptionsFolder() that returns different locations based on the platform. But I'm not sure if you have a preferred way.

Also, should the CMakeLists.txt file start with PROJECT(OpenTESArena CXX) instead of PROJECT(TESArena CXX)? Right now, that's what the app is being named, and I would have expected it to match the name of the project on GitHub.

[afritz1]

In this case, I guess the options path should have Resources/ prepended to it? Maybe SDL2 has the answer: https://wiki.libsdl.org/SDL_GetBasePath. This looks like the cross-platform way of getting the current working directory.

You're right that the project name should be OpenTESArena instead of TESArena, but I thought there was some issue with the naming on Linux (due to a collision between the executable name and a folder name).

[Thunderforge]

SDL_GetBasePath was exactly what I needed! Thanks for pointing that out. So now the code will look like this PPMFile::read(std::string(SDL_GetBasePath()) + "data/icon.ppm", iconWidth, iconHeight);.

[afritz1]

That looks about right. I'm not positive if SDL_free() needs to be used since it appears to just be an alias for free(), but it's probably fine to give SDL_GetBasePath() to the std::string constructor.

Actually, freeing it with free() doesn't work, but using SDL_free() and std::string() work. Hmm.

[Thunderforge]

Are there docs for SDL_free()? I've been using the SDL wiki, but it doesn't look like it exists there.

[afritz1]

I searched SDL_free() and I didn't find anything either, but looking in SDL_stdinc.h and SDL_stdinc.c, it seems like SDL_free() is probably just free(). I'm still wondering why free() didn't work but SDL_free() did, though.

[afritz1]

Oh, so I think the reason SDL_free() works and free() doesn't is because in Visual Studio debug mode, it uses a debug version of free() called _free_dbg(), and the SDL heap is separate from the primary heap in that case? Not sure, but it looks like SDL_free() is required after all and std::string() cannot be used on its own. I'll make some changes so SDL_GetBasePath() is only called once for the whole program.

[abelsromero]

In this case, I guess the options path should have Resources/ prepended to it? Maybe SDL2 has the answer: https://wiki.libsdl.org/SDL_GetBasePath. This looks like the cross-platform way of getting the current working directory.

Sorry to bring bad news, but this has introduced an issue. Previous to this change one could set an absolute path for the ARENA installation. Now this is not possible, the path must be relative to the execution path.
This was specially useful since one could have a single ARENA directory and multiple openTESArena installations reusing it.

[Thunderforge]

@abelsromero Are you sure that the issue is caused by this change? PR #81 only added SDL_GetBasePath() to options .txt and data/icon.ppm. I'm not seeing how that would affect the Arena installation path. For what it's worth, I can still use an absolute path on macOS.

[afritz1]

Maybe there could be a boolean in options.txt for whether ArenaPath is absolute or relative to SDL_GetBasePath()? Is there a better way?

[mdmallardi]

Couldn't that be determined from the path defined in options.txt? If it starts with a drive letter (x followed by a colon) or a leading forward slash, then treat the path as absolute. Otherwise, relative.

…

On Jul 12, 2017 5:05 PM, "Aaron" ***@***.***> wrote: Maybe there could be a boolean in options.txt for whether ArenaPath is absolute or relative to SDL_GetBasePath()? Is there a better way? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#74 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFcELNPr100D_uXKoTGiWmI7r4vCZEivks5sNTUmgaJpZM4ORh4Z> .

[afritz1]

If it starts with a drive letter (x followed by a colon) or a leading forward slash, then treat the path as absolute.

That sounds like the better way.

[abelsromero]

@abelsromero Are you sure that the issue is caused by this change?

As a rule, I am never sure of anything 💃 , but I checked a build against the previous commit (ca8d126) and it worked with the absolute path, after that, it doesn't. It says that the X path is not a valid ARENA folder.
I can run more tests tomorrow if you want to double check.

If it starts with a drive letter (x followed by a colon) or a leading forward
slash, then treat the path as absolute.

That's how I usually do it, I check agains some regex to check the type of path. In most projects I end up with a FileUtils or ResourceUtils class to encapsulate the location of different files. I do Java mostly, so it's a method that receives some strings and returns some reference like a DataStream, so I can even load data from HTTP or remote sources. Not sure what options C++ provides.

[Thunderforge]

Couldn't that be determined from the path defined in options.txt? If it
starts with a drive letter (x followed by a colon) or a leading forward
slash, then treat the path as absolute. Otherwise, relative.

This approach won't work on macOS or Linux, since absolute paths don't start with a letter. However, absolute paths begin with a leading slash, so I'm pretty sure you could check for that. For instance, the absolute path to my Arena installation is /Applications/Arena.

I imagine the code would basically be: If Windows, check for a drive letter. Else, check for a leading slash. If neither are present, it's a relative path.

[afritz1]

I imagine the code would basically be: If Windows, check for a drive letter. Else, check for a leading slash. If neither are present, it's a relative path.

I suppose this could be implemented as something along the lines of:

std::string platformName(SDL_GetPlatform());
if (platformName == "Windows")
{
    // Drive letter...
}
else
{
    // Leading forward slash...
}

SDL_GetPlatform() should help us avoid ifdef's, since it looks like the platform names under "Remarks" are pretty dependable.

[Thunderforge]

SDL_GetPlatform() should definitely work. By the way, I just checked on my Mac and that it will return Mac OS X, despite the official name being macOS (it was Mac OS X from 10.0-10.7, OS X from 10.8-10.11, and now macOS from 10.12 onward). So the good news there is that we can just check for "Mac OS X" and trust that it will work on any version we want to support.

[Thunderforge]

Found that CharacterClassParser is another spot where we'll want to use SDL_GetBasePath(), since we need to load data/text/classes.txt. I see that Game now has some more detailed code involving making sure that we can use that method, and converting the slashes. Perhaps we can extract this code to a method in File or something else?

[abelsromero]

Seeing the comments I guess the issue has been confirmed by others. But just in case I double-checked in another machine and the problem with absolute paths happens.

[Thunderforge]

So I have my work in progress branch here. Currently, I have it so that when compiled on macOS, CMake will create a bundled macOS application, with icon and everything.

I wouldn't consider this "ready" until it adds WildMidi support though, and that's got me stuck. If I don't add Wildmidi to the build path, everything works fine. But when I do add it, I get an immediate app crash:

 dyld: Library not loaded: @executable_path/libWildMidi.2.dylib
  Referenced from: /Users/Will/CLionProjects/OpenTESArena/cmake-build-debug/TESArena.app/Contents/MacOS/TESArena
  Reason: image not found

Any ideas about this?

[afritz1]

See #99; macOS is essentially supported at this point, just need to add a 0.6.0 build to the releases page and add macOS support to Travis CI (in #93).

[afritz1]

We have a 0.6.0 macOS build on the releases page now. We can figure out the problem with WildMIDI in the future. It's probably an easy fix.

[Thunderforge]

@afritz1 Created #107 so that we can track music on macOS.