[Buildroot] [PATCH 0/7 v3] autobuild: misc fixes and enhancements (branch yem/limits)

[Buildroot] [PATCH 0/7 v3] autobuild: misc fixes and enhancements (branch yem/limits)

[Yann E. MORIN]

Hello All!

This series, against the autobuilder tree, adds two new configuration
options, a sample runtime config file, and a SysV init startup script.
It also fixes an infinite loop when the host glibc is too old and the
Linaro toolchains are used.

The two new rutime options are:
  --nice N      specify the niceness of the 'make' command
  --pid-file    file where to save the main process PID

The niceness allows limitting the impact of the autobuilder on the rest
of the job, when on a shared machine. A better solution would be to
implement a container with cgroups, but a systemd unit file would be so
much convenient! ;-)

The PID file allows to simplify the startup script, and really ensure
that we get the autobuild process' PID.

A SysV init startup script, as well as a sample defaults config file are
provided, to ease running the autobuild script as a daemon.

A sample runtime configuration file is provided as well.

As for the infinite loop, the fix consist in two changes:
  - separating the check that the configuration is actually fixable
    from the actual fixup,
  - adding a safeguard that limits the loop to at most 100 iterationd,
    and ignores that configuration if the limit is reached.

Note that nothing in there is really bullet-proof, but has been running
smoothly on a private instance for about a few hours now, and the
failure/succes rate is on par with what we currently have in the
autobuilders (two patches to fix build issues due to the host
configuration have already been caught, and patches submitted)

Changes v2 -> v3:
  - add the infinite loop fixes

Changes v1 -> v2;
  - add PID file handling
  - add startup script and sample configuration files
  - typo in --nice help text

Regards,
Yann E. MORIN.


The following changes since commit 611b515aa40c635d1af24883a9eab9e1e363a33c:

  Add mips32r6 and mips64r6 toolchains (2015-11-22 15:31:07 +0100)

are available in the git repository at:

  git://git.busybox.net/~ymorin/git/buildroot-test yem/limits

for you to fetch changes up to a89f4ae40bd1f7d5600226eb0ac20f27098413fb:

  autobuild: check for Linaro toolchains earlier (2015-11-27 16:00:21 +0100)

----------------------------------------------------------------
Yann E. MORIN (7):
      autobuild: add config option to set niceness
      autobuild: store PID to PID file
      conf: add a sample run-time configuration file
      etc: add SysV-init startup script and sample config file
      autobuild: avoid infinite loop when sanitising the configuration
      autobuild: add a function to check if the configuration is fixable
      autobuild: check for Linaro toolchains earlier

 conf/buildroot-autobuild.sample |   5 ++
 etc/default/buildroot-autobuild |  23 +++++++
 etc/init.d/buildroot-autobuild  | 148 ++++++++++++++++++++++++++++++++++++++++
 scripts/autobuild-run           |  66 +++++++++++++++---
 4 files changed, 231 insertions(+), 11 deletions(-)
 create mode 100644 conf/buildroot-autobuild.sample
 create mode 100644 etc/default/buildroot-autobuild
 create mode 100755 etc/init.d/buildroot-autobuild

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/7 v3] autobuild: add config option to set niceness

[Yann E. MORIN]

When the machine running the autobuilder is shared, it is important
that the autobuilder instances do not hog all of the CPU.

Add an option to the configuration, so that the user can set the
niceness to run the instances with. By default, the niceness is 0,
which means the current niceness.

Properly doing so would require setting up a container with proper
CPU cgroup, but it's a bit complicated.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 scripts/autobuild-run | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index 76b7201..db60f5c 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -64,6 +64,7 @@ defaults = {
     '--njobs': '1',
     '--submitter': 'N/A',
     '--make-opts': '',
+    '--nice': 0,
 }
 
 doc = """autobuild-run - run Buildroot autobuilder
@@ -77,6 +78,8 @@ Options:
                                  Defaults to %(--ninstances)s.
   -j, --njobs NJOBS              number of parallel jobs
                                  Defaults to %(--njobs)s.
+  --nice N                       Niceness, positive number
+                                 Defaults to %(--nice)s.
   -s, --submitter SUBMITTER      name/machine of submitter
                                  Defaults to %(--submitter)s.
   --http-login LOGIN             username to send results with
@@ -101,6 +104,7 @@ Format of the configuration file:
    [main]
    ninstances = <value>
    njobs = <value>
+   nice = <value>
    http-login = <value>
    http-password = <value>
    submitter = <value>
@@ -540,6 +544,7 @@ def do_build(**kwargs):
 
     idir = "instance-%d" % kwargs['instance']
     log = kwargs['log']
+    nice = kwargs['nice']
 
     # We need the absolute path to use with O=, because the relative
     # path to the output directory here is not relative to the
@@ -551,7 +556,9 @@ def do_build(**kwargs):
     f = open(os.path.join(outputdir, "logfile"), "w+")
     log_write(log, "INFO: build started")
 
-    cmd = ["timeout", str(MAX_DURATION), "make", "O=%s" % outputdir,
+    cmd = ["timeout", str(MAX_DURATION),
+            "nice", "-n", nice,
+            "make", "O=%s" % outputdir,
             "-C", srcdir, "BR2_DL_DIR=%s" % dldir,
             "BR2_JLEVEL=%s" % kwargs['njobs']] \
           + kwargs['make_opts'].split()
@@ -851,6 +858,7 @@ def main():
                 http_password = args['--http-password'],
                 submitter = args['--submitter'],
                 make_opts = (args['--make-opts'] or ''),
+                nice = (args['--nice'] or 0),
                 upload = upload,
                 buildpid = buildpid
             ))
-- 
1.9.1

[Buildroot] [PATCH 1/7 v3] autobuild: add config option to set niceness

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:08 +0100, Yann E. MORIN wrote:
> When the machine running the autobuilder is shared, it is important
> that the autobuilder instances do not hog all of the CPU.
> 
> Add an option to the configuration, so that the user can set the
> niceness to run the instances with. By default, the niceness is 0,
> which means the current niceness.
> 
> Properly doing so would require setting up a container with proper
> CPU cgroup, but it's a bit complicated.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  scripts/autobuild-run | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Applied to buildroot-test, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 2/7 v3] autobuild: store PID to PID file

[Yann E. MORIN]

Let the autobuild script store its own PID file, rather than retrieve it
from the init script.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 scripts/autobuild-run | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index db60f5c..397c770 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -65,6 +65,7 @@ defaults = {
     '--submitter': 'N/A',
     '--make-opts': '',
     '--nice': 0,
+    '--pid-file': '/tmp/buildroot-autobuild.pid',
 }
 
 doc = """autobuild-run - run Buildroot autobuilder
@@ -92,6 +93,8 @@ Options:
   --make-opts OPTSTRING          string of extra options to pass to Buildroot
                                  make, such as specific command wrappers
                                  Empty by default.
+  --pid-file PATH                path to a file where to store the PID
+                                 Defaults to %(--pid-file)s.
   -c, --config CONFIG            path to configuration file
                                  Not set by default.
 
@@ -807,6 +810,10 @@ def main():
     # load in defaults at lowest priority
     args = merge(args, defaults)
 
+    # Save our PID very early, so we can be stopped
+    with open(args['--pid-file'], "w+") as pidf:
+        pidf.write("%d" % os.getpid())
+
     # http_login/password could theoretically be allowed as empty, so check
     # explicitly on None.
     upload = (args['--http-login'] is not None) \
-- 
1.9.1

[Buildroot] [PATCH 2/7 v3] autobuild: store PID to PID file

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:09 +0100, Yann E. MORIN wrote:
> Let the autobuild script store its own PID file, rather than retrieve it
> from the init script.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  scripts/autobuild-run | 7 +++++++
>  1 file changed, 7 insertions(+)

Applied to buildroot-test, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 3/7 v3] conf: add a sample run-time configuration file

[Yann E. MORIN]

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 conf/buildroot-autobuild.sample | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 conf/buildroot-autobuild.sample

diff --git a/conf/buildroot-autobuild.sample b/conf/buildroot-autobuild.sample
new file mode 100644
index 0000000..dcb74b1
--- /dev/null
+++ b/conf/buildroot-autobuild.sample
@@ -0,0 +1,5 @@
+[main]
+ninstances = 4
+njobs = 4
+nice = 20
+submitter = John Doe
-- 
1.9.1

[Buildroot] [PATCH 3/7 v3] conf: add a sample run-time configuration file

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:10 +0100, Yann E. MORIN wrote:
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  conf/buildroot-autobuild.sample | 5 +++++
>  1 file changed, 5 insertions(+)
>  create mode 100644 conf/buildroot-autobuild.sample
> 
> diff --git a/conf/buildroot-autobuild.sample b/conf/buildroot-autobuild.sample
> new file mode 100644
> index 0000000..dcb74b1
> --- /dev/null
> +++ b/conf/buildroot-autobuild.sample
> @@ -0,0 +1,5 @@
> +[main]
> +ninstances = 4
> +njobs = 4
> +nice = 20

From man nice(1):

   Niceness values range  from  -20 (most favorable to the process) to
   19 (least favorable to the process).

So I've changed your example nice value from 20 to 19, and then applied
to buildroot-test.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 4/7 v3] etc: add SysV-init startup script and sample config file

[Yann E. MORIN]

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 etc/default/buildroot-autobuild |  23 +++++++
 etc/init.d/buildroot-autobuild  | 148 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+)
 create mode 100644 etc/default/buildroot-autobuild
 create mode 100755 etc/init.d/buildroot-autobuild

diff --git a/etc/default/buildroot-autobuild b/etc/default/buildroot-autobuild
new file mode 100644
index 0000000..93b7365
--- /dev/null
+++ b/etc/default/buildroot-autobuild
@@ -0,0 +1,23 @@
+# The absolute path, outside the chroot, where the
+# buildroot-test tree is cloned, optional
+AUTOBUILD_DIR="/home/johndoe/buildroot-test"
+
+# The absolute path, outside the chroot, where to
+# share downloads (not yet implemented)
+#AUTOBUILD_DL_DIR="/home/johndoe/dl"
+
+# The absolute path to chroot into to rn the autobuild script
+AUTOBUILD_CHROOT="/var/chroot-autobuild"
+
+# User (in the chroot) that runs the autobuild script)
+AUTOBUILD_USER="buildroot"
+
+# The absolute path, in the chroot, that is the base of the
+# autobuild work dir. In there, you'll have:
+# - AUTOBUILD_CHROOT_DIR/buildroot-test
+#       the buildroot-test tree (bind-mounted from AUTOBUILD_DIR if set)
+# - AUTOBUILD_CHROOT_DIR/buildroot-autobuild.conf
+#       the run-time configuration of the autobuild script
+# - AUTOBUILD_CHROOT_DIR/run/
+#       the parent directory for all build instances
+AUTOBUILD_CHROOT_DIR="/home/buildroot/autobuild/"
diff --git a/etc/init.d/buildroot-autobuild b/etc/init.d/buildroot-autobuild
new file mode 100755
index 0000000..8605533
--- /dev/null
+++ b/etc/init.d/buildroot-autobuild
@@ -0,0 +1,148 @@
+#!/bin/sh
+# vim: ft=sh
+
+### BEGIN INIT INFO
+# Provides:          buildroot-autobuild
+# Required-Start:    $network
+# Required-Stop:     $network
+# Default-Start:     2 3 4 5
+# Default-Stop:      1
+# Short-Description: Buildroot autobuilds
+### END INIT INFO
+
+# Expected configuration in the configuration file;
+#   AUTOBUILD_DIR           the absolute path to the shared the autobuild
+#                           git tree; optional
+#   AUTOBUILD_DL_DIR        the absolute path to the shared DL dir; optional
+#   AUTOBUILD_CHROOT        the absolute path to the chroot to run in
+#   AUTOBUILD_USER          the username to run as
+#
+# The following variable is to be interpreted inside the chroot:
+#   AUTOBUILD_CHROOT_DIR    the absolute path to the directory builds run in
+
+CFG_FILE="/etc/default/buildroot-autobuild"
+
+if [ ! -e "${CFG_FILE}" ]; then
+    printf "ERROR: no autobuilder configuration file\n" >&2
+    exit 1
+fi
+. "${CFG_FILE}"
+if [ -z "${AUTOBUILD_USER}" ]; then
+    printf "ERROR: no autobuild user\n" >&2
+    exit 1
+fi
+if [ -z "${AUTOBUILD_DIR}" ]; then
+    printf "ERROR: no autobuild dir\n" >&2
+    exit 1
+fi
+if [ -z "${AUTOBUILD_CHROOT}" ]; then
+    printf "ERROR: no autobuild chroot\n" >&2
+    exit 1
+fi
+if [ -z "${AUTOBUILD_CHROOT_DIR}" ]; then
+    printf "ERROR: no autobuild chroot dir\n" >&2
+    exit 1
+fi
+
+# Derived configuration:
+#   AUTOBUILD_RUN_DIR   instances will be created in there
+#   AUTOBUILD_CMD       the autobuild script to run
+#   AUTOBUILD_CFG       the autobuild runtime configuration
+AUTOBUILD_RUN_DIR="${AUTOBUILD_CHROOT_DIR}/run"
+AUTOBUILD_CMD="${AUTOBUILD_CHROOT_DIR}/buildroot-test/scripts/autobuild-run"
+AUTOBUILD_CFG="${AUTOBUILD_CHROOT_DIR}/buildroot-autobuild.conf"
+AUTOBUILD_CHROOT_PID_FILE="${AUTOBUILD_CHROOT_DIR}/buildroot-autobuild.pid"
+AUTOBUILD_PID_FILE="${AUTOBUILD_CHROOT}/${AUTOBUILD_CHROOT_DIR}/buildroot-autobuild.pid"
+
+autobuild_start() {
+    echo "Starting buildroot-autobuild"
+
+    CMD="cd '${AUTOBUILD_RUN_DIR}'"
+    CMD="${CMD}; '${AUTOBUILD_CMD}' -c '${AUTOBUILD_CFG}' --pid-file '${AUTOBUILD_CHROOT_PID_FILE}' &"
+
+    do_chroot "rm -rf '${AUTOBUILD_RUN_DIR}'"
+    do_chroot "mkdir -p '${AUTOBUILD_RUN_DIR}'"
+    do_chroot "${CMD}"
+}
+
+autobuild_stop() {
+    echo "Stopping buildroot-autobuild"
+    if [ -f "${AUTOBUILD_PID_FILE}" ]; then
+        kill $(cat "${AUTOBUILD_PID_FILE}")
+    fi
+    rm -f "${AUTOBUILD_PID_FILE}"
+}
+
+autobuild_status() {
+    if [ -f "${AUTOBUILD_PID_FILE}" ]; then
+        printf "buildroot-autobuild is running as PID %d\n" "$(cat "${AUTOBUILD_PID_FILE}")"
+    else
+        printf "buildroot-autobuild is not running (or missing PID file)\n"
+    fi
+}
+
+# This creates a bind mount of $1 to $2, if it doesn't already exists
+mount_on() {
+    mkdir -p "${2}"
+    mount | grep -q "^$1 on $2" || mount --bind $1 $2
+}
+
+# This function runs a command in the chroot *as* the autobuild user (i.e. not root)
+do_chroot() {
+    LANG=C chroot "${AUTOBUILD_CHROOT}" /bin/su -l "${AUTOBUILD_USER}" -c "( ${1} )"
+}
+
+# Create a number of bind mounts needed for the chroot to operate properly.
+prepare_chroot() {
+    # The autobuild source tree, if shared
+    if [ -d "${AUTOBUILD_DIR}" ]; then
+        do_chroot "mkdir -p '${AUTOBUILD_CHROOT_DIR}/buildroot-test'"
+        mount_on "${AUTOBUILD_DIR}" "${AUTOBUILD_CHROOT}/${AUTOBUILD_CHROOT_DIR}/buildroot-test"
+    fi
+
+    # Download directory, if shared
+    if [ -d "${AUTOBUILD_DL_DIR}" ]; then
+        do_chroot "mkdir -p '${AUTOBUILD_CHROOT_DIR}/dl'"
+        mount_on "${AUTOBUILD_DL_DIR}" "${AUTOBUILD_CHROOT}/${AUTOBUILD_CHROOT_DIR}/dl"
+    fi
+
+    mount_on /proc "${AUTOBUILD_CHROOT}/proc"
+    mount_on /run/shm "${AUTOBUILD_CHROOT}/run/shm"
+}
+
+teardown_chroot() {
+    # Leave time for the instances to quit
+    sleep 2
+    umount "${AUTOBUILD_CHROOT}/run/shm"
+    umount "${AUTOBUILD_CHROOT}/proc"
+    if [ -d "${AUTOBUILD_DL_DIR}" ]; then
+        umount "${AUTOBUILD_CHROOT}/${AUTOBUILD_CHROOT_DIR}/dl"
+    fi
+    if [ -d "${AUTOBUILD_DIR}" ]; then
+        umount "${AUTOBUILD_CHROOT}/${AUTOBUILD_CHROOT_DIR}/buildroot-test"
+    fi
+}
+
+case "$1" in
+start)
+    prepare_chroot
+    autobuild_start
+    ;;
+stop)
+    autobuild_stop
+    teardown_chroot
+    ;;
+restart|reload|force-reload)
+    autobuild_stop
+    teardown_chroot
+    prepare_chroot
+    autobuild_start
+    ;;
+status)
+    autobuild_status
+    ;;
+*)
+    echo "Error, unknown action $1"
+    exit 1
+    ;;
+esac
-- 
1.9.1

[Buildroot] [PATCH 4/7 v3] etc: add SysV-init startup script and sample config file

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:11 +0100, Yann E. MORIN wrote:

> diff --git a/etc/default/buildroot-autobuild b/etc/default/buildroot-autobuild
> new file mode 100644
> index 0000000..93b7365
> --- /dev/null
> +++ b/etc/default/buildroot-autobuild
> @@ -0,0 +1,23 @@
> +# The absolute path, outside the chroot, where the
> +# buildroot-test tree is cloned, optional
> +AUTOBUILD_DIR="/home/johndoe/buildroot-test"

What if my buildroot-test tree is just inside the chroot ?

> +# The absolute path, outside the chroot, where to
> +# share downloads (not yet implemented)

"not yet implemented" -> why do we have some code about it in the
script below ?

> +#AUTOBUILD_DL_DIR="/home/johndoe/dl"
> +
> +# The absolute path to chroot into to rn the autobuild script

rn -> run

> +AUTOBUILD_CHROOT="/var/chroot-autobuild"
> +
> +# User (in the chroot) that runs the autobuild script)

last closing parenthesis unneeded

> +AUTOBUILD_USER="buildroot"
> +
> +# The absolute path, in the chroot, that is the base of the
> +# autobuild work dir. In there, you'll have:

What does it mean "you'll have" ? Does it mean that the init script
will set up everything this way, or that it expect things to be already
set up that way ?

> +# - AUTOBUILD_CHROOT_DIR/buildroot-test
> +#       the buildroot-test tree (bind-mounted from AUTOBUILD_DIR if set)
> +# - AUTOBUILD_CHROOT_DIR/buildroot-autobuild.conf
> +#       the run-time configuration of the autobuild script
> +# - AUTOBUILD_CHROOT_DIR/run/
> +#       the parent directory for all build instances
> +AUTOBUILD_CHROOT_DIR="/home/buildroot/autobuild/"
> diff --git a/etc/init.d/buildroot-autobuild b/etc/init.d/buildroot-autobuild
> new file mode 100755
> index 0000000..8605533
> --- /dev/null
> +++ b/etc/init.d/buildroot-autobuild
> @@ -0,0 +1,148 @@
> +#!/bin/sh
> +# vim: ft=sh
> +
> +### BEGIN INIT INFO
> +# Provides:          buildroot-autobuild
> +# Required-Start:    $network
> +# Required-Stop:     $network
> +# Default-Start:     2 3 4 5
> +# Default-Stop:      1
> +# Short-Description: Buildroot autobuilds
> +### END INIT INFO
> +
> +# Expected configuration in the configuration file;
> +#   AUTOBUILD_DIR           the absolute path to the shared the autobuild
> +#                           git tree; optional
> +#   AUTOBUILD_DL_DIR        the absolute path to the shared DL dir; optional
> +#   AUTOBUILD_CHROOT        the absolute path to the chroot to run in
> +#   AUTOBUILD_USER          the username to run as
> +#
> +# The following variable is to be interpreted inside the chroot:
> +#   AUTOBUILD_CHROOT_DIR    the absolute path to the directory builds run in
> +
> +CFG_FILE="/etc/default/buildroot-autobuild"
> +
> +if [ ! -e "${CFG_FILE}" ]; then
> +    printf "ERROR: no autobuilder configuration file\n" >&2
> +    exit 1
> +fi
> +. "${CFG_FILE}"
> +if [ -z "${AUTOBUILD_USER}" ]; then
> +    printf "ERROR: no autobuild user\n" >&2
> +    exit 1
> +fi
> +if [ -z "${AUTOBUILD_DIR}" ]; then
> +    printf "ERROR: no autobuild dir\n" >&2
> +    exit 1
> +fi
> +if [ -z "${AUTOBUILD_CHROOT}" ]; then
> +    printf "ERROR: no autobuild chroot\n" >&2
> +    exit 1
> +fi
> +if [ -z "${AUTOBUILD_CHROOT_DIR}" ]; then
> +    printf "ERROR: no autobuild chroot dir\n" >&2
> +    exit 1
> +fi
> +
> +# Derived configuration:
> +#   AUTOBUILD_RUN_DIR   instances will be created in there
> +#   AUTOBUILD_CMD       the autobuild script to run
> +#   AUTOBUILD_CFG       the autobuild runtime configuration
> +AUTOBUILD_RUN_DIR="${AUTOBUILD_CHROOT_DIR}/run"
> +AUTOBUILD_CMD="${AUTOBUILD_CHROOT_DIR}/buildroot-test/scripts/autobuild-run"
> +AUTOBUILD_CFG="${AUTOBUILD_CHROOT_DIR}/buildroot-autobuild.conf"
> +AUTOBUILD_CHROOT_PID_FILE="${AUTOBUILD_CHROOT_DIR}/buildroot-autobuild.pid"
> +AUTOBUILD_PID_FILE="${AUTOBUILD_CHROOT}/${AUTOBUILD_CHROOT_DIR}/buildroot-autobuild.pid"
> +
> +autobuild_start() {
> +    echo "Starting buildroot-autobuild"
> +
> +    CMD="cd '${AUTOBUILD_RUN_DIR}'"
> +    CMD="${CMD}; '${AUTOBUILD_CMD}' -c '${AUTOBUILD_CFG}' --pid-file '${AUTOBUILD_CHROOT_PID_FILE}' &"
> +
> +    do_chroot "rm -rf '${AUTOBUILD_RUN_DIR}'"

Why ? Not only this is unnecessary because the autobuild-run script
automatically removes instance-X/output. But it is actively harmful
because it completely removes the download cache of each instance.

> +    do_chroot "mkdir -p '${AUTOBUILD_RUN_DIR}'"
> +    do_chroot "${CMD}"
> +}
> +
> +autobuild_stop() {
> +    echo "Stopping buildroot-autobuild"
> +    if [ -f "${AUTOBUILD_PID_FILE}" ]; then
> +        kill $(cat "${AUTOBUILD_PID_FILE}")
> +    fi
> +    rm -f "${AUTOBUILD_PID_FILE}"

The rm -f could possibly be within the if, no ?

Other than that, it generally looks good, but maybe
the /etc/default/buildroot-autobuild file needs a few more comments to
explain the expected directory layout and preparations to be done by
the user in order to be able to use this script.

Also, maybe you should make the use of the chroot optional: both of us
are running autobuild-run in a chroot, but not everyone is doing this.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 4/7 v3] etc: add SysV-init startup script and sample config file

[Yann E. MORIN]

Thomas, All,

On 2015-11-28 15:36 +0100, Thomas Petazzoni spake thusly:
> On Fri, 27 Nov 2015 23:39:11 +0100, Yann E. MORIN wrote:
> 
> > diff --git a/etc/default/buildroot-autobuild b/etc/default/buildroot-autobuild
> > new file mode 100644
> > index 0000000..93b7365
> > --- /dev/null
> > +++ b/etc/default/buildroot-autobuild
> > @@ -0,0 +1,23 @@
> > +# The absolute path, outside the chroot, where the
> > +# buildroot-test tree is cloned, optional
> > +AUTOBUILD_DIR="/home/johndoe/buildroot-test"
> 
> What if my buildroot-test tree is just inside the chroot ?

That setting is "optional". See below for how it is used.

> > +# The absolute path, outside the chroot, where to
> > +# share downloads (not yet implemented)
> 
> "not yet implemented" -> why do we have some code about it in the
> script below ?

Damn, I forgot to remove it...

> > +AUTOBUILD_USER="buildroot"
> > +
> > +# The absolute path, in the chroot, that is the base of the
> > +# autobuild work dir. In there, you'll have:
> 
> What does it mean "you'll have" ? Does it mean that the init script
> will set up everything this way, or that it expect things to be already
> set up that way ?

Ok, I should have expanded on it. Here it is:

  - the buildroot-autobuild.conf has to be provided
  - the run/ directory is created
  - the buildroot-test/ directory is either:
    - bind-mounted if AUTOBUILD_DIR is set, or
    - must be already present otherwise

> > +autobuild_start() {
> > +    echo "Starting buildroot-autobuild"
> > +
> > +    CMD="cd '${AUTOBUILD_RUN_DIR}'"
> > +    CMD="${CMD}; '${AUTOBUILD_CMD}' -c '${AUTOBUILD_CFG}' --pid-file '${AUTOBUILD_CHROOT_PID_FILE}' &"
> > +
> > +    do_chroot "rm -rf '${AUTOBUILD_RUN_DIR}'"
> 
> Why ? Not only this is unnecessary because the autobuild-run script
> automatically removes instance-X/output.

Ah, damn. It's in prepare_build(). I missed it.

> > +autobuild_stop() {
> > +    echo "Stopping buildroot-autobuild"
> > +    if [ -f "${AUTOBUILD_PID_FILE}" ]; then
> > +        kill $(cat "${AUTOBUILD_PID_FILE}")
> > +    fi
> > +    rm -f "${AUTOBUILD_PID_FILE}"
> 
> The rm -f could possibly be within the if, no ?

Yes.

> Other than that, it generally looks good, but maybe
> the /etc/default/buildroot-autobuild file needs a few more comments to
> explain the expected directory layout and preparations to be done by
> the user in order to be able to use this script.

Will update.

> Also, maybe you should make the use of the chroot optional: both of us
> are running autobuild-run in a chroot, but not everyone is doing this.

Yes. It will anyway be needed when we implement the multi-chroot setup.

Thanks!

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 5/7 v3] autobuild: avoid infinite loop when sanitising the configuration

[Yann E. MORIN]

There are (hard-to-debug) cases where we can't quickly converge to a
valid configuration. In which case, the sanitising loop is just a plain
infinite loop.

Limit the loop to at most 100 (arbitrary) iterations, so we're not
forever stuck.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 scripts/autobuild-run | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index 397c770..0fce008 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -518,7 +518,14 @@ def gen_config(**kwargs):
 
     # Now, generate the random selection of packages, and fixup
     # things if needed.
+    # Safe-guard, in case we can not quickly come to a valid
+    # configuration: allow at most 100 (arbitrary) iterations.
+    bounded_loop = 100
     while True:
+        if bounded_loop == 0:
+            log_write(log, "ERROR: cannot generate random configuration after 100 iterations")
+            return -1
+        bounded_loop -= 1
         ret = subprocess.call(["make", "O=%s" % outputdir, "-C", srcdir,
                                "KCONFIG_PROBABILITY=%d" % randint(1,30), "randpackageconfig"],
                               stdout=devnull, stderr=devnull)
-- 
1.9.1

[Buildroot] [PATCH 5/7 v3] autobuild: avoid infinite loop when sanitising the configuration

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:12 +0100, Yann E. MORIN wrote:
> There are (hard-to-debug) cases where we can't quickly converge to a
> valid configuration. In which case, the sanitising loop is just a plain
> infinite loop.
> 
> Limit the loop to at most 100 (arbitrary) iterations, so we're not
> forever stuck.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  scripts/autobuild-run | 7 +++++++
>  1 file changed, 7 insertions(+)

Applied to buildroot-test, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 6/7 v3] autobuild: add a function to check if the configuration is fixable

[Yann E. MORIN]

Currently, the fixup_config() function is responsible for two things:
  - checking whether the configuration can be fixed,
  - actually fixing it.

When we have an initial basic configuration, we currently loop
over-and-over again until the configuration is actually fixed (limted to
100 iterations).

However, there are cases where the configuration can *not* be fixed,
because it contains options incompatible with the host system [0], and
wich are not randomised (i.e. looping will never fix the configuration).

Introduce a new function, is_config_fixable() which is responsible for
checking that the initial configuration does not contain any option that
is incompatible with the host system.

That function currently does nothing ands always consider the
configuration to be fixable; actual tests will be added in followup
commits.

[0] like the latest Linaro ARM toolchains which requires a glibc >= 2.14.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 scripts/autobuild-run | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index 0fce008..1c7c91f 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -332,6 +332,15 @@ def prepare_build(**kwargs):
 
     return 0
 
+def is_config_fixable(**kwargs):
+    """Check if the configuration is actually fixable
+
+    This functions checks that the configuration does not contain
+    any unfixable options.
+    """
+
+    return True
+
 def fixup_config(**kwargs):
     """Finalize the configuration and reject any problematic combinations
 
@@ -516,6 +525,10 @@ def gen_config(**kwargs):
         log_write(log, "ERROR: cannot oldconfig")
         return -1
 
+    if not is_config_fixable(**kwargs):
+        log_write(log, "ERROR: configuration is not fixable")
+        return -1
+
     # Now, generate the random selection of packages, and fixup
     # things if needed.
     # Safe-guard, in case we can not quickly come to a valid
-- 
1.9.1

[Buildroot] [PATCH 6/7 v3] autobuild: add a function to check if the configuration is fixable

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:13 +0100, Yann E. MORIN wrote:
> Currently, the fixup_config() function is responsible for two things:
>   - checking whether the configuration can be fixed,
>   - actually fixing it.
> 
> When we have an initial basic configuration, we currently loop
> over-and-over again until the configuration is actually fixed (limted to
> 100 iterations).
> 
> However, there are cases where the configuration can *not* be fixed,
> because it contains options incompatible with the host system [0], and
> wich are not randomised (i.e. looping will never fix the configuration).
> 
> Introduce a new function, is_config_fixable() which is responsible for
> checking that the initial configuration does not contain any option that
> is incompatible with the host system.
> 
> That function currently does nothing ands always consider the
> configuration to be fixable; actual tests will be added in followup
> commits.
> 
> [0] like the latest Linaro ARM toolchains which requires a glibc >= 2.14.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

I've applied, but after doing some fairly significant changes, both to
the code and to the commit log. I didn't like the name
"is_config_fixable", because really it's not about the configuration
being "fixable" or anything like that. It is just about the toolchain
being usable or not. So "is_toolchain_usable" was IMO a much better
choice here.

Also, showing an error in the logs is not appropriate: it is an
expected situation to sometimes have a toolchain that cannot be used.
Frightening the user with an error in this case is not appropriate.

See the final commit at
http://git.buildroot.net/buildroot-test/commit/?id=f762f3630c270bbde27fec0585bb597b472b53f1.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 7/7 v3] autobuild: check for Linaro toolchains earlier

[Yann E. MORIN]

When we get a configuration with those toolchains, we can not fix it.
So we need to abort that configuration.

Fix the glibc version we check against (2.14, not 2.24).

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 scripts/autobuild-run | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/scripts/autobuild-run b/scripts/autobuild-run
index 1c7c91f..55619b6 100755
--- a/scripts/autobuild-run
+++ b/scripts/autobuild-run
@@ -339,6 +339,25 @@ def is_config_fixable(**kwargs):
     any unfixable options.
     """
 
+    idir = "instance-%d" % kwargs['instance']
+    sysinfo = kwargs['sysinfo']
+
+    outputdir = os.path.join(idir, "output")
+    with open(os.path.join(outputdir, ".config")) as configf:
+        configlines = configf.readlines()
+
+    # The latest Linaro toolchains on x86-64 hosts requires glibc
+    # 2.14+ on the host.
+    if platform.machine() == 'x86_64':
+        if 'BR2_TOOLCHAIN_EXTERNAL_LINARO_ARM=y\n' in configlines or \
+           'BR2_TOOLCHAIN_EXTERNAL_LINARO_AARCH64=y\n' in configlines or \
+           'BR2_TOOLCHAIN_EXTERNAL_LINARO_ARMEB=y\n' in configlines:
+            ldd_version_output = subprocess.Popen(['ldd', '--version'], stdout=subprocess.PIPE).communicate()[0]
+            glibc_version = ldd_version_output.splitlines()[0].split()[-1]
+            if StrictVersion('2.14') > StrictVersion(glibc_version):
+                log_write(log, "WARN: ignoring the Linaro ARM toolchains becausee too old host glibc")
+                return False
+
     return True
 
 def fixup_config(**kwargs):
@@ -457,16 +476,6 @@ def fixup_config(**kwargs):
     if 'BR2_PACKAGE_WESTON=y\n' in configlines and \
        'BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/mipsel-ctng-linux-uclibc.tar.xz"\n' in configlines:
         return False
-    # The latest Linaro toolchains on x86-64 hosts requires glibc
-    # 2.24+ on the host.
-    if platform.machine() == 'x86_64':
-        if 'BR2_TOOLCHAIN_EXTERNAL_LINARO_ARM=y\n' in configlines or \
-           'BR2_TOOLCHAIN_EXTERNAL_LINARO_AARCH64=y\n' in configlines or \
-           'BR2_TOOLCHAIN_EXTERNAL_LINARO_ARMEB=y\n' in configlines:
-            ldd_version_output = subprocess.Popen(['ldd', '--version'], stdout=subprocess.PIPE).communicate()[0]
-            glibc_version = ldd_version_output.splitlines()[0].split()[-1]
-            if StrictVersion('2.24') > StrictVersion(glibc_version):
-                return False
 
     with open(os.path.join(outputdir, ".config"), "w+") as configf:
         configf.writelines(configlines)
-- 
1.9.1

[Buildroot] [PATCH 7/7 v3] autobuild: check for Linaro toolchains earlier

[Thomas Petazzoni]

Yann,

On Fri, 27 Nov 2015 23:39:14 +0100, Yann E. MORIN wrote:
> When we get a configuration with those toolchains, we can not fix it.
> So we need to abort that configuration.
> 
> Fix the glibc version we check against (2.14, not 2.24).
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  scripts/autobuild-run | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)

Applied to buildroot-test, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com