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

changes to build for Android #1521

Merged
merged 2 commits into from Dec 6, 2017
Merged

changes to build for Android #1521

merged 2 commits into from Dec 6, 2017

Conversation

INRIX-joel-winarske
Copy link
Contributor

Hello,

This PR enables building H2O for Android NDK r16 (Clang 5.0). Validated on devices in the range of 15 - 26 (armeabi-v7a, arm64-v8a, x86, x86_64).

Regards,
Joel

@kazuho
Copy link
Member

kazuho commented Dec 6, 2017

Thank you for the PR.

Would you mind elaborating why we need to include spawn.h? I believe that the code does not use posix_spawnp (and it's family) on linux.

Other than that the changes look good to me.

@INRIX-joel-winarske
Copy link
Contributor Author

linux and ANDROID are both defined in an Android build. I'm excluding spawn.h only in the case of Android. This avoids having to include the sysroot/usr/include directory. I can add a variable to CMake like this, which avoids the #ifndef cases:

INCLUDE_DIRECTORIES(
    include
    deps/cloexec
    deps/brotli/enc
    deps/golombset
    deps/hiredis
    deps/libgkc
    deps/libyrmcds
    deps/klib
    deps/neverbleed
    deps/picohttpparser
    deps/picotest
    deps/yaml/include
    deps/yoml
    ${EXTRA_INCLUDE_DIRS})

I like the CMake route if you're good with that.

Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the answer. I missed the fact that n was in the middle of ifndef.

Could you please consider the following? I think that we do not need to include spawn.h at all, except for serverutil.h.

@@ -25,7 +25,9 @@
#include <pthread.h>
#include <pwd.h>
#include <signal.h>
#ifndef __ANDROID__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please change this to #ifndef __linux__? That's when we use our own implementation instead of posix_spawnp.

#include <spawn.h>
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove #include <spawn.h> as a whole and see what happens?

I believe that this is no longer needed; it was necessary when we called posix_spawnp directly, but now we use h2o_spawnp.

src/main.c Outdated
#include <spawn.h>
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same goes here as well.

@INRIX-joel-winarske
Copy link
Contributor Author

Note with these build changes, it will produce a Uni-Proc server, as Android has a non-standard way of accessing CPU count.

@kazuho kazuho merged commit 6502abc into h2o:master Dec 6, 2017
@kazuho
Copy link
Member

kazuho commented Dec 6, 2017

Thank you for the changes. Merged to master.

And thank you for raising the concern about the number of CPU cores recognized. I think we can live with it; it's just be a matter of what the default value would be.

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

Successfully merging this pull request may close these issues.

None yet

3 participants