Re: [PATCH v5 1/3] automation: Create Yocto docker images

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Anthony,

> On 30 Nov 2022, at 16:25, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
> 
> On Wed, Nov 30, 2022 at 12:15:07PM +0000, Bertrand Marquis wrote:
>> diff --git a/automation/build/Makefile b/automation/build/Makefile
>> index a4b2b85178cf..72a5335baec1 100644
>> --- a/automation/build/Makefile
>> +++ b/automation/build/Makefile
>> @@ -1,13 +1,18 @@
>> 
>> # the base of where these containers will appear
>> REGISTRY := registry.gitlab.com/xen-project/xen
>> -CONTAINERS = $(subst .dockerfile,,$(wildcard */*.dockerfile))
>> +CONTAINERS = $(filter-out yocto/%,$(subst .dockerfile,,$(wildcard 
>> */*.dockerfile)))
> 
> Nit: while there, could you use ":=" instead of "=" ? The value of
> CONTAINERS is always going to be evaluated by make because it's used as
> a prerequisite of "all", so we can at least tell make to evaluate the
> value right away.

Will do

> 
>> +CONTAINERS_EXTRA =
>> DOCKER_CMD ?= docker
>> 
>> +include yocto/yocto.inc
>> +
>> help:
>>      @echo "Builds containers for building Xen based on different distros"
>>      @echo "To build one run 'make DISTRO/VERSION'. Available containers:"
>>      @$(foreach file,$(sort $(CONTAINERS)),echo ${file};)
>> +    @echo "Extra containers (not built using make all):"
>> +    @$(foreach file,$(sort $(CONTAINERS_EXTRA)),echo ${file};)
> 
> I wonder why the help syntax uses both ${} and $() for make variables, is
> it to confuse people? :-)
> 
> You can write $(file) instead of ${file}, I think this would be less
> confusing. I rarely see ${} been used in make, so seen ${} can be
> confusing. I've learned (relearned?) this alternative syntax only a few
> weeks ago as it's used by automake or autoconf.

Definitely a typo, I will fix that in v6 (you have good eyes)

> 
>>      @echo "To push container builds, set the env var PUSH"
>> 
>> %: %.dockerfile ## Builds containers
>> @@ -16,5 +21,10 @@ help:
>>              $(DOCKER_CMD) push $(REGISTRY)/$(@D):$(@F); \
>>      fi
>> 
>> -.PHONY: all
>> +.PHONY: all clean
>> all: $(CONTAINERS)
>> +
>> +# Remove generated dockerfiles for yocto
>> +clean:
>> +    rm -f yocto/*.dockerfiles
> 
> There's an extra 's', I guess you want to remove "*.dockerfile" instead
> of "*.dockerfiles".

Ack

> 
> You could also add those to a .gitignore, even if there are likely to be
> removed by make.

Sure

Thanks for the review
Bertrand

> 
> 
> Cheers,
> 
> -- 
> Anthony PERARD