| Message ID | 20240210074601.5363-2-xtex@envs.net (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | kbuild: Fix install errors when INSTALL_PATH does not exist | expand |
On Sat, Feb 10, 2024 at 03:46:00PM +0800, Zhang Bingwu wrote: > From: Zhang Bingwu <xtexchooser@duck.com> > > Setting '-e' flag tells shells to exit with error exit code immediately > after any of commands fails, and causes make(1) to regard recipes as > failed. > > Before this, make will still continue to succeed even after the > installation failed, for example, for insufficient permission or > directory does not exist. > > Signed-off-by: Zhang Bingwu <xtexchooser@duck.com> > --- > arch/arm/boot/install.sh | 2 ++ > arch/arm64/boot/install.sh | 2 ++ > arch/m68k/install.sh | 2 ++ > arch/nios2/boot/install.sh | 2 ++ > arch/parisc/install.sh | 2 ++ > arch/riscv/boot/install.sh | 2 ++ > arch/s390/boot/install.sh | 2 ++ > arch/sparc/boot/install.sh | 2 ++ > arch/x86/boot/install.sh | 2 ++ > 9 files changed, 18 insertions(+) > > diff --git a/arch/arm/boot/install.sh b/arch/arm/boot/install.sh > index 9ec11fac7d8d..34e2c6e31fd1 100755 > --- a/arch/arm/boot/install.sh > +++ b/arch/arm/boot/install.sh > @@ -17,6 +17,8 @@ > # $3 - kernel map file > # $4 - default install path (blank if root directory) > > +set -e > + What about #!/bin/sh -e on the first line, which is the more normal way to do this for an entire script?
On Saturday, February 10, 2024 6:29:00 PM CST Russell King (Oracle) wrote: > What about #!/bin/sh -e on the first line, which is the more normal way > to do this for an entire script? Will do this in V2.
On Sat, Feb 10, 2024 at 10:29:00AM +0000 Russell King (Oracle) wrote: > On Sat, Feb 10, 2024 at 03:46:00PM +0800, Zhang Bingwu wrote: > > From: Zhang Bingwu <xtexchooser@duck.com> > > > > Setting '-e' flag tells shells to exit with error exit code immediately > > after any of commands fails, and causes make(1) to regard recipes as > > failed. > > > > Before this, make will still continue to succeed even after the > > installation failed, for example, for insufficient permission or > > directory does not exist. > > > > Signed-off-by: Zhang Bingwu <xtexchooser@duck.com> > > --- Thanks for fixing! [...] > > diff --git a/arch/arm/boot/install.sh b/arch/arm/boot/install.sh > > index 9ec11fac7d8d..34e2c6e31fd1 100755 > > --- a/arch/arm/boot/install.sh > > +++ b/arch/arm/boot/install.sh > > @@ -17,6 +17,8 @@ > > # $3 - kernel map file > > # $4 - default install path (blank if root directory) > > > > +set -e > > + > > What about #!/bin/sh -e on the first line, which is the more normal way > to do this for an entire script? are you sure? I can find many more occurrences of 'set -e' than the shebang version in the Linux tree, especially in the kbuild scripts, thus it's bike-shedding, isn't it? Reviewed-by: Nicolas Schier <nicolas@fjasle.eu> Kind regards, Nicolas
On Sun, Feb 11, 2024 at 6:21 AM Nicolas Schier <nicolas@fjasle.eu> wrote: > > On Sat, Feb 10, 2024 at 10:29:00AM +0000 Russell King (Oracle) wrote: > > On Sat, Feb 10, 2024 at 03:46:00PM +0800, Zhang Bingwu wrote: > > > From: Zhang Bingwu <xtexchooser@duck.com> > > > > > > Setting '-e' flag tells shells to exit with error exit code immediately > > > after any of commands fails, and causes make(1) to regard recipes as > > > failed. > > > > > > Before this, make will still continue to succeed even after the > > > installation failed, for example, for insufficient permission or > > > directory does not exist. > > > > > > Signed-off-by: Zhang Bingwu <xtexchooser@duck.com> > > > --- > > Thanks for fixing! > > [...] > > > diff --git a/arch/arm/boot/install.sh b/arch/arm/boot/install.sh > > > index 9ec11fac7d8d..34e2c6e31fd1 100755 > > > --- a/arch/arm/boot/install.sh > > > +++ b/arch/arm/boot/install.sh > > > @@ -17,6 +17,8 @@ > > > # $3 - kernel map file > > > # $4 - default install path (blank if root directory) > > > > > > +set -e > > > + > > > > What about #!/bin/sh -e on the first line, which is the more normal way > > to do this for an entire script? > > are you sure? I can find many more occurrences of 'set -e' than the > shebang version in the Linux tree, especially in the kbuild scripts, thus > it's bike-shedding, isn't it? > > Reviewed-by: Nicolas Schier <nicolas@fjasle.eu> > > Kind regards, > Nicolas When you put -e on the shebang line, like #!/bin/sh -e the option -e is set when you do: $ arch/arm/boot/install.sh But, -e is not set when you do: $ sh arch/arm/boot/install.sh The reason is obvious because the latter case does not use the shebang line. In Kbuild, some places run the script directly like the former case, and others use CONFIG_SHELL like $(CONFIG_SHELL) arch/arm/boot/install.sh The inconsistency is not nice, but that is a different issue. The separate 'set -e' statement works for both cases, so I think this is safer, though it is kind of bike-shedding.
On Sunday, February 11, 2024 7:35:35 AM CST Masahiro Yamada wrote: > > The separate 'set -e' statement works for both cases, > so I think this is safer, though it is kind of bike-shedding. Thanks! I also think it is safer to use 'set -e' in the case of 'sh install.sh', so I support not to use 'sh -e' in the shebang line. The planned V2 patch for this disappeared.
diff --git a/arch/arm/boot/install.sh b/arch/arm/boot/install.sh index 9ec11fac7d8d..34e2c6e31fd1 100755 --- a/arch/arm/boot/install.sh +++ b/arch/arm/boot/install.sh @@ -17,6 +17,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ "$(basename $2)" = "zImage" ]; then # Compressed install echo "Installing compressed kernel" diff --git a/arch/arm64/boot/install.sh b/arch/arm64/boot/install.sh index 9b7a09808a3d..cc2f4ccca6c0 100755 --- a/arch/arm64/boot/install.sh +++ b/arch/arm64/boot/install.sh @@ -17,6 +17,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ "$(basename $2)" = "Image.gz" ] || [ "$(basename $2)" = "vmlinuz.efi" ] then # Compressed install diff --git a/arch/m68k/install.sh b/arch/m68k/install.sh index af65e16e5147..b6829b3942b3 100755 --- a/arch/m68k/install.sh +++ b/arch/m68k/install.sh @@ -16,6 +16,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ -f $4/vmlinuz ]; then mv $4/vmlinuz $4/vmlinuz.old fi diff --git a/arch/nios2/boot/install.sh b/arch/nios2/boot/install.sh index 34a2feec42c8..1161f2bf59ec 100755 --- a/arch/nios2/boot/install.sh +++ b/arch/nios2/boot/install.sh @@ -16,6 +16,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ -f $4/vmlinuz ]; then mv $4/vmlinuz $4/vmlinuz.old fi diff --git a/arch/parisc/install.sh b/arch/parisc/install.sh index 933d031c249a..664c2d77f776 100755 --- a/arch/parisc/install.sh +++ b/arch/parisc/install.sh @@ -16,6 +16,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ "$(basename $2)" = "vmlinuz" ]; then # Compressed install echo "Installing compressed kernel" diff --git a/arch/riscv/boot/install.sh b/arch/riscv/boot/install.sh index 4c63f3f0643d..a59639dff64f 100755 --- a/arch/riscv/boot/install.sh +++ b/arch/riscv/boot/install.sh @@ -17,6 +17,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ "$(basename $2)" = "Image.gz" ]; then # Compressed install echo "Installing compressed kernel" diff --git a/arch/s390/boot/install.sh b/arch/s390/boot/install.sh index a13dd2f2aa1c..fa41486258ee 100755 --- a/arch/s390/boot/install.sh +++ b/arch/s390/boot/install.sh @@ -15,6 +15,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + echo "Warning: '${INSTALLKERNEL}' command not available - additional " \ "bootloader config required" >&2 if [ -f "$4/vmlinuz-$1" ]; then mv -- "$4/vmlinuz-$1" "$4/vmlinuz-$1.old"; fi diff --git a/arch/sparc/boot/install.sh b/arch/sparc/boot/install.sh index 4f130f3f30d6..68de67c5621e 100755 --- a/arch/sparc/boot/install.sh +++ b/arch/sparc/boot/install.sh @@ -16,6 +16,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ -f $4/vmlinuz ]; then mv $4/vmlinuz $4/vmlinuz.old fi diff --git a/arch/x86/boot/install.sh b/arch/x86/boot/install.sh index 0849f4b42745..93784abcd66d 100755 --- a/arch/x86/boot/install.sh +++ b/arch/x86/boot/install.sh @@ -16,6 +16,8 @@ # $3 - kernel map file # $4 - default install path (blank if root directory) +set -e + if [ -f $4/vmlinuz ]; then mv $4/vmlinuz $4/vmlinuz.old fi