[Buildroot] [PATCH 1/2] package/pkg-generic.mk: create folders before calling hooks

[Buildroot] [PATCH 1/2] package/pkg-generic.mk: create folders before calling hooks

[Thomas Petazzoni]

In commit 0e2be4db8ab01d479177a3a187c22525752195ae
("package/pkg-generic: make file list logic parallel build
compatible"), the logic to create the list of files installed by a
particular package was significantly reworked to be compatible with
top-level parallel build.

Before this commit, there was only a after-install step of listing the
files in HOST_DIR/TARGET_DIR/STAGING_DIR. But after this commit, we
now have a before-install logic and an after-install logic.

It turns out that when the before-install logic is called for the very
first host package, $(HOST_DIR) doesn't exist yet, and therefore the
cd $(2) fails, with an error message:

/bin/sh: line 0: cd: /home/thomas/buildroot/output/host: No such file or directory

In fact, $(HOST_DIR), $(STAGING_DIR), $(TARGET_DIR) and
$(BINARIES_DIR) are created by the make rules for host installation,
staging installation, target installation and images installation, but
*after* calling the step_start hooks.

So, we simply fix this problem by creating the directories *before*
calling the step_start hooks.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
This is different solution than the one initially proposed at
http://patchwork.ozlabs.org/patch/1252046/, following some discussion
with Yann E. Morin on IRC. Since the creation of the directories is
already done somewhere, it makes sense to rely on that rather than
adding more code to create them in a difference place.

This patch needs to be backported to 2020.02.x.
---
 package/pkg-generic.mk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index c1b9fe2e59..47238f2649 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -273,9 +273,9 @@ $(BUILD_DIR)/%/.stamp_built::
 
 # Install to host dir
 $(BUILD_DIR)/%/.stamp_host_installed:
+	@mkdir -p $(HOST_DIR)
 	@$(call step_start,install-host)
 	@$(call MESSAGE,"Installing to host directory")
-	@mkdir -p $(HOST_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
@@ -303,9 +303,9 @@ $(BUILD_DIR)/%/.stamp_host_installed:
 # empty when we use an internal toolchain.
 #
 $(BUILD_DIR)/%/.stamp_staging_installed:
+	@mkdir -p $(STAGING_DIR)
 	@$(call step_start,install-staging)
 	@$(call MESSAGE,"Installing to staging directory")
-	@mkdir -p $(STAGING_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
@@ -345,8 +345,8 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 
 # Install to images dir
 $(BUILD_DIR)/%/.stamp_images_installed:
-	@$(call step_start,install-image)
 	@mkdir -p $(BINARIES_DIR)
+	@$(call step_start,install-image)
 	@$(call MESSAGE,"Installing to images directory")
 	$(foreach hook,$($(PKG)_PRE_INSTALL_IMAGES_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_IMAGES_CMDS)
@@ -356,9 +356,9 @@ $(BUILD_DIR)/%/.stamp_images_installed:
 
 # Install to target dir
 $(BUILD_DIR)/%/.stamp_target_installed:
+	@mkdir -p $(TARGET_DIR)
 	@$(call step_start,install-target)
 	@$(call MESSAGE,"Installing to target")
-	@mkdir -p $(TARGET_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_TARGET_CMDS)
 	$(if $(BR2_INIT_SYSTEMD),\
-- 
2.24.1

[Buildroot] [PATCH 2/2] Makefile: don't hang the build if there are no file lists

[Thomas Petazzoni]

In very limited configurations, it is possible to have a case where no
.files-list-staging.txt files are created. In this case:

	cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt)) > \
		$(BUILD_DIR)/packages-file-list-staging.txt

becomes:

	cat > \
		$(BUILD_DIR)/packages-file-list-staging.txt

which of course makes the build hang.. forever.

So we fix this by checking the list is not empty. To keep the code
readable, we introduce an intermediate variable to store the list of
these files.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
This is a fix, it should be backported to 2020.02.x
---
 Makefile | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 074d08c04a..3fdd52c677 100644
--- a/Makefile
+++ b/Makefile
@@ -727,6 +727,10 @@ $(TARGETS_ROOTFS): target-finalize
 # Avoid the rootfs name leaking down the dependency chain
 target-finalize: ROOTFS=
 
+TARGET_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt))
+HOST_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt))
+STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt))
+
 .PHONY: host-finalize
 host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
 	@$(call MESSAGE,"Finalizing host directory")
@@ -807,12 +811,12 @@ endif # merged /usr
 
 	touch $(TARGET_DIR)/usr
 
-	cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt)) > \
-		$(BUILD_DIR)/packages-file-list.txt
-	cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt)) > \
-		$(BUILD_DIR)/packages-file-list-host.txt
-	cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt)) > \
-		$(BUILD_DIR)/packages-file-list-staging.txt
+	$(if $(TARGET_DIR_FILES_LISTS), \
+		cat $(TARGET_DIR_FILES_LISTS) > $(BUILD_DIR)/packages-file-list.txt)
+	$(if $(HOST_DIR_FILES_LISTS), \
+		cat $(HOST_DIR_FILES_LISTS) > $(BUILD_DIR)/packages-file-list-host.txt)
+	$(if $(STAGING_DIR_FILES_LISTS), \
+		cat $(STAGING_DIR_FILES_LISTS) > $(BUILD_DIR)/packages-file-list-staging.txt)
 
 .PHONY: target-post-image
 target-post-image: $(TARGETS_ROOTFS) target-finalize staging-finalize
-- 
2.24.1

[Buildroot] [PATCH 2/2] Makefile: don't hang the build if there are no file lists

[Thomas De Schampheleire]

El jue., 12 mar. 2020 a las 10:15, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribi?:
>
> In very limited configurations, it is possible to have a case where no
> .files-list-staging.txt files are created. In this case:
>
>         cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt)) > \
>                 $(BUILD_DIR)/packages-file-list-staging.txt
>
> becomes:
>
>         cat > \
>                 $(BUILD_DIR)/packages-file-list-staging.txt
>
> which of course makes the build hang.. forever.
>
> So we fix this by checking the list is not empty. To keep the code
> readable, we introduce an intermediate variable to store the list of
> these files.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> This is a fix, it should be backported to 2020.02.x
> ---
>  Makefile | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 074d08c04a..3fdd52c677 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -727,6 +727,10 @@ $(TARGETS_ROOTFS): target-finalize
>  # Avoid the rootfs name leaking down the dependency chain
>  target-finalize: ROOTFS=
>
> +TARGET_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt))
> +HOST_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt))
> +STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt))
> +
>  .PHONY: host-finalize
>  host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
>         @$(call MESSAGE,"Finalizing host directory")
> @@ -807,12 +811,12 @@ endif # merged /usr
>
>         touch $(TARGET_DIR)/usr
>
> -       cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt)) > \
> -               $(BUILD_DIR)/packages-file-list.txt
> -       cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt)) > \
> -               $(BUILD_DIR)/packages-file-list-host.txt
> -       cat $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt)) > \
> -               $(BUILD_DIR)/packages-file-list-staging.txt
> +       $(if $(TARGET_DIR_FILES_LISTS), \
> +               cat $(TARGET_DIR_FILES_LISTS) > $(BUILD_DIR)/packages-file-list.txt)
> +       $(if $(HOST_DIR_FILES_LISTS), \
> +               cat $(HOST_DIR_FILES_LISTS) > $(BUILD_DIR)/packages-file-list-host.txt)
> +       $(if $(STAGING_DIR_FILES_LISTS), \
> +               cat $(STAGING_DIR_FILES_LISTS) > $(BUILD_DIR)/packages-file-list-staging.txt)


An alternative approach that does not require checking for empty, is
to quote the argument to cat. I.e.
cat "$(sort ...)" will not hang. It would give an error though, so
your check is actually better.

But I found two additional problems in this area:

1. packages that set FOO_SUBDIR will have their file list created in
the FOO_SUBDIR, and it is not picked up by the current '$(wildcard
$(BUILD_DIR)/*/.files-list.txt)'.  Since make does not support a '**'
as recursive wildcard, we'd need to use a find or a more clever
method.

2. Previously, the packages-file-list.txt was already available by the
time the post-build scripts are run. But with the recent changes, this
is no longer true. Moving the 'cat' lines up solves that.

For 2 I have a patch but it conflicts with the above ones, so better
to apply yours first. Alternatively I can send the entire series. Let
me know what you prefer.

Best regards,
Thomas

[Buildroot] [PATCH 2/2] Makefile: don't hang the build if there are no file lists

[Thomas Petazzoni]

Hello Thomas,

On Tue, 17 Mar 2020 12:13:24 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> An alternative approach that does not require checking for empty, is
> to quote the argument to cat. I.e.
> cat "$(sort ...)" will not hang. It would give an error though, so
> your check is actually better.

OK. By the way, did you test the new logic in your use-cases? I know
you've been using these file lists as you've contributed some changes
in this area. Peter applied my patches very quickly (which is good, of
course), but that didn't leave a lot of time for people to give some
feedback.

> But I found two additional problems in this area:
> 
> 1. packages that set FOO_SUBDIR will have their file list created in
> the FOO_SUBDIR, and it is not picked up by the current '$(wildcard
> $(BUILD_DIR)/*/.files-list.txt)'.  Since make does not support a '**'
> as recursive wildcard, we'd need to use a find or a more clever
> method.

What about generating the files in $($(PKG)_DIR) instead of
$($(PKG)_BUILDDIR) ? This way, it will always be in
output/build/foo-version/ and never in a sub-directory.

> 2. Previously, the packages-file-list.txt was already available by the
> time the post-build scripts are run. But with the recent changes, this
> is no longer true. Moving the 'cat' lines up solves that.
> 
> For 2 I have a patch but it conflicts with the above ones, so better
> to apply yours first. Alternatively I can send the entire series. Let
> me know what you prefer.

You can send the entire series I'd say. If you have tested my patch,
add your Reviewed-by/Tested-by, so that I can apply my own patch as
well :-)

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/2] Makefile: don't hang the build if there are no file lists

[Thomas De Schampheleire]

El mar., 17 mar. 2020 a las 13:14, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribi?:
>
> Hello Thomas,
>
> On Tue, 17 Mar 2020 12:13:24 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > An alternative approach that does not require checking for empty, is
> > to quote the argument to cat. I.e.
> > cat "$(sort ...)" will not hang. It would give an error though, so
> > your check is actually better.
>
> OK. By the way, did you test the new logic in your use-cases? I know
> you've been using these file lists as you've contributed some changes
> in this area. Peter applied my patches very quickly (which is good, of
> course), but that didn't leave a lot of time for people to give some
> feedback.

At first sight it seems to work, but I still need to make a real
comparison between the old situation and the new to be sure.

>
> > But I found two additional problems in this area:
> >
> > 1. packages that set FOO_SUBDIR will have their file list created in
> > the FOO_SUBDIR, and it is not picked up by the current '$(wildcard
> > $(BUILD_DIR)/*/.files-list.txt)'.  Since make does not support a '**'
> > as recursive wildcard, we'd need to use a find or a more clever
> > method.
>
> What about generating the files in $($(PKG)_DIR) instead of
> $($(PKG)_BUILDDIR) ? This way, it will always be in
> output/build/foo-version/ and never in a sub-directory.

Ok that makes sense and works too.

>
> > 2. Previously, the packages-file-list.txt was already available by the
> > time the post-build scripts are run. But with the recent changes, this
> > is no longer true. Moving the 'cat' lines up solves that.
> >
> > For 2 I have a patch but it conflicts with the above ones, so better
> > to apply yours first. Alternatively I can send the entire series. Let
> > me know what you prefer.
>
> You can send the entire series I'd say. If you have tested my patch,
> add your Reviewed-by/Tested-by, so that I can apply my own patch as
> well :-)

Ok, I made the proposed changes, will run some further tests, and then
send them.

Thanks for the feedback,
Thomas

[Buildroot] [PATCH 1/2] package/pkg-generic.mk: create folders before calling hooks

[Yann E. MORIN]

Thomas, All,

On 2020-03-12 10:15 +0100, Thomas Petazzoni spake thusly:
> In commit 0e2be4db8ab01d479177a3a187c22525752195ae
> ("package/pkg-generic: make file list logic parallel build
> compatible"), the logic to create the list of files installed by a
> particular package was significantly reworked to be compatible with
> top-level parallel build.
> 
> Before this commit, there was only a after-install step of listing the
> files in HOST_DIR/TARGET_DIR/STAGING_DIR. But after this commit, we
> now have a before-install logic and an after-install logic.
> 
> It turns out that when the before-install logic is called for the very
> first host package, $(HOST_DIR) doesn't exist yet, and therefore the
> cd $(2) fails, with an error message:
> 
> /bin/sh: line 0: cd: /home/thomas/buildroot/output/host: No such file or directory
> 
> In fact, $(HOST_DIR), $(STAGING_DIR), $(TARGET_DIR) and
> $(BINARIES_DIR) are created by the make rules for host installation,
> staging installation, target installation and images installation, but
> *after* calling the step_start hooks.
> 
> So, we simply fix this problem by creating the directories *before*
> calling the step_start hooks.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Applied to master, thanks.

I just changed the commit title: s/folders/directories/

Regards,
Yann E. MORIN.

> ---
> This is different solution than the one initially proposed at
> http://patchwork.ozlabs.org/patch/1252046/, following some discussion
> with Yann E. Morin on IRC. Since the creation of the directories is
> already done somewhere, it makes sense to rely on that rather than
> adding more code to create them in a difference place.
> 
> This patch needs to be backported to 2020.02.x.
> ---
>  package/pkg-generic.mk | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index c1b9fe2e59..47238f2649 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -273,9 +273,9 @@ $(BUILD_DIR)/%/.stamp_built::
>  
>  # Install to host dir
>  $(BUILD_DIR)/%/.stamp_host_installed:
> +	@mkdir -p $(HOST_DIR)
>  	@$(call step_start,install-host)
>  	@$(call MESSAGE,"Installing to host directory")
> -	@mkdir -p $(HOST_DIR)
>  	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
>  	+$($(PKG)_INSTALL_CMDS)
>  	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
> @@ -303,9 +303,9 @@ $(BUILD_DIR)/%/.stamp_host_installed:
>  # empty when we use an internal toolchain.
>  #
>  $(BUILD_DIR)/%/.stamp_staging_installed:
> +	@mkdir -p $(STAGING_DIR)
>  	@$(call step_start,install-staging)
>  	@$(call MESSAGE,"Installing to staging directory")
> -	@mkdir -p $(STAGING_DIR)
>  	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
>  	+$($(PKG)_INSTALL_STAGING_CMDS)
>  	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
> @@ -345,8 +345,8 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
>  
>  # Install to images dir
>  $(BUILD_DIR)/%/.stamp_images_installed:
> -	@$(call step_start,install-image)
>  	@mkdir -p $(BINARIES_DIR)
> +	@$(call step_start,install-image)
>  	@$(call MESSAGE,"Installing to images directory")
>  	$(foreach hook,$($(PKG)_PRE_INSTALL_IMAGES_HOOKS),$(call $(hook))$(sep))
>  	+$($(PKG)_INSTALL_IMAGES_CMDS)
> @@ -356,9 +356,9 @@ $(BUILD_DIR)/%/.stamp_images_installed:
>  
>  # Install to target dir
>  $(BUILD_DIR)/%/.stamp_target_installed:
> +	@mkdir -p $(TARGET_DIR)
>  	@$(call step_start,install-target)
>  	@$(call MESSAGE,"Installing to target")
> -	@mkdir -p $(TARGET_DIR)
>  	$(foreach hook,$($(PKG)_PRE_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
>  	+$($(PKG)_INSTALL_TARGET_CMDS)
>  	$(if $(BR2_INIT_SYSTEMD),\
> -- 
> 2.24.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

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

[Buildroot] [PATCH 2/2] Makefile: don't hang the build if there are no file lists

[Yann E. MORIN]

Thomas?, All,

On 2020-03-17 14:58 +0100, Thomas De Schampheleire spake thusly:
> El mar., 17 mar. 2020 a las 13:14, Thomas Petazzoni
> (<thomas.petazzoni@bootlin.com>) escribi?:
> > You can send the entire series I'd say. If you have tested my patch,
> > add your Reviewed-by/Tested-by, so that I can apply my own patch as
> > well :-)
> Ok, I made the proposed changes, will run some further tests, and then
> send them.

Since this discussion did not seem to be about the first patch, I
applied it. I however marked this second patch as Changes Requested in
patchwork.

Thanks!

Regards,
Yann E. MORIN.

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

[Buildroot] [PATCH 1/2] package/pkg-generic.mk: create folders before calling hooks

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

 > In commit 0e2be4db8ab01d479177a3a187c22525752195ae
 > ("package/pkg-generic: make file list logic parallel build
 > compatible"), the logic to create the list of files installed by a
 > particular package was significantly reworked to be compatible with
 > top-level parallel build.

 > Before this commit, there was only a after-install step of listing the
 > files in HOST_DIR/TARGET_DIR/STAGING_DIR. But after this commit, we
 > now have a before-install logic and an after-install logic.

 > It turns out that when the before-install logic is called for the very
 > first host package, $(HOST_DIR) doesn't exist yet, and therefore the
 > cd $(2) fails, with an error message:

 > /bin/sh: line 0: cd: /home/thomas/buildroot/output/host: No such file or directory

 > In fact, $(HOST_DIR), $(STAGING_DIR), $(TARGET_DIR) and
 > $(BINARIES_DIR) are created by the make rules for host installation,
 > staging installation, target installation and images installation, but
 > *after* calling the step_start hooks.

 > So, we simply fix this problem by creating the directories *before*
 > calling the step_start hooks.

 > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
 > ---
 > This is different solution than the one initially proposed at
 > http://patchwork.ozlabs.org/patch/1252046/, following some discussion
 > with Yann E. Morin on IRC. Since the creation of the directories is
 > already done somewhere, it makes sense to rely on that rather than
 > adding more code to create them in a difference place.

 > This patch needs to be backported to 2020.02.x.

Committed to 2020.02.x, thanks.

-- 
Bye, Peter Korsgaard