Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mac M1 Support for Cmake #87

Open
atsb opened this issue Sep 3, 2021 · 15 comments
Open

Mac M1 Support for Cmake #87

atsb opened this issue Sep 3, 2021 · 15 comments

Comments

@atsb
Copy link
Contributor

atsb commented Sep 3, 2021

Generating a Unix Makefile for an M1 Mac incorrectly adds -mtune=cortex to some build arguments for coal.

M1 Macs do not support and do not require regular ARM mtune arguments (Apple Clang on M1 does not support them either). These need to be removed when generating on an M1.

@Corbachu
Copy link
Contributor

Is there an example to compare against? I do not own a Macintosh of that type, unfortunately (only Intel based), and that probably will not change for quite some time.

If you are aware of the fix, please make a PR that targets CMake with the appropriate guards for that platform, otherwise I will be unable to incorporate or validate those arguments.

@atsb
Copy link
Contributor Author

atsb commented Sep 10, 2021

I will attempt a few fixes on my side and do a PR or a diff after some testing. Thanks!

@atsb
Copy link
Contributor Author

atsb commented Sep 11, 2021

So I did it (sort of).

I forked and pushed the few changes I did here:
https://github.com/atsb/EDGE/commit/a2e5f7b1ef050a573e0e0764cbafb6da42c910a3

I did have to add this: src/md5_conv/sse2neon.h -> it seems M1 Macs do not have this file at all (I compiled the header from its github repo).

I also have a binary here (with stack traces and link.txt):
https://github.com/atsb/EDGE/releases/tag/Gibbon

Maybe this will be useful to get it working in the future on M1's once they become more popular. I'm more than willing to assist with debugging this however.

@Corbachu
Copy link
Contributor

It looks like it crashes around where it does the config parse.

On your system, do you see "edge.ini" created anywhere? I only ask because that's where it appears to have segfaulted on your M1. If we have a config created then it might be further down in startup.

If a config file is not created in whatever directory EDGE writes it to, then it might be a bug with the way the file system is set up on MacOSX, where it might be unable to find, read or write to the homedir (or equivalent). .

@atsb
Copy link
Contributor Author

atsb commented Sep 15, 2021

So what happens is the EDGE2 directory is created in Library/Application Support. However, the EDGE.ini is not created. So I took the default EDGE.ini from Windows and put it in the same location.

Error is now:
WARNING: error parsing config file Library/Application Support/EDGE2/EDGE.ini: Bare key /language "ENGLISH" cannot contain whitespace at line 1

both Vi and Emacs do not list any whitespace in the file. I also used 'cp' rather than any GUI editors to copy the file, to preserve the contents.

@Corbachu
Copy link
Contributor

Corbachu commented Sep 16, 2021

This is sounding like a path issue. EDGE is probably not finding its own directory to read off the files/folders it needs; in the same directory you put EDGE.ini into, also put the "base" folder into it.
"LANGUAGE" is referring to DDFLANG, or "language.ldf", which is normally found in .\EDGEDIR\base\doom.

@Corbachu
Copy link
Contributor

Corbachu commented Sep 17, 2021

@atsb Looking into src\dstrings.h, I see some defines at the bottom. I'm not sure if recent versions of Macintosh have changed the subdir expectation or not.

It might be a combined issue between post-building on OSX (making sure files are copied via CMake/Clang to the right location), and EDGE itself setting this EDGEHOMESUBDIR directive properly:

#ifdef WIN32 //TODO: Application Data (WinXP?), do we need this explicit?
#define EDGEHOMESUBDIR  "Application Data\\EDGE"
#elif MACOSX
#define EDGEHOMESUBDIR  "Library/Application Support/EDGE2"
#else // Linux
#define EDGEHOMESUBDIR  ".EDGE2"
#endif

@atsb
Copy link
Contributor Author

atsb commented Sep 17, 2021

I think you may be right. Safest thing to do would be that I would modify them to simply look into the same directory as the binary. Then go from there.

@Corbachu
Copy link
Contributor

@atsb Let me know how I can help! Looking forward to supporting the M1 in EDGE 😊

@atsb
Copy link
Contributor Author

atsb commented Nov 15, 2021

So I've been working on this silently, I've been debugging it probably way to much, but it always fails at:

m_toml.h

        if (std::find_if(it, key_end,
                         [](char c) { return c == ' ' || c == '\t'; })
            != key_end)
        {
            throw_parse_exception("Bare key " + key
                                  + " cannot contain whitespace");
        }

It actually also fails on my Intel Mac too. The INI file generated is (pasted only part of it):

/language	"ENGLISH"
/ddf_strict	"0"
/ddf_lax	"0"
/ddf_quiet	"0"
/aggression	"0"
/in_grab	"1"
/in_keypad	"1"
/in_running	"0"

The only whitespace being the tabulature lines between the config and the value.

It may or may not be relevant, but I was also helping on the 'https://github.com/dashodanger/EDGE-classic' fork (a fork of EDGE 1.35). Now it uses the same format (.cfg instead of .ini) endings, and this one works fine. I know a lot changed from 1.x to 2.x but it definitely seems to be incorrectly parsing the config file.

I've also tried modifying the path to be the same as the binary itself, removing all whitespace from directory paths etc..

@Corbachu
Copy link
Contributor

I'll fire up my intel Macbook Pro here shortly. Please post the compiler error also if you can as well (assuming you are using XCode).

I've since fixed a few things with this via 695e8e7, so please pull and recompile to see if we are still running into the problem. I can't verify if it fixes Mac yet.

@atsb
Copy link
Contributor Author

atsb commented Nov 20, 2021

I tried it from that commit and from the latest master. Seems the CMake file is now broken on a Mac (and finds SDL1 instead of SDL2).

CMake Error at CMakeLists.txt:5 (add_subdirectory):
  add_subdirectory given source "source_files/ajbsp" which is not an existing
  directory.


CMake Error at CMakeLists.txt:6 (add_subdirectory):
  add_subdirectory given source "source_files/coal" which is not an existing
  directory.


CMake Error at CMakeLists.txt:7 (add_subdirectory):
  add_subdirectory given source "source_files/ddf" which is not an existing
  directory.


CMake Error at CMakeLists.txt:8 (add_subdirectory):
  add_subdirectory given source "source_files/deh_edge" which is not an
  existing directory.


CMake Error at CMakeLists.txt:9 (add_subdirectory):
  add_subdirectory given source "source_files/epi" which is not an existing
  directory.


CMake Error at CMakeLists.txt:10 (add_subdirectory):
  add_subdirectory given source "source_files/glew/build/cmake" which is not
  an existing directory.


CMake Error at CMakeLists.txt:11 (add_subdirectory):
  add_subdirectory given source "source_files/libjpeg" which is not an
  existing directory.


CMake Error at CMakeLists.txt:12 (add_subdirectory):
  add_subdirectory given source "source_files/libpng" which is not an
  existing directory.


CMake Error at CMakeLists.txt:16 (add_subdirectory):
  add_subdirectory given source "source_files/zlib" which is not an existing
  directory.


-- The CXX compiler identification is AppleClang 13.0.0.13000029
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found SDL: /usr/local/lib/libSDLmain.a;/usr/local/lib/libSDL.dylib;-framework Cocoa (found version "1.2.15") 

@Corbachu
Copy link
Contributor

Wait - that’s not EDGE. I think you are building EDGE-Classic.. They didn’t fork from the official EDGE repo so their structure and build system is completely different. For instance, we don’t use GLEW anymore and our repo structure isn’t like how Classic structures it but instead largely how it’s been supported historically.

Please make sure you are pulling from the EDGE repo and try again 🤭😅😎

@atsb
Copy link
Contributor Author

atsb commented Nov 21, 2021

Oh dear, I didn't even realise. Serves me right for naming the two folders almost identically!

Edit: Make still adds ''-mtune=cortex-a7'' to M1 builds, I'll apply my local fixes to get it building. I'll test also on my intel mac as that is a bit easier (does not require this patching).

@atsb
Copy link
Contributor Author

atsb commented Nov 21, 2021

So I made this little change to stop the cortex arm setting on Macs.

if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm" AND NOT APPLE)
		set ( CMAKE_CXX_FLAGS "-mfpu=neon-vfpv4 -mtune=cortex-a7 -mhard-float ${CMAKE_CXX_FLAGS}" )
	elseif(${CMAKE_SYSTEM_PROCESSOR} MATCHES "aarch64" AND NOT APPLE)
		set( CMAKE_CXX_FLAGS "-mcpu=cortex-a72 -mtune=cortex-a72 -march=armv8-a+fp+simd+crc ${CMAKE_CXX_FLAGS}" )
	elseif(${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm64" AND APPLE)
		set( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" )
	elseif(CMAKE_SIZEOF_VOID_P EQUAL 4 AND NOT MSVC)
		set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -msse2")
	endif()

Edit

So it still fails at the exact same point. I have base, all the ddf's, the epk file etc..

EDGE (ARM64 Clang) <unknown version> compiled on Nov 21 2021 at 08:20:05
EDGE homepage is at http://EDGE.sourceforge.net/
EDGE Wiki is at http://3dfxdev.net/edgewiki/
EDGE forums are located at http://tdgmods.net/smf
EDGE problems should be reported via https://github.com/3dfxdev/EDGE/issues
EDGE is based on id Tech by id Software http://www.idsoftware.com/
Command-line Options:
  ./EDGE 
==============================================================================
E_ShowCPU: Getting CPU information...
==============================================================================
M_LoadDefaults from /Users/<USERHERE>/Library/Application Support/EDGE/EDGE.ini
WARNING: error parsing config file /Users/<USERHERE>/Library/Application Support/EDGE/EDGE.ini: Bare key /language	"ENGLISH" cannot contain whitespace at line 1

I also use this sse2neon header from its GitHub repo (because M1 macs do not contain it at all, I'm guessing officially, it is no longer supported, because it cannot be supported on non-Intel hardware anymore). It compiles obviously, but might be worth checking and disabling such things for M1 architectures.

sse2neon.txt

Edit

Another fix I did (to fix the link.txt errors)

elseif(APPLE)
	set_target_properties(EDGE PROPERTIES LINK_FLAGS "-framework OpenGL -framework Cocoa -framework IOKit")
	target_link_libraries(EDGE stdc++ "-framework Foundation" "-framework Cocoa" "-framework IOKIT" objc)

You don't need the SDL2 and COCOA_LIBRARY in the target_link_libraries line. Since it is also linking against the frameworks and dynamic library from SDL2_LIBRARY_DIR. I confirmed this on both Intel and M1 Macs (Big Sur and Monterey).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants