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

On Sun, Aug 31, 2014 at 09:25:30PM +0300, Juhani Toivonen wrote:
> On 31.08.2014, at 21:01, Diego Biurrun <email address hidden> wrote:
> > 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.

b)

> > I fully agree with René, these inline patches are ugly.
>
> Fine, I’ll find a place to host them and have the script pull them in.

You don't need to. Adding the files to HIPL is not a problem, but why
have them inlined into the script?

Diego