Android Support #157
Android Support #157
Conversation
|
Hi Oliver, Please do the same also for the other PRs, encryptogroup/ENCRYPTO_utils#28 and encryptogroup/OTExtension#37. |
|
Hi,
I rebased and solved the conflicts for all 3 projects.
Am So., 9. Feb. 2020 um 15:05 Uhr schrieb Lennart Braun <
notifications@github.com>:
… 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
<encryptogroup/ENCRYPTO_utils#28> and
encryptogroup/OTExtension#37
<encryptogroup/OTExtension#37>.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#157?email_source=notifications&email_token=AKXROQ72YH5SNFQDDTQ5MQLRCAET5A5CNFSM4KR76BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELGNJ2Y#issuecomment-583849195>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKXROQ275HGH4HZC3WR2RMLRCAET5ANCNFSM4KR76BNQ>
.
|
|
Something went wrong. It seems that you performed a rebase as well as an additional merge. The history now includes your commits twice: |
|
Ok. Does it build? On my side the system builds without any problems.
Lennart Braun <notifications@github.com> schrieb am Mo., 10. Feb. 2020,
15:07:
… 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
|\ \
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#157?email_source=notifications&email_token=AKXROQ2PAGLQIYOXCZKITJDRCFNSVA5CNFSM4KR76BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELIUA7Q#issuecomment-584138878>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKXROQ5QAITDSX2ONVYC3DDRCFNSVANCNFSM4KR76BNQ>
.
|
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. |
|
You can probably drop the most recent commit, i.e., the merge commit, and push again to clean up the history. |
oliver-schick
force-pushed
the
public
branch
from
0a4d813
to
bb2b2c7
Compare
|
I rebased and force pushed all 3 projects.
Am Mo., 10. Feb. 2020 um 23:24 Uhr schrieb Lennart Braun <
notifications@github.com>:
… You can probably drop the most recent commit, i.e., the merge commit, and
push again to clean up the history.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#157?email_source=notifications&email_token=AKXROQ4HL75PLXF6X37C6QLRCHH2XA5CNFSM4KR76BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELKQ3BY#issuecomment-584387975>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKXROQZVLI7O36W6NW5ZUU3RCHH2XANCNFSM4KR76BNQ>
.
|
|
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:
More general: Why do we need all the new CMake code? |
|
The more general answer is that we need all that code because the old
version of ABY did not support cross compilation and that this cross
compilation is needed to build for Android. Furthermore, due to the nature
of android builds, it is necessary to provide a seperate build for each
Android ABI, since you cannot e.g. use libraries compiled for armeabi-v7a
on a arm64-v8a platform. I'm not even sure, if you can e.g. use libraries
compiled for Android API leven 16 for Android API level 22 without any
restriction. Even if, it is likely better to use code optimized for API
level 22, than code optimized for 16. That's the main reason why the files
BuildGMP and BuildOpenSSL are provided, to build these dependencies for the
specific target ABI and platform.
I took care that my CMake code works when not building for Android, so you
shouldn't have too many problems including my code. Except for the fact,
that the master branch of OTExtension and ENCRYPTO_utils dependencies is
downloaded through FetchContent, building ABY for Linux works exactly the
same way as with the old version of ABY.
The Git submodules of ENCRYPTO_utils and OTExtension were changed to calls
to FetchContent, which in the default case downloads the master branch of
the git repository, thus doing the same thing as the old calls to git
submodule, except for being more stable, omitting download, if the
repositories were already downloaded or are found on the disk (previously
ENCRYPTO_utils was downloaded twice, once for ABY and once for OTExtension)
and is consistent with the rest of the project, especially allowing to use
FetchHelper.cmake for the download process, which is used for every
dependency except GMP and OpenSSL.
The reason why I reference my archives is that the official ones currently
do not support Android builds, thus breaking the build for Android, if ABY
downloads the official OTExtension repository. The files
FetchENCRYPTO_utils.cmake and FetchOTExtension.cmake will have to be
updated, to reference the official ones, as soon as they are merged. There
is also support for downloading specific commit ids if the (advanced)
option DOWNLOAD_<Project> is set. Regarding the other dependencies, only
specific commits are downloaded, to prevent that changes in external
projects break ABY, as these are maintained seperately. Downloading zip
archives of a specific commit from github or downloading it by setting
GIT_TAG to a commit hash is somewhat equivalent. I can therefore change the
URL download method to a git download method, if you want to (but I do not
recommend downloading the master branch of external projects, like I'm
doing with ENCRYPTO_utils and OTExtension, as ABY might break when an
external project is updated).
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. If you prefer one
directory tree for all module and config files, I can adapt the
implementation to reflect your preference.
The duplicate files are a mistake. I moved every *Config.cmake.in files
being in src/abycore/cmake/ to cmake/ at some point. You can remove all *
Config.cmake.in files in src/abycore/cmake/ when merging or I can remove
them with the next commit, if you want me to make additional changes or
correct some errors.
Am Di., 11. Feb. 2020 um 17:13 Uhr schrieb Lennart Braun <
notifications@github.com>:
… 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?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#157?email_source=notifications&email_token=AKXROQ4SBWHAP6HFQIKXK6TRCLFDLA5CNFSM4KR76BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELNAREA#issuecomment-584714384>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKXROQ74S55P42CDDUTBY3DRCLFDLANCNFSM4KR76BNQ>
.
|
|
I removed the duplicates and fixed some issues with the config files. I
further fixed the issue that install was creating unnecessary directories.
Please make sure to update to the new version.
Am Mi., 12. Feb. 2020 um 07:09 Uhr schrieb Oliver Schick <
oliver.schick92@gmail.com>:
… The more general answer is that we need all that code because the old
version of ABY did not support cross compilation and that this cross
compilation is needed to build for Android. Furthermore, due to the nature
of android builds, it is necessary to provide a seperate build for each
Android ABI, since you cannot e.g. use libraries compiled for armeabi-v7a
on a arm64-v8a platform. I'm not even sure, if you can e.g. use libraries
compiled for Android API leven 16 for Android API level 22 without any
restriction. Even if, it is likely better to use code optimized for API
level 22, than code optimized for 16. That's the main reason why the files
BuildGMP and BuildOpenSSL are provided, to build these dependencies for the
specific target ABI and platform.
I took care that my CMake code works when not building for Android, so you
shouldn't have too many problems including my code. Except for the fact,
that the master branch of OTExtension and ENCRYPTO_utils dependencies is
downloaded through FetchContent, building ABY for Linux works exactly the
same way as with the old version of ABY.
The Git submodules of ENCRYPTO_utils and OTExtension were changed to calls
to FetchContent, which in the default case downloads the master branch of
the git repository, thus doing the same thing as the old calls to git
submodule, except for being more stable, omitting download, if the
repositories were already downloaded or are found on the disk (previously
ENCRYPTO_utils was downloaded twice, once for ABY and once for OTExtension)
and is consistent with the rest of the project, especially allowing to use
FetchHelper.cmake for the download process, which is used for every
dependency except GMP and OpenSSL.
The reason why I reference my archives is that the official ones currently
do not support Android builds, thus breaking the build for Android, if ABY
downloads the official OTExtension repository. The files
FetchENCRYPTO_utils.cmake and FetchOTExtension.cmake will have to be
updated, to reference the official ones, as soon as they are merged. There
is also support for downloading specific commit ids if the (advanced)
option DOWNLOAD_<Project> is set. Regarding the other dependencies, only
specific commits are downloaded, to prevent that changes in external
projects break ABY, as these are maintained seperately. Downloading zip
archives of a specific commit from github or downloading it by setting
GIT_TAG to a commit hash is somewhat equivalent. I can therefore change the
URL download method to a git download method, if you want to (but I do not
recommend downloading the master branch of external projects, like I'm
doing with ENCRYPTO_utils and OTExtension, as ABY might break when an
external project is updated).
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. If you prefer one
directory tree for all module and config files, I can adapt the
implementation to reflect your preference.
The duplicate files are a mistake. I moved every *Config.cmake.in files
being in src/abycore/cmake/ to cmake/ at some point. You can remove all *
Config.cmake.in files in src/abycore/cmake/ when merging or I can remove
them with the next commit, if you want me to make additional changes or
correct some errors.
Am Di., 11. Feb. 2020 um 17:13 Uhr schrieb Lennart Braun <
***@***.***>:
> 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?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#157?email_source=notifications&email_token=AKXROQ4SBWHAP6HFQIKXK6TRCLFDLA5CNFSM4KR76BN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELNAREA#issuecomment-584714384>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AKXROQ74S55P42CDDUTBY3DRCLFDLANCNFSM4KR76BNQ>
> .
>
|
|
Regarding the filesystem library: We should try to use 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. |
This does not happen for me on the current public branch: |
|
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. |
Shouldn't that be handled by |
|
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
force-pushed
the
public
branch
5 times, most recently
from
36d7aef
to
5fb70c7
Compare
|
Hi there, I have looked at this PR again. First some comments:
I think we discussed most at the last meeting (some time ago). Building ABY from your branch (oliver-schick@5fb70c7) fails on my machine:
|
oliver-schick
force-pushed
the
public
branch
from
5fb70c7
to
a4273b4
Compare
|
Hi, I addressed every of your points except for the following two points:
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:
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) |
There was a problem hiding this comment.
What's the reason for this?
That's how a user would build ABY, but I would like to have some documentation how the whole system works
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.? |
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.