Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootloader: rely on unified GRUB configuration for UEFI and BIOS #5996

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bootloader: rely on unified GRUB configuration for UEFI and BIOS #5996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions anaconda.spec.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ Requires: python3-pid
Requires: crypto-policies
Requires: crypto-policies-scripts

Requires: grub2-common
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be in install-env-deps (eq. the package that pulls in dependencies needed to do a a full installation run) as otherwise it could get pulled in when just the Anaconda package gets installed, say a an initial-setup dependency.

IIRC some people potentially use something else than grub (systemd-boot ? at least there seem to be enough users to complain on fedora-devel when its broken by grub specifics). If it adds post-transaction scriptlets, I guess it could actively break something.


# required because of the rescue mode and RDP question
Requires: anaconda-tui = %{version}-%{release}

Expand Down
25 changes: 0 additions & 25 deletions pyanaconda/modules/storage/bootloader/efi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,31 +177,6 @@ def efi_config_file(self):
""" Full path to EFI configuration file. """
return "%s/%s" % (self.efi_config_dir, self._config_file)

def write_config(self):
config_path = "%s%s" % (conf.target.system_root, self.efi_config_file)

with open(config_path, "w") as fd:
grub_dir = self.config_dir
if self.stage2_device.format.type != "btrfs":
fs_uuid = self.stage2_device.format.uuid
else:
fs_uuid = self.stage2_device.format.vol_uuid

if fs_uuid is None:
raise BootLoaderError("Could not get stage2 filesystem UUID")

grub_dir = util.execWithCapture("grub2-mkrelpath", [grub_dir],
root=conf.target.system_root)
if not grub_dir:
raise BootLoaderError("Could not get GRUB directory path")

fd.write("search --no-floppy --fs-uuid --set=dev %s\n" % fs_uuid)
fd.write("set prefix=($dev)%s\n" % grub_dir)
fd.write("export $prefix\n")
fd.write("configfile $prefix/grub.cfg\n")

super().write_config()


class EFISystemdBoot(EFIBase, SystemdBoot):
"""EFI Systemd-boot"""
Expand Down
25 changes: 17 additions & 8 deletions pyanaconda/modules/storage/bootloader/grub2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
from pyanaconda.core import util
from pyanaconda.core.configuration.anaconda import conf
from pyanaconda.core.i18n import _
from pyanaconda.core.path import open_with_perm
from pyanaconda.core.path import join_paths, open_with_perm
from pyanaconda.core.product import get_product_name

from pyanaconda.anaconda_loggers import get_module_logger
Expand Down Expand Up @@ -363,14 +363,23 @@ def write_config(self):
if rc:
log.error("failed to set menu_auto_hide=1")

# now tell grub2 to generate the main configuration file
rc = util.execWithRedirect(
"grub2-mkconfig",
["-o", self.config_file],
root=conf.target.system_root
)
# Executes the grub2-common posttrans script to generate grub config files
# This should be autohandled by grub2-common, but posttrans scriptlets
# in live image scenarios are not triggered.
# FIXME: https://bugzilla.redhat.com/show_bug.cgi?id=2327644

# Drop the first line as it's the posttrans scriptlet header
script = util.execWithCapture("rpm", ["-q", "--scripts", "grub2-common"], root=conf.target.system_root).splitlines()[1:]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work only as long as the rootfs deployed by the payload:

  • has an RPM database
  • has the rpm binary
    For very minimal systems this might not be the case - eq. the expectation is they will be re-installed instead of updated/modified in place.

Also I'm not sure about immutable system like ostree or bootc/image mode. Eq. if they even have scriptlets like this or if they can/expect changes being done in chroot.

Lastly I wonder about ordering or re-entrancy. Eq. can it depend on some other scritplet being called & can it survive being called more times (or do we need to make sure not to call it twice on regular RPM installs).

script = "\n".join(script)

# Write the script to a temporary file in the chroot
with open(join_paths(conf.target.system_root, "/tmp/grub2-posttrans.sh"), "w") as f:
f.write(script)

# Run the script in the chroot
rc = util.execWithRedirect("/bin/sh", ["/tmp/grub2-posttrans.sh"], root=conf.target.system_root)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main issue with this would be that is basically a black box & implementation detail of the GRUB 2 package - eq. some changes unless properly coordinated with us could potentially break things due to changes in GRUB, its packaging or Anaconda. Also unlike a more widely used tool, it would be IMHO quite hard (to impossible) to test, especially outside of this very specific environment.

In comparison, the proposed "shared script" solution could be called by this scriplet (making it much simpler), could be versions (with both GRUB 2 & Anaconda being able to see & control the installed version) as well as presumably being much easier to test a debug separately ("this folder is now /boot, do your thing").

if rc:
raise BootLoaderError("failed to write boot loader configuration")
raise BootLoaderError("grub2-common posttrans script failed")

#
# installation
Expand Down