Merge lp:~milo/linaro-ci/bug1064686 into lp:linaro-ci

[Milo Casagrande]

As per bug 1064686, this should be the only necessary change to do.
It has to be verified.

This fixes when the make dtbs target is executed, as per Andrey comment.

[Paul Sokolovsky (pfalcon)]

Can we be sure that all kernel we build contain "dtbs" target? If not (recommended direction of thought), the how do we handle errors? Will it break the build? Should it be "make dtbs || true"?

[Milo Casagrande (milo)]

Hey Paul, indeed, it should at least be "... || true". Now it is fixed.

[Deepti B. Kalakeri (deeptik)]

I am not sure if all of the folks using this script would like to do a make dtbs. so you should enable this command only for interested folks.

review: Needs Fixing

[Milo Casagrande (milo)]

I updated the bash script, now it is using a variable that needs to be set, and also set to true, in order to execute the "make dtbs" step.

[Paul Sokolovsky (pfalcon)]

Milo, I don't know where from you got this idiom:

 if [ "${MAKE_DTBS}" ] && $MAKE_DTBS;

But it's dangerous to allow literal input string data to be executed as shell commands. You may say that in this case we allow user to pass commands as inputs (*1), but the above is bad *pattern*: MAKE_DTBS has string value, and should not be allowed to be interpreted as command. Failing that would lead from mildly unpleasant to highly dangerous results, so better never to use that at all.

*1 I'm not sure if that's true for this specific build type or not, but for some build types we may.

review: Needs Fixing

[Milo Casagrande (milo)]

Idiom was to check if variable is present and also set to 'true', but indeed, I got your point.

Other solution I had in mind was:

: {$MAKE_DTBS?} && make dtbs

but has the slighty drawback of printing error messages if MAKE_DTBS is not set, and am not able to suppress that, and MAKE_DTBS can be set to whatever value.

[Milo Casagrande (milo)]

The syntax above should be:

: ${MAKE_DTBS?} && make dtbs

[Данило Шеган (danilo)]

What's wrong with just checking if the value is equal to string "true" (or "yes", "1" or whatever we want to accept)? If MAKE_DTBS is set to any of the other values (eg. "yes" or "no"), it's still going to fail with your command.

[Milo Casagrande (milo)]

On Mon, Oct 15, 2012 at 11:16 AM, Данило Шеган <email address hidden> wrote:
> What's wrong with just checking if the value is equal to string "true" (or "yes", "1" or whatever we want to accept)? If MAKE_DTBS is set to any of the other values (eg. "yes" or "no"), it's still going to fail with your command.

There is nothing wrong, I just wanted to avoid exactly that string
check trying to use real boolean values set from Jenkins, but it is
the only thing we can do if we want to accept a value and be somewhat
safe. The other idea (the last syntax) was to just check the presence
of the variable (and execute the command if it is), not its content.

Since Jenkins supports boolean values in parametrized build (not
string or text in Jenkins term, but true and false bash values), I
thought that &VARIABLE would have been safe enough in an if-clause,
since it was coming from a trusted party (Jenkins).

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Milo Casagrande (milo)]

Simple string check has been added: MAKE_DTBS value has to be set to 'true' in this case. It is possible to expand it to include "yes" and "1" too, but using "true" we can use default Jenkins boolean parameter.

[Paul Sokolovsky (pfalcon)]

Looks good. Supporting one value (especially if standardize on it) is ok by me.

review: Approve

[Deepti B. Kalakeri (deeptik)]

looks good.

review: Approve

[Andrey Konovalov (andrey-konovalov)]

'make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" dtbs' should be perfomed *before* creating the package with 'make ... deb-pkg'. In the current implementation the dtb is created, but is not included into the linux-image deb as it should.

review: Needs Fixing

[Deepti B. Kalakeri (deeptik)]

+1 looks good.

review: Approve

[Andrey Konovalov (andrey-konovalov)]

review: Approve

=== modified file 'jenkins_common_lib'
--- jenkins_common_lib	2012-10-17 15:38:51 +0000
+++ jenkins_common_lib	2012-10-22 07:42:19 +0000
@@ -78,11 +78,11 @@
     yes "" | make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" oldconfig	
     make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" -j$j_count uImage
     make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" -j$j_count modules
-    make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" KBUILD_DEBARCH=armhf V=1 deb-pkg
     if [ "$MAKE_DTBS" = "true" ]; then
         echo "Building with DTBS support..."
         make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" dtbs
     fi
+    make ARCH=arm O=$pkg_dir KERNELVERSION="$kernel_version" KERNELRELEASE="$kernel_version" CROSS_COMPILE="$TOOLCHAIN_PREFIX" KBUILD_DEBARCH=armhf V=1 deb-pkg
     END=$(date +%s)
     EXECUTION_TIME_IN_SEC=$(( $END - $START ))