Re: [PATCH v2] kbuild: factor out the common installation code into scripts/install.sh
From: Nicolas Schier
Date: Tue May 10 2022 - 09:46:04 EST
On Tue, May 10, 2022 at 09:56:45PM +0900, Masahiro Yamada wrote:
> On Tue, May 10, 2022 at 7:04 PM Nicolas Schier <n.schier@xxxxxx> wrote:
> >
> > On Tue, 3 May 2022 11:47 +0900 Masahiro Yamada wrote:
> > > Many architectures have similar install.sh scripts.
> > >
> > > The first half is really generic; verifies that the kernel image and
> > > System.map exist, then executes ~/bin/${INSTALLKERNEL} or
> > > /sbin/${INSTALLKERNEL} if available.
> > >
> > > The second half is kind of arch-specific. It just copies the kernel image
> > > and System.map to the destination, but the code is slightly different.
> > >
> > > This patch factors out the generic part into scripts/install.sh.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > > ---
> > >
> > > Changes in v2:
> > > - Move the installkernel parameters to scripts/install.sh
> > >
> > > Makefile | 3 ++-
> > > arch/arm/Makefile | 4 ++--
> > > arch/arm/boot/install.sh | 21 ------------------
> > > arch/arm64/Makefile | 6 ++----
> > > arch/arm64/boot/install.sh | 21 ------------------
> > > arch/ia64/Makefile | 3 ++-
> > > arch/ia64/install.sh | 10 ---------
> > > arch/m68k/Makefile | 3 ++-
> > > arch/m68k/install.sh | 22 -------------------
> > > arch/nios2/Makefile | 3 +--
> > > arch/nios2/boot/install.sh | 22 -------------------
> > > arch/parisc/Makefile | 11 +++++-----
> > > arch/parisc/install.sh | 28 ------------------------
> > > arch/powerpc/Makefile | 3 +--
> > > arch/powerpc/boot/install.sh | 23 --------------------
> > > arch/riscv/Makefile | 7 +++---
> > > arch/riscv/boot/install.sh | 21 ------------------
> > > arch/s390/Makefile | 3 +--
> > > arch/s390/boot/install.sh | 6 ------
> > > arch/sparc/Makefile | 3 +--
> > > arch/sparc/boot/install.sh | 22 -------------------
> > > arch/x86/Makefile | 3 +--
> > > arch/x86/boot/install.sh | 22 -------------------
> > > scripts/install.sh | 41 ++++++++++++++++++++++++++++++++++++
> > > 24 files changed, 64 insertions(+), 247 deletions(-)
> > > mode change 100644 => 100755 arch/arm/boot/install.sh
> > > mode change 100644 => 100755 arch/arm64/boot/install.sh
> > > mode change 100644 => 100755 arch/ia64/install.sh
> > > mode change 100644 => 100755 arch/m68k/install.sh
> > > mode change 100644 => 100755 arch/nios2/boot/install.sh
> > > mode change 100644 => 100755 arch/parisc/install.sh
> > > mode change 100644 => 100755 arch/powerpc/boot/install.sh
> > > mode change 100644 => 100755 arch/riscv/boot/install.sh
> > > mode change 100644 => 100755 arch/s390/boot/install.sh
> > > mode change 100644 => 100755 arch/sparc/boot/install.sh
> > > mode change 100644 => 100755 arch/x86/boot/install.sh
> > > create mode 100755 scripts/install.sh
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 9a60f732bb3c..154c32af8805 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1298,7 +1298,8 @@ scripts_unifdef: scripts_basic
> > > # to this Makefile to build and install external modules.
> > > # Cancel sub_make_done so that options such as M=, V=, etc. are parsed.
> > >
> > > -install: sub_make_done :=
> > > +quiet_cmd_install = INSTALL $(INSTALL_PATH)
> > > + cmd_install = unset sub_make_done; $(srctree)/scripts/install.sh
> >
> > This is the third 'cmd_install' in the tree; might it be better to take
> > a more unique name (e.g. cmd_installkernel) to prevent confusion?
>
> If this is confusing, we can rename the ones in
> scripts/Makefile.{modinst,headersinst}.
>
> This command name matches the build target ("make install"), so
> I believe this name is good.
>
>
>
> >
> > For me, it would have been more clear, if we'd also move the default
> > KBUILD_IMAGE definition here (similar to the corresponding part in
> > arch/parisc/Makefile):
> >
> > zinstall: KBUILD_IMAGE := $(boot)/Image.gz
> >
> > ($(KBUILD_IMAGE) seems not to be used anywhere else in arch/arm64/
> > tree; but I haven't checked in depth.)
>
>
> KBUILD_IMAGE is _indirectly_ used to specify
> the kernel image for package builds. [1]
>
> This target returns the value IMAGE_BUILD. [2]
>
>
> $(KBUILD_IMAGE) is definitely used for arm64.
>
> [1]: https://github.com/torvalds/linux/blob/v5.17/scripts/package/builddeb#L150
> [2]: https://github.com/torvalds/linux/blob/v5.17/Makefile#L1943
>
>
>
>
>
>
>
> > > diff --git a/arch/parisc/Makefile b/arch/parisc/Makefile
> > > index 7583fc39ab2d..aca1710fd658 100644
> > > --- a/arch/parisc/Makefile
> > > +++ b/arch/parisc/Makefile
> > > @@ -184,12 +184,11 @@ vdso_install:
> > > $(Q)$(MAKE) $(build)=arch/parisc/kernel/vdso $@
> > > $(if $(CONFIG_COMPAT_VDSO), \
> > > $(Q)$(MAKE) $(build)=arch/parisc/kernel/vdso32 $@)
> > > -install:
> > > - $(CONFIG_SHELL) $(srctree)/arch/parisc/install.sh \
> > > - $(KERNELRELEASE) vmlinux System.map "$(INSTALL_PATH)"
> > > -zinstall:
> > > - $(CONFIG_SHELL) $(srctree)/arch/parisc/install.sh \
> > > - $(KERNELRELEASE) vmlinuz System.map "$(INSTALL_PATH)"
> > > +
> > > +install: KBUILD_IMAGE := vmlinux
> > > +zinstall: KBUILD_IMAGE := vmlinuz
> >
> > Does this make the KBUILD_IMAGE definition in line 19 obsolete and
> > unused?
>
>
> As I said above for arm64, KBUILD_IMAGE is used for package builds.
>
> If you delete line 19, it will change the behavior.
> (fall back to the global default [3])
>
>
> [3]: https://github.com/torvalds/linux/blob/v5.17/Makefile#L1059
>
>
>
>
>
> > > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> > > index eb541e730d3c..45a9caa37b4e 100644
> > > --- a/arch/powerpc/Makefile
> > > +++ b/arch/powerpc/Makefile
> > > @@ -408,8 +408,7 @@ endef
> > >
> > > PHONY += install
> > > install:
> >
> > I can't find a KBUILD_IMAGE definition in arch/powerpc/Makefile.
> > Should it be set here as a target-specific varibable, too?
>
>
> Right, powerpc does not define KBUILD_IMAGE explicitly.
> It falls back to [3].
>
Thanks for all those illuming clarifications, I have nothing more to ask.
Reviewed-by: Nicolas Schier <n.schier@xxxxxx>