| Message ID | 20240122182253.17973-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rpm-pkg: simplify installkernel %post | expand |
On Tue, Jan 23, 2024 at 3:23 AM Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote: > > The new installkernel application that is now included in systemd-udev > package allows installation although destination files are already present > in the boot directory of the kernel package, but is failing with the > implemented workaround for the old installkernel application from grubby > package. > > For the new installkernel application, as Davide says: > <<The %post currently does a shuffling dance before calling installkernel. > This isn't actually necessary afaict, and the current implementation > ends up triggering downstream issues such as > https://github.com/systemd/systemd/issues/29568 > This commit simplifies the logic to remove the shuffling. For reference, > the original logic was added in commit 3c9c7a14b627("rpm-pkg: add %post > section to create initramfs and grub hooks").>> > > But we need to keep the old behavior as well, because the old installkernel > application from grubby package, does not allow this simplification and > we need to be backward compatible to avoid issues with the different > packages. > > Mimic Fedora shipping process and store vmlinuz, config amd System.map > in the module directory instead of the boot directory. In this way, we will > avoid the commented problem for all the cases, because the new destination > files are not going to exist in the boot directory of the kernel package. > > Replace installkernel tool with kernel-install tool, because the latter is > more complete. > > Besides, after installkernel tool execution, check to complete if the > correct package files vmlinuz, System.map and config files are present > in /boot directory, and if necessary, copy manually for install operation. > In this way, take into account if files were not previously copied from > /usr/lib/kernel/install.d/* scripts and if the suitable files for the > requested package are present (it could be others if the rpm files were > replace with a new pacakge with the same release and a different build). > > Tested with Fedora 38, Fedora 39, RHEL 9, Oracle Linux 9.3, > openSUSE Tumbleweed and openMandrive ROME, using dnf/zypper and rpm tools. > > cc: stable@vger.kernel.org > Co-Developed-by: Davide Cavalca <dcavalca@meta.com> > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> > --- > V1 -> V2: > - Complete to be backward compatible with the previous installkernel > application. > V2 -> V3: > - Follow the suggestions from Masahiro Yamada and change the installation > destination to avoid problems instead of checking the package. > V3 -> V4: > - Make the patch applicable to linux-kbuild/for-next (ia64 support was > already removed). > V4 -> V5: > - Complete for other Linux distributions. > V5 -> V6 > - Simplify and do more compatible checks when copied files wants to be > replaced. > - Remove %preun because it will be better done with another patch. > - Add indentation and quotation > > scripts/package/kernel.spec | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec > index 89298983a169..0bff257ec3d4 100644 > --- a/scripts/package/kernel.spec > +++ b/scripts/package/kernel.spec > @@ -55,12 +55,12 @@ patch -p1 < %{SOURCE2} > %{make} %{makeflags} KERNELRELEASE=%{KERNELRELEASE} KBUILD_BUILD_VERSION=%{release} > > %install > -mkdir -p %{buildroot}/boot > -cp $(%{make} %{makeflags} -s image_name) %{buildroot}/boot/vmlinuz-%{KERNELRELEASE} > +mkdir -p %{buildroot}/lib/modules/%{KERNELRELEASE} > +cp $(%{make} %{makeflags} -s image_name) %{buildroot}/lib/modules/%{KERNELRELEASE}/vmlinuz > %{make} %{makeflags} INSTALL_MOD_PATH=%{buildroot} modules_install > %{make} %{makeflags} INSTALL_HDR_PATH=%{buildroot}/usr headers_install > -cp System.map %{buildroot}/boot/System.map-%{KERNELRELEASE} > -cp .config %{buildroot}/boot/config-%{KERNELRELEASE} > +cp System.map %{buildroot}/lib/modules/%{KERNELRELEASE} > +cp .config %{buildroot}/lib/modules/%{KERNELRELEASE}/config > ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEASE}/build > %if %{with_devel} > %{make} %{makeflags} run-command KBUILD_RUN_COMMAND='${srctree}/scripts/package/install-extmod-build %{buildroot}/usr/src/kernels/%{KERNELRELEASE}' > @@ -70,31 +70,31 @@ ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEA > rm -rf %{buildroot} > > %post > -if [ -x /sbin/installkernel -a -r /boot/vmlinuz-%{KERNELRELEASE} -a -r /boot/System.map-%{KERNELRELEASE} ]; then > -cp /boot/vmlinuz-%{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm > -cp /boot/System.map-%{KERNELRELEASE} /boot/.System.map-%{KERNELRELEASE}-rpm > -rm -f /boot/vmlinuz-%{KERNELRELEASE} /boot/System.map-%{KERNELRELEASE} > -/sbin/installkernel %{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm > -rm -f /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm > +if [ -x /usr/bin/kernel-install ]; then > + /usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNELRELEASE}/vmlinuz > fi > +for file in vmlinuz System.map config; do > + if [ ! -e "/boot/${file}-%{KERNELRELEASE}" ] || ! cmp --silent "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}"; then > + cp "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}" > + fi > +done I am fine with this approach, but [ ! -e "/boot/${file}-%{KERNELRELEASE}" ] is a redundant check now. When the file does not exist, "cmp --silent" exits with 2 instead of 1. Anyway, "cmp --silent" fails. You can simplify the conditional to: if ! cmp --silent "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}"; then > %preun > if [ -x /sbin/new-kernel-pkg ]; then > -new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=/boot/initramfs-%{KERNELRELEASE}.img > + new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=/boot/initramfs-%{KERNELRELEASE}.img This is a noise change. Please drop. > elif [ -x /usr/bin/kernel-install ]; then > -kernel-install remove %{KERNELRELEASE} > + /usr/bin/kernel-install remove %{KERNELRELEASE} > fi > > %postun > if [ -x /sbin/update-bootloader ]; then > -/sbin/update-bootloader --remove %{KERNELRELEASE} > + /sbin/update-bootloader --remove %{KERNELRELEASE} Ditto. Please create a separate patch if you change the coding style. But, rather, I am thinking of reverting 6ef41e22a320 and 27c3bffd230ab > fi > > %files > %defattr (-, root, root) > /lib/modules/%{KERNELRELEASE} > %exclude /lib/modules/%{KERNELRELEASE}/build > -/boot/* > > %files headers > %defattr (-, root, root) > -- > 2.43.0 >
Ok, I will simplify the check to copy as you say (cpm --silent returns error if file doesn't exist) and and I will limit the indentation to the modifications. If you are going to modify the rest, I think other style changes are not necessary. Thanks Best regards José Ignacio
diff --git a/scripts/package/kernel.spec b/scripts/package/kernel.spec index 89298983a169..0bff257ec3d4 100644 --- a/scripts/package/kernel.spec +++ b/scripts/package/kernel.spec @@ -55,12 +55,12 @@ patch -p1 < %{SOURCE2} %{make} %{makeflags} KERNELRELEASE=%{KERNELRELEASE} KBUILD_BUILD_VERSION=%{release} %install -mkdir -p %{buildroot}/boot -cp $(%{make} %{makeflags} -s image_name) %{buildroot}/boot/vmlinuz-%{KERNELRELEASE} +mkdir -p %{buildroot}/lib/modules/%{KERNELRELEASE} +cp $(%{make} %{makeflags} -s image_name) %{buildroot}/lib/modules/%{KERNELRELEASE}/vmlinuz %{make} %{makeflags} INSTALL_MOD_PATH=%{buildroot} modules_install %{make} %{makeflags} INSTALL_HDR_PATH=%{buildroot}/usr headers_install -cp System.map %{buildroot}/boot/System.map-%{KERNELRELEASE} -cp .config %{buildroot}/boot/config-%{KERNELRELEASE} +cp System.map %{buildroot}/lib/modules/%{KERNELRELEASE} +cp .config %{buildroot}/lib/modules/%{KERNELRELEASE}/config ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEASE}/build %if %{with_devel} %{make} %{makeflags} run-command KBUILD_RUN_COMMAND='${srctree}/scripts/package/install-extmod-build %{buildroot}/usr/src/kernels/%{KERNELRELEASE}' @@ -70,31 +70,31 @@ ln -fns /usr/src/kernels/%{KERNELRELEASE} %{buildroot}/lib/modules/%{KERNELRELEA rm -rf %{buildroot} %post -if [ -x /sbin/installkernel -a -r /boot/vmlinuz-%{KERNELRELEASE} -a -r /boot/System.map-%{KERNELRELEASE} ]; then -cp /boot/vmlinuz-%{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm -cp /boot/System.map-%{KERNELRELEASE} /boot/.System.map-%{KERNELRELEASE}-rpm -rm -f /boot/vmlinuz-%{KERNELRELEASE} /boot/System.map-%{KERNELRELEASE} -/sbin/installkernel %{KERNELRELEASE} /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm -rm -f /boot/.vmlinuz-%{KERNELRELEASE}-rpm /boot/.System.map-%{KERNELRELEASE}-rpm +if [ -x /usr/bin/kernel-install ]; then + /usr/bin/kernel-install add %{KERNELRELEASE} /lib/modules/%{KERNELRELEASE}/vmlinuz fi +for file in vmlinuz System.map config; do + if [ ! -e "/boot/${file}-%{KERNELRELEASE}" ] || ! cmp --silent "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}"; then + cp "/lib/modules/%{KERNELRELEASE}/${file}" "/boot/${file}-%{KERNELRELEASE}" + fi +done %preun if [ -x /sbin/new-kernel-pkg ]; then -new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=/boot/initramfs-%{KERNELRELEASE}.img + new-kernel-pkg --remove %{KERNELRELEASE} --rminitrd --initrdfile=/boot/initramfs-%{KERNELRELEASE}.img elif [ -x /usr/bin/kernel-install ]; then -kernel-install remove %{KERNELRELEASE} + /usr/bin/kernel-install remove %{KERNELRELEASE} fi %postun if [ -x /sbin/update-bootloader ]; then -/sbin/update-bootloader --remove %{KERNELRELEASE} + /sbin/update-bootloader --remove %{KERNELRELEASE} fi %files %defattr (-, root, root) /lib/modules/%{KERNELRELEASE} %exclude /lib/modules/%{KERNELRELEASE}/build -/boot/* %files headers %defattr (-, root, root)
The new installkernel application that is now included in systemd-udev package allows installation although destination files are already present in the boot directory of the kernel package, but is failing with the implemented workaround for the old installkernel application from grubby package. For the new installkernel application, as Davide says: <<The %post currently does a shuffling dance before calling installkernel. This isn't actually necessary afaict, and the current implementation ends up triggering downstream issues such as https://github.com/systemd/systemd/issues/29568 This commit simplifies the logic to remove the shuffling. For reference, the original logic was added in commit 3c9c7a14b627("rpm-pkg: add %post section to create initramfs and grub hooks").>> But we need to keep the old behavior as well, because the old installkernel application from grubby package, does not allow this simplification and we need to be backward compatible to avoid issues with the different packages. Mimic Fedora shipping process and store vmlinuz, config amd System.map in the module directory instead of the boot directory. In this way, we will avoid the commented problem for all the cases, because the new destination files are not going to exist in the boot directory of the kernel package. Replace installkernel tool with kernel-install tool, because the latter is more complete. Besides, after installkernel tool execution, check to complete if the correct package files vmlinuz, System.map and config files are present in /boot directory, and if necessary, copy manually for install operation. In this way, take into account if files were not previously copied from /usr/lib/kernel/install.d/* scripts and if the suitable files for the requested package are present (it could be others if the rpm files were replace with a new pacakge with the same release and a different build). Tested with Fedora 38, Fedora 39, RHEL 9, Oracle Linux 9.3, openSUSE Tumbleweed and openMandrive ROME, using dnf/zypper and rpm tools. cc: stable@vger.kernel.org Co-Developed-by: Davide Cavalca <dcavalca@meta.com> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- V1 -> V2: - Complete to be backward compatible with the previous installkernel application. V2 -> V3: - Follow the suggestions from Masahiro Yamada and change the installation destination to avoid problems instead of checking the package. V3 -> V4: - Make the patch applicable to linux-kbuild/for-next (ia64 support was already removed). V4 -> V5: - Complete for other Linux distributions. V5 -> V6 - Simplify and do more compatible checks when copied files wants to be replaced. - Remove %preun because it will be better done with another patch. - Add indentation and quotation scripts/package/kernel.spec | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)