[PATCH] mk: fix parallel build of test resources

[PATCH] mk: fix parallel build of test resources

[Thomas Monjalon]

The build was failing sometimes when building with multiple
parallel jobs:
    # rm build/build/app/test/*res*
    # make -j6
    objcopy: 'resource.tmp': No such file

The reason is that each resource was built from the same temporary file.
The failure is seen because of a race condition when removing the
temporary file after each resource creation.
It also means that some resources may be created from the wrong source.

The fix is to have a different temporary file for each resource.
We do not use the original source file because it has a long path
which is used by objcopy to name the symbols after some transformations.
The trick is to create a symbolic link of the source file if it is not
already in the current build directory. Then we just have to replace
the dot by an underscore to predict the symbol names to redefine.

Fixes: 1e9e0a6270 ("app/test: fix resource creation with objcopy on FreeBSD")

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 app/test/Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/test/Makefile b/app/test/Makefile
index 9fa03fb..46f6005 100644
--- a/app/test/Makefile
+++ b/app/test/Makefile
@@ -43,14 +43,14 @@ define linked_resource
 SRCS-y += $(1).res.o
 $(1).res.o: $(2)
 	@  echo '  MKRES $$@'
-	$Q ln -fs $$< resource.tmp
+	$Q [ "$$(<D)" = . ] || ln -fs $$<
 	$Q $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_ARCH) -O $(RTE_OBJCOPY_TARGET) \
 		--rename-section                                         \
 			.data=.rodata,alloc,load,data,contents,readonly  \
-		--redefine-sym _binary_resource_tmp_start=beg_$(1)       \
-		--redefine-sym _binary_resource_tmp_end=end_$(1)         \
-		--redefine-sym _binary_resource_tmp_size=siz_$(1)        \
-		resource.tmp $$@ && rm -f resource.tmp
+		--redefine-sym _binary_$$(subst .,_,$$(<F))_start=beg_$(1) \
+		--redefine-sym _binary_$$(subst .,_,$$(<F))_end=end_$(1)   \
+		--redefine-sym _binary_$$(subst .,_,$$(<F))_size=siz_$(1)  \
+		$$(<F) $$@
 endef
 
 ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
-- 
2.7.0

[PATCH v2] mk: fix parallel build of test resources

[Thomas Monjalon]

The build was failing sometimes when building with multiple
parallel jobs:
    # rm build/build/app/test/*res*
    # make -j6
    objcopy: 'resource.tmp': No such file

The reason is that each resource was built from the same temporary file.
The failure is seen because of a race condition when removing the
temporary file after each resource creation.
It also means that some resources may be created from the wrong source.

The fix is to have a different input file for each resource.
The source file is not directly used because it may have a long path
which is used by objcopy to name the symbols after some transformations.
When linking a tar resource, the input file is already in the current
directory. The hard case is for simply linked resources.
The trick is to create a symbolic link of the source file if it is not
already in the current build directory.
Then there is a replacement of dot by an underscore to predict the
symbol names computed by objcopy which must be redefined.

There is an additional change for the test_resource_c which is both
a real source file and a test resource. An intermediate file
test_resource.res is created to avoid compiling resource.c from the
wrong directory through a symbolic link.

Fixes: 1e9e0a6270 ("app/test: fix resource creation with objcopy on FreeBSD")

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
v2: fix rebuild error due to link test_resource.c in build directory
---
 app/test/Makefile | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/test/Makefile b/app/test/Makefile
index 9fa03fb..1f24dd6 100644
--- a/app/test/Makefile
+++ b/app/test/Makefile
@@ -43,14 +43,14 @@ define linked_resource
 SRCS-y += $(1).res.o
 $(1).res.o: $(2)
 	@  echo '  MKRES $$@'
-	$Q ln -fs $$< resource.tmp
+	$Q [ "$$(<D)" = . ] || ln -fs $$<
 	$Q $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_ARCH) -O $(RTE_OBJCOPY_TARGET) \
 		--rename-section                                         \
 			.data=.rodata,alloc,load,data,contents,readonly  \
-		--redefine-sym _binary_resource_tmp_start=beg_$(1)       \
-		--redefine-sym _binary_resource_tmp_end=end_$(1)         \
-		--redefine-sym _binary_resource_tmp_size=siz_$(1)        \
-		resource.tmp $$@ && rm -f resource.tmp
+		--redefine-sym _binary_$$(subst .,_,$$(<F))_start=beg_$(1) \
+		--redefine-sym _binary_$$(subst .,_,$$(<F))_end=end_$(1)   \
+		--redefine-sym _binary_$$(subst .,_,$$(<F))_size=siz_$(1)  \
+		$$(<F) $$@
 endef
 
 ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
@@ -76,7 +76,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) := commands.c
 SRCS-y += test.c
 SRCS-y += resource.c
 SRCS-y += test_resource.c
-$(eval $(call linked_resource,test_resource_c,resource.c))
+test_resource.res: test_resource.c
+	@ cp $< $@
+$(eval $(call linked_resource,test_resource_c,test_resource.res))
 $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
 SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
 $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
-- 
2.7.0

Re: [PATCH v2] mk: fix parallel build of test resources

[Olivier Matz]

Hi Thomas,

On 06/24/2016 01:22 PM, Thomas Monjalon wrote:
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -43,14 +43,14 @@ define linked_resource
>  SRCS-y += $(1).res.o
>  $(1).res.o: $(2)
>  	@  echo '  MKRES $$@'
> -	$Q ln -fs $$< resource.tmp
> +	$Q [ "$$(<D)" = . ] || ln -fs $$<

Maybe the symbolic link file in the build directory could be prefixed
with "lnk_"... (see following below)

>  	$Q $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_ARCH) -O $(RTE_OBJCOPY_TARGET) \
>  		--rename-section                                         \
>  			.data=.rodata,alloc,load,data,contents,readonly  \
> -		--redefine-sym _binary_resource_tmp_start=beg_$(1)       \
> -		--redefine-sym _binary_resource_tmp_end=end_$(1)         \
> -		--redefine-sym _binary_resource_tmp_size=siz_$(1)        \
> -		resource.tmp $$@ && rm -f resource.tmp
> +		--redefine-sym _binary_$$(subst .,_,$$(<F))_start=beg_$(1) \
> +		--redefine-sym _binary_$$(subst .,_,$$(<F))_end=end_$(1)   \
> +		--redefine-sym _binary_$$(subst .,_,$$(<F))_size=siz_$(1)  \
> +		$$(<F) $$@
>  endef
>  
>  ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
> @@ -76,7 +76,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) := commands.c
>  SRCS-y += test.c
>  SRCS-y += resource.c
>  SRCS-y += test_resource.c
> -$(eval $(call linked_resource,test_resource_c,resource.c))
> +test_resource.res: test_resource.c
> +	@ cp $< $@
> +$(eval $(call linked_resource,test_resource_c,test_resource.res))
>  $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
>  SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
>  $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))

...this would avoid to rename the resource file and make the patch
easier to understand.

Olivier

Re: [PATCH v2] mk: fix parallel build of test resources

[Thomas Monjalon]

2016-06-24 16:06, Olivier Matz:
> Hi Thomas,
> 
> On 06/24/2016 01:22 PM, Thomas Monjalon wrote:
> > --- a/app/test/Makefile
> > +++ b/app/test/Makefile
> > @@ -43,14 +43,14 @@ define linked_resource
> >  SRCS-y += $(1).res.o
> >  $(1).res.o: $(2)
> >  	@  echo '  MKRES $$@'
> > -	$Q ln -fs $$< resource.tmp
> > +	$Q [ "$$(<D)" = . ] || ln -fs $$<
> 
> Maybe the symbolic link file in the build directory could be prefixed
> with "lnk_"... (see following below)

yes...
 
> >  	$Q $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_ARCH) -O $(RTE_OBJCOPY_TARGET) \
> >  		--rename-section                                         \
> >  			.data=.rodata,alloc,load,data,contents,readonly  \
> > -		--redefine-sym _binary_resource_tmp_start=beg_$(1)       \
> > -		--redefine-sym _binary_resource_tmp_end=end_$(1)         \
> > -		--redefine-sym _binary_resource_tmp_size=siz_$(1)        \
> > -		resource.tmp $$@ && rm -f resource.tmp
> > +		--redefine-sym _binary_$$(subst .,_,$$(<F))_start=beg_$(1) \
> > +		--redefine-sym _binary_$$(subst .,_,$$(<F))_end=end_$(1)   \
> > +		--redefine-sym _binary_$$(subst .,_,$$(<F))_size=siz_$(1)  \
> > +		$$(<F) $$@
> >  endef
> >  
> >  ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
> > @@ -76,7 +76,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) := commands.c
> >  SRCS-y += test.c
> >  SRCS-y += resource.c
> >  SRCS-y += test_resource.c
> > -$(eval $(call linked_resource,test_resource_c,resource.c))
> > +test_resource.res: test_resource.c
> > +	@ cp $< $@
> > +$(eval $(call linked_resource,test_resource_c,test_resource.res))
> >  $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
> >  SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
> >  $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
> 
> ...this would avoid to rename the resource file and make the patch
> easier to understand.

... but it would be harder to understand how are named the resources in
the build directory ;)
Ideally we should not use a source file (referenced in SRCS-y) as a
test resource.

Re: [PATCH v2] mk: fix parallel build of test resources

[Olivier Matz]

On 06/24/2016 04:19 PM, Thomas Monjalon wrote:
> 2016-06-24 16:06, Olivier Matz:
>> Hi Thomas,
>>
>> On 06/24/2016 01:22 PM, Thomas Monjalon wrote:
>>> --- a/app/test/Makefile
>>> +++ b/app/test/Makefile
>>> @@ -43,14 +43,14 @@ define linked_resource
>>>  SRCS-y += $(1).res.o
>>>  $(1).res.o: $(2)
>>>  	@  echo '  MKRES $$@'
>>> -	$Q ln -fs $$< resource.tmp
>>> +	$Q [ "$$(<D)" = . ] || ln -fs $$<
>>
>> Maybe the symbolic link file in the build directory could be prefixed
>> with "lnk_"... (see following below)
> 
> yes...
>  
>>>  	$Q $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_ARCH) -O $(RTE_OBJCOPY_TARGET) \
>>>  		--rename-section                                         \
>>>  			.data=.rodata,alloc,load,data,contents,readonly  \
>>> -		--redefine-sym _binary_resource_tmp_start=beg_$(1)       \
>>> -		--redefine-sym _binary_resource_tmp_end=end_$(1)         \
>>> -		--redefine-sym _binary_resource_tmp_size=siz_$(1)        \
>>> -		resource.tmp $$@ && rm -f resource.tmp
>>> +		--redefine-sym _binary_$$(subst .,_,$$(<F))_start=beg_$(1) \
>>> +		--redefine-sym _binary_$$(subst .,_,$$(<F))_end=end_$(1)   \
>>> +		--redefine-sym _binary_$$(subst .,_,$$(<F))_size=siz_$(1)  \
>>> +		$$(<F) $$@
>>>  endef
>>>  
>>>  ifeq ($(CONFIG_RTE_APP_TEST_RESOURCE_TAR),y)
>>> @@ -76,7 +76,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) := commands.c
>>>  SRCS-y += test.c
>>>  SRCS-y += resource.c
>>>  SRCS-y += test_resource.c
>>> -$(eval $(call linked_resource,test_resource_c,resource.c))
>>> +test_resource.res: test_resource.c
>>> +	@ cp $< $@
>>> +$(eval $(call linked_resource,test_resource_c,test_resource.res))
>>>  $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
>>>  SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
>>>  $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
>>
>> ...this would avoid to rename the resource file and make the patch
>> easier to understand.
> 
> ... but it would be harder to understand how are named the resources in
> the build directory ;)
> Ideally we should not use a source file (referenced in SRCS-y) as a
> test resource.
> 

OK alright :)

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [PATCH v2] mk: fix parallel build of test resources

[Thomas Monjalon]

|...]
> 
> OK alright :)
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied