#3157 PR `merged`: Remove hardcoded architecture-dependend strings from EFI code¶

Labels: enhancement, cleanup, fixed / solved / done

pcahyna opened issue at 2024-02-19 13:12:¶

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.

lzaoral commented at 2024-02-19 14:17:¶

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

lzaoral commented at 2024-02-28 09:21:¶

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

https://github.com/rear/rear/blob/8564a7b952481e0fdde562207ff88ff1f4b4df9d/usr/share/rear/lib/uefi-functions.sh#L39-L126

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 commented at 2024-02-28 12:50:¶

There is still one instance of BOOTX64.efi left:
https://github.com/rear/rear/blob/48ebbf974b852e6e2c1c9c92646d7036f5a19674/usr/share/rear/output/ISO/Linux-i386/260_EFISTUB_populate.sh#L36

pcahyna commented at 2024-03-04 15:22:¶

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

https://github.com/rear/rear/blob/8564a7b952481e0fdde562207ff88ff1f4b4df9d/usr/share/rear/lib/uefi-functions.sh#L39-L126

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 commented at 2024-03-04 15:34:¶

@pcahyna Could you generalise the build_bootx86_efi function as well, please?
https://github.com/rear/rear/blob/8564a7b952481e0fdde562207ff88ff1f4b4df9d/usr/share/rear/lib/uefi-functions.sh#L39-L126

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

pcahyna commented at 2024-03-04 15:46:¶

I changed also the name now

jsmeix commented at 2024-03-04 16:55:¶

According to
https://github.com/rear/rear/pull/3157#discussion_r1511419803
and
https://github.com/rear/rear/pull/3157#pullrequestreview-1914753506
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 commented at 2024-03-04 17:23:¶

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 commented at 2024-03-04 17:37:¶

@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 commented at 2024-03-04 18:08:¶

@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.

pcahyna commented at 2024-03-05 14:37:¶

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 commented at 2024-03-06 12:59:¶

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 commented at 2024-03-06 13:00:¶

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

pcahyna commented at 2024-03-12 15:44:¶

@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.

pcahyna commented at 2024-03-14 22:18:¶

thank you all for the reviews - merging!

pcahyna commented at 2024-03-14 22:19:¶

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

[Export of Github issue for rear/rear.]

#3157 PR merged: Remove hardcoded architecture-dependend strings from EFI code¶