[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/android-hipfw into lp:hipl
- From: "Juhani Toivonen" <dmarc-noreply@xxxxxxxxxxxxx> (Redacted sender "juhani.toivonen@xxxxxxxxxxxxxx" for DMARC)
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Sun, 31 Aug 2014 21:25:30 +0300
On 31.08.2014, at 21:01, Diego Biurrun <diego@xxxxxxxxxx> wrote:
> review needs-changes
>
> On Thu, Aug 28, 2014 at 08:14:21PM -0000, Juhani Toivonen wrote:
>> --- INSTALL 2014-03-27 21:25:22 +0000
>> +++ INSTALL 2014-08-28 20:13:36 +0000
>> @@ -115,29 +115,47 @@
>>
>> 5. In the HIPL source tree, run:
>> - ./configure --enable-android --disable-firewall --host=arm-linux \
>> - --prefix=/usr --sysconfdir=/etc \
>> - CC=${ANDROID_TOOLCHAIN}/bin/arm-linux-androideabi-gcc \
>> - CFLAGS="-std=c99 -mbionic -fPIC -fno-exceptions \
>> - --sysroot=${ANDROID_SYSROOT}" \
>> + ./configure --enable-android --host=arm-linux \
>> + --prefix=/usr --sysconfdir=/etc \
>> + CC=${ANDROID_TOOLCHAIN}/bin/arm-linux-androideabi-gcc \
>> + CFLAGS="-std=c99 -mbionic -fPIC -pie -fno-exceptions \
>> + --sysroot=${ANDROID_SYSROOT}" \
>> LDFLAGS="-Wl,-rpath-link=${ANDROID_SYSROOT}/usr/lib,\
>> --L${ANDROID_SYSROOT}/usr/lib" \
>> +-L${ANDROID_SYSROOT}/usr/lib" \
>> LIBS="-lc -lm -lgcc -lcrypto"
>
> If you prettyprint this, don't leave out the LDFLAGS line.
The LDFLAGS line is like that by intention.
It’s too long to fit into 80 characters, and it doesn’t work if there’s
whitespace inside it. Like that, at least copying and pasting the line into
terminal works.
To how I see, my options are:
a) Leave it like that. It’s a small eyesore but it works.
b) Ignore the instruction that lines longer than 80 characters should be
divided.
c) Make the line pretty like the others and instruct user to remove whitespace
from within LDFLAGS before executing.
d) For consistency, not pretty print the command at all because of that one
part.
>
>> --- hipfw/helpers.c 2012-05-12 06:54:33 +0000
>> +++ hipfw/helpers.c 2014-08-28 20:13:36 +0000
>> @@ -35,6 +35,7 @@
>> #include <stdarg.h>
>> #include <arpa/inet.h>
>> #include <netinet/in.h>
>> +#include <sys/wait.h>
>>
>> #include "libcore/debug.h"
>> #include "helpers.h"
>
> I suggest pushing such fixes directly, independent of this patch set.
Ok, will do.
>> --- tools/prepare_android_toolchain.sh 2013-11-18 14:14:39 +0000
>> +++ tools/prepare_android_toolchain.sh 2014-08-28 20:13:36 +0000
>> @@ -91,17 +91,206 @@
>>
>> +patch_toolchain()
>> +{
>> +# ------------------------------ PATCH ------------------------------ #
>> +# ---- Add definition for __fswab64 needed by libnetfilter_queue ---- #
>> +# ---- https://code.google.com/p/android/issues/detail?id=14475 ---- #
>> +
>> +patch -N ${ANDROID_SYSROOT}/usr/include/linux/byteorder/swab.h << EOF
>> +--- swab.h 2011-02-02 13:43:37.711530000 +0000
>> ++++ platforms/android-9/arch-arm/usr/include/linux/byteorder/swab.h
>> 2011-02-02 13:37:58.011530001 +0000
>> +@@ -64,9 +64,52 @@
>> + #define __swab64(x) __fswab64(x)
>> + #endif
>> +
>> ++
>> ++static __inline__ __attribute_const__ __u16 __fswab16(__u16 x)
>> ++{
>> ++ return __arch__swab16(x);
>> ++}
>> ++static __inline__ __u16 __swab16p(__u16 *x)
>> ++{
>> ++ return __arch__swab16p(x);
>
> Tabs everywhere - some pre-commit hook should reject this.
>
>> +# ------------------------------ PATCH ------------------------------ #
>> +# --------- Add some depricated stuff back to netinet/ip.h ---------- #
>
> depr_E_cated
>
>> @@ -143,34 +333,69 @@
>> if [ ! "${1}xxx" = "xxx" ]; then
>> if [ "$1" = "--auto-insert-bashrc" ]; then
>> insert_vars_to_bashrc
>> + elif [ "$1" = "--install-sdk" ]; then
>> + get_package "Android SDK"
>> http://dl.google.com/android/android-sdk_r22.6.2-linux.tgz android-sdk-linux
>> + install_sdk_platform_tools
>> + echo "To adb etc. to your path, run:"
>
> This sentence no verb.
Ok, I will the verb to the sentence.
>
>> + echo "export
>> PATH=$PATH:${TOOLCHAIN_INSTALL_FOLDER}/android-sdk-linux/platform-tools"
>
> tabs
>
> I fully agree with René, these inline patches are ugly.
>
> Diego
Fine, I’ll find a place to host them and have the script pull them in.
- Juhani
Other related posts:
