[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Robert Richter]

(trimmed cc list)

On 12.07.13 11:57:21, Robert Richter wrote:
> On 11.07.13 10:16:18, Linus Torvalds wrote:
> > On Thu, Jul 11, 2013 at 6:54 AM, Michal Marek <mmarek@suse.cz> wrote:
> > >
> > > Yeah. It also reveals another bug that we rewrite the kernel.release
> > > file each time.
> 
> The odd thing I have in a specific configuration is that depmod is
> missing the kernelrelease when I only build install rules, though I
> had built everything before and kernelrelease should be there:
> 
>  $ make <makeflags>
>  $ make <makeflags> modules_install zinstall
>  ...
>    DEPMOD  
>  Usage: .../scripts/depmod.sh /sbin/depmod <kernelrelease>
> 
> Note that makeflags include the -j option, INSTALL_MOD_PATH and
> INSTALL_PATH variables set, so no root perms required in this case.

I narrowed this down. The problem is that zinstall on ARCH=arm has a
dependency to vmlinux which does a prepare/prepare3 and finally does a
forced rebuild of kernel.release even if it exists already.

Rebuilding it removes kernel.release first and then recreates it. This
might race with another parallel make job running depmod.

So on arm and maybe other archs we need the same as for x86:

 1648e4f8 x86, kbuild: make "make install" not depend on vmlinux

The patch below fixes this for arm. It is build-tested in my
environment, but please test it in others too if possible.

Thanks,

-Robert



>From 7e751c909830af4459296e7f2e0cb2321295e661 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@linaro.org>
Date: Wed, 17 Jul 2013 16:17:20 +0200
Subject: [PATCH] arm, kbuild: make "make install" not depend on vmlinux

Install targets (install, zinstall, uinstall) on arm have a dependency
to vmlinux. This may cause parts of the kernel to be rebuilt during
installation. We must avoid this since this may run as root. Install
targets "ABSOLUTELY MUST NOT MODIFY THE SOURCE TREE." as Linus
emphasized this in:

 http://lkml.org/lkml/2013/7/10/600

So on arm and maybe other archs we need the same as for x86:

 1648e4f8 x86, kbuild: make "make install" not depend on vmlinux

This patch fixes this for arm. Dependencies are removed and instead a
check to install.sh is added for the files that are needed.

This issue was uncovered by this build error where the -j option is
used in conjunction with install targets:

 $ make <makeflags>
 $ make <makeflags> zinstall
 ...
   DEPMOD
 Usage: .../scripts/depmod.sh /sbin/depmod <kernelrelease>

(INSTALL_MOD_PATH and INSTALL_PATH variables set, so no root perms
required in this case.)

The problem is that zinstall on arm due to its dependency to vmlinux
does a prepare/prepare3 and finally does a forced rewrite of
kernel.release even if it exists already.

Rebuilding kernel.release removes it first and then recreates it. This
might race with another parallel make job running depmod.

So this patch should fix this one too.

Also quoting $(KERNELRELEASE) arg for install.sh as this messes
argument order in case it is empty (which is the case if the kernel
was not built yet).

Signed-off-by: Robert Richter <robert.richter@linaro.org>
Signed-off-by: Robert Richter <rric@kernel.org>
---
 arch/arm/Makefile        |    9 +++++++--
 arch/arm/boot/Makefile   |   16 ++++++++--------
 arch/arm/boot/install.sh |   14 ++++++++++++++
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index c0ac0f5..fe63986 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -295,10 +295,15 @@ archprepare:
 # Convert bzImage to zImage
 bzImage: zImage
 
-zImage Image xipImage bootpImage uImage: vmlinux
+BOOT_TARGETS	= zImage Image xipImage bootpImage uImage
+INSTALL_TARGETS	= zinstall uinstall install
+
+PHONY += bzImage $(BOOT_TARGETS) $(INSTALL_TARGETS)
+
+$(BOOT_TARGETS): vmlinux
 	$(Q)$(MAKE) $(build)=$(boot) MACHINE=$(MACHINE) $(boot)/$@
 
-zinstall uinstall install: vmlinux
+$(INSTALL_TARGETS):
 	$(Q)$(MAKE) $(build)=$(boot) MACHINE=$(MACHINE) $@
 
 %.dtb: | scripts
diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
index 84aa2ca..ec2f806 100644
--- a/arch/arm/boot/Makefile
+++ b/arch/arm/boot/Makefile
@@ -95,24 +95,24 @@ initrd:
 	@test "$(INITRD)" != "" || \
 	(echo You must specify INITRD; exit -1)
 
-install: $(obj)/Image
-	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh $(KERNELRELEASE) \
+install:
+	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh "$(KERNELRELEASE)" \
 	$(obj)/Image System.map "$(INSTALL_PATH)"
 
-zinstall: $(obj)/zImage
-	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh $(KERNELRELEASE) \
+zinstall:
+	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh "$(KERNELRELEASE)" \
 	$(obj)/zImage System.map "$(INSTALL_PATH)"
 
-uinstall: $(obj)/uImage
-	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh $(KERNELRELEASE) \
+uinstall:
+	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh "$(KERNELRELEASE)" \
 	$(obj)/uImage System.map "$(INSTALL_PATH)"
 
 zi:
-	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh $(KERNELRELEASE) \
+	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh "$(KERNELRELEASE)" \
 	$(obj)/zImage System.map "$(INSTALL_PATH)"
 
 i:
-	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh $(KERNELRELEASE) \
+	$(CONFIG_SHELL) $(srctree)/$(src)/install.sh "$(KERNELRELEASE)" \
 	$(obj)/Image System.map "$(INSTALL_PATH)"
 
 subdir-	    := bootp compressed dts
diff --git a/arch/arm/boot/install.sh b/arch/arm/boot/install.sh
index 06ea7d4..2a45092 100644
--- a/arch/arm/boot/install.sh
+++ b/arch/arm/boot/install.sh
@@ -20,6 +20,20 @@
 #   $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
-- 
1.7.7.6

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Linus Torvalds]

On Wed, Jul 17, 2013 at 9:05 AM, Robert Richter <rric@kernel.org> wrote:
>
> I narrowed this down. The problem is that zinstall on ARCH=arm has a
> dependency to vmlinux which does a prepare/prepare3 and finally does a
> forced rebuild of kernel.release even if it exists already.
>
> Rebuilding it removes kernel.release first and then recreates it. This
> might race with another parallel make job running depmod.
>
> So on arm and maybe other archs we need the same as for x86:
>
>  1648e4f8 x86, kbuild: make "make install" not depend on vmlinux
>
> The patch below fixes this for arm. It is build-tested in my
> environment, but please test it in others too if possible.

Ack. This looks like the right thing to do.

That said, I think for clarity you might perhaps want to make the
unrelated change of adding double quotes around $(KERNELRELEASE) a
separate patch. Not a big deal, but it took me a moment to say "why
did he do that" (in fact, I'm not sure KERNEL_RELEASE can actually
validly have spaces in it, but quoting it is certainly not wrong
either - but it might be unnecessary.

              Linus

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Robert Richter]

On 17.07.13 09:36:57, Linus Torvalds wrote:
> On Wed, Jul 17, 2013 at 9:05 AM, Robert Richter <rric@kernel.org> wrote:
> >
> > I narrowed this down. The problem is that zinstall on ARCH=arm has a
> > dependency to vmlinux which does a prepare/prepare3 and finally does a
> > forced rebuild of kernel.release even if it exists already.
> >
> > Rebuilding it removes kernel.release first and then recreates it. This
> > might race with another parallel make job running depmod.
> >
> > So on arm and maybe other archs we need the same as for x86:
> >
> >  1648e4f8 x86, kbuild: make "make install" not depend on vmlinux
> >
> > The patch below fixes this for arm. It is build-tested in my
> > environment, but please test it in others too if possible.
> 
> Ack. This looks like the right thing to do.

Thanks for looking at this.

> That said, I think for clarity you might perhaps want to make the
> unrelated change of adding double quotes around $(KERNELRELEASE) a
> separate patch. Not a big deal, but it took me a moment to say "why
> did he do that" (in fact, I'm not sure KERNEL_RELEASE can actually
> validly have spaces in it, but quoting it is certainly not wrong
> either - but it might be unnecessary.

If kernel.release doen't exist KERNEL_RELEASE is empty, thus all args
for install.sh shift one arg left. Putting it in quotes avoids this.
Noticed this since the first verified file in install.sh ($2) should
be the image file but was already System.map ($3).

Will splitt the patches and resend.

-Robert

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Linus Torvalds]

On Wed, Jul 17, 2013 at 9:57 AM, Robert Richter <rric@kernel.org> wrote:
>
> If kernel.release doen't exist KERNEL_RELEASE is empty, thus all args
> for install.sh shift one arg left.

You know what? I read the patch, but I didn't read your changelog well
enough. You already explained that in the commit log, and I just
missed it like some blind monkey. My bad.

> Will splitt the patches and resend.

No need to split, I was wrong, it wasn't some unrelated cleanup, and
you were right the first time.

So Michal (or ARM people - whoever wants to take the patch), just take
my ack. No objections.

                 Linus

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Michal Marek]

On 17.7.2013 19:03, Linus Torvalds wrote:
> On Wed, Jul 17, 2013 at 9:57 AM, Robert Richter <rric@kernel.org> wrote:
>>
>> If kernel.release doen't exist KERNEL_RELEASE is empty, thus all args
>> for install.sh shift one arg left.
> 
> You know what? I read the patch, but I didn't read your changelog well
> enough. You already explained that in the commit log, and I just
> missed it like some blind monkey. My bad.
> 
>> Will splitt the patches and resend.
> 
> No need to split, I was wrong, it wasn't some unrelated cleanup, and
> you were right the first time.
> 
> So Michal (or ARM people - whoever wants to take the patch), just take
> my ack. No objections.

I can add it to the kbuild tree if needed. Otherwise you can add
Acked-by: Michal Marek <mmarek@suse.cz>.

Michal

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Robert Richter]

On 18.07.13 11:22:24, Michal Marek wrote:
> > So Michal (or ARM people - whoever wants to take the patch), just take
> > my ack. No objections.
> 
> I can add it to the kbuild tree if needed. Otherwise you can add
> Acked-by: Michal Marek <mmarek@suse.cz>.

This didn't make it upstream yet, can somebody at it to a tree?

Thanks,

-Robert

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Yann E. MORIN]

Robert, All,

On 2013-09-30 10:49 +0200, Robert Richter spake thusly:
> On 18.07.13 11:22:24, Michal Marek wrote:
> > > So Michal (or ARM people - whoever wants to take the patch), just take
> > > my ack. No objections.
> > 
> > I can add it to the kbuild tree if needed. Otherwise you can add
> > Acked-by: Michal Marek <mmarek@suse.cz>.
> 
> This didn't make it upstream yet, can somebody at it to a tree?

Since it's been acked-by Linus, I'll queue it in my tree, for Michal to
pull from. Expect a pull-request soon.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Geert Uytterhoeven]

On Mon, Sep 30, 2013 at 6:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> On 2013-09-30 10:49 +0200, Robert Richter spake thusly:
>> On 18.07.13 11:22:24, Michal Marek wrote:
>> > > So Michal (or ARM people - whoever wants to take the patch), just take
>> > > my ack. No objections.
>> >
>> > I can add it to the kbuild tree if needed. Otherwise you can add
>> > Acked-by: Michal Marek <mmarek@suse.cz>.
>>
>> This didn't make it upstream yet, can somebody at it to a tree?
>
> Since it's been acked-by Linus, I'll queue it in my tree, for Michal to
> pull from. Expect a pull-request soon.

Sorry for chiming in that late, but I didn't think of this when reading the
original submission.

Just doing "make oldconfig; make install" used to work.
Removing the dependency of "make vmlinux" on vmlinux breaks this, doesn't it?

I had the habit of doing the above many years ago, when I was mostly doing
native builds, and before I had my own custom linux-install-kernel
script that e.g.
knows how to copy kernels and modules around to NFS servers.

Not that I'm strongly attached to it, but there may be other users...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Michal Marek]

On 9.10.2013 09:18, Geert Uytterhoeven wrote:
> On Mon, Sep 30, 2013 at 6:31 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>> On 2013-09-30 10:49 +0200, Robert Richter spake thusly:
>>> On 18.07.13 11:22:24, Michal Marek wrote:
>>>>> So Michal (or ARM people - whoever wants to take the patch), just take
>>>>> my ack. No objections.
>>>>
>>>> I can add it to the kbuild tree if needed. Otherwise you can add
>>>> Acked-by: Michal Marek <mmarek@suse.cz>.
>>>
>>> This didn't make it upstream yet, can somebody at it to a tree?
>>
>> Since it's been acked-by Linus, I'll queue it in my tree, for Michal to
>> pull from. Expect a pull-request soon.
> 
> Sorry for chiming in that late, but I didn't think of this when reading the
> original submission.
> 
> Just doing "make oldconfig; make install" used to work.

On ARM and maybe other architectures.


> Removing the dependency of "make vmlinux" on vmlinux breaks this, doesn't it?

Yes.


> I had the habit of doing the above many years ago, when I was mostly doing
> native builds, and before I had my own custom linux-install-kernel
> script that e.g.
> knows how to copy kernels and modules around to NFS servers.
> 
> Not that I'm strongly attached to it, but there may be other users...

We can't eat the cake and have it :). What can be done is to make
arch/arm/boot/install.sh print a friendlier error message, like the x86
version does:

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

Michal

[PATCH] arm, kbuild: make "make install" not depend on vmlinux

[Geert Uytterhoeven]

On Wed, Oct 9, 2013 at 12:28 PM, Michal Marek <mmarek@suse.cz> wrote:
> On 9.10.2013 09:18, Geert Uytterhoeven wrote:
>> Sorry for chiming in that late, but I didn't think of this when reading the
>> original submission.
>>
>> Just doing "make oldconfig; make install" used to work.
>
> On ARM and maybe other architectures.

Until 2009 it also worked on x86 ;-)

>> Removing the dependency of "make vmlinux" on vmlinux breaks this, doesn't it?
>
>> Not that I'm strongly attached to it, but there may be other users...
>
> We can't eat the cake and have it :). What can be done is to make

Sure.

> arch/arm/boot/install.sh print a friendlier error message, like the x86
> version does:
>
> 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

Definitely!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds