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

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

[James Knight]

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.
---
 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}"
 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
+(
+BINARIES_DIR="${0%/*}/"
+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}"
+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

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

[Yann E. MORIN]

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

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

[Arnout Vandecappelle]

On 10/04/2023 23:11, Yann E. MORIN wrote:
> 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>

[snip]
>> +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" \

  Have you tested this? AFAIK, multiple expressions need a -e to interpret them 
as expressions (now they're interpreted as file names).

>          "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}"

  Global set -e probably would have been useful for this script.

> 
>> +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).

  Should be rather easy since the script is put in BINARIES_DIR.

  Actually, if we assume that the script is indeed not used by anyone except our 
CI, we can put it in HOST_DIR and make finding HOST_DIR even more trivial! Of 
course, finding BINARIES_DIR becomes a little more difficult then :-)


  Regards,
  Arnout

> 
> 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
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot