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

[Arnout Vandecappelle <arnout@mind.be>]

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