Code review comment for lp:~hipl-core/hipl/android-hipfw

On 31.08.2014, at 21:01, Diego Biurrun <email address hidden> 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