-
-
Notifications
You must be signed in to change notification settings - Fork 18
Enable 16 KB ELF alignment #64
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
Conversation
Hey @phaax, thanks for the contribution. The change in the build script makes sense, nice work! However, since we distribute the prebuilt libraries with this package, I'll ask you to add another commit with them as well. I'll approve the build workflow to run in GitHub Actions so you can just download one if you don't want to manually build it (although I guess you already did, since you said you tested it). Let me ask you, have you tested the build before the fix and did it not work? Just for curiosity.
Well, recreate the .meta and commit them as well then. If you concern is with the regeneration of the GUID, you can manually revert it back to the original after recreating the file. |
I think I've done the things you asked. If anything seems off I'll try to fix it. :) I tested the original binaries and they did indeed crash the app (in the emulator). I cannot guarantee that the crash was due to the binaries however because I have a teeny tiny suspicion that the emulator setup itself might have been broken in some way. (I'm on a different computer now). |
Thanks for the quick update @phaax, having the binaries updated is necessary or else people won't be able to use the fix. Now that I see the .meta files are using I'm really sorry about that, I had no idea such update would break older versions of Unity. I'll kindly ask you to revert just the .meta files. Unity 6 will give warnings but still work, but we keep support for Unity 2022 and 2021. |
@gilzoide I've reverted the .meta files. Hopefully Unity still manages to figure out that they're 16 KB aligned during build time. I won't have any time to actually test this until tomorrow. |
Hey @phaax, thanks for such a quick fix. The PR looks good now, I'll merge this and release a new patch version.
This is not up to Unity at all, the build just copies the libraries as they are and the device is the one that accepts or not the binaries. Unity is issuing a warning because the library might crash in devices with 16KB page size, since it's not marked as compatible explicitly. Also, you did test it and checked that it works, right? I've confirmed here that after reverting the .meta files the build made with Unity 2022 worked in an Android without 16KB page size. So I guess that's it, works in Unity 2022, Unity 6, devices with and without the page size thing, awesome! Thanks again, I'll let you know when this is released. |
Done, release in version 1.2.3. |
@gilzoide Yep. Tested with a build in Unity 6 as I mentioned above. Works in 16 KB page size emulator and on my phone, which is still on Android 14 without 16 KB page size. Sidenote: I'm still getting compatibility warnings from lib-burst.so (something like that) in the emulator, but it doesn't crash anymore. I suppose that could be due to something else. I'm working on a pretty big project with a bunch of dependencies after all. :) |
https://developer.android.com/guide/practices/page-sizes
Important! The meta files don't seem to auto update, so Unity (6000.0.44f) will still warn about the .so files not being properly aligned. Recreating the meta files fixes this.
Disclaimer: I'm not used to building native code (or creating pull requests). But I have tested this on an emulator with 16 KB page size enabled and it works.