NACK/Cmnt: [linux-uc20-efi][FOCAL][PATCH 2/2] UBUNTU: [Packaging] automatically detect flavours from build-deps

[Stefan Bader]

On 10.06.21 11:25, Dimitri John Ledkov wrote:
> On Thu, Jun 10, 2021 at 10:05 AM Stefan Bader
> <stefan.bader at canonical.com> wrote:
>>
>> On 10.06.21 10:56, Dimitri John Ledkov wrote:
>>> On Wed, Jun 9, 2021 at 9:03 AM Stefan Bader <stefan.bader at canonical.com> wrote:
>>>>
>>>> On 08.06.21 13:56, Dimitri John Ledkov wrote:
>>>>> Automatically detected from build-deps which flavours to build
>>>>> kernel.efi apps for. This means that suffixes of
>>>>> linux-image-unsigned-$(ABI)-* are used to determine the desired
>>>>> flavours.
>>>>>
>>>>> If this parsing becomes insufficient, it might need to be updated to
>>>>> use python-apt in the future, or encode flavours in
>>>>> $(debian)/flavors.mk or some such.
>>>>>
>>>>> This makes it trivial to build derivative kernel.efi apps, by simply
>>>>> adjusting appropriate control.stub with the desired flavour
>>>>> build-deps.
>>>>>
>>>>> BugLink: https://bugs.launchpad.net/bugs/1931242
>>>>> Signed-off-by: Dimitri John Ledkov <dimitri.ledkov at canonical.com>
>>>>> ---
>>>>>     debian/rules | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/debian/rules b/debian/rules
>>>>> index f6e6ec0..18adb20 100755
>>>>> --- a/debian/rules
>>>>> +++ b/debian/rules
>>>>> @@ -12,7 +12,7 @@ KERNEL_ABI_VERSION=$(shell echo "$(VERSION)" | sed -ne 's/\([0-9]*\.[0-9]*\.[0-9
>>>>>
>>>>>     DEB_HOST_ARCH=$(shell dpkg-architecture -qDEB_HOST_ARCH)
>>>>>
>>>>> -FLAVOURS=generic lowlatency
>>>>> +FLAVOURS=$(shell sed -n 's/linux-image-unsigned-$(KERNEL_ABI_VERSION)-//p' debian/control | sed 's/[ ,]//g')
>>>>
>>>> debian/control is a generated file. This will cause errors running clean after
>>>> git clean -d -x -f and potentially could pick up the wrong flavours if there is
>>>> a left over conrol. The latter might only be relevant once there are multiple
>>>> efi intermediates since switching between normal kernels and those requires a
>>>> git clean -d -x -f.
>>>>
>>>
>>> In Makefiles '=' is a lazy evaluated assignment, meaning it is only
>>> evaluated and called when used.
>>> clean target does not use the FLAVOURS variable, and I am not
>>> anticipating it will be in the future.
>>> Therefore there are no errors running clean after `git clean -d -x -f`.
>>> FLAVOURS is only used during the build stage, that must be done after
>>> invoking the clean target (as per debian policy)
>>> Meaning wrong flavour will not be picked up from a stale control file
>>> at build time.
>>>
>>> $ git clean -d -x -f
>>> Removing debian/changelog
>>> Removing debian/control
>>>
>>> $ fakeroot ./debian/rules clean
>>> sed <debian.uc20-efi/control.stub >debian/control \
>>> -e 's/@SERIES@/focal/g' \
>>> -e 's/@KERNEL_ABI_VERSION@/5.4.0-74/g' \
>>> -e 's/@SRCPKGNAME@/linux-uc20-efi/g'
>>> cp debian.uc20-efi/changelog debian/changelog
>>> dh_testdir
>>> dh_testroot
>>> rm -rf debian/tmp SIGNING
>>> dh_clean
>>> dh_clean: warning: Compatibility levels before 10 are deprecated
>>> (level 9 in use)
>>>
>>> Which errors are you seeing that I am not able to reproduce?
>>>
>> I did not see the error but expected it from reading the code. And from your
>> explanation this sounds like too much hackery to maintainable. Why not
>> evaluating the input stub?
> 
> Because it contains pre-substitution strings. which debian/rules
> substitutes during clean. So you would accept the below instead?
> 
> FLAVOURS=$(shell sed -n
> 's/linux-image-unsigned- at KERNEL_ABI_VERSION@-//p'
> $(DEBIAN)/control.stub | sed 's/[ ,]//g')

Yes, that would be better IMO because it does not require to know when the 
assignment is made and whether the evaluation really happens at the right time.
> 
> It still feels a bit odd that one has to specify the two flavours five
> times to build it right in the control.stub:

In the end those are the parts you need and I guess the binary pkgs are directly 
used to save waiting for respective meta pkgs to build. Also the meta 
dependencies were not always quite tight to prevent build failures which 
actually were depwaits. Things can be complicated.
> 
>   linux-image-unsigned- at KERNEL_ABI_VERSION@-generic,
>   linux-modules- at KERNEL_ABI_VERSION@-generic,
>   linux-modules-extra- at KERNEL_ABI_VERSION@-generic,
>   linux-image-unsigned- at KERNEL_ABI_VERSION@-lowlatency,
>   linux-modules- at KERNEL_ABI_VERSION@-lowlatency,
> 
> I.e. it would be nice if we could generate the above from just
> $(DEBIAN)/flavours.mk file or some such.
> Also, I am not too sure why we build-depend on the modules-extra given
> that I would have expected everything that ends up in the core-initrd
> is in just regular modules.
> Refactoring that will have to be done another time.
> 

The extra modules split is not quite boot essential only. Rather it is modules 
not needed for virtual environments. When you select linux-virtual, you only get 
modules. When using linux-generic this give modules _and_ modules extra. So as 
long as snaps are not only for virtual environments you want all modules.

Also note that flavours are per arch. Which could mean the whole thing already 
is not complete. At least if uc20-efi is for armhf and arm64. Depends also on 
when things got introduced.  The listing below is from memory, so I likely 
forgot something but just to illustrate:

flavours:
   generic:      ["amd64", "arm64", "armhf", "ppc64le", "s390x"]
   generic-64k:  ["arm64"]
   generic-lpia: ["armhf"]
   lowlatency:   ["amd64"]

Not quite sure whether it should be by flavour or by arch but something like 
this should be in kernel-series.yaml and then could be used in some way by scripts.

-Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210610/1cc57c76/attachment.sig>