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

Android support #162

Merged
merged 1 commit into from Nov 27, 2016
Merged

Android support #162

merged 1 commit into from Nov 27, 2016

Conversation

@gonzalolarralde
Copy link
Contributor

@gonzalolarralde gonzalolarralde commented Aug 24, 2016

This changeset adds support for Android cross-compilation with the same restrictions applied to swift project.

Options

  • --disable-build-tests: tests are failing to be built due to the lack of spawn. Until this is fixed, the flag will be useful to complete compilation
  • --with-android-ndk: Android NDK path to be used as a base for cross toolchain, SDK and support files.
  • --with-android-ndk-gcc-version: NDK GCC version to be used (defaults to 4.9)
  • --with-android-api-level: API version to be used for compilation
  • --enable-android: Enabled Android cross-compilation. In this implementation, as it is being done right now in swift, this is enforcing arm-linux-androideabi armeabi-v7a.

Issues

There are a few things in this changeset that I'd like to iterate, assuming there are better or more generic solution that can be applied:

  • src/shims/android includes: In this directory I created sys/sysctl.h + syscall.h to redirect headers to their locations in Android. -- update: moved to compiler conditionals instead of include overlay. commit
  • src/shims/android libraries: libpthread.a + librt.a are dummy libraries because of the difference described here. To avoid this both libkqueue and libpwq should be modified to avoid asking libtool to link against them.
  • configure.ac and src/Makefile.am: This the first time I deal with autotools, so I think there should be a better way to implement this. (C|CXX|LD)FLAGS params I think will be needed explicitly declared somewhere to enable cross compilation, but maybe in AM_$0_FLAGS? I think there are some overwrites in the am files that were causing troubles, but they can be fixed if this is the right direction. Removing flags from ac_configure_args like that with sed seems like a terrible idea, maybe both libkqueue and libpwq need to be extended somehow to support this in a better way?
  • and more…

Build

This is an example call to compile libdispatch for Android. This is similarly formatted to the call that build script in swift does to compile libdispatch.

$ sh autogen.sh
$ export SWIFT_ANDROID=<...>
$ export NDK_PATH=<...>
$ env \
CC="$SWIFT_ANDROID/build/Ninja-ReleaseAssert/llvm-linux-x86_64/bin/clang" \
CXX="$SWIFT_ANDROID/build/Ninja-ReleaseAssert/llvm-linux-x86_64/bin/clang++" \
SWIFTC="$SWIFT_ANDROID/build/Ninja-ReleaseAssert/swift-linux-x86_64/bin/swiftc" \
$SWIFT_ANDROID/swift-corelibs-libdispatch/configure \
--with-swift-toolchain="$SWIFT_ANDROID/build/Ninja-ReleaseAssert/swift-linux-x86_64/" \
--with-build-variant=release \
--enable-android \
--host=arm-linux-androideabi \
--with-android-ndk=$NDK_PATH \
--with-android-api-level=21 \
--disable-build-tests
$ make

Side note: va_list issue mentioned in #155 is affecting the build process. There's no patch for that issues here, to avoid mixing problems that may be addressed in different ways.

Outcome

This compilation process creates the shared library with the following ELF header:

ELF Header:
  Magic:   7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00 
  Class:                             ELF32
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              DYN (Shared object file)
  Machine:                           ARM
  Version:                           0x1
  Entry point address:               0x0
  Start of program headers:          52 (bytes into file)
  Start of section headers:          1060700 (bytes into file)
  Flags:                             0x5000200, Version5 EABI, soft-float ABI
  Size of this header:               52 (bytes)
  Size of program headers:           32 (bytes)
  Number of program headers:         9
  Size of section headers:           40 (bytes)
  Number of section headers:         50
  Section header string table index: 49

and dependencies

 0x00000001 (NEEDED)                     Shared library: [libstdc++.so]
 0x00000001 (NEEDED)                     Shared library: [libm.so]
 0x00000001 (NEEDED)                     Shared library: [libc.so]
 0x00000001 (NEEDED)                     Shared library: [libdl.so]
@@ -0,0 +1 @@
#import <sys/syscall.h>

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

please just have

#ifdef __ANDROID__
#include <sys/syscall.h>
#endif

in the right place instead of these overlaying of headers

@@ -23,11 +23,18 @@
#define __DISPATCH_SHIMS_GETPROGNAME__

#if !HAVE_GETPROGNAME

#if __ANDROID__

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

should be ifdef

static inline char *
getprogname(void)
{
# if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME
return program_invocation_short_name;
# elif __ANDROID__

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

should be elif defined()

@@ -154,6 +154,8 @@
#define DISPATCH_PURE_C 1
#endif

#include <pthread.h>

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

why? you should have it from "private/private.h" already

This comment has been minimized.

@gonzalolarralde

gonzalolarralde Aug 25, 2016
Author Contributor

@MadCoder pthread_attr_t is required by private/queue_private.h. In other platforms, this type gets declared by an include in sys/types.h to pthreadtypes.h.

Android says in sys/types.h:106

/* while POSIX wants these in <sys/types.h>, we
 * declare then in <pthread.h> instead */
#if 0
typedef  .... pthread_attr_t;
typedef  .... pthread_cond_t;
typedef  .... pthread_condattr_t;
typedef  .... pthread_key_t;
typedef  .... pthread_mutex_t;
typedef  .... pthread_once_t;
typedef  .... pthread_rwlock_t;
typedef  .... pthread_rwlock_attr_t;
typedef  .... pthread_t;
#endif

so we need to include it before private/queue_private.h. I can put it in that file instead if preferred.

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

oh I see, then I would move that #include right after this comment:

/* private.h must be included last to avoid picking up installed headers. */
@@ -368,6 +370,17 @@ DISPATCH_EXPORT DISPATCH_NOTHROW void dispatch_atfork_child(void);
TRASHIT((head)->tqh_last); \
} while (0)

#ifndef TAILQ_FOREACH_SAFE

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

all this should be in a "shims/androind_stubs.h" like linux has

@@ -976,6 +976,7 @@ _dispatch_get_mach_host_port(void)
#include <unistd.h>
#include <sys/syscall.h>

#ifndef __ANDROID__

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

does android already defines gettid() ? because dispatch won't work without it

This comment has been minimized.

@gonzalolarralde

gonzalolarralde Aug 25, 2016
Author Contributor

@MadCoder gettid is already defined in unistd.h:57 for API 21 (and lower also) as extern pid_t gettid(void) __pure2. Android declaration is not inline as src/queue.c, but it shouldn’t have a performance impact in this context, right?

This comment has been minimized.

@MadCoder

MadCoder Aug 25, 2016
Member

perfect, I just wanted to make sure. we don't care about the performance because this is cached once per thread.

@@ -64,10 +65,15 @@ DISPATCH_CFLAGS=-Wall $(VISIBILITY_FLAGS) $(OMIT_LEAF_FP_FLAGS) \
if DISPATCH_ENABLE_ASSERTS
DISPATCH_CFLAGS+=-DDISPATCH_DEBUG=1
endif

if ANDROID
ANDROID_LIBS= -L$(top_srcdir)/src/shims/android

This comment has been minimized.

@gonzalolarralde

gonzalolarralde Aug 25, 2016
Author Contributor

@MadCoder now that header overlays are gone I'm even more uncomfortable by having this dummy files (libpthread.a and librt.a) in src/shims/android. I was thinking on vendors/android/lib(pthread|rt).a or deps/android/lib(pthread|rt).a. What do you think?

More details on why this is required here

This comment has been minimized.

@MadCoder

MadCoder Aug 26, 2016
Member

you should do pull requests for both these projects to be merged in their upstreams and update the submodules here instead.

these .a's are really gross IMNSHO.

This comment has been minimized.

@gonzalolarralde

gonzalolarralde Aug 26, 2016
Author Contributor

Trust me, I'm not proud about them. The submodules are not in the apple organization, so I wasn't sure how that may affect the chances + delay to include this. I'll start working on that tho.

This comment has been minimized.

@dgrove-oss

dgrove-oss Aug 26, 2016
Collaborator

@frankeh is a committer on libpwq, so processing a pull request there should be fast. We usually get good time turn around time from libkqueue (hours to days). So, it's worth trying to do the right thing and maybe dropping this hunk from the PR

This comment has been minimized.

@gonzalolarralde

gonzalolarralde Aug 26, 2016
Author Contributor

Great! I'll send a PR to both projects then. Thanks @dgrove-oss :)

@@ -71,7 +71,9 @@ typedef void (*dispatch_mach_msg_destructor_t)(void*);
#endif

// SIZE_T_MAX should not be hardcoded like this here.
#ifndef SIZE_T_MAX
#define SIZE_T_MAX (0x7fffffff)

This comment has been minimized.

@MadCoder

MadCoder Aug 26, 2016
Member

while you're at it, can you change this as (~(size_t)0) please, this just makes me cringe each time I see it :P

This comment has been minimized.

@gonzalolarralde

gonzalolarralde Aug 26, 2016
Author Contributor

haha sure

@MadCoder
Copy link
Member

@MadCoder MadCoder commented Aug 26, 2016

I think the .a issue should be resolved with other upstreams.
@dgrove-oss or @seabaylea or other linux porters should look at the sanity of the autoconf part, I didn't even read it that carefully.

@dgrove-oss
Copy link
Collaborator

@dgrove-oss dgrove-oss commented Aug 26, 2016

The autoconf/automake looks plausible to me. I tried the disable-tests argument to configure and a vanilla build on linux and both work fine. I assume the big android block in configure.ac does what you want (didn't check it).

@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Aug 31, 2016

Ok, dependency PRs created.

@frankeh would you mind to take a look?

Thanks.

@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Sep 14, 2016

@MadCoder @dgrove-oss Changes in libpwq and libkqueue has been approved and merged. I just rebased the branch and updated the submodule heads. Let me know if there's anything else I should do. I can squash the commits also.

Cheers

@MadCoder
Copy link
Member

@MadCoder MadCoder commented Sep 14, 2016

the patch looks reasonnable, please squash in a single commit with a good message indeed

@dgrove-oss
Copy link
Collaborator

@dgrove-oss dgrove-oss commented Sep 20, 2016

We realized yesterday that the libpwq and libkqueue change actually broke the build on linux because the default for bionic-libc was backwards. Fix has been merged to libpwq and is pending for libkqueue. We'll need to wait for the libkqueue fix to be merged, then update the submodule heads again before this can be merged.

@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Sep 21, 2016

@dgrove-oss I just seen it, so sorry about that. Thanks for submitting the patch. I'll wait until it is merged to update the heads and squash everything.

@dgrove-oss
Copy link
Collaborator

@dgrove-oss dgrove-oss commented Sep 25, 2016

I've submitted #179 to update the submodule versions in isolation. All the changes we need were merged into the upstream repositories over the weekend. Once #179 is merged, you can drop the submodule changes from this PR and squash the rest of it to a single commit to prepare to merge.

@gonzalolarralde gonzalolarralde force-pushed the gonzalolarralde:android-support branch 2 times, most recently from 2391cbe to cde5131 Sep 28, 2016
@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Sep 28, 2016

@dgrove-oss @MadCoder Squashed :)

@MadCoder
Copy link
Member

@MadCoder MadCoder commented Sep 28, 2016

@dgrove-oss can you check it doesn't break linux? I'm not familiar with the CI env. I'm ok with the change otherwise

@dgrove-oss
Copy link
Collaborator

@dgrove-oss dgrove-oss commented Sep 28, 2016

pull request builds fine for me on ubuntu 16.04 and tests pass.

I don't think I can trigger a CI test (not in apple organization), but @MadCoder I think if you put @swift-ci please test in a comment it will schedule a CI test for you.

@modocache
Copy link
Collaborator

@modocache modocache commented Oct 7, 2016

@swift-ci please test

@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Oct 19, 2016

TestFoundation: src/common/knote.c:155: knote_disable: Assertion `!(kn->kev.flags & 0x0008)' failed.
/home/buildnode/jenkins/workspace/swift-corelibs-libdispatch-PR-Linux@2/swift/utils/build-script-impl: line 249: 40378 Aborted                 (core dumped) "$@"
/home/buildnode/jenkins/workspace/swift-corelibs-libdispatch-PR-Linux@2/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 134, aborting
Build step 'Execute shell' marked build as failure

This is the error the CI is reporting, however I don't see any relation with the changes in this PR. Is it possible that this is a failure somewhere else? Let me know if I need to do anything else.

Thanks!

@gonzalolarralde gonzalolarralde force-pushed the gonzalolarralde:android-support branch from cde5131 to 5d624a8 Nov 2, 2016
@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Nov 2, 2016

Rebased. Is there any change I should work on? Please let me know. @modocache make sense to re-trigger CI test now?

@modocache
Copy link
Collaborator

@modocache modocache commented Nov 8, 2016

@swift-ci please test

@modocache
Copy link
Collaborator

@modocache modocache commented Nov 8, 2016

Sorry I didn't see this sooner, @gonzalolarralde! I can't wait to see this in! :)

@johnno1962
Copy link
Contributor

@johnno1962 johnno1962 commented Nov 8, 2016

Looks like an assertion failure in libkqueue. You could try the tests again perhaps? This is unlikely to be to do with this Android port which is largely to do with Makefiles and headers.

https://github.com/mheily/libkqueue/blob/b9e015210bb3e432f44afd054b24a8ceb99eb8e7/src/common/knote.c#L155

Test Case 'TestURLSession.test_dataTaskWithURL' started at 12:48:08.279
TestFoundation: src/common/knote.c:155: knote_disable: Assertion `!(kn->kev.flags & 0x0008)' failed.
/home/buildnode/jenkins/workspace/swift-corelibs-libdispatch-PR-Linux/swift/utils/build-script-impl: line 251: 3921 Aborted "$@"
/home/buildnode/jenkins/workspace/swift-corelibs-libdispatch-PR-Linux/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 134, aborting
Build step 'Execute shell' marked build as failure

@modocache
Copy link
Collaborator

@modocache modocache commented Nov 8, 2016

@swift-ci please test

constraints to the options added in `swift` to add Android Support.
This will allow initially those features to be just passed from
`utils/build-script` without any change until a full strategy for
cross-compilation in all the involved projects is defined.

This is an example call to build for Android with Swift support:
```
env \
CC="${swift_android_path}/build/Ninja-ReleaseAssert/llvm-linux-x86_64/bi
n/clang" \
CXX="${swift_android_path}/build/Ninja-ReleaseAssert/llvm-linux-x86_64/b
in/clang++" \
SWIFTC="${swift_android_path}/build/Ninja-ReleaseAssert/swift-linux-x86_
64/bin/swiftc" \
${swift_android_path}/swift-corelibs-libdispatch/configure \
--with-swift-toolchain=“${swift_android_path}/build/Ninja-ReleaseAssert/
swift-linux-x86_64/" \
--with-build-variant=release \
--enable-android \
--host=arm-linux-androideabi \
--with-android-ndk=${ndk_path} \
--with-android-api-level=21 \
--disable-build-tests
```
@gonzalolarralde gonzalolarralde force-pushed the gonzalolarralde:android-support branch from 5d624a8 to deaa565 Nov 27, 2016
@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Nov 27, 2016

Rebased.

@MadCoder MadCoder merged commit b628e5c into apple:master Nov 27, 2016
@hpux735
Copy link
Contributor

@hpux735 hpux735 commented Dec 1, 2016

@gonzalolarralde Do you have a companion PR for integrating with build-script in the swift project, or does this have to be done manually outside of the greater build?

If it is separate, is it possible to build a dispatch-aware foundation using this work?

By the way, this is fantastic! Thanks for doing all this!!

@gonzalolarralde
Copy link
Contributor Author

@gonzalolarralde gonzalolarralde commented Dec 1, 2016

Hi @hpux735, I don't have a companion PR yet, however I've defined a set of options similar to the options needed to call build-script when compiling for Android.

However, I've been working on a dev environment that is compiling all the libraries, and enabling dispatch in foundation.

The project is here: https://github.com/gonzalolarralde/swifty-robot-environment/

The specific compilation script for foundation with all the dependencies + dispatch here: https://github.com/gonzalolarralde/swifty-robot-environment/blob/master/util/prepare_environment/070_build_corelibs_foundation.sh

Some shortcuts have been taken in this scripts. Sorry about that :) I'll try to put some time to add this to build-script and build-script-impl (not sure if the maintainers are accepting changes in this last one due to the Python refactor).

Cheers!

@hpux735
Copy link
Contributor

@hpux735 hpux735 commented Dec 2, 2016

😁👌 Thanks!!

das added a commit that referenced this pull request Feb 21, 2017
Android support

Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
@das das removed the darwin pending label Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.