Closed lucyllewy closed 1 year ago
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 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 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.
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.
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.
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.
@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:
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.
Let's see if commit b8ff4db fixes the testsuite. I've readded CMAKE_OSX_ARCHITECTURES
and set it to $NATIVE_ARCH_ACTUAL
by default.
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).
yeah, that's made it worse - now the non static build is broken..
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
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.
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.
@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.
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?
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
:x86_64
in macOS buildsCMP0068
(RPATH on macOS) toNEW
NekoTargets.cmake
is set to append inexport(TARGETS
command fornekoml
to appease newer CMakePossibly fixes #221 Fixes #215
Signed-off-by: Daniel Llewellyn diddledan@ubuntu.com