neko icon indicating copy to clipboard operation
neko copied to clipboard

Build on m1 macOS 11.3

Open lucyllewy opened this issue 3 years ago • 14 comments

Lightly tested: I have succeeded in compiling and executing a hello-world on my m1 MacBook Air. Please treat with extreme prejudice and test it hard, and on other systems than m1 Mac, to ensure I haven't broken things :-)

  • CMakeLists.txt:
    • Remove hard-coded x86_64 in macOS builds
    • Set CMake Policy CMP0068 (RPATH on macOS) to NEW
    • Ensure that NekoTargets.cmake is set to append in export(TARGETS command for nekoml to appease newer CMake
    • Drop duplicate arch64 detection block

Possibly fixes #224 Fixes #215

Signed-off-by: Daniel Llewellyn [email protected]

lucyllewy avatar May 04 '21 04:05 lucyllewy

Maybe I am wrong here but this PR as it sits right now seems to kill 32-bit Intel builds for macOS. Does homebrew also no longer support such? I realize Apple might not support such much but I do not see a reason to exclude such from neko builds.

I also think it would be better to port libs/ui/ui.c from the old Carbon interfaces to the Cocoa ones since Carbon has gone away (instead of hackishly declaring things like RunApplicationEventLoop and QuitApplicationEventLoop manually for 64-bit targets).

Uzume avatar Oct 31 '21 18:10 Uzume

@Uzume The fix for libs/ui/ui.c was already applied in #218... If what you're suggesting isn't too complicated then you could open a PR to make a better fix, however otherwise neko is now deprecated so large ports are probably not likely to happen.

As a side note, in case this PR is updated in the future, the fixes to libs/ssl/ssl.c have also been applied in #239 as they were necessary for mac in general.

tobil4sk avatar Oct 31 '21 19:10 tobil4sk

@tobil4sk Yes, I noticed some of the changes had already been merged but thanks for calling them out with links to the other PRs. I see @diddledani just rebased the changes here to fix such.

I am not in a position to work on a Carbon to Cocoa port for libs/ui/ui.c as I do not have a macOS build environment available to me for development and testing but maybe one day.

Uzume avatar Oct 31 '21 20:10 Uzume

The libs/std/process.c fix had also been done in #219, however not on the same line so git didn't pick it up, now signal.h is included twice in that file.

tobil4sk avatar Oct 31 '21 20:10 tobil4sk

Thanks for the ping about the changes already included. I've updated the PR with a rebased copy to include the changes from the main branch so that there isn't any duplicated code - I saw that I had to edit the ui.c (and also as pointed out in comment above by @tobil4sk process.c) file because my change and the one committed already were conflicting due to being changed on different lines; I've dropped my change.

lucyllewy avatar Oct 31 '21 20:10 lucyllewy

The libs/std/process.c fix had also been done in #219, however not on the same line so git didn't pick it up, now signal.h is included twice in that file.

oops, I missed that one. Thanks for pointing it out. I've pushed again with my change there dedpulicated.

lucyllewy avatar Oct 31 '21 20:10 lucyllewy

@diddledani I think you would be better off not removing the 32-bit Intel macOS build target and instead just adding the 64-bit M1 build target. Neither of those platforms currently is fully supported here by the Azure Pipelines checks (which likely need to be updated).

You probably should not test the value of CMAKE_OSX_ARCHITECTURES as parsing it is non-trivial; see:

  • https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_ARCHITECTURES.html
  • https://cmake.org/cmake/help/v2.8.12/cmake.html#prop_tgt:OSX_ARCHITECTURES

I included docs link for cmake 2.8.12 since that is the earliest version supported here. It contains a list of system architectures not a specific single one.

Currently, you will notice this code is failing the MacStatic check:

CMake Error at libs/mysql/CMakeLists.txt:7 (if):
   if given arguments:
 
     "STREQUAL" "i386"
 
   Unknown arguments specified

You probably should add a line specifying darwin64-arm64-cc and not removing i386 the target.

Uzume avatar Oct 31 '21 21:10 Uzume

Let's see if commit b8ff4db fixes the testsuite. I've readded CMAKE_OSX_ARCHITECTURES and set it to $NATIVE_ARCH_ACTUAL by default.

lucyllewy avatar Oct 31 '21 21:10 lucyllewy

That might work but CMAKE_OSX_ARCHITECTURES is used mostly for making fat binaries, e.g., we could change the line to:

set(CMAKE_OSX_ARCHITECTURES "arm64;x86_64") 

Of course the later compares would then always fail. Those should probably be rectified differently (and not use CMAKE_OSX_ARCHITECTURES to determine system arch).

Uzume avatar Oct 31 '21 21:10 Uzume

yeah, that's made it worse - now the non static build is broken..

lucyllewy avatar Oct 31 '21 22:10 lucyllewy

On this line the CMAKE_OSX_ARCHITECTURES string is being evaluated as an empty string... https://github.com/HaxeFoundation/neko/blob/b8ff4db70aa1376b4133e5936e6bb6ef68c6bd87/CMakeLists.txt#L60

Maybe on this line NATIVE_ARCH_ACTUAL doesn't exist for some reason?? https://github.com/HaxeFoundation/neko/blob/b8ff4db70aa1376b4133e5936e6bb6ef68c6bd87/CMakeLists.txt#L10

tobil4sk avatar Oct 31 '21 22:10 tobil4sk

You had the right idea: remove CMAKE_OSX_ARCHITECTURES all together.

A quick search of the code shows that arch_64 is only used in one place libs/mysql/CMakeLists.txt for the WIN32 stuff. That should probably be changed to use CMAKE_SIZEOF_VOID_P directly and arch_64 removed everywhere else.

As for libs/mysql/CMakeLists.txt I am not sure but I do not think it should be trying to parse CMAKE_OSX_ARCHITECTURES either. Obviously it needs to have darwin64-arm64-cc added but it needs to use something else to determine that. Maybe CMAKE_SYSTEM_PROCESSOR is the right thing.

Uzume avatar Oct 31 '21 22:10 Uzume

ok, I think this should get the build working for now. I think we should add a followup issue/pr for removing the parsing of CMAKE_OSX_ARCHITECTURES with a better mechanism.

lucyllewy avatar Oct 31 '21 22:10 lucyllewy

@diddledani You probably could have used CMAKE_SYSTEM_PROCESSOR but at least that works and seems to have passed all the checks. I agree further cleanup could be useful.

Uzume avatar Oct 31 '21 23:10 Uzume

Thanks for making this work on M1 Currently neko works on my M1, but nekotools fails with

❯ nekotools
Uncaught exception - load.c(237) : Failed to load library : /usr/local/lib/neko/std.ndll (dlopen(/usr/local/lib/neko/std.ndll, 0x0001): tried: '/usr/local/lib/neko/std.ndll' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')), '/System/Volumes/Preboot/Cryptexes/OS/usr/local/lib/neko/std.ndll' (no such file), '/usr/local/lib/neko/std.ndll' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')))

Does this PR fix that?

danielo515 avatar Jan 04 '23 06:01 danielo515