Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH] support/scripts: add script to test a package
@ 2017-02-06 18:02 Yann E. MORIN
  2017-02-06 20:40 ` Luca Ceresoli
  2017-02-06 20:44 ` Thomas De Schampheleire
  0 siblings, 2 replies; 10+ messages in thread
From: Yann E. MORIN @ 2017-02-06 18:02 UTC (permalink / raw)
  To: buildroot

This script helps in testing that a pacakge builds fine on a wide range
of architectures and toolchains: BE/LE, 32/64-bit, musl/glibc/uclibc...

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
[yann.morin.1998 at free.fr:
 - completely rewrite the script from Thomas, with help from Luca
]
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
---
 support/scripts/test-pkg | 158 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100755 support/scripts/test-pkg

diff --git a/support/scripts/test-pkg b/support/scripts/test-pkg
new file mode 100755
index 0000000..40b373e
--- /dev/null
+++ b/support/scripts/test-pkg
@@ -0,0 +1,158 @@
+#!/bin/bash
+set -e
+
+TOOLCHAINS_BASE_URL='http://autobuild.buildroot.org/toolchains/configs'
+
+main() {
+    local o O opts
+    local cfg dir pkg toolchain out
+    local -a toolchains
+
+    o='hc:d:p:'
+    O='help,config-snippet:build-dir:package:'
+    opts="$( getopt -n "${my_name}" -o "${o}" -l "${O}" -- "${@}"  )"
+    eval set -- "${opts}"
+
+    while [ ${#} -gt 0 ]; do
+        case "${1}" in
+        (-h|--help)
+            help; exit 0
+            ;;
+        (-c|--config-snippet)
+            cfg="${2}"; shift 2
+            ;;
+        (-d|--build-dir)
+            dir="${2}"; shift 2
+            ;;
+        (-p|--package)
+            pkg="${2}"; shift 2
+            ;;
+        (--)
+            shift; break
+            ;;
+        esac
+    done
+    if [ -z "${cfg}" ]; then
+        printf "error: no config snippet specified\n" 2>&1; exit 1
+    fi
+    if [ -z "${dir}" ]; then
+        dir="${HOME}/br-test-pkg"
+    fi
+
+    # Extract the toolchains names
+    toolchains=( $( curl -s "${TOOLCHAINS_BASE_URL}/toolchain-configs.csv" \
+                    |sed -r -e 's/,.*//; s:.*/(.*)\.config:\1:; /internal/d;'
+                  )
+               )
+
+    if [ ${#toolchains[@]} -eq 0 ]; then
+        printf "error: no toolchain found (networking issue?)\n" 2>&1; exit 1
+    fi
+
+    for toolchain in "${toolchains[@]}"; do
+        build_one "${dir}" "${toolchain}" "${cfg}" "${pkg}"
+    done
+}
+
+build_one() {
+    local dir="${1}"
+    local tc="${2}"
+    local cfg="${3}"
+    local pkg="${4}"
+    local line
+
+    printf "%40s: " "${tc}"
+
+    dir="${dir}/${tc}"
+    mkdir -p "${dir}"
+
+    printf "download config"
+    if ! curl -s "${TOOLCHAINS_BASE_URL}/${toolchain}.config" >"${dir}/.config"; then
+        printf ": FAILED\n"
+        return
+    fi
+
+    cat >>"${dir}/.config" <<-_EOF_
+	BR2_INIT_NONE=y
+	BR2_SYSTEM_BIN_SH_NONE=y
+	# BR2_PACKAGE_BUSYBOX is not set
+	# BR2_TARGET_ROOTFS_TAR is not set
+	_EOF_
+    cat "${cfg}" >>"${dir}/.config"
+
+    printf ", olddefconfig"
+    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
+        printf ": FAILED\n"
+        return
+    fi
+    while read line; do
+        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
+            printf ", SKIPPED\n"
+            return
+        fi
+    done <"${cfg}"
+
+    printf ", source"
+    if ! make O=${dir} source >> ${dir}/logfile 2>&1; then
+        printf ": FAILED\n"
+        return
+    fi
+
+    if [ -n "${pkg}" ]; then
+        printf ", dirclean"
+        if ! make O=${dir} "${pkg}-dirclean" >> ${dir}/logfile 2>&1; then
+            printf ": FAILED\n"
+            return
+        fi
+    fi
+
+    printf ", build"
+    if ! make O=${dir} ${pkg} >> ${dir}/logfile 2>&1; then
+        printf ": FAILED\n"
+        return
+    fi
+
+    printf ": OK\n"
+}
+
+help() {
+    cat <<_EOF_
+${my_name}: test that a package builds with various toolchains and archs
+
+${my_name} will test-build a package (as specified in a .config snippet)
+against various toolchains on different architectures.
+
+The list of toolchains is retrieved from Buidlroot autobuilders.
+
+In case failures are noticed, you have the opportunity to fix the issue
+and relaunch the test.
+
+Options
+
+    -h, --help
+        Print this help.
+
+    -c CFG, --config-snippet CFG
+        Use the CFG file as the source for the config snippet. This file
+        should contain all the config options required to build a package.
+
+    -d DIR, --build-dir DIR
+        Do the builds in directory DIR, one sub-dir per toolchain.
+
+    -p PKG, --package PKG
+        Test-build the package PKG.
+
+Example:
+
+    Testing libcec would require a config snippet that contains:
+        BR2_PACKAGE_LIBCEC=y
+
+    Testing libcurl with openSSL support would require a snippet such as:
+        BR2_PACKAGE_OPENSSL=y
+        BR2_PACKAGE_LIBCURL=y
+
+_EOF_
+}
+
+my_name="${0##*/}"
+main "${@}"
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-06 18:02 [Buildroot] [PATCH] support/scripts: add script to test a package Yann E. MORIN
@ 2017-02-06 20:40 ` Luca Ceresoli
  2017-02-06 20:43   ` Thomas Petazzoni
  2017-02-07  9:33   ` Yann E. MORIN
  2017-02-06 20:44 ` Thomas De Schampheleire
  1 sibling, 2 replies; 10+ messages in thread
From: Luca Ceresoli @ 2017-02-06 20:40 UTC (permalink / raw)
  To: buildroot

Hi,

On 06/02/2017 19:02, Yann E. MORIN wrote:
> This script helps in testing that a pacakge builds fine on a wide range
> of architectures and toolchains: BE/LE, 32/64-bit, musl/glibc/uclibc...
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> [yann.morin.1998 at free.fr:
>  - completely rewrite the script from Thomas, with help from Luca
> ]
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>

I like very much this script and it already works very well. I have
several remarks below, but only one or two are really relevant. But I'd
definitely prefer to see it on master ASAP (i.e. before 2017.02-rc1) and
improve it later.

> diff --git a/support/scripts/test-pkg b/support/scripts/test-pkg
> new file mode 100755
> index 0000000..40b373e
> --- /dev/null
> +++ b/support/scripts/test-pkg
> @@ -0,0 +1,158 @@
[...]
> +    # Extract the toolchains names

I think the correct wording is "toolchain names".

> +    toolchains=( $( curl -s "${TOOLCHAINS_BASE_URL}/toolchain-configs.csv" \
> +                    |sed -r -e 's/,.*//; s:.*/(.*)\.config:\1:; /internal/d;'
> +                  )
> +               )

Detecting internal toolchains from the filename alone seems a bit
fragile. The check would be more robust if we at least fetched the
toolchain config and grepped for "BR2_TOOLCHAIN_EXTERNAL=y" there. Of
course it would make the script more complex, and I'm OK if this is done
in a later step.

> +build_one() {
> +    local dir="${1}"
> +    local tc="${2}"
> +    local cfg="${3}"
> +    local pkg="${4}"
> +    local line
> +
> +    printf "%40s: " "${tc}"
> +
> +    dir="${dir}/${tc}"

Minor nit: why not simply doing in the preamble:

    local tc="${2}"
    local dir="${1}/${tc}"

> +    printf ", olddefconfig"
> +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
> +        printf ": FAILED\n"
> +        return
> +    fi
> +    while read line; do
> +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then

I suggest removing the redirects and using 'grep -q' here.

> +    printf ", source"
> +    if ! make O=${dir} source >> ${dir}/logfile 2>&1; then
> +        printf ": FAILED\n"
> +        return
> +    fi

Splitting the source step from the build step is nice since it clearly
separated download-related issues (wrong URL, network down...) from
build errors.

But as discussed IRL it also means we download the ACTUAL_SOURCE files,
which means hundreds of megabytes with external toolchains. Possible
solutions:

 * remove the "make source" step and let "make ${pkg}" download only the
   sources that are really needed for building
 * remove the "make source" step as above, but only if ${pkg} is empty
 * add a "source-only-filed-needed-for-build" :) target and use that

> +help() {
> +    cat <<_EOF_
> +${my_name}: test that a package builds with various toolchains and archs
> +
> +${my_name} will test-build a package (as specified in a .config snippet)
> +against various toolchains on different architectures.
> +
> +The list of toolchains is retrieved from Buidlroot autobuilders.

Buidlroot -> Buildroot

Also why not appending "available at ${TOOLCHAINS_BASE_URL}"?

> +In case failures are noticed, you have the opportunity to fix the issue
> +and relaunch the test.
> +
> +Options
> +
> +    -h, --help
> +        Print this help.
> +
> +    -c CFG, --config-snippet CFG
> +        Use the CFG file as the source for the config snippet. This file
> +        should contain all the config options required to build a package.
> +
> +    -d DIR, --build-dir DIR
> +        Do the builds in directory DIR, one sub-dir per toolchain.
> +
> +    -p PKG, --package PKG
> +        Test-build the package PKG.

Add: 'if not specified, runs "make" without a target'

-- 
Luca

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-06 20:40 ` Luca Ceresoli
@ 2017-02-06 20:43   ` Thomas Petazzoni
  2017-02-06 20:59     ` Luca Ceresoli
  2017-02-07  9:33   ` Yann E. MORIN
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Petazzoni @ 2017-02-06 20:43 UTC (permalink / raw)
  To: buildroot

Hello,

On Mon, 6 Feb 2017 21:40:58 +0100, Luca Ceresoli wrote:

> Splitting the source step from the build step is nice since it clearly
> separated download-related issues (wrong URL, network down...) from
> build errors.
> 
> But as discussed IRL it also means we download the ACTUAL_SOURCE files,
> which means hundreds of megabytes with external toolchains. Possible
> solutions:
> 
>  * remove the "make source" step and let "make ${pkg}" download only the
>    sources that are really needed for building
>  * remove the "make source" step as above, but only if ${pkg} is empty
>  * add a "source-only-filed-needed-for-build" :) target and use that

I don't really see the need for separating the build step from the
source step in this script. Why not just do the build, and that's it?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-06 18:02 [Buildroot] [PATCH] support/scripts: add script to test a package Yann E. MORIN
  2017-02-06 20:40 ` Luca Ceresoli
@ 2017-02-06 20:44 ` Thomas De Schampheleire
  2017-02-07  9:52   ` Yann E. MORIN
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas De Schampheleire @ 2017-02-06 20:44 UTC (permalink / raw)
  To: buildroot

On Mon, Feb 6, 2017 at 7:02 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> This script helps in testing that a pacakge builds fine on a wide range

package

> of architectures and toolchains: BE/LE, 32/64-bit, musl/glibc/uclibc...
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> [yann.morin.1998 at free.fr:
>  - completely rewrite the script from Thomas, with help from Luca
> ]
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  support/scripts/test-pkg | 158 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100755 support/scripts/test-pkg
>
> diff --git a/support/scripts/test-pkg b/support/scripts/test-pkg
> new file mode 100755
> index 0000000..40b373e
> --- /dev/null
> +++ b/support/scripts/test-pkg
> @@ -0,0 +1,158 @@
> +#!/bin/bash
> +set -e
> +
> +TOOLCHAINS_BASE_URL='http://autobuild.buildroot.org/toolchains/configs'
> +
> +main() {
> +    local o O opts
> +    local cfg dir pkg toolchain out
> +    local -a toolchains
> +
> +    o='hc:d:p:'
> +    O='help,config-snippet:build-dir:package:'
> +    opts="$( getopt -n "${my_name}" -o "${o}" -l "${O}" -- "${@}"  )"
> +    eval set -- "${opts}"
> +
> +    while [ ${#} -gt 0 ]; do
> +        case "${1}" in
> +        (-h|--help)
> +            help; exit 0
> +            ;;
> +        (-c|--config-snippet)
> +            cfg="${2}"; shift 2
> +            ;;
> +        (-d|--build-dir)
> +            dir="${2}"; shift 2
> +            ;;
> +        (-p|--package)
> +            pkg="${2}"; shift 2
> +            ;;
> +        (--)
> +            shift; break
> +            ;;
> +        esac
> +    done
> +    if [ -z "${cfg}" ]; then
> +        printf "error: no config snippet specified\n" 2>&1; exit 1
> +    fi
> +    if [ -z "${dir}" ]; then
> +        dir="${HOME}/br-test-pkg"
> +    fi
> +
> +    # Extract the toolchains names
> +    toolchains=( $( curl -s "${TOOLCHAINS_BASE_URL}/toolchain-configs.csv" \
> +                    |sed -r -e 's/,.*//; s:.*/(.*)\.config:\1:; /internal/d;'
> +                  )
> +               )

I always find it useful that such text transformation code carries a
short comment giving example input and output.

> +
> +    if [ ${#toolchains[@]} -eq 0 ]; then
> +        printf "error: no toolchain found (networking issue?)\n" 2>&1; exit 1

What is the purpose of 2>&1 ? Don't you mean >&2 , i.e. send the print
command to stderr ?
Same for other error prints in this file.

> +    fi
> +
> +    for toolchain in "${toolchains[@]}"; do
> +        build_one "${dir}" "${toolchain}" "${cfg}" "${pkg}"
> +    done
> +}
> +
> +build_one() {
> +    local dir="${1}"
> +    local tc="${2}"

Pretty cryptic, 'tc' for 'toolchain, IMO.

> +    local cfg="${3}"
> +    local pkg="${4}"
> +    local line
> +
> +    printf "%40s: " "${tc}"
> +
> +    dir="${dir}/${tc}"
> +    mkdir -p "${dir}"
> +
> +    printf "download config"
> +    if ! curl -s "${TOOLCHAINS_BASE_URL}/${toolchain}.config" >"${dir}/.config"; then

Here you use 'toolchain' iso 'tc', which is the loop variable used
before calling this function. This works 'by luck', but is not the
intended design.

> +        printf ": FAILED\n"
> +        return
> +    fi
> +
> +    cat >>"${dir}/.config" <<-_EOF_
> +       BR2_INIT_NONE=y
> +       BR2_SYSTEM_BIN_SH_NONE=y
> +       # BR2_PACKAGE_BUSYBOX is not set
> +       # BR2_TARGET_ROOTFS_TAR is not set
> +       _EOF_
> +    cat "${cfg}" >>"${dir}/.config"
> +
> +    printf ", olddefconfig"
> +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
> +        printf ": FAILED\n"
> +        return
> +    fi
> +    while read line; do
> +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
> +            printf ", SKIPPED\n"
> +            return
> +        fi
> +    done <"${cfg}"

I think a brief comment of what is happening before each block would be useful.
What is happening here? You try to set the config provided by the user
and then check if it worked? If that understanding is correct,
couldn't the 'SKIPPED' message not be more verbose in explaining _why_
it was skipped? A short explanation, and the line/config option that
caused this skip.

> +
> +    printf ", source"
> +    if ! make O=${dir} source >> ${dir}/logfile 2>&1; then
> +        printf ": FAILED\n"
> +        return
> +    fi
> +
> +    if [ -n "${pkg}" ]; then
> +        printf ", dirclean"
> +        if ! make O=${dir} "${pkg}-dirclean" >> ${dir}/logfile 2>&1; then
> +            printf ": FAILED\n"
> +            return
> +        fi
> +    fi
> +
> +    printf ", build"
> +    if ! make O=${dir} ${pkg} >> ${dir}/logfile 2>&1; then
> +        printf ": FAILED\n"
> +        return
> +    fi
> +
> +    printf ": OK\n"
> +}
> +
> +help() {
> +    cat <<_EOF_
> +${my_name}: test that a package builds with various toolchains and archs

I would start discussing about 'archs' vs 'arches' so perhaps we
should just use 'architectures'.

> +
> +${my_name} will test-build a package (as specified in a .config snippet)
> +against various toolchains on different architectures.
> +
> +The list of toolchains is retrieved from Buidlroot autobuilders.
> +
> +In case failures are noticed, you have the opportunity to fix the issue
> +and relaunch the test.

I find it a bit odd to 'get the opportunity to fix', isn't this
obvious that if it failed you need to fix and retest?

> +
> +Options
> +
> +    -h, --help
> +        Print this help.
> +
> +    -c CFG, --config-snippet CFG
> +        Use the CFG file as the source for the config snippet. This file
> +        should contain all the config options required to build a package.
> +
> +    -d DIR, --build-dir DIR
> +        Do the builds in directory DIR, one sub-dir per toolchain.
> +
> +    -p PKG, --package PKG
> +        Test-build the package PKG.
> +
> +Example:
> +
> +    Testing libcec would require a config snippet that contains:
> +        BR2_PACKAGE_LIBCEC=y
> +
> +    Testing libcurl with openSSL support would require a snippet such as:
> +        BR2_PACKAGE_OPENSSL=y
> +        BR2_PACKAGE_LIBCURL=y
> +
> +_EOF_
> +}
> +
> +my_name="${0##*/}"
> +main "${@}"
> --
> 2.7.4
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-06 20:43   ` Thomas Petazzoni
@ 2017-02-06 20:59     ` Luca Ceresoli
  0 siblings, 0 replies; 10+ messages in thread
From: Luca Ceresoli @ 2017-02-06 20:59 UTC (permalink / raw)
  To: buildroot

Hi,

On 06/02/2017 21:43, Thomas Petazzoni wrote:
> Hello,
> 
> On Mon, 6 Feb 2017 21:40:58 +0100, Luca Ceresoli wrote:
> 
>> Splitting the source step from the build step is nice since it clearly
>> separated download-related issues (wrong URL, network down...) from
>> build errors.
>>
>> But as discussed IRL it also means we download the ACTUAL_SOURCE files,
>> which means hundreds of megabytes with external toolchains. Possible
>> solutions:
>>
>>  * remove the "make source" step and let "make ${pkg}" download only the
>>    sources that are really needed for building
>>  * remove the "make source" step as above, but only if ${pkg} is empty
>>  * add a "source-only-filed-needed-for-build" :) target and use that
> 
> I don't really see the need for separating the build step from the
> source step in this script. Why not just do the build, and that's it?

For two reasons:

 1. catch wrong URLs early, before building all dependencies
 2. make URL errors stand out clearly, thus being more informative

I agree these are not crucial to the goal of the script. They are "nice
to have", so I won't complain if we get rid of this step.

-- 
Luca

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-06 20:40 ` Luca Ceresoli
  2017-02-06 20:43   ` Thomas Petazzoni
@ 2017-02-07  9:33   ` Yann E. MORIN
  2017-02-07 10:48     ` Luca Ceresoli
  2017-02-07 14:41     ` Peter Korsgaard
  1 sibling, 2 replies; 10+ messages in thread
From: Yann E. MORIN @ 2017-02-07  9:33 UTC (permalink / raw)
  To: buildroot

Luca, All,

On 2017-02-06 21:40 +0100, Luca Ceresoli spake thusly:
> On 06/02/2017 19:02, Yann E. MORIN wrote:
[--SNIP--]
> > +    printf ", olddefconfig"
> > +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
> > +        printf ": FAILED\n"
> > +        return
> > +    fi
> > +    while read line; do
> > +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
> 
> I suggest removing the redirects and using 'grep -q' here.

grep -q is not POSIX. ;-)

> > +    printf ", source"
> > +    if ! make O=${dir} source >> ${dir}/logfile 2>&1; then
> > +        printf ": FAILED\n"
> > +        return
> > +    fi
> 
> Splitting the source step from the build step is nice since it clearly
> separated download-related issues (wrong URL, network down...) from
> build errors.
> 
> But as discussed IRL it also means we download the ACTUAL_SOURCE files,
> which means hundreds of megabytes with external toolchains. Possible
> solutions:
> 
>  * remove the "make source" step and let "make ${pkg}" download only the
>    sources that are really needed for building
>  * remove the "make source" step as above, but only if ${pkg} is empty
>  * add a "source-only-filed-needed-for-build" :) target and use that

I'll remove the separate source action, it makes it simpler.

> > +help() {
> > +    cat <<_EOF_
> > +${my_name}: test that a package builds with various toolchains and archs
> > +
> > +${my_name} will test-build a package (as specified in a .config snippet)
> > +against various toolchains on different architectures.
> > +
> > +The list of toolchains is retrieved from Buidlroot autobuilders.
> 
> Buidlroot -> Buildroot

Buidlroot sounds more like Beetlejuice. ;-)

> Also why not appending "available at ${TOOLCHAINS_BASE_URL}"?

Can do.

> > +    -p PKG, --package PKG
> > +        Test-build the package PKG.
> 
> Add: 'if not specified, runs "make" without a target'

Can do.

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-06 20:44 ` Thomas De Schampheleire
@ 2017-02-07  9:52   ` Yann E. MORIN
  0 siblings, 0 replies; 10+ messages in thread
From: Yann E. MORIN @ 2017-02-07  9:52 UTC (permalink / raw)
  To: buildroot

Thomas DS, All,

On 2017-02-06 21:44 +0100, Thomas De Schampheleire spake thusly:
> On Mon, Feb 6, 2017 at 7:02 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > This script helps in testing that a pacakge builds fine on a wide range
> 
> package

Meh... ;-)

> > +        printf "error: no toolchain found (networking issue?)\n" 2>&1; exit 1
> What is the purpose of 2>&1 ? Don't you mean >&2 , i.e. send the print
> command to stderr ?

Yup...

> > +    local tc="${2}"
> Pretty cryptic, 'tc' for 'toolchain, IMO.
[--SNIP--]
> > +    if ! curl -s "${TOOLCHAINS_BASE_URL}/${toolchain}.config" >"${dir}/.config"; then
> Here you use 'toolchain' iso 'tc', which is the loop variable used
> before calling this function. This works 'by luck', but is not the
> intended design.

Fixed.

> > +    printf ", olddefconfig"
> > +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
> > +        printf ": FAILED\n"
> > +        return
> > +    fi
> > +    while read line; do
> > +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
> > +            printf ", SKIPPED\n"
> > +            return
> > +        fi
> > +    done <"${cfg}"
> 
> I think a brief comment of what is happening before each block would be useful.
> What is happening here? You try to set the config provided by the user
> and then check if it worked?

Would the following be OK? ;-)

    We want all the options from the snippet to be present as-is (=y or
    not set) in the actual .config; if one of them is not, it means some
    dependency from the toolchain or arch is not available, in which
    case this config is untestable and we skip it.

> If that understanding is correct,
> couldn't the 'SKIPPED' message not be more verbose in explaining _why_
> it was skipped? A short explanation, and the line/config option that
> caused this skip.

I'd like to keep the output as simple as possible. The missing stuff can
be stored in a file in the output dir.

> > +${my_name}: test that a package builds with various toolchains and archs
> 
> I would start discussing about 'archs' vs 'arches' so perhaps we
> should just use 'architectures'.

OK.

> > +${my_name} will test-build a package (as specified in a .config snippet)
> > +against various toolchains on different architectures.
> > +
> > +The list of toolchains is retrieved from Buidlroot autobuilders.
> > +
> > +In case failures are noticed, you have the opportunity to fix the issue
> > +and relaunch the test.
> 
> I find it a bit odd to 'get the opportunity to fix', isn't this
> obvious that if it failed you need to fix and retest?

I'll try to rephrase that.

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-07  9:33   ` Yann E. MORIN
@ 2017-02-07 10:48     ` Luca Ceresoli
  2017-02-07 14:41     ` Peter Korsgaard
  1 sibling, 0 replies; 10+ messages in thread
From: Luca Ceresoli @ 2017-02-07 10:48 UTC (permalink / raw)
  To: buildroot

Hi Yann, Thomas,

On 07/02/2017 10:33, Yann E. MORIN wrote:
> Luca, All,
> 
> On 2017-02-06 21:40 +0100, Luca Ceresoli spake thusly:
>> On 06/02/2017 19:02, Yann E. MORIN wrote:
> [--SNIP--]
>>> +    printf ", olddefconfig"
>>> +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
>>> +        printf ": FAILED\n"
>>> +        return
>>> +    fi
>>> +    while read line; do
>>> +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
>>
>> I suggest removing the redirects and using 'grep -q' here.
> 
> grep -q is not POSIX. ;-)

:-(

In the meantime I finished building one package (exim) with all
toolchains, with and without '-p exim'. I also introduced various
errors: wrong URL, wrong build/configure commands, wrong config snippet,
network disconnection.

It all work as expected. So, with the discussed fixed, I expect to add
my Acked and Tested tags.

Thanks.
-- 
Luca

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-07  9:33   ` Yann E. MORIN
  2017-02-07 10:48     ` Luca Ceresoli
@ 2017-02-07 14:41     ` Peter Korsgaard
  2017-02-07 14:46       ` Yann E. MORIN
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Korsgaard @ 2017-02-07 14:41 UTC (permalink / raw)
  To: buildroot

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Luca, All,
 > On 2017-02-06 21:40 +0100, Luca Ceresoli spake thusly:
 >> On 06/02/2017 19:02, Yann E. MORIN wrote:
 > [--SNIP--]
 >> > +    printf ", olddefconfig"
 >> > +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
 >> > +        printf ": FAILED\n"
 >> > +        return
 >> > +    fi
 >> > +    while read line; do
 >> > +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
 >> 
 >> I suggest removing the redirects and using 'grep -q' here.

 > grep -q is not POSIX. ;-)

But is that a real concern? We do use grep -q quite a bit already
(including in dependencies.sh):

git grep 'grep -q' | wc -l
46

-- 
Bye, Peter Korsgaard

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Buildroot] [PATCH] support/scripts: add script to test a package
  2017-02-07 14:41     ` Peter Korsgaard
@ 2017-02-07 14:46       ` Yann E. MORIN
  0 siblings, 0 replies; 10+ messages in thread
From: Yann E. MORIN @ 2017-02-07 14:46 UTC (permalink / raw)
  To: buildroot

Peter, All,

On 2017-02-07 15:41 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
>  > Luca, All,
>  > On 2017-02-06 21:40 +0100, Luca Ceresoli spake thusly:
>  >> On 06/02/2017 19:02, Yann E. MORIN wrote:
>  > [--SNIP--]
>  >> > +    printf ", olddefconfig"
>  >> > +    if ! make O="${dir}" olddefconfig >/dev/null 2>&1; then
>  >> > +        printf ": FAILED\n"
>  >> > +        return
>  >> > +    fi
>  >> > +    while read line; do
>  >> > +        if ! grep "^${line}\$" "${dir}/.config" >/dev/null 2>&1; then
>  >> 
>  >> I suggest removing the redirects and using 'grep -q' here.
> 
>  > grep -q is not POSIX. ;-)
> 
> But is that a real concern?

No, of course it is not. But I prefer not having non-portable stuff when
the portable solution is pretty easy and straightforward... Personal
preference...

Regards,
Yann E. MORIN.

> We do use grep -q quite a bit already
> (including in dependencies.sh):
> 
> git grep 'grep -q' | wc -l
> 46
> 
> -- 
> Bye, Peter Korsgaard

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-02-07 14:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-06 18:02 [Buildroot] [PATCH] support/scripts: add script to test a package Yann E. MORIN
2017-02-06 20:40 ` Luca Ceresoli
2017-02-06 20:43   ` Thomas Petazzoni
2017-02-06 20:59     ` Luca Ceresoli
2017-02-07  9:33   ` Yann E. MORIN
2017-02-07 10:48     ` Luca Ceresoli
2017-02-07 14:41     ` Peter Korsgaard
2017-02-07 14:46       ` Yann E. MORIN
2017-02-06 20:44 ` Thomas De Schampheleire
2017-02-07  9:52   ` Yann E. MORIN

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox