[3/6] infra: fix 'packages-file-list.txt' with TLP

Until now, `$(BUILD_DIR)/packages-file-list.txt' was not filled properly when
top level parallelization is enabled. Therefore, all scripts that rely on
packages-file-list.txt did not work.

In order to fix it,this patch place target installation task in a critical
section.

Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
---
 package/pkg-generic.mk | 4 ++++
 1 file changed, 4 insertions(+)

Hello,

Adding Gustavo in Cc. Gustavo, you are working on TLP support. Could
you comment on the below patch?

See also my comments below.

On Mon, 14 Nov 2016 14:22:35 +0100, Jérôme Pouiller wrote:
> Until now, `$(BUILD_DIR)/packages-file-list.txt' was not filled properly when
> top level parallelization is enabled. Therefore, all scripts that rely on
> packages-file-list.txt did not work.
> 
> In order to fix it,this patch place target installation task in a critical
> section.
> 
> Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
> ---
>  package/pkg-generic.mk | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 987efa6..c5f70e0 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -62,6 +62,9 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time
>  # files currently installed in the target. Note that the MD5 is also
>  # stored, in order to identify if the files are overwritten.
>  define step_pkg_size_start
> +	while ! flock $(BUILD_DIR) -c "[ ! -e $(BUILD_DIR)/.target_lock ] && touch $(BUILD_DIR)/.target_lock"; do \
> +		sleep 0.5; \
> +	done

I personally don't really like this retry loop around flock, but since
package-file-list.txt is a global file, I don't really see how to do
otherwise. Unless storing a per-package file with just the time/date of
the start/end of each step for this package, and then have a
post-processing logic at the very end of the build that regroups all
those per-package files into a single global file, ordering the entries
by their timestamp. Don't know if it's really better.


>  	(cd $(TARGET_DIR) ; find . -type f -print0 | xargs -0 md5sum) | sort > \
>  		$($(PKG)_DIR)/.br_filelist_before
>  endef
> @@ -78,6 +81,7 @@ define step_pkg_size_end
>  		while read hash file ; do \
>  			echo "$(1),$${file}" >> $(BUILD_DIR)/packages-file-list.txt ; \
>  		done
> +	rm $(BUILD_DIR)/.target_lock
>  endef
>  
>  define step_pkg_size

Thanks!

Thomas

Hello Thomas,

On Thursday 9 February 2017 23:24:29 CET Thomas Petazzoni wrote:
> Hello,
> 
> Adding Gustavo in Cc. Gustavo, you are working on TLP support. Could
> you comment on the below patch?
> 
> See also my comments below.
> 
> On Mon, 14 Nov 2016 14:22:35 +0100, Jérôme Pouiller wrote:
> > Until now, `$(BUILD_DIR)/packages-file-list.txt' was not filled properly
> > when top level parallelization is enabled. Therefore, all scripts that
> > rely on packages-file-list.txt did not work.
> > 
> > In order to fix it,this patch place target installation task in a critical
> > section.
> > 
> > Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
> > ---
> > 
> >  package/pkg-generic.mk | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 987efa6..c5f70e0 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -62,6 +62,9 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time
> > 
> >  # files currently installed in the target. Note that the MD5 is also
> >  # stored, in order to identify if the files are overwritten.
> >  define step_pkg_size_start
> > 
> > +	while ! flock $(BUILD_DIR) -c "[ ! -e $(BUILD_DIR)/.target_lock ] &&
> > touch $(BUILD_DIR)/.target_lock"; do \ +		sleep 0.5; \
> > +	done
> 
> I personally don't really like this retry loop around flock, but since
> package-file-list.txt is a global file, I don't really see how to do
> otherwise. Unless storing a per-package file with just the time/date of
> the start/end of each step for this package, and then have a
> post-processing logic at the very end of the build that regroups all
> those per-package files into a single global file, ordering the entries
> by their timestamp. Don't know if it's really better.

I think that any tool involving parallel processing need one day or another
to protect code inside a critical section. Even if we find another way to 
solve current problem, I am pretty sure we will need it later, anyway.

However, I think I am going to modify my patch in order to provide a
generic mutex implementation that would be enabled iff TLP is enable.

I wonder where I should place lock file. In add, tools like fakedate,
check-shlibs-deps and other QA tools could also generate log or
temporary files. Currently, we place all of them in build/ without any
specific rules. Maybe we should have a naming policy for these files?
(prefix them with "BR_"?) or place them elsewhere than in build/?

[...]

Hello,

On Mon, 14 Nov 2016 14:22:35 +0100, Jérôme Pouiller wrote:
> Until now, `$(BUILD_DIR)/packages-file-list.txt' was not filled properly when
> top level parallelization is enabled. Therefore, all scripts that rely on
> packages-file-list.txt did not work.
> 
> In order to fix it,this patch place target installation task in a critical
> section.
> 
> Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>

Arnout said:

"""
 I don't think this is the proper approach. The proper approach is
map/reduce, i.e. generate the pieces in per-package files and collect
them together in a final gathering stage. Obviously, this requires a
per-package target first, but that's anyway the way we want to go.

 I thought someone already made that comment but I can't find it in
 that thread...
"""

So I've marked this patch as Changes Requested in patchwork.

Best regards,

Thomas