Message ID | 20161114132238.6569-4-jezz@sysmic.org |
---|---|
State | Changes Requested |
Headers | show |
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
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 (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
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(+)