Re: [PATCH v2 04/24] linux-module: Do not use shell environment

[Jan Kiszka <jan.kiszka@siemens.com>]

On 19.11.21 13:44, Jan Kiszka wrote:
> On 19.11.21 13:13, Uladzimir Bely wrote:
>> From: Anton Mikanovich <amikan@ilbers.de>
>>
>> To make package build process independent of the shell environment we
>> should remove KDIR and PN passing through export call.
>> KDIR can be prepared during package build. This also will allow not to
>> rely on previous builddeps install task.
>> To pass PN variable we can just migrate to template-based debian/rules
>> file.
>>
>> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
>> Signed-off-by: Uladzimir Bely <ubely@ilbers.de>
>> ---
>>  .../files/debian/{rules => rules.tmpl}        | 14 ++++++++++-
>>  meta/recipes-kernel/linux-module/module.inc   | 23 +++++++------------
>>  2 files changed, 21 insertions(+), 16 deletions(-)
>>  rename meta/recipes-kernel/linux-module/files/debian/{rules => rules.tmpl} (50%)
>>
>> diff --git a/meta/recipes-kernel/linux-module/files/debian/rules b/meta/recipes-kernel/linux-module/files/debian/rules.tmpl
>> similarity index 50%
>> rename from meta/recipes-kernel/linux-module/files/debian/rules
>> rename to meta/recipes-kernel/linux-module/files/debian/rules.tmpl
>> index 59720b37..d8f1d48a 100755
>> --- a/meta/recipes-kernel/linux-module/files/debian/rules
>> +++ b/meta/recipes-kernel/linux-module/files/debian/rules.tmpl
>> @@ -24,14 +24,26 @@ ifneq (,$(findstring 86,$(DEB_HOST_GNU_CPU)))
>>  export ARCH=x86
>>  endif
>>  
>> +# Custom kernels contain the build folder directly.
>> +KDIR := $(shell dpkg -L linux-headers-${KERNEL_NAME} | grep "/lib/modules/.*/build")
>> +ifeq ($(KDIR),)
>> +# Debian kernels install that folder indirectly via a dependency.
>> +KERNEL_DEP := $(shell dpkg-query -W -f '$${Depends}' linux-headers-${KERNEL_NAME} | sed 's/.*\(linux-headers-[[:graph:]]*\).*/\1/')
> 
> Could we wrap this long line?
> 
>> +KDIR := $(shell dpkg -L $(KERNEL_DEP) | grep "/lib/modules/.*/build")
>> +endif
>> +
>> +# With sbuild `dh clean` is called before chroot's apt database updated. So,
>> +# KDIR is empty at that moment and the override causes error. Will skip it.
>> +ifneq ($(KDIR),)
>>  override_dh_auto_clean:
>>  	$(MAKE) -C $(KDIR) M=$(PWD) clean
>> +endif
> 
> This looks wrong, or at least fragile. Why can we live without a proper
> clean on the kernel tree?
> 
>>  
>>  override_dh_auto_build:
>>  	$(MAKE) -C $(KDIR) M=$(PWD) modules
>>  
>>  override_dh_auto_install:
>> -	$(MAKE) -C $(KDIR) M=$(PWD) INSTALL_MOD_PATH=$(PWD)/debian/$(PN) modules_install
>> +	$(MAKE) -C $(KDIR) M=$(PWD) INSTALL_MOD_PATH=$(PWD)/debian/${PN} modules_install
>>  
>>  %:
>>  	CFLAGS= LDFLAGS= dh $@ --parallel
>> diff --git a/meta/recipes-kernel/linux-module/module.inc b/meta/recipes-kernel/linux-module/module.inc
>> index 0515621a..5b086233 100644
>> --- a/meta/recipes-kernel/linux-module/module.inc
>> +++ b/meta/recipes-kernel/linux-module/module.inc
>> @@ -22,8 +22,9 @@ AUTOLOAD ?= ""
>>  inherit dpkg
>>  
>>  TEMPLATE_FILES = "debian/control.tmpl \
>> -                  debian/changelog.tmpl"
>> -TEMPLATE_VARS += "KERNEL_NAME"
>> +                  debian/changelog.tmpl \
>> +                  debian/rules.tmpl"
>> +TEMPLATE_VARS += "KERNEL_NAME PN"
>>  
>>  do_prepare_build() {
>>      cp -r ${WORKDIR}/debian ${S}/
>> @@ -31,18 +32,10 @@ do_prepare_build() {
>>      for module in "${AUTOLOAD}"; do
>>          echo "echo $module >> /etc/modules" >> ${S}/debian/postinst
>>      done
>> -}
>>  
>> -dpkg_runbuild_prepend() {
>> -    # Custom kernels contain the build folder directly.
>> -    export KDIR=$(dpkg -L --root=${BUILDCHROOT_DIR} linux-headers-${KERNEL_NAME} | \
>> -                  grep "/lib/modules/.*/build")
>> -    if [ -z "$KDIR" ]; then
>> -        # Debian kernels install that folder indirectly via a dependency.
>> -        KERNEL_DEP=$(dpkg-query -W -f '${Depends}' --admindir=${BUILDCHROOT_DIR}/var/lib/dpkg \
>> -                     linux-headers-${KERNEL_NAME} | sed 's/.*\(linux-headers-[[:graph:]]*\).*/\1/')
>> -        export KDIR=$(dpkg -L --root=${BUILDCHROOT_DIR} ${KERNEL_DEP} | \
>> -                      grep "/lib/modules/.*/build")
>> -    fi
>> -    export PN=${PN}
>> +    # remove templates from the source tree
>> +    find ${S}/debian -name *.tmpl | xargs rm -f
> 
> Hmm...
> 
>> +
>> +    # restore execute permissions
>> +    chmod a+x ${S}/debian/rules
> 
> The mode should be fixed (aligned to the template) in template.bbclass
> to avoid bothering every recipe with such boilerplate code. I'm seeing
> this in other changes as well.
> 
> With all those "do not use shell env" - how will a downstream user
> detect that a recipe is not properly converted? That looks like a nice
> trap during migration to sbuild (because of silent / non-obvious errors).
> 

Oh, and then you still also add sbuild_export - why not use that for
cases that are ugly to convert (like this one)?

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux