https://bugzilla.redhat.com/show_bug.cgi?id=2134972 Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@xxxxxxxxx --- Comment #12 from Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> --- Sorry, but this package seems like a step in the wrong direction. It hardcodes too many things and in general seems like trying to repeat the way that things were done in the past. The package does two things: 1. It provides /boot/efi/loader/entries.srel 2. It provides /sbin/updateloaderentries.sh Re 1: This is a fixed path. We should be adding any more packages that contain *any* files under /boot or /efi. Those shouldn't be managed by rpm, but instead by boot loader installation tools that take into account the state of the system. A user should be allowed to install a package and if things are not configured so, the package should not modify the system. Thus, we shouldn't have files in those paths as rpm payload. Also, it really seems easy enough to do `pathlib.Path('…').write_text('type1')`, no need to this via the package manager. Also, this hardcodes /boot/efi as the path. We have machinery to detect the location of the EFI partition dynamically instead. Also, the package has Conflicts:grubby, and grubby is depended-on (transitively) by many packages, so this would prevent this package from being installed, or grubby from being installed. It's much better to avoid Conflicts, and instead allow packages to be installed together, and DTRT depending on the state of the system. See first para above. Re 2: > /usr/bin/ln -s /usr/bin/kernel-install /sbin/installkernel This seems very wrong. Stuff like this should be part of the package payload. I see that grubby has a script there, that generally completely ignores the kernel-install machinery that the rest of the distro uses. AFAIK, installkernel is used by kernel's make install, so it's not directly related to the changes in Anaconda. If you think we should have this symlink, let's drop the old file from grubby and add the symlink to systemd-udev.rpm. > /usr/bin/awk -i inplace "$fixcmdline" /boot/efi/loader/entries/* Yikes. That just seems so complicated: Anaconda → bash → updateloaderentries.sh → awk. Instead of "fixing up" entries after the fact, let's not write broken entries in the first place. The thing that writes those entries should do stripping. And really, it's much nicer to have a few clean lines of Python code than go through three layers to do some awk. Please don't introduce this package. Instead of adding workarounds for brokenness or missing functionality in other places, let's fix the original issue. This is all free software under our control. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2134972 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
