-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
luajit: fix building for iOS and Android #26577
base: master
Are you sure you want to change the base?
luajit: fix building for iOS and Android #26577
Conversation
if "clang" in str(self.settings.compiler): | ||
args.append("DEFAULT_CC=clang") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect and basically reproduces the existing sloppy handling of CC in the recipe. It hackily replaces the default CC=gcc
with clang
, but it ignores the exact compiler executable set in the Conan profile and won't work for any other compilers in general.
It should really be something like
cc = AutotoolsToolchain(self).vars()["CC"]
args.append(f"CC={cc}")
Similarly, HOST_CC
needs to be set to the CC_FOR_BUILD
value from the toolchain vars. Something like conan-io/conan#17602 would be very welcome here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what iOS build docs suggest to do. So should I add this variable only for iOS then and revert the general compiler adjustment?
I have not opened a PR yet, but most of the make args except |
@valgur would it be ok to add another commit that fixes Android build (it builds on top of this change w.r.t. clearing CFLAGS / LDFLAGS) or wait for this PR to be merged and then open another one? |
Would be ok by me, but I have no say in this as a regular contibutor. |
7efbc4f
to
9849471
Compare
e.g. building for Android on macOS the patch was adjusted to apply on 2.1.0-beta3
Summary
Changes to recipe: luajit/all
Motivation
CPPFLAGS
asTARGET_CFLAGS
since upstream doesn't read this standard variableldconfig
is unavailable (e.g. building for Android on macOS) via upstream patch, but I had to adjust it a bit that it applies on 2.1.0-beta3Details
Followed the documentation at https://luajit.org/install.html#cross, tested building 2.1.0-beta3 for iOS on macOS and for Android on macOS and Linux.
I will explain why env modification is done. Without it target flags (i.e.
host
in Conan terms) "taint" the host ones (build
in Conan terms) which results in non-working executable that's meant to run on thebuild
platform, it can be seen in the build log:Checking how the host executable is produced:
first I came up with the following solution that overwrites those target flags with proper ones:
which results in proper executable:
But later I figured out that it's much easier to simply drop CFLAGS / LDFLAGS from the environment, as LuaJIT uses its own variables for host/target flags. And Android has quite a similar issue.
Also when building a build binary in Android build on macOS, a workaround is required - passing
-isysroot
in HOST_CFLAGS and HOST_LDFLAGS. This looks like a common issue related to the NDK recipe, I've opened #26585 about that.