Re: [PATCH] packages: seabios: fix payload not being added to the GNU Boot image

[Denis 'GNUtoo' Carikli]

Hi,

Thanks a lot for the patch. For the record it's one of my patches that
broke it.

On Thu, 28 Nov 2024 20:16:13 +0100
Adrien 'neox' Bourmault <neox@gnu.org> wrote:

> A bug has been introduced in a202dce646307f7b0da71da4715f63981a79e9c8
> where we simplified images names without taking care of renaming the
> filename used as the SeaBIOS build target.
Could you also add the commit summary like that:
a202dce646307f7b0da71da4715f63981a79e9c8 ("images: remove 'libgfxinit'
from the image names."). This makes sure that if the commit hash
changes somehow people can still get to the commit. It's also a
practice that it's in use in projects like Linux.

> This error was visible during the generation of the images:
> 
>       payload/seabios/seabios.elf: No such file or directory
>       E: Could not load file 'payload/seabios/seabios.elf'.
>       E: Failed while operating on 'COREBOOT' region!
>       E: The image will be left unmodified.

Right before that there is something important: 
> Creating new ROM image: [the path of an image].

I've doing some bisecting to confirm that it's indeed this commit that
broke SeaBIOS images, and the message above didn't show in all the
cases (I didn't rebuild everything from scratch though). And in this
bisecting understanding that the error message is linked to the path
right above was useful to make test more reliable and faster.

> This commit fixes this bug by setting variables to hold the actual
> payload location (making future changes easier), in the relevant
> files. Also, this commit removes the generation of the unused
> seabios_vgarom.elf image.
Ideally it would be best to have 2 patches for that. Though I'm also OK
if that's included if it's too much work.

> Signed-off-by: Adrien 'neox' Bourmault <neox@gnu.org>
> ---
>  resources/packages/roms_helper/boot | 10 +++++-----
>  resources/packages/seabios/payload  | 16 ++++++----------
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/resources/packages/roms_helper/boot
> b/resources/packages/roms_helper/boot index 07121d8..35d55f4 100755
> --- a/resources/packages/roms_helper/boot
> +++ b/resources/packages/roms_helper/boot
> @@ -135,6 +135,7 @@ fi
>  cbfstool="${cbdir}/util/cbfstool/cbfstool"
>  corebootrom="${cbdir}/build/coreboot.rom"
>  seavgabiosrom="payload/seabios/seavgabios.bin"
> +seabiosrom="payload/seabios/seabios.elf"

>  if [ ! -d "${cbdir}" ]; then
>       ./download coreboot ${cbtree}
> @@ -164,8 +165,7 @@ if [ ! -f "${cbfstool}" ]; then
>  fi
>  
>  if [ ! -f "${seavgabiosrom}" ] \
> -             || [ ! -f payload/seabios/seabios_libgfxinit.elf ] \
> -             || [ ! -f payload/seabios/seabios_vgarom.elf ]; then
> +             || [ ! -f ${seabiosrom} ]; then
>       if [ "${payload_seabios}" = "y" ]; then
>               ./build payload seabios
>       elif [ "${payload_grub}" = "y" ] \
> @@ -266,7 +266,7 @@ mkCoreboot() {
>       printf "%s-%s\n" "$(cat projectname)" "$(cat version)" >
> "${cbdir}/.coreboot-version" (
>               cd "${cbdir}"
> -             make distclean          
> +             make distclean
This looks like a whitespace fix. This should go in another patch.

> -     target_seabioself="payload/seabios/seabios.elf"
> -     target_seavgabios_rom="payload/seabios/seavgabios.bin"
> +     target_seabioself="${seabiosrom}"
> +     target_seavgabios_rom="${seavgabiosrom}"
>  
>       tmprom=$(mktemp -t coreboot_rom.XXXXXXXXXX)
>  
> diff --git a/resources/packages/seabios/payload
> b/resources/packages/seabios/payload index 9120bff..35f54ff 100755
> --- a/resources/packages/seabios/payload
> +++ b/resources/packages/seabios/payload
> @@ -24,6 +24,10 @@ set -u -e
>  # Build SeaBIOS
>  #
> --------------------------------------------------------------------- 
> +# This is where SeaBIOS binaries are expected to be built
> +seabiosrom="../payload/seabios/seabios.elf"
> +seavgabiosrom="../payload/seabios/seavgabios.bin"
Do you know what builds them? If so could you mention it so someone
seeing the above variable definitions will know what other part of the
code also has them hardcoded so that they could be modified together in
the same patch. And in that case adding a comment to point to the other
file to point here would also be needed for the same reasons.

>  printf "Building SeaBIOS payloads and SeaVGABIOS\n"
>  
>  [ ! -d "payload/" ] && mkdir -p payload/
> @@ -42,16 +46,8 @@ cd seabios/
>  cp ../resources/seabios/config/libgfxinit .config
>  make silentoldconfig -j$(nproc)
>  make -j$(nproc)
> -mv out/bios.bin.elf ../payload/seabios/seabios_libgfxinit.elf
> -mv out/vgabios.bin ../payload/seabios/seavgabios.bin
> -rm .config
> -
> -# for vgarom setup:
> -[[ -f Makefile ]] && make distclean
> -cp ../resources/seabios/config/vgarom .config
> -make silentoldconfig -j$(nproc)
> -make -j$(nproc)
> -mv out/bios.bin.elf ../payload/seabios/seabios_vgarom.elf
> +mv out/bios.bin.elf ${seabiosrom}
> +mv out/vgabios.bin ${seavgabiosrom}
>  rm .config
If you can split the patch here that would be great. It would also
improve readability here.

Thanks again for the patch.

Also I didn't build test this patch yet but I'll do it at some point
because I've patches that adds test for making sure that this issue
doesn't come back again, and I'd like to test them as well before
sending them.

Denis.

pgp6N4VaVCMX_.pgp
Description: OpenPGP digital signature