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

Jump to bottom

packaging: add a systemd unit to run every boot #716

Closed
wants to merge 1 commit into from
+20 −1
Closed

packaging: add a systemd unit to run every boot #716

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
3 changes: 3 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ install-grub-static:
install -m 644 -D -t ${DESTDIR}$(PREFIX)/lib/bootupd/grub2-static src/grub2/*.cfg
install -m 755 -d ${DESTDIR}$(PREFIX)/lib/bootupd/grub2-static/configs.d

install-systemd-unit:
install -m 644 -D -t "${DESTDIR}$(PREFIX)/lib/systemd/system/" contrib/packaging/bootupctl-update.service
Copy link
Member

Choose a reason for hiding this comment

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

An overall challenge here is that we'll have come full circle since #663 ...and then e.g. people maintaining SELinux policy are still going to come by and say "oh hey there's a systemd service, we need to write a policy for it...".

With this combined I would probably say that instead of this we should implement bootupd support into rpm-ostree and bootc.

(This also relates to #432 )


bin-archive:
rm target/inst -rf
$(MAKE) install install-grub-static DESTDIR=$$(pwd)/target/inst
Expand Down
13 changes: 13 additions & 0 deletions contrib/packaging/bootupctl-update.service
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[Unit]
Copy link
Member

Choose a reason for hiding this comment

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

Let's name this unit bootloader-update.service as its what will be user visible and users ideally should not have to know about bootupd.

Description=Update Bootloader on boot
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Description=Update Bootloader on boot
Description=Update bootloader on boot

Documentation=https://github.com/coreos/bootupd
ConditionPathExists=/dev/disk/by-label/EFI-SYSTEM
Copy link
Member

@travier travier Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should add as well:

ConditionPathExists=|/dev/disk/by-partlabel/EFI\x20System\x20Partition

Copy link
Member

Choose a reason for hiding this comment

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

There's already also ConditionFirmware=uefi

Copy link
Member

Choose a reason for hiding this comment

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

That said we need to support BIOS too...so probably the best would be e.g.

RequiresMountsFor=/boot
ConditionPathExists=/boot/bootupd-state.json

Copy link
Author

Choose a reason for hiding this comment

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

ConditionPathExists=/boot/bootupd-state.json
wouldn't that prevent it to run on atomic desktops as bootupd was not enabled until now ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but that's by design in bootupd today; there is bootupctl adopt-and-update but that's a distinct thing intended only for explicit invocation in tested scenarios, distinct from a "bootupd -> bootupd" update.

Copy link
Member

Choose a reason for hiding this comment

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

I am OK with ConditionFirmware=uefi, can remove it until we add adopt-and-update for BIOS later.
One minor suggestion, should we add ConditionKernelCommandLine=ostree to skip this for live boots like coreos/rpm-ostree#4944 ?

Copy link
Author

Choose a reason for hiding this comment

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

i think the overall agreement was to close this PR and carry this in the distribution

Copy link
Member

@travier travier Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
ConditionPathExists=/dev/disk/by-label/EFI-SYSTEM
ConditionPathExists=|/dev/disk/by-partlabel/EFI-SYSTEM

As that's what's used in https://github.com/coreos/bootupd/blob/main/src/efi.rs#L39


[Service]
Type=oneshot
ExecStart=/usr/bin/bootupctl update
RemainAfterExit=yes
MountFlags=slave

[Install]
WantedBy=multi-user.target
5 changes: 4 additions & 1 deletion contrib/packaging/bootupd.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ BuildRequires: cargo
BuildRequires: git
BuildRequires: openssl-devel
BuildRequires: systemd-devel
BuildRequires: systemd-rpm-macros

%description
%{summary}
Expand All @@ -31,6 +32,7 @@ BuildRequires: systemd-devel
%{_bindir}/bootupctl
%{_libexecdir}/bootupd
%{_prefix}/lib/bootupd/grub2-static/
%{_unitdir}/bootupctl-update.service

%prep
%autosetup -n %{crate}-%{version} -p1 -Sgit
Expand All @@ -50,7 +52,8 @@ cargo build --release
%install
%make_install INSTALL="install -p -c"
make install-grub-static DESTDIR=%{?buildroot} INSTALL="%{__install} -p"
make install-systemd-unit DESTDIR=%{?buildroot} INSTALL="%{__install} -p"

%changelog
* Tue Oct 18 2022 Colin Walters <[email protected]> - 0.2.8-3
- Dummy changelog
- Dummy changelog
Loading