-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix(cmake): resolve build error due to malformed if() expression on Android platform. #4444
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
17d1d45 to
daeceb8
Compare
| set(ZSTD_MULTITHREAD_SUPPORT_DEFAULT OFF) | ||
| # Handle old Android API levels | ||
| if((NOT ANDROID_PLATFORM_LEVEL) OR (ANDROID_PLATFORM_LEVEL VERSION_LESS 24)) | ||
| if((NOT ANDROID_PLATFORM_LEVEL) OR ANDROID_PLATFORM_LEVEL VERSION_LESS 24) |
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.
So the patch removes one parenthesis level,
and now it works better for cmake ?
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.
Yes. Removing the pair of parenthesis makes cmake happy.
Here's an explanation provided by ChatGPT: https://poe.com/s/6bcOKmapQY5A2mpfW5sc
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.
Interesting ...
|
Probably a parallel concern to this PR, |
This repo already appears to support Android cross-compilation through this workflow, albeit by directly invoking the make command rather than using |
is expecte to fail, due to #4444
|
I just tried to add a Is there anything different in this test, compared to yours ? |
is expecte to fail, due to #4444
We’re using the default CMake version (v3.22.1) provided by Ubuntu Jammy (22.04 LTS) package manager. Your CI run uses version v3.31.6. I’ll try upgrading and do some experiment. |
|
Here's the output using cmake version Here's the output using cmake version This means some how between v1.5.7 and the latest dev branch, the build problem disappeared. I am not sure what the root cause for this yet. |
|
That's weird. |
|
Wait, it did change, the line used to be : and now it's this: That's a small difference, but it was enough to create the problem you observed.
|
I guess this PR is no longer needed. Do we want to leave the current code as-is or improve it some how? |
|
I think indeed that the current code is fine, and the proposed change doesn't improve it. |
Description
When building Zstd v1.5.7 for the Android platform using CMake, the following error occurs during configuration:
This regression was introduced in 49fe2ec (refactor: modularize CMakeLists.txt for better maintainability). The problematic conditional logic is:
CMake is picky about parentheses for logical grouping inside if() statements. This leads to a parsing failure on valid Android configurations.
Fix
The parentheses surrounding the version check is removed to comply with CMake syntax rules.
Validation
Manually triggered a successful cross-compiled build with Android NDK26.