| Message ID | 1446569533-25368-1-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | Accepted |
| Headers | show |
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, 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@free-electrons.com] > Sent: Tuesday, November 03, 2015 11:34 AM > To: Antoine Tenart > Cc: buildroot@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
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
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
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))