Initial checkin CI related scripts and testlist files #3

liuguo09 · 2020-01-31T13:58:33Z

clone repos including nuttx, apps and tools
setup tools including gcc toolchains, kconfig-frontends, genromfs and ccache
do check build or nightly full build accordingly

Change-Id: I5a2cb9e0a3dba69f16582340d19f6bf4d4d02195
Signed-off-by: liuhaitao liuhaitao@xiaomi.com

maht

I have some comments that may be useful, but in general it looks ok to me (they could be addressed as a correction later).

maht · 2020-02-01T00:35:28Z

cibuild.sh

@@ -0,0 +1,171 @@
+#!/usr/bin/env bash
+


Maybe add set -x and set -e options to aid CI log/debug?

That's right. set -x and -e would be good to debug.

maht · 2020-02-01T00:35:37Z

cibuild.sh

+tools=$WD/../tools
+prebuilt=$WD/../prebuilt
+
+# genromfs


Maybe genromfs can be skipped if already in the distribution. I guess that is the case in the builds.apache.org Jenkins slaves marked as 'ubuntu' (could be just apt install genromfs ... it seems is the same https://launchpad.net/ubuntu/+source/genromfs/0.5.2-2build2 , same sha256 hash). Anyway maybe is safer by now to use it from nuttx/tools, maybe there is a reason for that I am not aware of.

I think there should be sudo permissions to apt install genromfs and others in apache jenkins slaves, it would be hard to get and maintain. So use our owned prebuilt tools may be safer.

ok, it makes sense.

maht · 2020-02-01T00:35:42Z

cibuild.sh

+install+=("if [ ! -d \"$prebuilt/kconfig-frontends/bin\" ]; then \
+             cd $tools/kconfig-frontends; \
+             ./configure --prefix=$prebuilt/kconfig-frontends --enable-mconf --disable-gconf --disable-qconf --enable-static; \
+             make install; \


could this also benefit of the PREFIX parameter?

same as above.

maht · 2020-02-01T00:35:44Z

cibuild.sh

+             ./configure --prefix=$prebuilt/kconfig-frontends --enable-mconf --disable-gconf --disable-qconf --enable-static; \
+             make install; \
+             cd $tools; git clean -xfd; \
+           fi; \


check the binary generated is ok? (like in previous case with genromfs)

Yes, check the binary generated is more correctly.

maht · 2020-02-01T00:35:49Z

cibuild.sh

+           ")
+path+=("$prebuilt/riscv64-unknown-elf-gcc/bin")
+
+# ccache


I think having the option to enable/disable ccache would be useful: ccache is used to cache compilations, but in CI is better to compile from scratch, since usually repeatability is desired despite of the speed. Or is ccache being used because the script testbuild.sh tries to compile the same files many times due to different configurations? If the caching is not being used disabling it would be better as the cache miss seems to have a significant performance cost (see https://ccache.dev/performance.html), and only enable it for the case a developer want to test the script locally before sending to upstream.

I'm doing check/full build in local machine with ccache enable/disable to see the check/full build results. Still need some time.I think it would be fine to provide ccache enable/disable option.

Add ./cibuild.sh -c option to enable ccache now. The tests in my local machine, it shows ccache would speedup 20% ~30% compile time in general, but it also failed in some case.
Any way, we could enalbe/disable in Apache CI to test and verify later. Then decide enalbe or disable it defaultly.

sim/mips/riscv/x86 configs

ccache noccache

real 12m52.185s 15m24.466s
real 15m48.828s 15m20.406s
real 12m1.289s 15m14.861s
real 11m56.963s 15m16.283s

arm configs

ccache: noccache

real 137m39.394s 182m37.709s
real 135m40.322s 184m59.780s
real 179m58.482s

maht · 2020-02-01T00:35:53Z

cibuild.sh

+function install_tools {
+  for ((i=0; i<${#install[@]}; ++i)); do
+    export PATH=${path[$i]}:$PATH
+    eval ${install[$i]}


Using eval on a long strings of code makes difficult to edit (editors will not highlight the code inside the literal string). To me it looks like extra complexity for no apparent benefit. Why not putting directly the code like in install_tools() or at least define function for each one, and then call them from install_tools()?.

In fact, with set -e and set -x in bash, it's not much difficult to debug. IMO, add tools into an array may be clear than more functions. Once tools need update, just update the array.

The fact that -x helps with that problem should not be a reason if it can be avoided. And if you prefer to control the tools installed using an array it can be done also using functions. Also since the functions have names, the execution with -x gives extra info. For example:

#!/usr/bin/env bash set -x set -e PATH="initial_path" function add_path { PATH=$1:$PATH } function tool1 { echo installing tool 1 add_path "/path/to/tool1" } function tool2 { echo installing tool 2 add_path "/path/to/tool2" } install="tool1 tool2" # ... and a few lines later ... for f in $install; do $f done

will output

+ set -e + PATH=initial_path + install='tool1 tool2' + for f in $install + tool1 + echo installing tool 1 installing tool 1 + add_path /path/to/tool1 + PATH=/path/to/tool1:initial_path + for f in $install + tool2 + echo installing tool 2 installing tool 2 + add_path /path/to/tool2 + PATH=/path/to/tool2:/path/to/tool1:initial_path

Thanks, it makes sense. update on going.

done, maht help to review, thanks.

maht · 2020-02-01T00:35:56Z

cibuild.sh

+}
+
+function run_builds {
+  local JOBS=`grep -c ^processor /proc/cpuinfo`


I would change JOBS by ncpus, as stated in the help of testbuild.sh. JOBS in a CI script makes me think in Jenkins jobs (despite the -j argument).

Use ncpus instead would be fine. Update it later.

maht · 2020-02-01T00:35:59Z

cibuild.sh

+
+  $nuttx/tools/testbuild.sh -si -j $JOBS $WD/testlist/${build}list.dat
+  if [ $? != 0 ]; then
+    echo "ERROR: $BUILD build failed."


Add the error code to message: echo "ERROR: $BUILD build failed (error $?)"

Return $? would be ok, now testbuild.sh would just return fail value 0 or 1 in fact. Since testbuild.sh keep on going build even build fail in some build stage, error values would be overwrited. So it just keep the fail value as failed 1 or success 0.

But the final error value would not be overwritten, it isn't? So why not add it in the message?

Right, thanks, I should also make a PR to update testbuild.sh to correct fail value as the final errno value.

done, and testbuild.sh PR also upstream merged

maht · 2020-02-01T00:36:01Z

cibuild.sh

+    build=$1
+    break
+    ;;
+  * )


maybe having a additional argument to just install the tools could be useful, like cibuild.sh install_tools. This way a Docker image can be prepared with all the tools ready to go, and just start with an empty workspace (in benefit of speed with reasonable repeatability). In fact, a companion Dockerfile would be a nice extra (but it could wait for another pull-request).

It's a good idea to provide install_tools for docker image. I'll update later.

Besides added -i option for install_tools, -s option for setup_repos and -b for run_builds also added. I think it would also be good for Jenkins pipeline job.

maht · 2020-02-01T00:36:04Z

cibuild.sh

+  * )
+    usage
+    ;;
+  esac


this loop seems to lack a shift built-in so that the parameter list is completely consumed eventually.

You are right.

maht

If you have time to address my comments that would be great. But if not and we and other members prefer to merge this as soon as possible we could improve them later. I have no strong opposition as they are minor issues. I haven't run it though, so maybe I will try if no one have checked it aside yourself.

maht · 2020-02-01T12:13:04Z

cibuild.sh

+function install_tools {
+  for ((i=0; i<${#install[@]}; ++i)); do
+    export PATH=${path[$i]}:$PATH
+    eval ${install[$i]}


The fact that -x helps with that problem should not be a reason if it can be avoided. And if you prefer to control the tools installed using an array it can be done also using functions. Also since the functions have names, the execution with -x gives extra info. For example:

#!/usr/bin/env bash set -x set -e PATH="initial_path" function add_path { PATH=$1:$PATH } function tool1 { echo installing tool 1 add_path "/path/to/tool1" } function tool2 { echo installing tool 2 add_path "/path/to/tool2" } install="tool1 tool2" # ... and a few lines later ... for f in $install; do $f done

will output

+ set -e + PATH=initial_path + install='tool1 tool2' + for f in $install + tool1 + echo installing tool 1 installing tool 1 + add_path /path/to/tool1 + PATH=/path/to/tool1:initial_path + for f in $install + tool2 + echo installing tool 2 installing tool 2 + add_path /path/to/tool2 + PATH=/path/to/tool2:/path/to/tool1:initial_path

maht · 2020-02-01T12:13:48Z

cibuild.sh

+}
+
+function run_builds {
+  local JOBS=`grep -c ^processor /proc/cpuinfo`


maht · 2020-02-01T12:15:05Z

cibuild.sh

+
+  $nuttx/tools/testbuild.sh -si -j $JOBS $WD/testlist/${build}list.dat
+  if [ $? != 0 ]; then
+    echo "ERROR: $BUILD build failed."


But the final error value would not be overwritten, it isn't? So why not add it in the message?

maht · 2020-02-01T12:15:15Z

cibuild.sh

+    build=$1
+    break
+    ;;
+  * )


maht · 2020-02-01T12:15:51Z

cibuild.sh

+tools=$WD/../tools
+prebuilt=$WD/../prebuilt
+
+# genromfs


ok, it makes sense.

maht · 2020-02-01T12:16:07Z

cibuild.sh

+install+=("if [ ! -d \"$prebuilt/kconfig-frontends/bin\" ]; then \
+             cd $tools/kconfig-frontends; \
+             ./configure --prefix=$prebuilt/kconfig-frontends --enable-mconf --disable-gconf --disable-qconf --enable-static; \
+             make install; \


maht · 2020-02-01T12:18:34Z

cibuild.sh

+           ")
+path+=("$prebuilt/riscv64-unknown-elf-gcc/bin")
+
+# ccache


1. clone repos including nuttx, apps and tools 2. setup tools including gcc toolchains, kconfig-frontends, genromfs and ccache 3. do check build or nightly full build accordingly Change-Id: I5a2cb9e0a3dba69f16582340d19f6bf4d4d02195 Signed-off-by: liuhaitao <liuhaitao@xiaomi.com>

maht

Thanks for addressing the suggested changed and some extra ones in that direction. I have a minor comment (see inline) in case you agree and have time to change it, but anyway I think this could be merged.

maht · 2020-02-03T13:53:03Z

cibuild.sh

+tools=$WD/../tools
+prebuilt=$WD/../prebuilt
+
+install="gen-romfs kconfig-frontends arm-gcc-toolchain mips-gcc-toolchain riscv-gcc-toolchain c-cache"


A minor comment: I think that maybe these names are ok with the dash '-', since they look as option parameters, but since they are used as function names, I am not sure, since think that using '-' instead of '_' is less common. I haven't find any NuttX shell script coding style, so maybe it is ok to leave it like that. Also, I don't see why the 'c-cache' is spelled that way instead of simply 'ccache', since that seems to be the name of the tool.

Using '-' instead of '_' is mainly align to the names of install tools. But genromfs and ccache functions names would conflict with the genromfs/ccache commands names. So I have to rename genromfs -> gen-romfs, ccache -> c-cache to avoid run abnormally : (

maht · 2020-02-03T13:53:34Z

cibuild.sh

+    build=$1
+    break
+    ;;
+  * )


liuguo09 force-pushed the master branch from 57dd141 to e7c2d9b Compare January 31, 2020 15:11

maht reviewed Feb 1, 2020

View reviewed changes

maht suggested changes Feb 1, 2020

View reviewed changes

liuguo09 force-pushed the master branch from e7c2d9b to c397755 Compare February 3, 2020 07:07

maht approved these changes Feb 3, 2020

View reviewed changes

xiaoxiang781216 merged commit b2588d3 into apache:master Feb 4, 2020

Initial checkin CI related scripts and testlist files #3

Initial checkin CI related scripts and testlist files #3

Uh oh!

Conversation

liuguo09 commented Jan 31, 2020

Uh oh!

maht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sim/mips/riscv/x86 configs

arm configs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment