Re: [Buildroot] [PATCH v2 1/3] board/qemu: define start qemu script outside of post-image script

["Yann E. MORIN" <yann.morin.1998@free.fr>]

James, All,

On 2023-04-07 01:21 -0400, James Knight spake thusly:
> The following moves the definition of the QEMU board's `start-qemu.sh`
> helper script from being inlined in the post-image script into its own
> file. This should, in theory, make it easier to maintain the script in
> the future.
> 
> Signed-off-by: James Knight <james.d.knight@live.com>
> ---
> Changes v1 -> v2:
>   - Split original into three patches (suggested by Arnout).
>   - This patch moves cat script into a new 'start-qemu.sh.in'
>     file (suggested by Arnout).
>   - Script template dropped escape characters; uses VAR_ prefixes.

Why? The usual way to sed-n-replace is by using a placeholder for the
variable, like:

    do something @VARIABLE@

which gets replaced with something like:

    sed "s|@VARIABLE@|${VARIABLE}|"

and since we are already doing this in numerous places in Buildroot, it
is only logical to do it here too.

> ---
>  board/qemu/post-image.sh    | 21 ++++-----------------
>  board/qemu/start-qemu.sh.in | 14 ++++++++++++++
>  2 files changed, 18 insertions(+), 17 deletions(-)
>  create mode 100644 board/qemu/start-qemu.sh.in
> 
> diff --git a/board/qemu/post-image.sh b/board/qemu/post-image.sh
> index 88f04134961ea7a105e506045ca16a0d9b810925..c9043a1364e8e03dbfa9ff3370d4e4a34357bef5 100755
> --- a/board/qemu/post-image.sh
> +++ b/board/qemu/post-image.sh
> @@ -41,21 +41,8 @@ case ${DEFCONFIG_NAME} in
>      ;;
>  esac
>  
> -cat <<-_EOF_ > "${START_QEMU_SCRIPT}"
> -	#!/bin/sh
> -	(
> -	BINARIES_DIR="\${0%/*}/"
> -	cd \${BINARIES_DIR}
> -
> -	if [ "\${1}" = "serial-only" ]; then
> -	    EXTRA_ARGS='${SERIAL_ARGS}'
> -	else
> -	    EXTRA_ARGS='${DEFAULT_ARGS}'
> -	fi
> -
> -	export PATH="${HOST_DIR}/bin:\${PATH}"
> -	exec ${QEMU_CMD_LINE} \${EXTRA_ARGS}
> -	)
> -_EOF_
> -
> +cp "${QEMU_BOARD_DIR}/start-qemu.sh.in" "${START_QEMU_SCRIPT}"
> +sed -i "s|VAR_DEFAULT_ARGS|${DEFAULT_ARGS}|g" "${START_QEMU_SCRIPT}"
> +sed -i "s|VAR_QEMU_CMD_LINE|${QEMU_CMD_LINE}|g" "${START_QEMU_SCRIPT}"
> +sed -i "s|VAR_SERIAL_ARGS|${SERIAL_ARGS}|g" "${START_QEMU_SCRIPT}"

This can all be done with a single call to sed, without cp either:

    sed "s|@SERIAL_ARGS@|${SERIAL_ARGS}|g" \
        "s|@DEFAULT_ARGS@|${DEFAULT_ARGS}|g" \
        "s|@QEMU_CMD_LINE@|${QEMU_CMD_LINE}|g" \
        <"${QEMU_BOARD_DIR}/start-qemu.sh.in" \
        >"${START_QEMU_SCRIPT}"

>  chmod +x "${START_QEMU_SCRIPT}"
> diff --git a/board/qemu/start-qemu.sh.in b/board/qemu/start-qemu.sh.in
> new file mode 100644
> index 0000000000000000000000000000000000000000..c2d77734c7a6b318a5f7adedfd9b0b5875e84f59
> --- /dev/null
> +++ b/board/qemu/start-qemu.sh.in
> @@ -0,0 +1,14 @@
> +#!/bin/sh
> +(

No need for a sub-shell.

> +BINARIES_DIR="${0%/*}/"
> +cd ${BINARIES_DIR}

    $ shellcheck board/qemu/start-qemu.sh.in
    In board/qemu/start-qemu.sh.in line 4:
    cd ${BINARIES_DIR}
    ^----------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
       ^-------------^ SC2086: Double quote to prevent globbing and word splitting.

The first is spurious (BINARIES_DIR is eaxctly where the running shell
is), so ignoreit , but the second is valid, so:
    # shellcheck disable=SC2164
    cd "${BINARIES_DIR}"

> +if [ "${1}" = "serial-only" ]; then
> +    EXTRA_ARGS='VAR_SERIAL_ARGS'
> +else
> +    EXTRA_ARGS='VAR_DEFAULT_ARGS'
> +fi
> +
> +export PATH="${HOST_DIR}/bin:${PATH}"

This is the pain point: what is going to set HOST_DIR when this script
is called? I think it should be substituted like the other variables, so
this is what I did. The script can't be easily relocated, but that was
not the goal for this script to be relocatable so far (it was only ever
used in our CI), so making it relocatable can be done later (if
possible).

Applied to master with all the above fixed, thanks.

Regards,
Yann E. MORIN.

> +exec VAR_QEMU_CMD_LINE ${EXTRA_ARGS}
> +)
> -- 
> 2.39.1.windows.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot