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.

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

Preliminary support for systemd-boot bootloader. #23

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

Conversation

ikelos
Copy link

@ikelos ikelos commented Dec 27, 2018

This adds preliminary support for systemd-boot using the kernel-install tool. It's dependent upon systemd pull request #11281 (systemd/systemd#11281) to work properly with
initrd files.

It's a little ugly because it wipes out (to avoid double copying files) the ones that genkernel has already copied to /boot, since genkernel copies those in multiple different places in the code. Ideally those would be refactored and delayed until everything had built successfully, but that was a large refactoring, so for now this adds support as is.

This adds preliminary support for systemd-boot using the kernel-install
tool.  It's dependent upon systemd pull request #11281
(systemd/systemd#11281) to work properly with
initrd files.

It's a little ugly because it wipes (to avoid double copying files) it
wipes out the ones that genkernel has already copied to /boot, since
genkernel copies those in multiple different places in the code.

Ideally those would be refactored and delayed until everything had built
successfully, but that was a large refactoring, so for now this adds
support as is.

Signed-off-by: Mike Auty <[email protected]>
@robbat2
Copy link
Owner

robbat2 commented Mar 29, 2019

@Whissi do you know if this is still needed?

@Whissi
Copy link
Contributor

Whissi commented Mar 30, 2019

Well, if we want to support systemd's kernel-install tool, we need this -- at the moment we don't support that tool.

But I don't like this patch. Incoming review.

Copy link
Contributor

@Whissi Whissi left a comment

Choose a reason for hiding this comment

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

In addition, documentation (longusage and man page) must be updated.

@@ -8,12 +8,36 @@ set_bootloader() {
grub2)
set_bootloader_grub2
;;
systemd-boot)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just name it "systemd"?

*)
print_warning "Bootloader ${BOOTLOADER} is not currently supported"
;;
esac
}

set_bootloader_systemd() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line.

fi

# Clear out the mistakenly installed copies, since kernel-install copies its own versions over
# We do this first, to prevent people thinking they have enough free space but needing twice the amount
Copy link
Contributor

Choose a reason for hiding this comment

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

compile_kernel() in gen_compile.sh should be modified:

@@ -416,19 +416,24 @@ compile_kernel() {

        if isTrue "${CMD_INSTALL}"
        then
-               copy_image_with_preserve "kernel" \
-                       "${tmp_kernel_binary}" \
-                       "kernel-${KNAME}-${ARCH}-${KV}"
+               if [ -n "${BOOTLOADER}" -a "${BOOTLOADER}" = "systemd" ]
+               then
+                       print_info 1 "$(getIndent 1)Not installing kernel; systemd's kernel-install will handle installation for us!"
+               else
+                       copy_image_with_preserve "kernel" \
+                               "${tmp_kernel_binary}" \
+                               "kernel-${KNAME}-${ARCH}-${KV}"

-               copy_image_with_preserve "System.map" \
-                       "${systemmap}" \
-                       "System.map-${KNAME}-${ARCH}-${KV}"
+                       copy_image_with_preserve "System.map" \
+                               "${systemmap}" \
+                               "System.map-${KNAME}-${ARCH}-${KV}"

-               if isTrue "${GENZIMAGE}"
-               then
-                       copy_image_with_preserve "kernelz" \
-                               "${tmp_kernel_binary2}" \
-                               "kernelz-${KV}"
+                       if isTrue "${GENZIMAGE}"
+                       then
+                               copy_image_with_preserve "kernelz" \
+                                       "${tmp_kernel_binary2}" \
+                                       "kernelz-${KV}"
+                       fi
                fi
        else
                cp "${tmp_kernel_binary}" "${TMPDIR}/kernel-${KNAME}-${ARCH}-${KV}" ||

Copy link
Author

Choose a reason for hiding this comment

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

This feels like it's moving bootloader logic into the compile_kernel logic. I'm happy to do that, I just figured you'd want it all stand-alone in its own little area, so that the it doesn't set a precedent for the next bootloader that comes along to start tinkering with the internals too?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right. The suggestion was based on genkernel-3, before I decided to touch so much code and create genkernel-4. Now I think we can do it correctly: But before we are able to do that we need to tackle https://bugs.gentoo.org/505810 - i.e. I'll remove install logic from compile_* to genkernel main script. In the main script we will keep something like https://github.com/gentoo/genkernel/blob/v4.0.0_beta4/genkernel#L337, adjusted of course, which will call 1set_bootloader1 when genkernel was called with --install. We will use the case statement currently used to display a warning to decide if we have to call copy_image_with_preserve or not... then, when we finally call set_bootloader, which would call kernel-install in systemd-boot case.

You get my idea?

tl;dr
Before we get back to adding systemd-boot support, I'll have to refactor install logic. :)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I think I follow. Just give me a prod on here when it's ready for some reworking. I'm happy to pitch in, but don't want to step on your plans... 5:)

fi

# Do the call
echo "Executing: kernel-install add $KV \"${KERNEL_IMAGE}\" \"${INITRD_FILE}\""
Copy link
Contributor

Choose a reason for hiding this comment

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

Use

print_info 2 "COMMAND: ${NICEOPTS}${MAKE} ${MAKEOPTS} -j1 ${ARGS} ${target} $*" 1 0 1

like command to show command..


# Do the call
echo "Executing: kernel-install add $KV \"${KERNEL_IMAGE}\" \"${INITRD_FILE}\""
kernel-install add $KV "${KERNEL_IMAGE}" "${INITRD_FILE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add error check, || gen_die "..."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants