Remove hardcoded architecture-dependend strings from EFI code #3157

pcahyna · 2024-02-19T13:12:39Z

Type: Enhancement
Impact: Normal
Reference to related issue (URL):
How was this pull request tested?
Description of the changes in this pull request:

Introduce variables EFI_ARCH{_UPPER} and GRUB2_IMAGE_FORMAT. Use them for various EFI bootloader suffixes, parameters and paths instead of hardcoding strings like BOOTX64.EFI. EFI_ARCH is "x64" and EFI_ARCH_UPPER is "X64" and GRUB2_IMAGE_FORMAT is "x86_64-efi" on x86_64 architecture.

Should make it easier to port the code to e.g. Arm.

See e.g.
https://github.com/rhboot/shim/blob/main/Make.defaults
for possible values.

Use them for various EFI bootloader suffixes instead of hardcoding strings like BOOTX64.EFI. EFI_ARCH is "x64" and EFI_ARCH_UPPER is "X64" on x86_64 architecture. Should make it easier to port the code to e.g. Arm. See e.g. https://github.com/rhboot/shim/blob/main/Make.defaults for possible values.

lzaoral · 2024-02-19T14:17:32Z

Thank you a lot, @pcahyna! This will make the EFI support on aarch64 I have implemented locally. much easier to upstream!

usr/share/rear/prep/Linux-i386/330_set_efi_arch.sh

lzaoral · 2024-02-28T09:21:47Z

@pcahyna Could you generalise the build_bootx86_efi function as well, please?

rear/usr/share/rear/lib/uefi-functions.sh

Lines 39 to 126 in 8564a7b

    
           function build_bootx86_efi { 
        
               local outfile="$1" 
        
               local embedded_config="" 
        
               local gmkstandalone="" 
        
               local gprobe="" 
        
               local dirs=() 
        
               # modules is the list of modules to load 
        
               # If GRUB2_MODULES_UEFI_LOAD is nonempty, it determines what modules to load 
        
               local modules=( ${GRUB2_MODULES_UEFI_LOAD:+"${GRUB2_MODULES_UEFI_LOAD[@]}"} ) 
        
               # Configuration file is optional for image creation. 
        
               shift 
        
               if [[ -n "$1" ]] ; then 
        
                   # graft point syntax. $1 will appear as /boot/grub/grub.cfg in the image 
        
                   embedded_config="/boot/grub/grub.cfg=$1" 
        
                   shift 
        
                   # directories that should be accessible by GRUB2 (e.g. because they contain the kernel) 
        
                   dirs=( ${@:+"$@"} ) 
        
               fi 
        
               if has_binary grub-mkstandalone ; then 
        
                   gmkstandalone=grub-mkstandalone 
        
               elif has_binary grub2-mkstandalone ; then 
        
                   # At least SUSE systems use 'grub2' prefixed names for GRUB2 programs: 
        
                   gmkstandalone=grub2-mkstandalone 
        
               else 
        
                   # This build_bootx86_efi function is only called in output/ISO/Linux-i386/250_populate_efibootimg.sh 
        
                   # and output/USB/Linux-i386/100_create_efiboot.sh and output/default/940_grub2_rescue.sh 
        
                   # only if UEFI is used so that we simply error out here if we cannot make a bootable EFI image of GRUB2 
        
                   # (normally a function should not exit but return to its caller with a non-zero return code): 
        
                   Error "Cannot make bootable EFI image of GRUB2 (neither grub-mkstandalone nor grub2-mkstandalone found)" 
        
               fi 
        
               # Determine what modules need to be loaded in order to access given directories 
        
               # (if the list of modules is not overriden by GRUB2_MODULES_UEFI_LOAD) 
        
               if (( ${#dirs[@]} )) && ! (( ${#modules[@]} )) ; then 
        
                   if has_binary grub-probe ; then 
        
                       gprobe=grub-probe 
        
                   elif has_binary grub2-probe ; then 
        
                       # At least SUSE systems use 'grub2' prefixed names for GRUB2 programs: 
        
                       gprobe=grub2-probe 
        
                   else 
        
                       LogPrint "Neither grub-probe nor grub2-probe found" 
        
                       # Since openSUSE Leap 15.1 things were moved from /usr/lib/grub2/ to /usr/share/grub2/ 
        
                       # cf. https://github.com/rear/rear/issues/2338#issuecomment-594432946 
        
                       if test /usr/*/grub*/x86_64-efi/partmap.lst ; then 
        
                           LogPrint "including all partition modules" 
        
                           modules=( $( cat /usr/*/grub*/x86_64-efi/partmap.lst ) ) 
        
                       else 
        
                           Error "Can not determine partition modules, ${dirs[*]} would be likely inaccessible in GRUB2" 
        
                       fi 
        
                   fi 
        
                   if [ -n "$gprobe" ]; then 
        
                       # This is unfortunately only a crude approximation of the Grub internal probe_mods() function. 
        
                       # $gprobe --target=partmap "$p" | sed -e 's/^/part_/' does not always returns part_msdos 
        
                       # Therefore, we explicit do an echo 'part_msdos' (the sort -u will make sure it is listed only once) 
        
                       modules=( $( for p in "${dirs[@]}" ; do 
        
                                        $gprobe --target=fs "$p" 
        
                                        $gprobe --target=partmap "$p" | sed -e 's/^/part_/' 
        
                                        echo 'part_msdos' 
        
                                        $gprobe --target=abstraction "$p" 
        
                                    done | sort -u ) ) 
        
                   fi 
        
               fi 
        
               # grub-mkstandalone needs a .../grub*/x86_64-efi/moddep.lst file (cf. https://github.com/rear/rear/issues/1193) 
        
               # At least on SUSE systems that is in different 'grub2' directories (cf. https://github.com/rear/rear/issues/2338) 
        
               # e.g. on openSUSE Leap 15.0 it is in /usr/lib/grub2/x86_64-efi/moddep.lst 
        
               # but on openSUSE Leap 15.1 that was moved to /usr/share/grub2/x86_64-efi/moddep.lst 
        
               # and the one in /boot/grub2/x86_64-efi/moddep.lst is a copy of the one in /usr/*/grub2/x86_64-efi/moddep.lst 
        
               # so we do not error out if we do not find a /x86_64-efi/moddep.lst file because it could be "anywhere else" in the future 
        
               # but we inform the user here in advance about possible problems when there is no /x86_64-efi/moddep.lst file. 
        
               # Careful: usr/sbin/rear sets nullglob so that /usr/*/grub*/x86_64-efi/moddep.lst gets empty if nothing matches 
        
               # and 'test -f' succeeds with empty argument so that we cannot use 'test -f /usr/*/grub*/x86_64-efi/moddep.lst' 
        
               # also 'test -n' succeeds with empty argument but (fortunately/intentionally?) plain 'test' fails with empty argument. 
        
               # Another implicit condition that this 'test' works is that '/usr/*/grub*/x86_64-efi/moddep.lst' matches at most one file 
        
               # because otherwise e.g. "test /usr/*/grub*/x86_64-efi/mod*" where two files moddep.lst and modinfo.sh match 
        
               # would falsely fail with "bash: test: ... unary operator expected": 
        
               test /usr/*/grub*/x86_64-efi/moddep.lst || LogPrintError "$gmkstandalone may fail to make a bootable EFI image of GRUB2 (no /usr/*/grub*/x86_64-efi/moddep.lst file)" 
        
               (( ${#GRUB2_MODULES_UEFI[@]} )) && LogPrint "Installing only ${GRUB2_MODULES_UEFI[*]} modules into $outfile memdisk" 
        
               (( ${#modules[@]} )) && LogPrint "GRUB2 modules to load: ${modules[*]}" 
        
               if ! $gmkstandalone $v ${GRUB2_MODULES_UEFI:+"--install-modules=${GRUB2_MODULES_UEFI[*]}"} ${modules:+"--modules=${modules[*]}"} -O x86_64-efi -o $outfile $embedded_config ; then 
        
                   Error "Failed to make bootable EFI image of GRUB2 (error during $gmkstandalone of $outfile)" 
        
               fi 
        
           }

It hardcodes the x86_64 architecture and the name itself is quite misleading because x86 is not the same thing as x86_64. Note that GRUB2 uses the arm64-efi directory on aarch64 Fedora so plain uname -m won't work.

edit: typos

lzaoral · 2024-02-28T12:50:36Z

There is still one instance of BOOTX64.efi left:

rear/usr/share/rear/output/ISO/Linux-i386/260_EFISTUB_populate.sh

Line 36 in 48ebbf9

cp $v "$OUTPUT_EFISTUB_SYSTEMD_BOOTLOADER" $efi_boot_tmp_dir/BOOTX64.efi

Use it as the argument of the -O option to grub-mkstandalone/grub-mkimage instead of the hardcoded x86_64-efi. For easier porting to non-x86_64 EFI platforms.

Mostly for completeness and documentation of the possible values.

pcahyna · 2024-03-04T15:22:34Z

@pcahyna Could you generalise the build_bootx86_efi function as well, please?

rear/usr/share/rear/lib/uefi-functions.sh

Lines 39 to 126 in 8564a7b

function build_bootx86_efi {

local outfile="$1"

local embedded_config=""

local gmkstandalone=""

local gprobe=""

local dirs=()

# modules is the list of modules to load

# If GRUB2_MODULES_UEFI_LOAD is nonempty, it determines what modules to load

local modules=( ${GRUB2_MODULES_UEFI_LOAD:+"${GRUB2_MODULES_UEFI_LOAD[@]}"} )

# Configuration file is optional for image creation.

shift

if [[ -n "$1" ]] ; then

# graft point syntax. $1 will appear as /boot/grub/grub.cfg in the image

embedded_config="/boot/grub/grub.cfg=$1"

shift

# directories that should be accessible by GRUB2 (e.g. because they contain the kernel)

dirs=( ${@:+"$@"} )

fi

if has_binary grub-mkstandalone ; then

gmkstandalone=grub-mkstandalone

elif has_binary grub2-mkstandalone ; then

# At least SUSE systems use 'grub2' prefixed names for GRUB2 programs:

gmkstandalone=grub2-mkstandalone

else

# This build_bootx86_efi function is only called in output/ISO/Linux-i386/250_populate_efibootimg.sh

# and output/USB/Linux-i386/100_create_efiboot.sh and output/default/940_grub2_rescue.sh

# only if UEFI is used so that we simply error out here if we cannot make a bootable EFI image of GRUB2

# (normally a function should not exit but return to its caller with a non-zero return code):

Error "Cannot make bootable EFI image of GRUB2 (neither grub-mkstandalone nor grub2-mkstandalone found)"

fi

# Determine what modules need to be loaded in order to access given directories

# (if the list of modules is not overriden by GRUB2_MODULES_UEFI_LOAD)

if (( ${#dirs[@]} )) && ! (( ${#modules[@]} )) ; then

if has_binary grub-probe ; then

gprobe=grub-probe

elif has_binary grub2-probe ; then

# At least SUSE systems use 'grub2' prefixed names for GRUB2 programs:

gprobe=grub2-probe

else

LogPrint "Neither grub-probe nor grub2-probe found"

# Since openSUSE Leap 15.1 things were moved from /usr/lib/grub2/ to /usr/share/grub2/

# cf. https://github.com/rear/rear/issues/2338#issuecomment-594432946

if test /usr/*/grub*/x86_64-efi/partmap.lst ; then

LogPrint "including all partition modules"

modules=( $( cat /usr/*/grub*/x86_64-efi/partmap.lst ) )

else

Error "Can not determine partition modules, ${dirs[*]} would be likely inaccessible in GRUB2"

fi

fi

if [ -n "$gprobe" ]; then

# This is unfortunately only a crude approximation of the Grub internal probe_mods() function.

# $gprobe --target=partmap "$p" | sed -e 's/^/part_/' does not always returns part_msdos

# Therefore, we explicit do an echo 'part_msdos' (the sort -u will make sure it is listed only once)

modules=( $( for p in "${dirs[@]}" ; do

$gprobe --target=fs "$p"

$gprobe --target=partmap "$p" | sed -e 's/^/part_/'

echo 'part_msdos'

$gprobe --target=abstraction "$p"

done | sort -u ) )

fi

fi

# grub-mkstandalone needs a .../grub*/x86_64-efi/moddep.lst file (cf. https://github.com/rear/rear/issues/1193)

# At least on SUSE systems that is in different 'grub2' directories (cf. https://github.com/rear/rear/issues/2338)

# e.g. on openSUSE Leap 15.0 it is in /usr/lib/grub2/x86_64-efi/moddep.lst

# but on openSUSE Leap 15.1 that was moved to /usr/share/grub2/x86_64-efi/moddep.lst

# and the one in /boot/grub2/x86_64-efi/moddep.lst is a copy of the one in /usr/*/grub2/x86_64-efi/moddep.lst

# so we do not error out if we do not find a /x86_64-efi/moddep.lst file because it could be "anywhere else" in the future

# but we inform the user here in advance about possible problems when there is no /x86_64-efi/moddep.lst file.

# Careful: usr/sbin/rear sets nullglob so that /usr/*/grub*/x86_64-efi/moddep.lst gets empty if nothing matches

# and 'test -f' succeeds with empty argument so that we cannot use 'test -f /usr/*/grub*/x86_64-efi/moddep.lst'

# also 'test -n' succeeds with empty argument but (fortunately/intentionally?) plain 'test' fails with empty argument.

# Another implicit condition that this 'test' works is that '/usr/*/grub*/x86_64-efi/moddep.lst' matches at most one file

# because otherwise e.g. "test /usr/*/grub*/x86_64-efi/mod*" where two files moddep.lst and modinfo.sh match

# would falsely fail with "bash: test: ... unary operator expected":

test /usr/*/grub*/x86_64-efi/moddep.lst || LogPrintError "$gmkstandalone may fail to make a bootable EFI image of GRUB2 (no /usr/*/grub*/x86_64-efi/moddep.lst file)"

(( ${#GRUB2_MODULES_UEFI[@]} )) && LogPrint "Installing only ${GRUB2_MODULES_UEFI[*]} modules into $outfile memdisk"

(( ${#modules[@]} )) && LogPrint "GRUB2 modules to load: ${modules[*]}"

if ! $gmkstandalone $v ${GRUB2_MODULES_UEFI:+"--install-modules=${GRUB2_MODULES_UEFI[*]}"} ${modules:+"--modules=${modules[*]}"} -O x86_64-efi -o $outfile $embedded_config ; then

Error "Failed to make bootable EFI image of GRUB2 (error during $gmkstandalone of $outfile)"

fi

}

It hardcodes the x86_64 architecture and the name itself is quite misleading because x86 is not the same thing as x86_64. Note that GRUB2 uses the arm64-efi directory on aarch64 Fedora so plain uname -m won't work.

@lzaoral done in 3db2724

pcahyna · 2024-03-04T15:34:58Z

@pcahyna Could you generalise the build_bootx86_efi function as well, please?

rear/usr/share/rear/lib/uefi-functions.sh

Lines 39 to 126 in 8564a7b

function build_bootx86_efi {

local outfile="$1"

local embedded_config=""

local gmkstandalone=""

local gprobe=""

local dirs=()

# modules is the list of modules to load

# If GRUB2_MODULES_UEFI_LOAD is nonempty, it determines what modules to load

local modules=( ${GRUB2_MODULES_UEFI_LOAD:+"${GRUB2_MODULES_UEFI_LOAD[@]}"} )

# Configuration file is optional for image creation.

shift

if [[ -n "$1" ]] ; then

# graft point syntax. $1 will appear as /boot/grub/grub.cfg in the image

embedded_config="/boot/grub/grub.cfg=$1"

shift

# directories that should be accessible by GRUB2 (e.g. because they contain the kernel)

dirs=( ${@:+"$@"} )

fi

if has_binary grub-mkstandalone ; then

gmkstandalone=grub-mkstandalone

elif has_binary grub2-mkstandalone ; then

# At least SUSE systems use 'grub2' prefixed names for GRUB2 programs:

gmkstandalone=grub2-mkstandalone

else

# This build_bootx86_efi function is only called in output/ISO/Linux-i386/250_populate_efibootimg.sh

# and output/USB/Linux-i386/100_create_efiboot.sh and output/default/940_grub2_rescue.sh

# only if UEFI is used so that we simply error out here if we cannot make a bootable EFI image of GRUB2

# (normally a function should not exit but return to its caller with a non-zero return code):

Error "Cannot make bootable EFI image of GRUB2 (neither grub-mkstandalone nor grub2-mkstandalone found)"

fi

# Determine what modules need to be loaded in order to access given directories

# (if the list of modules is not overriden by GRUB2_MODULES_UEFI_LOAD)

if (( ${#dirs[@]} )) && ! (( ${#modules[@]} )) ; then

if has_binary grub-probe ; then

gprobe=grub-probe

elif has_binary grub2-probe ; then

# At least SUSE systems use 'grub2' prefixed names for GRUB2 programs:

gprobe=grub2-probe

else

LogPrint "Neither grub-probe nor grub2-probe found"

# Since openSUSE Leap 15.1 things were moved from /usr/lib/grub2/ to /usr/share/grub2/

# cf. https://github.com/rear/rear/issues/2338#issuecomment-594432946

if test /usr/*/grub*/x86_64-efi/partmap.lst ; then

LogPrint "including all partition modules"

modules=( $( cat /usr/*/grub*/x86_64-efi/partmap.lst ) )

else

Error "Can not determine partition modules, ${dirs[*]} would be likely inaccessible in GRUB2"

fi

fi

if [ -n "$gprobe" ]; then

# This is unfortunately only a crude approximation of the Grub internal probe_mods() function.

# $gprobe --target=partmap "$p" | sed -e 's/^/part_/' does not always returns part_msdos

# Therefore, we explicit do an echo 'part_msdos' (the sort -u will make sure it is listed only once)

modules=( $( for p in "${dirs[@]}" ; do

$gprobe --target=fs "$p"

$gprobe --target=partmap "$p" | sed -e 's/^/part_/'

echo 'part_msdos'

$gprobe --target=abstraction "$p"

done | sort -u ) )

fi

fi

# grub-mkstandalone needs a .../grub*/x86_64-efi/moddep.lst file (cf. https://github.com/rear/rear/issues/1193)

# At least on SUSE systems that is in different 'grub2' directories (cf. https://github.com/rear/rear/issues/2338)

# e.g. on openSUSE Leap 15.0 it is in /usr/lib/grub2/x86_64-efi/moddep.lst

# but on openSUSE Leap 15.1 that was moved to /usr/share/grub2/x86_64-efi/moddep.lst

# and the one in /boot/grub2/x86_64-efi/moddep.lst is a copy of the one in /usr/*/grub2/x86_64-efi/moddep.lst

# so we do not error out if we do not find a /x86_64-efi/moddep.lst file because it could be "anywhere else" in the future

# but we inform the user here in advance about possible problems when there is no /x86_64-efi/moddep.lst file.

# Careful: usr/sbin/rear sets nullglob so that /usr/*/grub*/x86_64-efi/moddep.lst gets empty if nothing matches

# and 'test -f' succeeds with empty argument so that we cannot use 'test -f /usr/*/grub*/x86_64-efi/moddep.lst'

# also 'test -n' succeeds with empty argument but (fortunately/intentionally?) plain 'test' fails with empty argument.

# Another implicit condition that this 'test' works is that '/usr/*/grub*/x86_64-efi/moddep.lst' matches at most one file

# because otherwise e.g. "test /usr/*/grub*/x86_64-efi/mod*" where two files moddep.lst and modinfo.sh match

# would falsely fail with "bash: test: ... unary operator expected":

test /usr/*/grub*/x86_64-efi/moddep.lst || LogPrintError "$gmkstandalone may fail to make a bootable EFI image of GRUB2 (no /usr/*/grub*/x86_64-efi/moddep.lst file)"

(( ${#GRUB2_MODULES_UEFI[@]} )) && LogPrint "Installing only ${GRUB2_MODULES_UEFI[*]} modules into $outfile memdisk"

(( ${#modules[@]} )) && LogPrint "GRUB2 modules to load: ${modules[*]}"

if ! $gmkstandalone $v ${GRUB2_MODULES_UEFI:+"--install-modules=${GRUB2_MODULES_UEFI[*]}"} ${modules:+"--modules=${modules[*]}"} -O x86_64-efi -o $outfile $embedded_config ; then

Error "Failed to make bootable EFI image of GRUB2 (error during $gmkstandalone of $outfile)"

fi

}

It hardcodes the x86_64 architecture and the name itself is quite misleading because x86 is not the same thing as x86_64. Note that GRUB2 uses the arm64-efi directory on aarch64 Fedora so plain uname -m won't work.

@lzaoral done in 3db2724

I have not touched the name though

Change the name of the function to build_boot_efi, not functional change intended.

pcahyna · 2024-03-04T15:46:39Z

I changed also the name now

jsmeix · 2024-03-04T16:09:34Z

usr/share/rear/output/ISO/Linux-i386/250_populate_efibootimg.sh

@@ -28,7 +28,7 @@ if test -f "$SECURE_BOOT_BOOTLOADER" ; then
    # (cf. rescue/default/850_save_sysfs_uefi_vars.sh)
    # then Shim (usually shim.efi) was copied above as efi_boot_tmp_dir/BOOTX64.efi
    # and Shim's second stage bootloader must be also copied where Shim already is.
-    DebugPrint "Using Shim '$SECURE_BOOT_BOOTLOADER' as first stage UEFI bootloader BOOTX64.efi"
+    DebugPrint "Using Shim '$SECURE_BOOT_BOOTLOADER' as first stage UEFI bootloader BOOT${EFI_ARCH_UPPER}.efi"


I think ${VAR} is not needed here so

BOOT$EFI_ARCH_UPPER.efi

is sufficient and that is used some lines above in

cp ... $efi_boot_tmp_dir/BOOT$EFI_ARCH_UPPER.efi ...

Reasoning:

I get on command line with openSUSE Leap 15.5

# foo=foo # bar=bar$foo.baz # echo "'$bar'" 'barfoo.baz'

This matches "man bash" (for bash version 4.4.23)

DEFINITIONS ... name A word consisting only of alphanumeric characters and underscores, and beginning with an alphabetic character or an underscore. Also referred to as an identifier. ... PARAMETERS ... A variable may be assigned to by a statement of the form name=[value]

So all non-alphanumeric characters and non-underscores
should be delimiters of variable names.

I think ${VAR} is not needed here so

BOOT$EFI_ARCH_UPPER.efi

is sufficient

It is certainly not needed for the shell, but I find that variable expansion in the middle of a longer string is hard to read without the curly braces. My judgement of what is hard enough to read and what is not yet is subjective, hence some inconsistency in the usage of the curly braces.

@pcahyna
this inconsistency is perfectly fine with me,
see my comment below
#3157 (review)

But shouldn't then for a specific prefix$VARIABLE.suffix
one of the two forms either prefix$VARIABLE.suffix
or prefix${VARIABLE}.suffix
be used consistently everywhere in the code
to have same readability everywhere in the code?

I'm for

concise

legible

variable names. So by default short without braces but in places where the eye has a hard time reading better with braces.

usr/share/rear/output/ISO/Linux-i386/250_populate_efibootimg.sh

usr/share/rear/output/ISO/Linux-i386/260_EFISTUB_populate.sh

jsmeix · 2024-03-04T16:14:59Z

usr/share/rear/output/RAWDISK/Linux-i386/260_create_syslinux_efi_bootloader.sh

@@ -35,7 +35,7 @@ local efi_boot_directory="$RAWDISK_BOOT_EFI_STAGING_ROOT/BOOT"

 mkdir $v -p "$efi_boot_directory" || Error "Could not create $efi_boot_directory"

-cp $v "$syslinux_efi" "$efi_boot_directory/BOOTX64.EFI" >&2
+cp $v "$syslinux_efi" "$efi_boot_directory/BOOT${EFI_ARCH_UPPER}.EFI" >&2


Same as
https://github.com/rear/rear/pull/3157/files#r1511412487

and >&2 should be usually no longer needed
since stdout and stderr are handled same, cf.
https://github.com/rear/rear/wiki/Coding-Style#what-to-do-with-stdin-stdout-and-stderr

usr/share/rear/output/RAWDISK/Linux-i386/270_create_grub2_efi_bootloader.sh

usr/share/rear/output/USB/Linux-i386/100_create_efiboot.sh

usr/share/rear/rescue/default/850_save_sysfs_uefi_vars.sh

jsmeix

From plain looking at the code it looks good to me
so I approve it "bona fide".

I only made some nitpicking comments.
It is questionable what code is easier to read

PREFIX${VARIABLE}.suffix

or

PREFIX$VARIABLE.suffix

To me both don't appear really easy to read.
In particular when the prefix is uppercase
and even more when there are uppercase 'S'
like in this artificial example

SCISSORS${SHAPE}.suffix

versus

SCISSORS$SHAPE.suffix

that almost hurts the eyes.

@pcahyna
feel free to keep in particular things like

BOOT${EFI_ARCH_UPPER}.EFI

when you think this is easier to read than

BOOT$EFI_ARCH_UPPER.EFI

cf.
"learn the rules so you know how to break them properly"
https://github.com/rear/rear/wiki/Coding-Style#why

jsmeix · 2024-03-04T16:55:56Z

According to
#3157 (comment)
and
#3157 (review)
I adapted
https://github.com/rear/rear/wiki/Coding-Style#variables
so that it now reads:

Variables

Curly braces only where really needed:
$FOO instead of ${FOO}, but for example ${FOO:-default_value}
except ${FOO} aids readability compared to $FOO for example as in
PREFIX${FOO}.SUFFIX versus PREFIX$FOO.SUFFIX that is harder to read.

pcahyna · 2024-03-04T17:23:50Z

I see now that this "hard to read" problem is partly my fault. I am using emacs and and syntax highlighting of strings like PREFIX$FOO.SUFFIX works, so this is readable without curly braces, but "PREFIX$FOO.SUFFIX" does not (the highlighting of the content of "..." overrides the highlighting of $FOO), so I need curly braces here. But in vim and perhaps other editors this is not a problem. https://stackoverflow.com/questions/10802864/in-emacs-how-do-i-get-variable-within-a-string-to-be-highlighted#comment14246359_10802864 https://emacs.stackexchange.com/questions/13128/highlighting-shell-variables-within-quotes

@rear/contributors what do you think? What does count as readable and what does count as unreadable for you?

jsmeix · 2024-03-04T17:37:54Z

@pcahyna

It is your code so in general you have
(to some reasonable extent, e.g. no real bugs)
final power over your code.

In particular when an issue is basically
about "aesthetic judgement" it is in general
a subjective judgement where you are free
to judge as you like (except exceptions), cf.
https://en.wikipedia.org/wiki/Critique_of_Judgment#Aesthetic_Judgement

pcahyna · 2024-03-04T18:08:17Z

@jsmeix sure, but I want to make my code readable for others. Therefore, I am genuinely curious what is the aesthetic judgement of other contributors.

lzaoral

LGTM! There are comments that still use explicit architecture but that is not critical.

pcahyna · 2024-03-05T14:37:39Z

yeah I have thought that the comments will be better readable if they contain concrete examples, even if this makes them incorrect for non-x64 architectures

jsmeix · 2024-03-06T12:59:34Z

I also prefer explicit real-world examples in my comments
in particular because I like to show what there really is
on my own system while I implement something so that
there is an actually true example (at least true on
my specific system at the time when I made the comment)
which has the advantage that later others can understand
how it had been when it was implemented and how it may
differ on their systems and/or at some later time.

jsmeix · 2024-03-06T13:00:22Z

@pcahyna
because it is approved by @lzaoral and me
feel free to merge it as you like.

pcahyna · 2024-03-12T15:44:36Z

@rear/contributors I am still curious how readable longer quoted strings with embedded variable expansions and curly braces vs. no curly braces are for you - most likely it will depend on the syntax highlighting capabilities of your favorite editor.
If there are no opinions that contradict what I did here (it is somewhat tuned for Emacs) I will merge the current state tomorrow.

schlomo · 2024-03-12T17:59:02Z

usr/share/rear/output/ISO/Linux-i386/250_populate_efibootimg.sh

@@ -68,7 +68,7 @@ if test "ebiso" = "$( basename $ISO_MKISOFS_BIN )" ; then
 fi

 if [[ -n "$(type -p grub)" ]]; then
-    cat > $efi_boot_tmp_dir/BOOTX64.conf << EOF
+    cat > $efi_boot_tmp_dir/BOOT$EFI_ARCH_UPPER.conf << EOF


In this kind of situation I wouldn't mind adding braces, to make it simpler to read. But again, 80% personal preference and I would accept PRs also without

It was my subjective choice, but not entirely arbitrary - emacs would highlight variable expansion in BOOT$EFI_ARCH_UPPER.conf but not in "BOOT$EFI_ARCH_UPPER.conf", that's why I am adding curly braces to the latter and not the former.

schlomo

LGTM. I'd suggest not being overly strict about quoting style and optimise for short and legible, adding braces where helpful to the reader.

pcahyna · 2024-03-14T22:18:26Z

thank you all for the reviews - merging!

pcahyna · 2024-03-14T22:19:49Z

my bad, I forgot to squash all the fixups ...

pcahyna marked this pull request as draft February 19, 2024 13:12

jsmeix reviewed Feb 19, 2024

View reviewed changes

usr/share/rear/prep/Linux-i386/330_set_efi_arch.sh Show resolved Hide resolved

jsmeix added the enhancement Adaptions and new features label Feb 22, 2024

jsmeix assigned pcahyna Feb 22, 2024

jsmeix added this to the ReaR v2.8 milestone Feb 22, 2024

lzaoral and others added 5 commits March 4, 2024 15:09

fixup! Introduce variables EFI_ARCH{_UPPER}

a4e3f53

Introduce EFI_ARCH* variables for arm

b18da08

Introduce a variable for GRUB image format

3db2724

Use it as the argument of the -O option to grub-mkstandalone/grub-mkimage instead of the hardcoded x86_64-efi. For easier porting to non-x86_64 EFI platforms.

Introduce EFI_ARCH* variables for IA64 (Itanium)

ae569b2

Mostly for completeness and documentation of the possible values.

fixup! Introduce variables EFI_ARCH{_UPPER}

838373f

pcahyna added the cleanup label Mar 4, 2024

pcahyna marked this pull request as ready for review March 4, 2024 15:28

fixup! Introduce variables EFI_ARCH{_UPPER}

3488a97

More general name & comments of build_bootx86_efi

1b2d5c9

Change the name of the function to build_boot_efi, not functional change intended.

pcahyna changed the title ~~WIP: Introduce variables EFI_ARCH{_UPPER}~~ Remove hardcoded architecture-dependend strings from EFI code Mar 4, 2024

pcahyna requested review from jsmeix, lzaoral and a team and removed request for lzaoral March 4, 2024 15:47