[Buildroot] [PATCH] wf111: fix overwriting module files during install

[Buildroot] [PATCH] wf111: fix overwriting module files during install

[Antoine Tenart]

From: Matthew Starr <mstarr@hedonline.com>

When installing the WF111 modules, the module.* files generated
during the kernel compilation were overrided. This ended up having
the wrong information about the modules compiled in a given image
(and only the one about the WF111 module). This could be verified
using the "modprobe -l" command, with only the wf111 module showing
up.

This patch fixes this, by removing the manual copy of the generated
files (in WF111_INSTALL_TARGET_CMDS) and by instead using the build
command to populate our target directory, containing the module.*
files. This way the files are not overrided but instead updated
with the additional WF111 informations.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Tested-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 package/wf111/wf111.mk | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/package/wf111/wf111.mk b/package/wf111/wf111.mk
index 479d665760b1..e63a1d2a19c7 100644
--- a/package/wf111/wf111.mk
+++ b/package/wf111/wf111.mk
@@ -24,11 +24,7 @@ endif
 define WF111_BUILD_CMDS
 	$(MAKE) -C $(@D) PWD=$(@D) \
 		$(LINUX_MAKE_FLAGS) KDIR=$(LINUX_DIR) \
-		install_static
-endef
-
-define WF111_INSTALL_TARGET_CMDS
-	cp -dpfr $(@D)/output/* $(TARGET_DIR)
+		OUTPUT=$(TARGET_DIR) install_static
 endef
 
 $(eval $(generic-package))
-- 
2.6.2

[Buildroot] [PATCH] wf111: fix overwriting module files during install

[Thomas Petazzoni]

Dear Antoine Tenart,

On Tue,  3 Nov 2015 17:52:13 +0100, Antoine Tenart wrote:
> From: Matthew Starr <mstarr@hedonline.com>
> 
> When installing the WF111 modules, the module.* files generated
> during the kernel compilation were overrided. This ended up having
> the wrong information about the modules compiled in a given image
> (and only the one about the WF111 module). This could be verified
> using the "modprobe -l" command, with only the wf111 module showing
> up.
> 
> This patch fixes this, by removing the manual copy of the generated
> files (in WF111_INSTALL_TARGET_CMDS) and by instead using the build
> command to populate our target directory, containing the module.*
> files. This way the files are not overrided but instead updated
> with the additional WF111 informations.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> Tested-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  package/wf111/wf111.mk | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/package/wf111/wf111.mk b/package/wf111/wf111.mk
> index 479d665760b1..e63a1d2a19c7 100644
> --- a/package/wf111/wf111.mk
> +++ b/package/wf111/wf111.mk
> @@ -24,11 +24,7 @@ endif
>  define WF111_BUILD_CMDS
>  	$(MAKE) -C $(@D) PWD=$(@D) \
>  		$(LINUX_MAKE_FLAGS) KDIR=$(LINUX_DIR) \
> -		install_static
> -endef
> -
> -define WF111_INSTALL_TARGET_CMDS
> -	cp -dpfr $(@D)/output/* $(TARGET_DIR)
> +		OUTPUT=$(TARGET_DIR) install_static
>  endef
>  
>  $(eval $(generic-package))

This is not great. Having WF111_BUILD_CMDS install things is wrong.
Please make sure the build step is separate from the install step.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] wf111: fix overwriting module files during install

[Matthew Starr]

Thomas,

The problem with this is the wf111 package depends on the manufacturer's
build and install method through their makefile included in the binary
file
they provide as an input to the wf111 package.  This is a single step
build and
install process.

You could make a new set of steps to build and install in the wf111.mk
file,
but then any manufacturer changes in future versions of the software
could and most likely will break the wf111.mk file.

Best regards,
 
Matthew Starr

> -----Original Message-----
> From: Thomas Petazzoni [mailto:thomas.petazzoni at free-electrons.com]
> Sent: Tuesday, November 03, 2015 11:34 AM
> To: Antoine Tenart
> Cc: buildroot at busybox.net; Matthew Starr
> Subject: Re: [Buildroot] [PATCH] wf111: fix overwriting module files
during
> install
> 
> Dear Antoine Tenart,
> 
> On Tue,  3 Nov 2015 17:52:13 +0100, Antoine Tenart wrote:
> > From: Matthew Starr <mstarr@hedonline.com>
> >
> > When installing the WF111 modules, the module.* files generated
during
> > the kernel compilation were overrided. This ended up having the
wrong
> > information about the modules compiled in a given image (and only
the
> > one about the WF111 module). This could be verified using the
> > "modprobe -l" command, with only the wf111 module showing up.
> >
> > This patch fixes this, by removing the manual copy of the generated
> > files (in WF111_INSTALL_TARGET_CMDS) and by instead using the build
> > command to populate our target directory, containing the module.*
> > files. This way the files are not overrided but instead updated with
> > the additional WF111 informations.
> >
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > Tested-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> >  package/wf111/wf111.mk | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/package/wf111/wf111.mk b/package/wf111/wf111.mk index
> > 479d665760b1..e63a1d2a19c7 100644
> > --- a/package/wf111/wf111.mk
> > +++ b/package/wf111/wf111.mk
> > @@ -24,11 +24,7 @@ endif
> >  define WF111_BUILD_CMDS
> >  	$(MAKE) -C $(@D) PWD=$(@D) \
> >  		$(LINUX_MAKE_FLAGS) KDIR=$(LINUX_DIR) \
> > -		install_static
> > -endef
> > -
> > -define WF111_INSTALL_TARGET_CMDS
> > -	cp -dpfr $(@D)/output/* $(TARGET_DIR)
> > +		OUTPUT=$(TARGET_DIR) install_static
> >  endef
> >
> >  $(eval $(generic-package))
> 
> This is not great. Having WF111_BUILD_CMDS install things is wrong.
> Please make sure the build step is separate from the install step.
> 
> Thanks,
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] wf111: fix overwriting module files during install

[Antoine Tenart]

On Tue, Nov 03, 2015 at 11:38:35AM -0600, Matthew Starr wrote:
> 
> The problem with this is the wf111 package depends on the manufacturer's
> build and install method through their makefile included in the binary
> file they
> provide as an input to the wf111 package.  This is a single step build
> and install
> process.  You could make a new set of steps to build and install in the
> wf111.mk file, but then any manufacturer changes in future versions of
> the
> software could and most likely will break the wf111.mk file.

I agree with Matthew, it is not possible to separate the build task from
the install one using the wf111 top Makefile as is. I see 3 solutions
here:

- We keep it this way, it's not the best solution from a Buildroot point
  of view but the package maintenance is easier.
- We make a patch to modify the top Makefile, separating properly the
  step phase from the install one.
- We explicitly do the compilation and installation in the wf111.mk
  file, by copying what is done in the Makefile (the install_static
  target calls other Makefile, and performs some actions to copy some
  files to destdir).

What do you recommend?

Thanks,

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] wf111: fix overwriting module files during install

[Thomas Petazzoni]

Hello,

On Tue,  3 Nov 2015 17:52:13 +0100, Antoine Tenart wrote:
> From: Matthew Starr <mstarr@hedonline.com>
> 
> When installing the WF111 modules, the module.* files generated
> during the kernel compilation were overrided. This ended up having
> the wrong information about the modules compiled in a given image
> (and only the one about the WF111 module). This could be verified
> using the "modprobe -l" command, with only the wf111 module showing
> up.
> 
> This patch fixes this, by removing the manual copy of the generated
> files (in WF111_INSTALL_TARGET_CMDS) and by instead using the build
> command to populate our target directory, containing the module.*
> files. This way the files are not overrided but instead updated
> with the additional WF111 informations.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> Tested-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  package/wf111/wf111.mk | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Sorry for the long delay. I've applied this patch, after changing it so
that the entire process takes place in the install-target step rather
than the build step. The rationale is mainly that for statistics
reasons, we do some tracking of which package installs what, and this
tracking is done around the install-target step, so it is quite
important that the build step doesn't install anything.

Also, I've done two other commits:

 - One that makes a proper use of the WF111_VERSION variable
   https://git.buildroot.net/buildroot/commit/?id=4389b88695ae01d0fd9e0ec69680b69c3d565095

 - One that updates the wf111 package to version 5.2.2-r2, which is
   needed for the driver to build with a modern kernel.
   https://git.buildroot.net/buildroot/commit/?id=47b9c8ae2265179bd215742fda5f572e1b1aa394

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com