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

libwallet_merged compilation on android armv7-eabi #3452

Closed
wants to merge 2 commits into
base: master
from

Conversation

5 participants
@naughtyfox
Contributor

naughtyfox commented Mar 20, 2018

Wallet client libs and dependencies made to compile on android.

Since mobile wallet libs don't need readline, boost::locale (it's used only on windows) and zmq libraries new build option EMBEDDED_WALLET was introduced (supposed to be used to compile static libs for mobile gui wallets) which excludes these libraries from client library building process.

Example:

$ mkdir armv7-eabi && cd armv7-eabi
$ PATH=/opt/android/tool32/arm-linux-androideabi/bin:/opt/android/tool32/bin:$PATH CC=clang CXX=clang++ cmake -D BUILD_TESTS=OFF -D ARCH="armv7-a" -D STATIC=ON -D BUILD_64=OFF -D CMAKE_BUILD_TYPE=release -D ANDROID=true -D BUILD_TAG="android" -D BOOST_ROOT=/opt/android/boost_1_63_0 -D BOOST_LIBRARYDIR=/opt/android/boost_1_63_0/android32/lib -D OPENSSL_ROOT_DIR=/opt/android/openssl-1.0.2n -D OPENSSL_CRYPTO_LIBRARY=/opt/android/lib/armeabi-v7a/libcrypto.a -D OPENSSL_SSL_LIBRARY=/opt/android/lib/armeabi-v7a/libssl.a -D CMAKE_POSITION_INDEPENDENT_CODE:BOOL=true -D EMBEDDED_WALLET=ON ..

Tested on armv7-eabi arch

New build option EMBEDDED_WALLET introduced.
libwallet_merged made to compile on android armv7
@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Mar 20, 2018

The boost thing seems useful. The embedded wallet not using zmq seems totally arbitrary. I'd rather you add an option to enable 0mq or not if there is none at the moment. The warning thing seems unrelated.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Mar 20, 2018

The warning thing seems unrelated

Actually it's necessary. Otherwise you can't compile monero with ndk (currently i'm using ndk 16b). This is the error:

In file included from /home/sergey/src/monero/src/cryptonote_core/cryptonote_core.cpp:33:
In file included from /home/sergey/src/monero/contrib/epee/include/include_base_utils.h:32:
In file included from /home/sergey/src/monero/contrib/epee/include/misc_log_ex.h:33:
In file included from /home/sergey/src/monero/external/easylogging++/easylogging++.h:383:
In file included from /opt/android/tool32/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/map:442:
/opt/android/tool32/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__tree:1819:22: error: the specified comparator type does not provide a const call operator [-Werror,-Wuser-defined-warnings]
                     __trigger_diagnostics()), "");
                     ^
/opt/android/tool32/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__tree:960:70: note: in instantiation of member function 'std::__ndk1::__tree<std::__ndk1::pair<std::__ndk1::pair<double, long>, crypto::hash>, cryptonote::txCompare,
      std::__ndk1::allocator<std::__ndk1::pair<std::__ndk1::pair<double, long>, crypto::hash> > >::~__tree' requested here
    template <class, class, class> friend class _LIBCPP_TEMPLATE_VIS set;
                                                                     ^
/opt/android/tool32/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__tree:970:7: note: from 'diagnose_if' attribute on '__trigger_diagnostics':
      _LIBCPP_DIAGNOSE_WARNING(!__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
      ^                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/android/tool32/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__config:1095:20: note: expanded from macro '_LIBCPP_DIAGNOSE_WARNING'
    __attribute__((diagnose_if(__VA_ARGS__, "warning")))
                   ^           ~~~~~~~~~~~
1 error generated.
src/cryptonote_core/CMakeFiles/obj_cryptonote_core.dir/build.make:86: recipe for target 'src/cryptonote_core/CMakeFiles/obj_cryptonote_core.dir/cryptonote_core.cpp.o' failed
make[3]: *** [src/cryptonote_core/CMakeFiles/obj_cryptonote_core.dir/cryptonote_core.cpp.o] Error 1
make[3]: *** Waiting for unfinished jobs....

i can't hack android ndk and fix it so the only way i see is to suppress this warning.

The embedded wallet not using zmq seems totally arbitrary. I'd rather you add an option to enable 0mq or not if there is none at the moment

What use cases for zmq do you see in mobile wallets' gui libs? I bet it's a big headache to build it manually for android. But if it's necessary i can add an option for it, no problem.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Mar 20, 2018

About 0mq, the use of it is RPC to the daemon. The current JSON RPC is still around, but the idea is to migrate at some point.
I'll let other give their opinions.

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Mar 20, 2018

probably it worths do it when zmq interface for client wallets is ready?

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Mar 21, 2018

@moneromooo-monero check this out please. i added possibility to optionally build zmq for embedded devices

@moneromooo-monero

I'm sorry that this is a subjective comment, but I really don't like the EMBEDED_ZMQ setup. It seems like it should build ZMQ differently, but it does not. Similarly, EMBEDDED_WALLET does build the wallet differently, but it's just a "configure it how I decided I want it", which has nothing to do with being embedded in general. I think it's just all obscure and confusing.

@stoffu

This comment has been minimized.

Contributor

stoffu commented May 31, 2018

I agree with @moneromooo-monero. Such a predefined set of flags seems to belong to the top level Makefile (or get_libwallet_api.sh for the GUI).

@jonathancross

This comment has been minimized.

Contributor

jonathancross commented Aug 8, 2018

@naughtyfox This needs to be rebased. Are you still working on it?

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Aug 9, 2018

I can rebase it if you plan to merge it

@jonathancross

This comment has been minimized.

Contributor

jonathancross commented Aug 11, 2018

@naughtyfox : it seems @moneromooo-monero and @stoffu still have some unaddressed concerns.
PS: I don't have permission to merge anyways. Was just trying to gauge if this was being worked on or not.

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Nov 20, 2018

Do you still want some version of this with better names or should this be closed ?

@naughtyfox

This comment has been minimized.

Contributor

naughtyfox commented Nov 20, 2018

I think you may close it

@moneromooo-monero

This comment has been minimized.

Contributor

moneromooo-monero commented Nov 20, 2018

+invalid

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