Issue 29345636: Issue 1414 - Document how to build libadblockplus for Android (Closed)
|
Created:
June 8, 2016, 1:26 p.m. by anton Modified:
June 30, 2016, 12:09 p.m. CC:
Felix Dahlke, René Jeschke Visibility:
Public. |
DescriptionIssue 1414 - Document how to build libadblockplus for Android
Patch Set 1 #
Total comments: 13
Patch Set 2 : With some Sergey's suggestions #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #MessagesTotal messages: 12
Building for android is pretty straight-forward and the main problem is getting appropriate android ndk version. I've tested with r9 (working for x86), r10c (working x86), r11c (the latest, not working since toolchain 4.6 is missing, not working even with symlinked 4.9 to required 4.6). Created issue for building: https://issues.adblockplus.org/ticket/4135
Cool, however I have several notes. https://codereview.adblockplus.org/29345636/diff/29345637/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) It's actually can be quite tricky to find the link to these NDKs because they are not the latest ones and as far as I remember there are no links to them on google's web site. Therefore could you please add links to them at the end of "Supported compilers" section. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) It also makes sense to say may be after the list of compilers that we have to use one of these NDK because they contain gcc-4.6 which is required to compile v8. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode66 README.md:66: ### Building for Android I think it should be before "Supported compilers" because there are already "Unix" and "Windows" and this section is very close to them. May be it will be even better to have "Supported compilers" as the first section of "Building". https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android NDK directory. There is additional space character between NDK and directory. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android NDK directory. It would be better if you could include an example.
On 2016/06/08 15:32:36, sergei wrote: > Cool, however I have several notes. > > https://codereview.adblockplus.org/29345636/diff/29345637/README.md > File README.md (right): > > https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 > README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) > It's actually can be quite tricky to find the link to these NDKs because they > are not the latest ones and as far as I remember there are no links to them on > google's web site. Therefore could you please add links to them at the end of > "Supported compilers" section. That can be useful so i will do it. However i'd prefer it to be compilable by the latest NDK (r11c) too/instead. > > https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 > README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) > It also makes sense to say may be after the list of compilers that we have to > use one of these NDK because they contain gcc-4.6 which is required to compile > v8. No problem. BTW, why can't we use any other (more recent) toolchain? F.e. if we try r11c we can choose gcc-4.9 only. Does it mean it can't be used? If it can be compiled by some specific toolchain i believe we should make it more clear to the user. > > https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode66 > README.md:66: ### Building for Android > I think it should be before "Supported compilers" because there are already > "Unix" and "Windows" and this section is very close to them. > > May be it will be even better to have "Supported compilers" as the first section > of "Building". No problem. > > https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 > README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android > NDK directory. > There is additional space character between NDK and directory. I will fix it. > > https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 > README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android > NDK directory. > It would be better if you could include an example. This relates to OS so i'm not sure it worth it doing here.
https://codereview.adblockplus.org/29345636/diff/29345637/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) >> https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 >> README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) >> It's actually can be quite tricky to find the link to these NDKs because they >> are not the latest ones and as far as I remember there are no links to them on >> google's web site. Therefore could you please add links to them at the end of >> "Supported compilers" section. > >That can be useful so i will do it. >However i'd prefer it to be compilable by the latest NDK (r11c) too/instead. Yeah, but unfortunately we cannot achieve it right now because this dependency origins from the version of v8. Currently v8 requires gcc-4.6, the next version (https://issues.adblockplus.org/ticket/1197) requires gcc-4.8. Since #1197 is landed I will look for the next version to update it further to suppor visual stuio 2015. After the latter update I hope it will be possible to use latest NDK, recent versions of v8 require clang and I don't see any concrete version although I have not tried to compile it yet. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) >> https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 >> README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) >> It also makes sense to say may be after the list of compilers that we have to >> use one of these NDK because they contain gcc-4.6 which is required to compile >> v8. > >No problem. BTW, why can't we use any other (more recent) toolchain? F.e. if we >try r11c we can choose gcc-4.9 only. >Does it mean it can't be used? If it can be compiled by some specific toolchain >i believe we should make it more clear to the user. Honestly I personally have not tried to compile it with gcc-4.9 from any of android ndk. It requires at least to dig into v8 gyp and Makefiles and according to previous experience v8 is quite fragile in terms of any modifications, so I would like to avoid it and rather update v8 to a version which does not require old version of compiler. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android NDK directory. >> https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 >> README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android >> NDK directory. >> It would be better if you could include an example. > >This relates to OS so i'm not sure it worth it doing here. Fair enough, albeit I guess nobody is trying to compile libdblockplus for Android on Windows. https://codereview.adblockplus.org/29345636/diff/29345686/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode26 README.md:26: * clang 3.6 for OS X Could you please also add yet one line here? * To compile for Android on linux one need to install g++-multilib It's quite essential and can be tricky for a new person to find the proper packet.
https://codereview.adblockplus.org/29345636/diff/29345686/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode28 README.md:28: You can download *android-ndk-r10c* for [Win 32](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86.exe), [Win 64](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86_64.exe), [OSX](http://dl.google.com/android/ndk/android-ndk-r10c-darwin-x86_64.bin), [Linux 32](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86.bin), [Linux 64](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86_64.bin). I think this is getting confusing as it sort of implies that you need the Android SDK for Windows in order to build the Windows version of libadblockplus. This belongs into the "clang for Android" line above somehow. https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode30 README.md:30: If you have a question about another compiler please [create an issue](https://issues.adblockplus.org/). While you didn't add this sentence: Are we seriously suggesting creating issues for questions? Maybe only create an issue if people try another compile and hit issues?
https://codereview.adblockplus.org/29345636/diff/29345637/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) On 2016/06/10 11:36:01, sergei wrote: > >> https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode61 > >> README.md:61: * clang 3.4 for Android (from android-ndk-r9, r10c) > >> It also makes sense to say may be after the list of compilers that we have to > >> use one of these NDK because they contain gcc-4.6 which is required to > compile > >> v8. > > > >No problem. BTW, why can't we use any other (more recent) toolchain? F.e. if we > >try r11c we can choose gcc-4.9 only. > >Does it mean it can't be used? If it can be compiled by some specific toolchain > >i believe we should make it more clear to the user. > > Honestly I personally have not tried to compile it with gcc-4.9 from any of > android ndk. It requires at least to dig into v8 gyp and Makefiles and according > to previous experience v8 is quite fragile in terms of any modifications, so I > would like to avoid it and rather update v8 to a version which does not require > old version of compiler. i've tried to compile with gcc-4.9 from r11c symlinked to gcc-4.6 and it's not working https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode66 README.md:66: ### Building for Android On 2016/06/08 15:32:35, sergei wrote: > I think it should be before "Supported compilers" because there are already > "Unix" and "Windows" and this section is very close to them. > > May be it will be even better to have "Supported compilers" as the first section > of "Building". Done. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode66 README.md:66: ### Building for Android On 2016/06/08 15:32:35, sergei wrote: > I think it should be before "Supported compilers" because there are already > "Unix" and "Windows" and this section is very close to them. > > May be it will be even better to have "Supported compilers" as the first section > of "Building". Done. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android NDK directory. On 2016/06/08 15:32:35, sergei wrote: > There is additional space character between NDK and directory. Done. https://codereview.adblockplus.org/29345636/diff/29345637/README.md#newcode68 README.md:68: First set ANDROID_NDK_ROOT environment variable to your Android NDK directory. On 2016/06/08 15:32:35, sergei wrote: > It would be better if you could include an example. Not sure it's needed as i assume people trying to compile it are a king of advanced users and they (should) know how to do this basic stuff. Also it relates to OS so i believe it's out of scope of this readme https://codereview.adblockplus.org/29345636/diff/29345686/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode26 README.md:26: * clang 3.6 for OS X On 2016/06/10 11:36:01, sergei wrote: > Could you please also add yet one line here? > > * To compile for Android on linux one need to install g++-multilib > > It's quite essential and can be tricky for a new person to find the proper > packet. Acknowledged. https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode30 README.md:30: If you have a question about another compiler please [create an issue](https://issues.adblockplus.org/). On 2016/06/10 21:54:55, Wladimir Palant wrote: > While you didn't add this sentence: Are we seriously suggesting creating issues > for questions? Maybe only create an issue if people try another compile and hit > issues? i've just left this sentence as it was and i agree we should get issues if the user have a problem instead of just a question. Sergei, are you agree to change this to "If you have a compilation problem with another compiler .." ?
https://codereview.adblockplus.org/29345636/diff/29345686/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode28 README.md:28: You can download *android-ndk-r10c* for [Win 32](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86.exe), [Win 64](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86_64.exe), [OSX](http://dl.google.com/android/ndk/android-ndk-r10c-darwin-x86_64.bin), [Linux 32](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86.bin), [Linux 64](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86_64.bin). On 2016/06/10 21:54:55, Wladimir Palant wrote: > I think this is getting confusing as it sort of implies that you need the > Android SDK for Windows in order to build the Windows version of libadblockplus. > This belongs into the "clang for Android" line above somehow. can you please explain what makes you think about SDK is required? The links are about NDK and the only reason for it is that we need pretty-old versions of NDK and they can't be found by the users on "Download NDK" google page.
https://codereview.adblockplus.org/29345636/diff/29345686/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode28 README.md:28: You can download *android-ndk-r10c* for [Win 32](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86.exe), [Win 64](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86_64.exe), [OSX](http://dl.google.com/android/ndk/android-ndk-r10c-darwin-x86_64.bin), [Linux 32](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86.bin), [Linux 64](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86_64.bin). On 2016/06/14 07:30:30, anton wrote: > On 2016/06/10 21:54:55, Wladimir Palant wrote: > > I think this is getting confusing as it sort of implies that you need the > > Android SDK for Windows in order to build the Windows version of > libadblockplus. > > This belongs into the "clang for Android" line above somehow. > > can you please explain what makes you think about SDK is required? The links are > about NDK and the only reason for it is that we need pretty-old versions of NDK > and they can't be found by the users on "Download NDK" google page. Yeah, the links for windows were also confusing me, I think this section should be restructured, like Target platform: - addition information. Something like the following: Win32: - Microsoft Visual Studio 2010, 2012 Linux: - g++ 5.2 Mac: clang 3.6 for OS X Android: - the host system should be linux or OS X (BTW, it seems there is some issue with recent updates on OS X) - version of Android NDK - links to Android NDK for linx and OS X (without links for Windows because nobody tried it yet). - g++ multilib. https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode30 README.md:30: If you have a question about another compiler please [create an issue](https://issues.adblockplus.org/). On 2016/06/14 07:16:47, anton wrote: > On 2016/06/10 21:54:55, Wladimir Palant wrote: > > While you didn't add this sentence: Are we seriously suggesting creating > issues > > for questions? Maybe only create an issue if people try another compile and > hit > > issues? > > i've just left this sentence as it was and i agree we should get issues if the > user have a problem instead of just a question. Sergei, are you agree to change > this to "If you have a compilation problem with another compiler .." ? Yes, I agree. Just in case, I would like to precise that the current form is fine for me but generally speaking we cannot support all other compilers (e.g. for MIPS). However I would like to have such sentence in the readme file, because we can always try to help with an advice or may be there is indeed a bug in our configuration, which is not visible on our machines, as well as it's useful to see who and what the interest has in this project.
https://codereview.adblockplus.org/29345636/diff/29345686/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29345686/README.md#newcode28 README.md:28: You can download *android-ndk-r10c* for [Win 32](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86.exe), [Win 64](http://dl.google.com/android/ndk/android-ndk-r10c-windows-x86_64.exe), [OSX](http://dl.google.com/android/ndk/android-ndk-r10c-darwin-x86_64.bin), [Linux 32](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86.bin), [Linux 64](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86_64.bin). On 2016/06/14 08:05:29, sergei wrote: > On 2016/06/14 07:30:30, anton wrote: > > On 2016/06/10 21:54:55, Wladimir Palant wrote: > > > I think this is getting confusing as it sort of implies that you need the > > > Android SDK for Windows in order to build the Windows version of > > libadblockplus. > > > This belongs into the "clang for Android" line above somehow. > > > > can you please explain what makes you think about SDK is required? The links > are > > about NDK and the only reason for it is that we need pretty-old versions of > NDK > > and they can't be found by the users on "Download NDK" google page. > > Yeah, the links for windows were also confusing me, I think this section should > be restructured, like > Target platform: > - addition information. > > Something like the following: > > Win32: > - Microsoft Visual Studio 2010, 2012 > Linux: > - g++ 5.2 > Mac: > clang 3.6 for OS X > Android: > - the host system should be linux or OS X (BTW, it seems there is some issue > with recent updates on OS X) > - version of Android NDK > - links to Android NDK for linx and OS X (without links for Windows because > nobody tried it yet). > - g++ multilib. Acknowledged.
LGTM after addressing the comment. https://codereview.adblockplus.org/29345636/diff/29346666/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29346666/README.md#newcode18 README.md:18: ### Supported compilers I think it would be better to call it now as "Supported target platforms and prerequisites".
LGTM except for two nits from me. https://codereview.adblockplus.org/29345636/diff/29346668/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29346668/README.md#newcode32 README.md:32: * the host system should be linux or OS X Nit: it should be "The" and "Linux" (capitalized). https://codereview.adblockplus.org/29345636/diff/29346668/README.md#newcode33 README.md:33: * android-ndk-r9, android-ndk-r10c. You can download the latter for [OS X](http://dl.google.com/android/ndk/android-ndk-r10c-darwin-x86_64.bin), [Linux 32](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86.bin), [Linux 64](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86_64.bin) Nit: period at the end of this sentence please.
https://codereview.adblockplus.org/29345636/diff/29346668/README.md File README.md (right): https://codereview.adblockplus.org/29345636/diff/29346668/README.md#newcode32 README.md:32: * the host system should be linux or OS X On 2016/06/20 15:17:51, Wladimir Palant wrote: > Nit: it should be "The" and "Linux" (capitalized). Acknowledged. https://codereview.adblockplus.org/29345636/diff/29346668/README.md#newcode33 README.md:33: * android-ndk-r9, android-ndk-r10c. You can download the latter for [OS X](http://dl.google.com/android/ndk/android-ndk-r10c-darwin-x86_64.bin), [Linux 32](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86.bin), [Linux 64](http://dl.google.com/android/ndk/android-ndk-r10c-linux-x86_64.bin) On 2016/06/20 15:17:51, Wladimir Palant wrote: > Nit: period at the end of this sentence please. Acknowledged. 
|
