openfl / lime

A foundational Haxe framework for cross-platform development
https://lime.openfl.org/
MIT License
749 stars 361 forks source link

Set HXCPP_ARM64 by default when building on Apple Silicon Mac #1752

Closed tobil4sk closed 5 months ago

player-03 commented 5 months ago

I'm not a fan of the preexisting if (!targetFlags.exists()) logic. It's hard to follow, and I think it's even incorrect because it was written assuming Mac would only use Intel chips.

Looking at the code, we see that only a 64-bit machine can compile the 64-bit ndll. Makes sense.

if (!targetFlags.exists("32") && System.hostArchitecture == X64)
{
    commands.push(["-Dmac", "-DHXCPP_CLANG", "-DHXCPP_M64"]);
}

It doesn't require the "64" flag to be set, just the absence of the "32" flag. That way, the user can run the rebuild command without having to specify their architecture, and Lime will default to building for their machine, which makes sense. (I agree with the goal, but I still feel like it's harder than necessary to read the code.)

A bit further down, we see that both 32- and 64-bit machines can compile a 32-bit ndll, the former by default and the latter if the "32" flag is set.

if (!targetFlags.exists("64") && (targetFlags.exists("32") || System.hostArchitecture == X86))
{
    commands.push(["-Dmac", "-DHXCPP_CLANG", "-DHXCPP_M32"]);
}

In no case can you compile both types at once, so there isn't a clear reason for these to be separate if blocks. (This isn't wrong, just a bit confusing.)

The real problem here is that the M1 exists. What if hostArchitecture == ARM64 and the "32" flag is set? Well, then it will attempt to compile for X86, which I'm pretty sure isn't allowed.

There has to be a better way to organize all of this code. Maybe switch (System.hostArchitecture), because host architecture determines what options make sense to consider? Flags are checked second, so we're choosing between options appropriate to the host.

tobil4sk commented 5 months ago

Thanks, the new version is fine with me.

player-03 commented 5 months ago

I was just trying to simplify the code a bit, but this is turning into a whole adventure. If I'd realized there would be this many issues, I'd have done it locally instead.

player-03 commented 5 months ago

Seems to be working now. But so much of this isn't relevant to this PR... Perhaps we should go back to 17ad0577d792cb7b2c54196784621d0daf9570bc, and I'll make another PR for the enum changes.

tobil4sk commented 5 months ago

Perhaps we should go back to https://github.com/openfl/lime/commit/17ad0577d792cb7b2c54196784621d0daf9570bc, and I'll make https://github.com/openfl/lime/pull/1754 for the enum changes.

That's fine with me, should I revert this branch to that commit then?

player-03 commented 5 months ago

Yeah, I think that's for the best. (I would but I haven't figured out how to push to someone else's PR branch from the command line.)

player-03 commented 5 months ago

Hmm, now we're back to choosing between three architectures, even though the switch (System.hostArchitecture) block implies there could be more. That's part of what I was trying to fix with 8a1c0ffff14c18501b5941a8abd288c5bffe31fd.

Ok, new idea...

player-03 commented 5 months ago

That ought to work the same without requiring all those changes to other files.

Other than that, I'm a bit concerned about HashLink forcing x64. That's the closest equivalent to is64 = true, but is64 = true was written without any consideration for Arm. Maybe we should do something like this?

else if (project.targetFlags.exists("hl"))
{
    targetType = "hl";
+   if (targetArchitecture != ARM64)
+   {
        targetArchitecture = X64;
+   }
}

Or perhaps if (targetArchitecture == X86)?

And why does HL force an architecture in the first place? I can only assume there's some bug when you try to compile for 32 bits, but I don't know if making an x86 machine try to compile an x64 binary is better...

tobil4sk commented 5 months ago

Other than that, I'm a bit concerned about HashLink forcing x64

Hashlink bytcode isn't supported on Apple Silicon right now anyway (and I don't think lime supports hlc compilation?), so it might be fine to leave it for now until Hashlink has proper arm64 support. I imagine compiling lime.hdll on arm64 will probably require a bit more work than this flag patch. This PR will sort out the flags for the cpp target at least.

I don't know if making an x86 machine try to compile an x64 binary is better...

From what I know, 32 bit mac systems are pretty hard to come by nowadays.

joshtynjala commented 5 months ago

and I don't think lime supports hlc compilation?

I have HL/C compilation working for Lime/OpenFL in a branch. Will be merging into 8.2.0-Dev soon.

tobil4sk commented 5 months ago

And why does HL force an architecture in the first place? I can only assume there's some bug when you try to compile for 32 bits, but I don't know if making an x86 machine try to compile an x64 binary is better...

Looks like is64 = true was added in this commit: 8d72e71357926d449d5f8374d69e4d529a410f52. Before that it was is64 = false, which was added in 59a40366ec89cce8dc1150a35297ec7ce7cda0e9, presumably because hashlink didn't have good support for 64 bit back then. It seems like the is64 = true change might just have been done to revert is64 = false, so that hashlink builds were no longer forced to be 32 bit. It doesn't seem like it would be a problem to remove this line now.

tobil4sk commented 5 months ago

Looks like Linux and Windows also have equivalent lines to the is64 = true line we discussed here, but I suppose we should stick to macos in this PR. Is there anything else that needs to be done here?

tobil4sk commented 5 months ago

Thanks!

joshtynjala commented 5 months ago

Based on the title alone, this PR initially sounded to me like something that shouldn't go into a hypothetical Lime 8.1.2 bug fix release, and should wait for Lime 8.2.0, so I was concerned to see that this was merged into develop instead of 8.2.0-Dev.

However, I just pulled from develop, rebuilt Lime tools, and I'm seeing that lime rebuild mac and lime build mac are both still creating x86_64 binaries on my Apple Silicon Mac.

Could it be because my Haxe is compiled for x86_64, and is running in Rosetta, so Lime tools is being tricked into thinking that it is running on x86_64 instead of Apple Silicon? If that's the case, then I'm much less concerned about this going into develop.

I just want to make sure that I understand what's going on.

tobil4sk commented 5 months ago

lime rebuild mac and lime build mac are both still creating x86_64 binaries on my Apple Silicon Mac.

In a rosetta environment, hxp detects the host architecture to be x86_64:

https://apple.stackexchange.com/questions/420452/running-uname-m-gives-x86-64-on-m1-mac-mini

joshtynjala commented 5 months ago

Perfect. Thanks!