[Buildroot] [PATCH v2 1/3] board/qemu: define start qemu script outside of post-image script
Arnout Vandecappelle
arnout at mind.be
Tue Apr 11 07:05:58 UTC 2023
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 at 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 at buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
>
More information about the buildroot
mailing list