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 #157

Open
wants to merge 1 commit into
base: public
Choose a base branch
from
Open

Conversation

oliver-schick
Copy link

Added support for Android devices. Tested to also work with linux devices. Differences in *.cpp and *.h are not relevant except for the two files boolsharing.cpp and sharing.cpp. Android devices don't support std::filesystem and std::error_code, so boost support was added for Android builds.

@lenerd
Copy link
Collaborator

lenerd commented Feb 9, 2020

Hi Oliver,
your changes are based on a state of ABY which is almost a year old now. Could you please rebase your changes on top of the current state of the public branch? That would make it clear what changes are actually contained in this PR.

Please do the same also for the other PRs, encryptogroup/ENCRYPTO_utils#28 and encryptogroup/OTExtension#37.

@oliver-schick
Copy link
Author

oliver-schick commented Feb 10, 2020 via email

@lenerd
Copy link
Collaborator

lenerd commented Feb 10, 2020

Something went wrong. It seems that you performed a rebase as well as an additional merge. The history now includes your commits twice:

*   0a4d813 (HEAD, oliver-schick/public) Rebased and merged branch 'public' of h
|\  
| * afd317e Fixed problem with install on non-android devices. Fixed issue with 
| * da9d860 GMP and OpenSSL are only built, when corresponding options are activ
| * 5f6f3f3 Support for multiple build directory added. Previously all dependenc
| * fc64adb Better explanation and display (cmake gui) of cache variables.
| * ce69efa Fixed install (install did not work correctly, when compiling for di
| * 6a83dc2 Added install support for Android (installing in android ndk)
* | a2fc640 Fixed problem with install on non-android devices. Fixed issue with 
* | bc65d6a GMP and OpenSSL are only built, when corresponding options are activ
* | cac7685 Support for multiple build directory added. Previously all dependenc
* | f1dfbeb Better explanation and display (cmake gui) of cache variables.
* | 5bd5d56 Fixed install (install did not work correctly, when compiling for di
* | 12d946f Added install support for Android (installing in android ndk)
* |   08baa85 (upstream/public, upstream/HEAD, origin/public, public) Merge pull
|\ \ 

@oliver-schick
Copy link
Author

oliver-schick commented Feb 10, 2020 via email

@lenerd
Copy link
Collaborator

lenerd commented Feb 10, 2020

Ok. Does it build?

So far, I have only looked at your commits and tried to figure out what you changed and why.

I still need to setup an Android build environment for testing.

@lenerd
Copy link
Collaborator

lenerd commented Feb 10, 2020

You can probably drop the most recent commit, i.e., the merge commit, and push again to clean up the history.

@oliver-schick
Copy link
Author

oliver-schick commented Feb 11, 2020 via email

@lenerd
Copy link
Collaborator

lenerd commented Feb 11, 2020

Thanks, that makes it easier to go through the changes.

@lenerd
Copy link
Collaborator

lenerd commented Feb 11, 2020

Thanks, that makes it easier to go through the changes.

Nonetheless, it will take a while since you made changes to approximately 100 files and added a lot of CMake code.

First comments:

  • Why did you replace git submodules with CMake scripts downloading zip archives from GitHub?
  • You reference your own repositories instead of the official ones at github.com/encryptogroup/.
  • Why is it necessary to have CMake files in two locations, i.e., cmake/ and src/abycore/cmake/?
  • There are duplicated files in the tree:
    • cmake/ForwardConfig.cmake.in and src/abycore/cmake/ForwardConfig.cmake.in
    • cmake/GMPXXConfig.cmake.in and src/abycore/cmake/GMPXXConfig.cmake.in
    • cmake/GMPConfig.cmake.in and src/abycore/cmake/GMPConfig.cmake.in

More general: Why do we need all the new CMake code?

@oliver-schick
Copy link
Author

oliver-schick commented Feb 12, 2020 via email

@oliver-schick
Copy link
Author

oliver-schick commented Feb 12, 2020 via email

@lenerd
Copy link
Collaborator

lenerd commented Feb 12, 2020

Regarding the filesystem library: We should try to use std::filesystem and can fallback to std::experimental::filesystem or boost::filesystem if the former is not available.

Using Boost was also requested for Mac OS (#112), so this should be implemented in a generic way that is not specific for Android or Mac OS.

@lenerd
Copy link
Collaborator

lenerd commented Feb 12, 2020

(previously ENCRYPTO_utils was downloaded twice, once for ABY and once for OTExtension)

This does not happen for me on the current public branch:

lennart:ABY/ (public) $ ll extern/ENCRYPTO_utils
total 16K
drwxr-xr-x 2 lennart users  100 Feb 12 15:14 cmake
-rw-r--r-- 1 lennart users 3.2K Feb 12 15:14 CMakeLists.txt
drwxr-xr-x 4 lennart users   80 Feb 12 15:14 extern
-rw-r--r-- 1 lennart users 7.5K Feb 12 15:14 LICENSE
-rw-r--r-- 1 lennart users  946 Feb 12 15:14 README.md
drwxr-xr-x 3 lennart users   80 Feb 12 15:14 src
drwxr-xr-x 2 lennart users  100 Feb 12 15:14 test
lennart:ABY/ (public) $ ll extern/OTExtension/extern/ENCRYPTO_utils                                                                                                                                                                 [15:16:28]
total 0

@lenerd
Copy link
Collaborator

lenerd commented Feb 12, 2020

Some general remark on commits:

It would be great for reviewing and keeping track of the changes in the repository if the changes are split into several commits that are mostly self-contained. Changes that belong together should be in the same commit with a meaningful commit message.

E.g., "Add fallback to Boost.Filesystem if std::filesystem is not available", "Add feature ABC", ... Furthermore, two commits "Add XYZ" and "Fix XYZ because it was broken" can be squashed together. The initial broken implementation does not need to be part of the repository.

@lenerd
Copy link
Collaborator

lenerd commented Feb 12, 2020

The reason why we have two locations is that the config files are found by
a path relative to the project source directory (as the config files
conceptually belong to the whole project) and the module files by a path
relative to the CMakeLists.txt that includes them.

Shouldn't that be handled by CMAKE_MODULE_PATH which you already modify in the top-level CMakeLists.txt?

@oliver-schick
Copy link
Author

Regarding std::filesystem I had a commit where I replaced it with boost::filesystem but then changed it, so that boost::filesystem is only used when compiling for android (ifdef ANDROID) and std::[experimental::]filesystem otherwise. Just look at more recent commits.

Putting all the Module files into one directory or separating them is completely arbitrary. It is not a matter of functionality, but a matter of preference. I personally prefer having the module files "beneath" the CMakeLists that includes them. If you prefer having all module files in one single directory, I can simply move them. Cmake will find them in either directory.

@oliver-schick oliver-schick force-pushed the public branch 5 times, most recently from 36d7aef to 5fb70c7 Compare May 3, 2020 14:33
@lenerd
Copy link
Collaborator

lenerd commented Aug 31, 2020

Hi there,

I have looked at this PR again.

First some comments:

  • Regarding dependencies:
    • Reference them by release. When a git repository is used, reference them by tags if available and commit hashes otherwise (e.g. for Encrypto). (Maybe this is already done?)
    • Use encryptogroup repositories instead of your own.
    • Set the versions of dependencies at one place instead of scattering them over multiple files. Makes it easier to check if they are up-to-date.
    • Use up-to-date versions of the dependencies.
    • Document how this works and how one would add additional dependencies. There is a lot of code and variables. It's not obvious to me.
  • Regarding the filesystem library:
    • What's the multiline comment in the abycore/CMakeLists.txt about? Remove it if the code is not necessary.
    • Always use Boost.Filesystem as fallback and drop std::experimental::filesystem.
  • Cleanup the history. I.e., your branch should contain the same commits as the public branch in this repository. Your additional commits should be on top of the others.

I think we discussed most at the last meeting (some time ago).

Building ABY from your branch (oliver-schick@5fb70c7) fails on my machine:

CMake Error at cmake/Modules/AddGMP.cmake:71 (message):
  Did not find gmp in standard location.  Either set GMP_LIBRARY_DIR and
  GMP_INCLUDES to valid locations or enable GMP build by setting BUILD_GMP.
Call Stack (most recent call first):
  src/abycore/CMakeLists.txt:39 (include)


-- Found Boost: /usr/lib64/cmake/Boost-1.72.0/BoostConfig.cmake (found suitable version "1.72.0", minimum required is "1.67") found components: filesystem 
-- ENCRYPTO_utils was not found. Fetching ENCRYPTO_utils...
CMake Error at /tmp/tmp.zzaP4Cep20/build/_deps/encrypto_utils-src/cmake/Modules/AddGMP.cmake:71 (message):
  Did not find gmp in standard location.  Either set GMP_LIBRARY_DIR and
  GMP_INCLUDES to valid locations or enable GMP build by setting BUILD_GMP.
Call Stack (most recent call first):
  /tmp/tmp.zzaP4Cep20/build/_deps/encrypto_utils-src/src/CMakeLists.txt:34 (include)


-- Found Boost: /usr/lib64/cmake/Boost-1.72.0/BoostConfig.cmake (found suitable version "1.72.0", minimum required is "1.67") found components: filesystem system thread 
-- Found OpenSSL.
CMake Warning (dev) at /tmp/tmp.zzaP4Cep20/build/_deps/encrypto_utils-src/cmake/Modules/AddRelic.cmake:18 (set):
  implicitly converting 'INTEGER' to 'STRING' type.
Call Stack (most recent call first):
  /tmp/tmp.zzaP4Cep20/build/_deps/encrypto_utils-src/src/CMakeLists.txt:37 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at /tmp/tmp.zzaP4Cep20/build/_deps/encrypto_utils-src/cmake/Modules/AddRelic.cmake:20 (set):
  implicitly converting 'BOOl' to 'STRING' type.
Call Stack (most recent call first):
  /tmp/tmp.zzaP4Cep20/build/_deps/encrypto_utils-src/src/CMakeLists.txt:37 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.
  • GMP is installed in a standard location; it should be found automatically:
$ pacman -Ql gmp
gmp /usr/
gmp /usr/include/
gmp /usr/include/gmp.h
gmp /usr/include/gmpxx.h
gmp /usr/lib/
gmp /usr/lib/libgmp.so
gmp /usr/lib/libgmp.so.10
gmp /usr/lib/libgmp.so.10.4.0
gmp /usr/lib/libgmpxx.so
gmp /usr/lib/libgmpxx.so.4
gmp /usr/lib/libgmpxx.so.4.6.0
gmp /usr/lib/pkgconfig/
gmp /usr/lib/pkgconfig/gmp.pc
gmp /usr/lib/pkgconfig/gmpxx.pc
gmp /usr/share/
gmp /usr/share/info/
gmp /usr/share/info/gmp.info-1.gz
gmp /usr/share/info/gmp.info-2.gz
gmp /usr/share/info/gmp.info.gz
  • The warnings should be fixed. 'BOOl' looks like a typo.

@oliver-schick
Copy link
Author

Hi,

I addressed every of your points except for the following two points:

Reference them by release. When a git repository is used, reference them by tags if available and commit hashes otherwise
Use encryptogroup repositories instead of your own.

I currently cannot reference a tag nor commit id without breaking the build system for android builds, as your repositories currently do not support android builds. We'll need to proceed the following way:

  • Merge and push ENCRYPTO_utils.
  • Update variables ENCRYPTO_utils_REPOSITORY and ENCRYPTO_utils_TAG in cmake/Modules/OTExtensionCacheVariables.cmake to refer to the newly merged ENCRYPTO_utils repository during merge of OTExtension and push it.
  • Do an analoguous procedure for the variables ENCRYPTO_utils_REPOSITORY, ENCRYPTO_utils_TAG, OTExtension_REPOSITORY, OTExtension_TAG in the cmake/Modules/ABYCacheVariables.cmake in the ABY repository.

Document how this works and how one would add additional dependencies. There is a lot of code and variables. It's not obvious to me.

How to build for Android is already described in the README.md file of the ABY repository. You can add a new dependency just like you would add a new dependency in an empty cmake project: Provide a FindXYZ.cmake file in the modules directory (cmake/Modules) and call find_package(XYZ ...). There is nothing special about it.

A little note about the warnings: I fixed them, but these were inherited from your repository (including the typo). If there is an issue about it, you might want to mark it as fixed after merging my pull request.

if(ABY_BUILD_EXE)
add_subdirectory(src/test)
#add_subdirectory(src/test)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for this?

@lenerd
Copy link
Collaborator

lenerd commented Sep 10, 2020

Document how this works and how one would add additional dependencies. There is a lot of code and variables. It's not obvious to me.

How to build for Android is already described in the README.md file of the ABY repository.

That's how a user would build ABY, but I would like to have some documentation how the whole system works
in case a developer needs to change something.

You can add a new dependency just like you would add a new dependency in an empty cmake project: Provide a FindXYZ.cmake file in the modules directory (cmake/Modules) and call find_package(XYZ ...). There is nothing special about it.

Ideally, you would use CMake Config files to find a package und use Find scripts only if the former are not available. What about the BuildXYZ.cmake scripts etc.?

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.

2 participants