Skip to content
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

Update Android Instructions for newer NDKs #9732

Closed
wants to merge 1 commit into from

Conversation

cpsauer
Copy link
Contributor

@cpsauer cpsauer commented Oct 14, 2022

Hi, @bagder!

First and foremost, thank you so much for such a wonderful tool and library--and for sharing it with the world.

I was building things across platforms, reading your good docs. Along the way, I noticed that some of the Android instructions seemed to have gotten out of date with the switch to LLVM toolchains in newer NDK versions. So, I figured I'd try to leave things even better than I found them, especially since the triggered error messages are different enough from their root causes. Here's my shot at that for your consideration.

Cheers!
Chris

P.S. To explain some of the changes:

  • ANDROID_NDK_HOME is the Google-standard variable for pointing into the NDK, so I thought I should change us over to that, since some people will already have it set and know about it from elsewhere.
  • The other variables are just the result of changed paths in the NDK with the switch to llvm tools. The old paths are no longer valid, which can cause issues that look like missing SSL libraries.
  • I think the LIBS/static linking issue probably predated all this. LMK if you have a cleaner way to express that--I'm certainly no auto tools expert, unfortunately. The error otherwise misleadingly manifests itself as the script thinking libcrypto is missing SSL_connect. And BoringSSL needs libc++, so I included it, thinking it was better to be safe than sorry.
  • Finally, the reason for the removal of the API-level 23 sentence, is that I was finding that ./configure detected SSL just fine targeting level 21, which I listed because it was the most backwards compatible API level with compiler wrappers in the current NDK. This matters because HAVE_GETIFADDRS was added in API level 24 and there are enough Android devices using older OSs, that I think people may well want to target back to at least 21.

P.P.S. What originally brought me to the doc was to create a cross-platforms set of build scripts for libcurl for Bazel, which is an increasingly popular build system from Google. Lots of folks have been spinning up Bazel BUILD files for libcurl online, mostly copying tensorflow's (fairly buggy) ones. Since those all looked problematic, I figured I'd take a shot at writing an improved set. If you'd be open to accepting my good set of Bazel BUILD files, please let me know! The advantage would be stupid easy (incremental) builds across platforms, making libcurl easily usable by Bazel users.

P.P.P.S :) Just to confirm a hypothesis: It's the need to support older compilers that prevents all the HAVE_[whatever]_H macros from being converted to __has_include's, right?

@cpsauer
Copy link
Contributor Author

cpsauer commented Oct 14, 2022

[Pretty sure the failures are unrelated to this change, since I only changed docs, and other PRs are failing, too.]

bagder
bagder approved these changes Oct 14, 2022
Copy link
Member

@bagder bagder left a comment

Thanks!

@bagder
Copy link
Member

bagder commented Oct 14, 2022

It's the need to support older compilers that prevents all the HAVE_[whatever]_H macros from being converted to __has_include's, right?

I'm not sure what you mean with __has_includes. What would be the difference? They're just defines that tell if the header file exits on the platform or not. And yes we have them to make sure curl builds on as many platforms as possible.

@bagder
Copy link
Member

bagder commented Oct 14, 2022

If you'd be open to accepting my good set of Bazel BUILD files, please let me know

I'm scared of the avalanche of issues and problems coming with that, without people around in the project to care for them. We already ship (too) many different build options and many of them lag behind the autotool build in terms of features and completeness a lot of the time.

@bagder
Copy link
Member

bagder commented Oct 14, 2022

The (new) uppercase check 1275 fails on this:

 ../docs/INSTALL.md:342:0:error: lowercase export after period
 export HOST_TAG=darwin-x86_64 # Same tag for Apple Silicon. Other OS values here: https://developer.android.com/ndk/guides/other_build_systems#overview
 ^

but I think that's an error in the script. I will work on a fix for that separately.

@cpsauer
Copy link
Contributor Author

cpsauer commented Oct 14, 2022

Thanks for looking and for your fast approval! Honor getting to interact with you.

Re Bazel: Totally get it and sorta assumed as much. I'll release it separately as a wrapper and see if I can get the TensorFlow guys to help support.

Re __has_includes: It's a sweet preprocessor operator that accomplishes the same goal, but without having to run a (slowish) check in each build system. Could be used to make headers/code that just does the right thing, regardless of build system!

@jzakrzewski
Copy link
Contributor

jzakrzewski commented Oct 14, 2022

Unfortunately, __has_include is only standardised for C++. For C some compilers define it as an extension (namely gcc, and clang), while others don't. So, sadly, that would solve nothing. :(

@bagder
Copy link
Member

bagder commented Oct 14, 2022

#9733 fixed the bogus test fail

@bagder
Copy link
Member

bagder commented Oct 14, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants