As a FYI, this is part of https://github.com/rhinstaller/anaconda/pull/4368

These two pieces and a tweak to -comps are the minimal systemd-boot surface at the moment.

Also, as this is in copr, there is a review built as well https://download.copr.fedorainfracloud.org/results/jlinton/sdubby/fedora-rawhide-aarch64/04944112-sdubby/fedora-review/review.txt. I'm looking at cleaning up the rpmlint a bit.

Ok, it's down to just a couple of warnings, which AFAIK are unavoidable or (somewhat wrong given the naming).

Taking this review.

Can you please provide spec and srpm URLs that fedora-review can consume? I need raw gettable URLs.

Hi,

Thanks for looking at this.

I think the current direct downloads can be:


https://pagure.io/sdubby/raw/master/f/sdubby.spec

and 

https://download.copr.fedorainfracloud.org/results/jlinton/sdubby/fedora-rawhide-x86_64/04990430-sdubby/sdubby-1.0-1.fc38.src.rpm

although, I think I'm going to drop the .sh from the updateloaderentries.sh file, so the warning goes away about the man page not matching since I'm also going to update the anaconda changes.

Here is an updated copr build:

https://pagure.io/sdubby/raw/master/f/sdubby.spec
https://download.copr.fedorainfracloud.org/results/jlinton/sdubby/fedora-rawhide-x86_64/04995544-sdubby/sdubby-1.0-1.fc38.src.rpm

I think I'm done with it for the moment (renamed that .sh file)

The current lint looks like:

sdubby.noarch: W: non-etc-or-var-file-marked-as-conffile /boot/efi/loader/entries.srel
sdubby.noarch: W: non-etc-or-var-file-marked-as-conffile /boot/efi/loader/loader.conf
sdubby.noarch: W: no-manual-page-for-binary installkernel
sdubby.noarch: W: dangling-relative-symlink /usr/sbin/installkernel ../bin/kernel-install

The first two, are sorta duh's, and there isn't anything to do about it since the config files have to exist on the ESP.

The second/third are sorta silly as I'm explicitly symlinking it in the rpm now to avoid the error about doing it in %post and AFAIK that's the best way to create a symlink from an RPM.

I think the user MUST items are fine too.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "GNU General Public License, Version 2". Detailed output of
     licensecheck in /home/ngompa/2134972-sdubby/licensecheck.txt
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /boot/efi/loader
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /boot/efi, /boot/efi/loader
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[-]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see
     attached diff).
     See: (this test has no URL)
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Cannot parse rpmlint output:


Rpmlint (installed packages)
----------------------------
/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
============================ rpmlint session starts ============================
rpmlint: 2.4.0
configuration:
    /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-legacy-licenses.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
checks: 31, packages: 1

sdubby.noarch: W: non-etc-or-var-file-marked-as-conffile /boot/efi/loader/entries.srel
sdubby.noarch: W: non-etc-or-var-file-marked-as-conffile /boot/efi/loader/loader.conf
sdubby.noarch: W: no-manual-page-for-binary installkernel
sdubby.noarch: W: dangling-relative-symlink /usr/sbin/installkernel ../bin/kernel-install
 1 packages and 0 specfiles checked; 0 errors, 4 warnings, 0 badness; has taken 0.0 s 



Requires
--------
sdubby (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/bash
    config(sdubby)
    coreutils
    findutils
    gawk
    systemd-udev
    util-linux



Provides
--------
sdubby:
    config(sdubby)
    sdubby



Diff spec file in url and in SRPM
---------------------------------
--- /home/ngompa/2134972-sdubby/srpm/sdubby.spec	2022-10-31 02:05:51.309565029 -0400
+++ /home/ngompa/2134972-sdubby/srpm-unpacked/sdubby.spec	2022-10-28 18:57:33.000000000 -0400
@@ -73,3 +73,2 @@
 * Fri Sep  9 2022 Jeremy Linton <jeremy.linton> - 1.0-1
 - Create package as a grubby alternative on systemd-boot systems
-


Generated by fedora-review 0.9.0 (6761b6c) last change: 2022-08-23
Command line :/usr/bin/fedora-review -b 2134972 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: C/C++, Python, R, Perl, fonts, Haskell, Java, PHP, Ocaml, SugarActivity
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

I'm not sure this makes sense to be noarch, given that it's only useful on architectures that sd-boot is available on. The reason grubby can be made available everywhere is that it supports non-EFI platforms and multiple bootloaders.

Also, how is it that sd-boot hasn't been split out of systemd-udev yet? That was supposed to happen a long time ago...

Ha, I turned it into noarch because fedora-review was complaining it didn't produce any binaries. Its easy to change it back to just x86 and arm.

But, to your second point, yeah I think that when/if that happens, it's a reasonable expectation that these little bits get merged into it.

As part of this process, It will be worthwhile to revisit systemd after my patch lands upstream, to post a PR extracting systemd-boot from systemd-udev.

(In reply to Jeremy Linton from comment #10)
> Ha, I turned it into noarch because fedora-review was complaining it didn't
> produce any binaries. Its easy to change it back to just x86 and arm.
> 

We do have %efi for arches. I used it for rEFInd: https://src.fedoraproject.org/rpms/rEFInd/blob/rawhide/f/rEFInd.spec#_49-51

Since you don't produce any binaries, you probably want to disable debuginfo packages so it doesn't complain.

> But, to your second point, yeah I think that when/if that happens, it's a
> reasonable expectation that these little bits get merged into it.
> 
> As part of this process, It will be worthwhile to revisit systemd after my
> patch lands upstream, to post a PR extracting systemd-boot from systemd-udev.

This should probably happen before this package lands.

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.

I don't think I agree with much of what you wrote.

Starting with the fact that I think the people who favor copying executables to security-sensitive contexts (the ESP) outside of the distro auditing path are in error. It should be possible to rpm -qf, and more importantly, rpm -Vf absolutely everything in the boot path. This means that anaconda should _never_ be performing functions that can be provided by packages when it comes to file creation and placement because those files don't have an audible path. So, I might buy that much of this should be moved to the systemd-boot package now that one exists. And given that package can be installed independently from the rest of systemd, it should probably be responsible for assuring that the systemd-boot efi executables and config files are installed in the correct location without using bootctl (which breaks the auditing path).

And from this flows the distro wide problem of fixed paths because RPM's don't generally support alternate install locations. So these paths are already hardcoded in in grubby, grub-efi, etc, etc, etc. So while its possible to hand shim the ESP into /efi or places other distro's provide it, fedora/etc, for better or worse at the moment, seems to have issues if the ESP is placed elsewhere. 

This plays into much of the remainder of your argument, including the fact that this package excludes grubby. One might argue that is somewhat in error, but grubby's current state breaks everything you are arguing above about fixed paths because the kernel install process and the like's path/bootloader detection logic breaks when grubby pulls in grub2-tools/etc. So, while it is possible to convert a machine to systemd-boot by hand, it usually involves moving some part of /boot elsewhere in order to make systemd-boot work properly in the face of dnf update/etc. And even bootctl won't create bootable machines in anaconda if this package is missing the /boot/efi/loader/entries.srel, nor will the kernel be placed on the ESP. And that file can't be easily created at the right time in anaconda anyway. And so, its probably a workable idea to have a utility that manipulates the BLS entries like grubby does, or just rework all the bits in grubby to so that it can also support systemd, but after looking at it, its basically another entire utility. 


So, I'm ok with killing this package, but the correct place to move functionality, including the creation of that simlink required to assure that kernel upgrades work and a grubby like utility for editing the systemd BLS entries is in the systemd-boot package not systemd-udev. And of course, as part of assuring it works, either grubby needs to be heavily rewritten to remove the hooking/etc its doing for grub, etc.

Are you signing up to add this functionality to systemd-boot, or merge this stuff there?

> It should be possible to rpm -qf, and more importantly, rpm -Vf absolutely everything in the boot path.

This does not work currently and is not intended to work. It currently does not work
because the initrds are generated locally, so 'rpm -Vf' cannot check their contents.
More broadly, the ESP (or XBOOTLDR) are intended to be shared between installations in
dual boot scenarios, and one of those installations should just ignore the other ones.
If boot counting [1] is implemented (and there are good reasons to do that), the boot
count is part of the file name, and this just can't be handled by 'rpm -Vf'.

[1] https://github.com/uapi-group/specifications/blob/main/specs/boot_loader_specification.md#boot-counting

Hardcoding of /boot and /boot/efi is problematic because there are good reasons to swith
a different path in the future. If we do that, effectively we'll have to support multiple
layouts — both the new one, but also the old one for compatibility. Locking ourselves into
a specific layout via packaging now is borrowing trouble.

> including the creation of that simlink required to assure that kernel upgrades work

That symlink is NOT required for kernel upgrades to work. It is used by 'make install'
in local kernel builds. Kernel package scripts use kernel-install. 

> Are you signing up to add this functionality to systemd-boot, or merge this stuff there?

I cannot give a blanket promise to merge things. I can promise to review pull
requests and maybe work on some feature requests. At this point, I don't quite understand
what is missing from sd-boot. If there's some general bug or missing feature, we'll
do our best to solve it like any other issue.

I guess I should point out that the updateloaderentries.sh script has been extended slightly to emulate enough of grubby that things like `kdumpctl` work in a systemd-boot enviroment. This was always the plan as I eluded to above, and part of the reason for having a script that anaconda uses as well. AKA, it avoids duplicating loader entry manipulations in both anaconda and in the runtime enviroment by using the same script, same as is currently being done with grubby (aka anaconda uses grubby to manipulate the loader entries). In this case the script shims to bootctl, sed, awk as needed to perform the requested operation. This is IMHO a better choice as coreutils is a more common environment than python, despite the heavy reliance on python by the RH installer.

As a FYI, I updated the package to use the rh rpms EFI macros for the ESP/etc. This fixes the complaints in #11 about the efi macros, and also I guess is the "machinery" referenced as well since in theory the rpm macros can update /boot/efi to somewhere else in the filesystem? To be clear I wasn't arguing against those changes (not sure if it came across that way, the point was that we needed to place files in the ESP/etc and ghost/config'ing them should be done regardless of whether anaconda/etc rewrites them.).

Also, after some mailing list discussions/etc a change was opened/approved for this and the related changes for F39, so its getting late I either need some clear directions or for this to be approved. https://fedoraproject.org/wiki/Changes/cleanup_systemd_install Thanks,

At this point, let's let it in...

I don't see anything else that needs fixing...

PACKAGE APPROVED.

The Pagure repository was created at https://src.fedoraproject.org/rpms/sdubby

FEDORA-2023-3b6fc9579c has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-3b6fc9579c

FEDORA-2023-3b6fc9579c has been pushed to the Fedora 39 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-3b6fc9579c \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-3b6fc9579c

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Proposed as a Freeze Exception for 39-beta by Fedora user frantisekz using the blocker tracking app because:

 New package is a low risk, would make it easier for users to leverage systemd-boot.

Removing FE proposal from this one, already accepted as FE in https://bugzilla.redhat.com/show_bug.cgi?id=2234638 , sigh.

FEDORA-2023-3b6fc9579c has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Zbigniew, I'm curious about something you wrote in comment #12:

"grubby is depended-on (transitively) by many packages"

is it? what packages?

I see:

[adamw@xps13a anaconda (f40 %)]$ sudo dnf repoquery --whatrequires grubby
Last metadata expiration check: 1:20:41 ago on Sun 17 Mar 2024 10:18:46 PM.
akmods-0:0.5.8-8.fc40.noarch
ec2-hibinit-agent-0:1.0.5-5.fc40.noarch
systemd-udev-0:255.3-1.fc40.x86_64
systemd-udev-0:255.4-1.fc40.x86_64

but of those, systemd-udev's dependency is actually "(grubby > 8.40-72 if grubby)", and akmods' is "(grubby or systemd-boot)". only ec2-inhibit-agent actually has an unconditional dependency on "grubby", AFAICT. I also checked F38 for a historical perspective, and there only ec2-inhibit-agent and something called 'imgbased' depend on grubby at all.

At some point, kexec-tools had a hard requirement on grubby, and systemd had a dependency on
kexec-tools (for /usr/sbin/kexec). This was dropped at some point.
So yeah, now it's much less.