android-ndk: Use clang for AS & LD as per docs.
#23346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 1706626android-ndk/r23b@#9fabefce0c54ef2e3349e09173d26795 |
|
Thanks a lot for such a detailed explanation and proper doc sourcing, it's really appreciated! I've passed this around those in the team that have more Android experience than I do, let's see what they have to say, but looks great on my side, thanks! :) |
|
@sizeak Thank you for providing this PR. I'm trying to track more documentation about the recommended configuration, and for Autotools it goes exactly how we are using: https://developer.android.com/ndk/guides/other_build_systems#autoconf So I would like to ask you a full build log using your configuration that actually works, so we can see diff of ld and ar usage. It could be a problem with ffmpeg trying to pass more arguments than supported too .... |
|
Hi, sorry, I might be misunderstanding you, however the android-ndk recipe it not currently following the recommendations in the documentation at that link because the recipe uses
I made the linker change because it was required to get my build working along with the mentioned I'll follow up with a build log in a little while. |
|
Here is the successful build output of a cut down ffmpeg build made with the android-ndk recipe package built locally with the changes from this PR. Click to expand log |
I misread it as |
|
I asked for more insight in android sdk forum, and it looks correct for general propose. We don't have a real package consuming the android NDK Conan package, but it looks fine. I would ask to check the build failure, but I'm not in the opposite of this PR. |
|
The logs of the failing build are included in the issue linked to this PR if you wanted to look at them. The configuration used may not be 100% the same, but should be close enough. #23342 I suppose that worst case, if this change does end up causing some unexpected unforeseen issue for someone, that they can open an issue here and we can iterate on this change or revert it. Until then they could use an old version of the recipe, so while we absolutely should be as sure as possible that this is safe to merge, it's not the end of the world if there does end up being a problem. |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
I just did a change to update the test package. Now it run the |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Excellent, thank you 👍🏻 |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit fbbe495android-ndk/r23b@#9fabefce0c54ef2e3349e09173d26795 |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e517a89android-ndk/r23b@#9fabefce0c54ef2e3349e09173d26795 |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi @uilianries @RubenRBS, this seems to have passed the CI checks now, what is the appetite for merging it? |
This was referenced May 2, 2024
Merged
|
@jcar87 @RubenRBS Any progress on this? |
jcar87
reviewed
Jun 11, 2024
jcar87
previously requested changes
Jun 11, 2024
There was a problem hiding this comment.
Thanks @sizeak - these changes look good.
Please see https://github.com/conan-io/conan-center-index/pull/23346/files#r1634932319 to address the issue with the test package - otherwise it just skips testing even in scenarios where it is expected to work.
jcar87
reviewed
Jun 11, 2024
Comment on lines
26
to
32
| # INFO: can_run allows mac M1, but it does not work for the NDK | ||
| # https://github.com/android/ndk/issues/1299 | ||
| if can_run(self) and not (self.settings.os == "Macos" and self.settings.arch == "armv8"): | ||
| if self.settings.os == "Windows": | ||
| self.run("ndk-build.cmd --version") | ||
| else: | ||
| self.run("ndk-build --version") |
There was a problem hiding this comment.
Suggested change
| # INFO: can_run allows mac M1, but it does not work for the NDK | |
| # https://github.com/android/ndk/issues/1299 | |
| if can_run(self) and not (self.settings.os == "Macos" and self.settings.arch == "armv8"): | |
| if self.settings.os == "Windows": | |
| self.run("ndk-build.cmd --version") | |
| else: | |
| self.run("ndk-build --version") | |
| ndk_build = "ndk-build.cmd" if self.settings.os == "Windows" else "ndk-build" | |
| ndk_version = Version(self.tested_reference_str.split('/')[1]) | |
| skip_run = platform.system() == "Darwin" and "arm" in platform.processor() and ndk_version < "r23c" | |
| if not skip_run: | |
| self.run(f"{ndk_build} --version", env="conanbuild") | |
| else: | |
| self.output.warning(f"Skipped running ndk-build on macOS Apple Silicon in arm64 mode, please use a newer" | |
| " version of the Android NDK") |
There was a problem hiding this comment.
@sizeak I just tested @jcar87 suggestion locally and it works good, please, include it to your PR (I have no permission, needed to mark the checkbox to allow maintainers change the PR). Plus, you will need to add the follow lines in your imports:
from conan.tools.scm import Version
import platform
About the test_v1_package, I would ask deleting the entire folder and its content. We actually don't use any CMake target generated by cmake_find_package (legacy), we just build a simple cpp using the Android SDK.
There was a problem hiding this comment.
Or, you can just merge the PR glean-notes#1 😄
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
3 tasks
Suggestion from PR Android NDK
This comment has been minimized.
This comment has been minimized.
|
@sizeak Thank you! The CI service is under maintenance now (we use Wednesdays to do scheduled changes), but should be back in few hours. The CI is prepared to restart your PR automatically after the maintenance. In case you note is delaying too much, please, don't avoid pinging us. |
|
No problem, I'm not being blocked by this atm. Thanks for the help! FYI there is a linter warning on the PR
Do we care about fixing that? |
@sizeak As the CI is stopped, you can remove the |
This comment has been minimized.
This comment has been minimized.
jcar87
approved these changes
Jun 19, 2024
Conan v1 pipeline ✔️All green in build 19 (
Conan v2 pipeline ✔️
All green in build 18 (
|
Hooks produced the following warnings for commit a971a4bandroid-ndk/r26d@#0a8a020a2e0ce2edbda6ff226a33a23a |
memsharded
approved these changes
Jun 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specify library name and version: android-ndk/*
I've been migrating our build to using the android-ndk package as a tool requirement instead of using an external NDK, however with the android-ndk package I am unable to build ffmpeg. This seems to be because my Android profile used different settings for
ASandLDto the android-ndk package.My external NDK settings used those recommended by the NDK documentation and pointed
ASandLDatclangrather than at the separate binaries, lettingclanginvoke them as needed. The docs here state:and similarly about
as:This PR changes the recipe so that
ASandLDare set to the same values asCC/CXX, which allows my ffmpeg build to succeed. I've been using these settings in my profile with an external NDK for years without issue, but obviously I am unable to verify that every recipe will still build successfully; however, given that these are the official recommendations for these variables, I am inclined to prefer them.I've set the value of
LDto the value ofCXX(i.e. clang++) rather thanCCbecause the docs stateand I don't think we can know whether the consumer of this package will be building purely C or not, so it seems safer to use
CXXsince this will work for both.Fixes #23342