Re: [PATCH v2] kbuild: factor out the common installation code into scripts/install.sh

[Nicolas Schier]

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?

>  
>  # ---------------------------------------------------------------------------
>  # Tools
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index a2391b8de5a5..187f4b2c5c73 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -318,9 +318,9 @@ $(BOOT_TARGETS): vmlinux
>  	$(Q)$(MAKE) $(build)=$(boot) MACHINE=$(MACHINE) $(boot)/$@
>  	@$(kecho) '  Kernel: $(boot)/$@ is ready'
>  
> +$(INSTALL_TARGETS): KBUILD_IMAGE = $(boot)/$(patsubst %install,%Image,$@)
>  $(INSTALL_TARGETS):
> -	$(CONFIG_SHELL) $(srctree)/$(boot)/install.sh "$(KERNELRELEASE)" \
> -	$(boot)/$(patsubst %install,%Image,$@) System.map "$(INSTALL_PATH)"
> +	$(call cmd,install)
>  
>  PHONY += vdso_install
>  vdso_install:
> diff --git a/arch/arm/boot/install.sh b/arch/arm/boot/install.sh
> old mode 100644
> new mode 100755
> index 2a45092a40e3..9ec11fac7d8d
> --- a/arch/arm/boot/install.sh
> +++ b/arch/arm/boot/install.sh
> @@ -1,7 +1,5 @@
>  #!/bin/sh
>  #
> -# arch/arm/boot/install.sh
> -#
>  # This file is subject to the terms and conditions of the GNU General Public
>  # License.  See the file "COPYING" in the main directory of this archive
>  # for more details.
> @@ -18,25 +16,6 @@
>  #   $2 - kernel image file
>  #   $3 - kernel map file
>  #   $4 - default install path (blank if root directory)
> -#
> -
> -verify () {
> -	if [ ! -f "$1" ]; then
> -		echo ""                                                   1>&2
> -		echo " *** Missing file: $1"                              1>&2
> -		echo ' *** You need to run "make" before "make install".' 1>&2
> -		echo ""                                                   1>&2
> -		exit 1
> -	fi
> -}
> -
> -# Make sure the files actually exist
> -verify "$2"
> -verify "$3"
> -
> -# User may have a custom install script
> -if [ -x ~/bin/${INSTALLKERNEL} ]; then exec ~/bin/${INSTALLKERNEL} "$@"; fi
> -if [ -x /sbin/${INSTALLKERNEL} ]; then exec /sbin/${INSTALLKERNEL} "$@"; fi
>  
>  if [ "$(basename $2)" = "zImage" ]; then
>  # Compressed install
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 2f1de88651e6..6d9d4a58b898 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -162,11 +162,9 @@ Image: vmlinux
>  Image.%: Image
>  	$(Q)$(MAKE) $(build)=$(boot) $(boot)/$@
>  
> -install: install-image := Image
> -zinstall: install-image := Image.gz
> +install: KBUILD_IMAGE := $(boot)/Image

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.)

>  install zinstall:
> -	$(CONFIG_SHELL) $(srctree)/$(boot)/install.sh $(KERNELRELEASE) \
> -	$(boot)/$(install-image) System.map "$(INSTALL_PATH)"
> +	$(call cmd,install)
>  
>  PHONY += vdso_install
>  vdso_install:
> diff --git a/arch/arm64/boot/install.sh b/arch/arm64/boot/install.sh
> old mode 100644
> new mode 100755
> index d91e1f022573..7399d706967a
> --- a/arch/arm64/boot/install.sh
> +++ b/arch/arm64/boot/install.sh
> @@ -1,7 +1,5 @@
>  #!/bin/sh
>  #
> -# arch/arm64/boot/install.sh
> -#
>  # This file is subject to the terms and conditions of the GNU General Public
>  # License.  See the file "COPYING" in the main directory of this archive
>  # for more details.
> @@ -18,25 +16,6 @@
>  #   $2 - kernel image file
>  #   $3 - kernel map file
>  #   $4 - default install path (blank if root directory)
> -#
> -
> -verify () {
> -	if [ ! -f "$1" ]; then
> -		echo ""                                                   1>&2
> -		echo " *** Missing file: $1"                              1>&2
> -		echo ' *** You need to run "make" before "make install".' 1>&2
> -		echo ""                                                   1>&2
> -		exit 1
> -	fi
> -}
> -
> -# Make sure the files actually exist
> -verify "$2"
> -verify "$3"
> -
> -# User may have a custom install script
> -if [ -x ~/bin/${INSTALLKERNEL} ]; then exec ~/bin/${INSTALLKERNEL} "$@"; fi
> -if [ -x /sbin/${INSTALLKERNEL} ]; then exec /sbin/${INSTALLKERNEL} "$@"; fi
>  
>  if [ "$(basename $2)" = "Image.gz" ]; then
>  # Compressed install
> diff --git a/arch/ia64/Makefile b/arch/ia64/Makefile
> index 6c4bfa54b703..e55c2f138656 100644
> --- a/arch/ia64/Makefile
> +++ b/arch/ia64/Makefile
> @@ -72,8 +72,9 @@ archheaders:
>  
>  CLEAN_FILES += vmlinux.gz
>  
> +install: KBUILD_IMAGE := vmlinux.gz
>  install:
> -	sh $(srctree)/arch/ia64/install.sh $(KERNELRELEASE) vmlinux.gz System.map "$(INSTALL_PATH)"
> +	$(call cmd,install)
>  
>  define archhelp
>    echo '* compressed	- Build compressed kernel image'
> diff --git a/arch/ia64/install.sh b/arch/ia64/install.sh
> old mode 100644
> new mode 100755
> index 0e932f5dcd1a..2d4b66a9f362
> --- a/arch/ia64/install.sh
> +++ b/arch/ia64/install.sh
> @@ -1,7 +1,5 @@
>  #!/bin/sh
>  #
> -# arch/ia64/install.sh
> -#
>  # This file is subject to the terms and conditions of the GNU General Public
>  # License.  See the file "COPYING" in the main directory of this archive
>  # for more details.
> @@ -17,14 +15,6 @@
>  #   $2 - kernel image file
>  #   $3 - kernel map file
>  #   $4 - default install path (blank if root directory)
> -#
> -
> -# User may have a custom install script
> -
> -if [ -x ~/bin/${INSTALLKERNEL} ]; then exec ~/bin/${INSTALLKERNEL} "$@"; fi
> -if [ -x /sbin/${INSTALLKERNEL} ]; then exec /sbin/${INSTALLKERNEL} "$@"; fi
> -
> -# Default install - same as make zlilo
>  
>  if [ -f $4/vmlinuz ]; then
>  	mv $4/vmlinuz $4/vmlinuz.old
> diff --git a/arch/m68k/Makefile b/arch/m68k/Makefile
> index 740fc97b9c0f..e358605b70ba 100644
> --- a/arch/m68k/Makefile
> +++ b/arch/m68k/Makefile
> @@ -138,5 +138,6 @@ CLEAN_FILES += vmlinux.gz vmlinux.bz2
>  archheaders:
>  	$(Q)$(MAKE) $(build)=arch/m68k/kernel/syscalls all
>  
> +install: KBUILD_IMAGE := vmlinux.gz
>  install:
> -	sh $(srctree)/arch/m68k/install.sh $(KERNELRELEASE) vmlinux.gz System.map "$(INSTALL_PATH)"
> +	$(call cmd,install)
> diff --git a/arch/m68k/install.sh b/arch/m68k/install.sh
> old mode 100644
> new mode 100755
> index 57d640d4382c..af65e16e5147
> --- a/arch/m68k/install.sh
> +++ b/arch/m68k/install.sh
> @@ -15,28 +15,6 @@
>  #   $2 - kernel image file
>  #   $3 - kernel map file
>  #   $4 - default install path (blank if root directory)
> -#
> -
> -verify () {
> -	if [ ! -f "$1" ]; then
> -		echo ""                                                   1>&2
> -		echo " *** Missing file: $1"                              1>&2
> -		echo ' *** You need to run "make" before "make install".' 1>&2
> -		echo ""                                                   1>&2
> -		exit 1
> -	fi
> -}
> -
> -# Make sure the files actually exist
> -verify "$2"
> -verify "$3"
> -
> -# User may have a custom install script
> -
> -if [ -x ~/bin/${INSTALLKERNEL} ]; then exec ~/bin/${INSTALLKERNEL} "$@"; fi
> -if [ -x /sbin/${INSTALLKERNEL} ]; then exec /sbin/${INSTALLKERNEL} "$@"; fi
> -
> -# Default install - same as make zlilo
>  
>  if [ -f $4/vmlinuz ]; then
>  	mv $4/vmlinuz $4/vmlinuz.old
> diff --git a/arch/nios2/Makefile b/arch/nios2/Makefile
> index 02d678559066..d6a7499b814c 100644
> --- a/arch/nios2/Makefile
> +++ b/arch/nios2/Makefile
> @@ -56,8 +56,7 @@ $(BOOT_TARGETS): vmlinux
>  	$(Q)$(MAKE) $(build)=$(nios2-boot) $(nios2-boot)/$@
>  
>  install:
> -	sh $(srctree)/$(nios2-boot)/install.sh $(KERNELRELEASE) \
> -	$(KBUILD_IMAGE) System.map "$(INSTALL_PATH)"
> +	$(call cmd,install)
>  
>  define archhelp
>    echo  '* vmImage         - Kernel-only image for U-Boot ($(KBUILD_IMAGE))'
> diff --git a/arch/nios2/boot/install.sh b/arch/nios2/boot/install.sh
> old mode 100644
> new mode 100755
> index 3cb3f468bc51..34a2feec42c8
> --- a/arch/nios2/boot/install.sh
> +++ b/arch/nios2/boot/install.sh
> @@ -15,28 +15,6 @@
>  #   $2 - kernel image file
>  #   $3 - kernel map file
>  #   $4 - default install path (blank if root directory)
> -#
> -
> -verify () {
> -	if [ ! -f "$1" ]; then
> -		echo ""                                                   1>&2
> -		echo " *** Missing file: $1"                              1>&2
> -		echo ' *** You need to run "make" before "make install".' 1>&2
> -		echo ""                                                   1>&2
> -		exit 1
> -	fi
> -}
> -
> -# Make sure the files actually exist
> -verify "$2"
> -verify "$3"
> -
> -# User may have a custom install script
> -
> -if [ -x ~/bin/${INSTALLKERNEL} ]; then exec ~/bin/${INSTALLKERNEL} "$@"; fi
> -if [ -x /sbin/${INSTALLKERNEL} ]; then exec /sbin/${INSTALLKERNEL} "$@"; fi
> -
> -# Default install - same as make zlilo
>  
>  if [ -f $4/vmlinuz ]; then
>  	mv $4/vmlinuz $4/vmlinuz.old
> 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?

> +install zinstall:
> +	$(call cmd,install)
>  
>  CLEAN_FILES	+= lifimage
>  MRPROPER_FILES	+= palo.conf
> diff --git a/arch/parisc/install.sh b/arch/parisc/install.sh
> old mode 100644
> new mode 100755
> index 70d3cffb0251..933d031c249a
> --- a/arch/parisc/install.sh
> +++ b/arch/parisc/install.sh
> @@ -1,7 +1,5 @@
>  #!/bin/sh
>  #
> -# arch/parisc/install.sh, derived from arch/i386/boot/install.sh
> -#
>  # This file is subject to the terms and conditions of the GNU General Public
>  # License.  See the file "COPYING" in the main directory of this archive
>  # for more details.
> @@ -17,32 +15,6 @@
>  #   $2 - kernel image file
>  #   $3 - kernel map file
>  #   $4 - default install path (blank if root directory)
> -#
> -
> -verify () {
> -	if [ ! -f "$1" ]; then
> -		echo ""                                                   1>&2
> -		echo " *** Missing file: $1"                              1>&2
> -		echo ' *** You need to run "make" before "make install".' 1>&2
> -		echo ""                                                   1>&2
> -		exit 1
> -	fi
> -}
> -
> -# Make sure the files actually exist
> -
> -verify "$2"
> -verify "$3"
> -
> -# User may have a custom install script
> -
> -if [ -n "${INSTALLKERNEL}" ]; then
> -  if [ -x ~/bin/${INSTALLKERNEL} ]; then exec ~/bin/${INSTALLKERNEL} "$@"; fi
> -  if [ -x /sbin/${INSTALLKERNEL} ]; then exec /sbin/${INSTALLKERNEL} "$@"; fi
> -  if [ -x /usr/sbin/${INSTALLKERNEL} ]; then exec /usr/sbin/${INSTALLKERNEL} "$@"; fi
> -fi
> -
> -# Default install
>  
>  if [ "$(basename $2)" = "vmlinuz" ]; then
>  # Compressed install
> 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?

Kind regards,
Nicolas