linux-firmware: use install-nodedup make target

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Versions starting from 20230919 require rdfind; at some point new make
target was introduced with commit 4124f8f928d5 ("Make rdfind optional").
The commit is not yet included even in 20231111, thus make even the
new make target optional.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
This patch will not help with 20231030 update, neither 20231111 will work
without rdfind. install-nodedup is not yet released as of now.

 meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb | 3 +++
 1 file changed, 3 insertions(+)

On Tue, 28 Nov 2023 at 14:20, A. Sverdlin
<alexander.sverdlin@siemens.com> wrote:
> Versions starting from 20230919 require rdfind; at some point new make
> target was introduced with commit 4124f8f928d5 ("Make rdfind optional").
> The commit is not yet included even in 20231111, thus make even the
> new make target optional.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
> This patch will not help with 20231030 update, neither 20231111 will work
> without rdfind. install-nodedup is not yet released as of now.
>  meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb | 3 +++
>  do_install() {
> +        # Versions 20231030 and 20231111 require rdfind, newer tags will have new "install-nodedup"
> +        # make target
> +        oe_runmake_call 'DESTDIR=${D}' 'FIRMWAREDIR=${nonarch_base_libdir}/firmware' install-nodedup || \
>          oe_runmake 'DESTDIR=${D}' 'FIRMWAREDIR=${nonarch_base_libdir}/firmware' install
>          cp GPL-2 LICEN[CS]E.* WHENCE ${D}${nonarch_base_libdir}/firmware/

20231030 builds fine without rdfind, and doesn't have install-nodedup
target in its Makefile because that was only introduced yesterday:

https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/Makefile?id=4124f8f928d51a1437e2fd2636b20d540edc2836

So what is the purpose of this patch? I understand that we would need
to use install-nodedup in lockstep with updating to newer
linux-firmware versions, but until then it's not needed. No?

In general, || in shell scripts should be avoided because it ignores
the reason for the failure (ignoring some fails may be ok, ignoring
all fails is not ok, as it will cause silent regressions).

Alex

Hi Alex!

On Tue, 2023-11-28 at 14:31 +0100, Alexander Kanavin wrote:
> On Tue, 28 Nov 2023 at 14:20, A. Sverdlin
> <alexander.sverdlin@siemens.com> wrote:
> > Versions starting from 20230919 require rdfind; at some point new make
> > target was introduced with commit 4124f8f928d5 ("Make rdfind optional").
> > The commit is not yet included even in 20231111, thus make even the
> > new make target optional.
> > 
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> > This patch will not help with 20231030 update, neither 20231111 will work
> > without rdfind. install-nodedup is not yet released as of now.
> >  meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb | 3 +++
> >  do_install() {
> > +        # Versions 20231030 and 20231111 require rdfind, newer tags will have new "install-nodedup"
> > +        # make target
> > +        oe_runmake_call 'DESTDIR=${D}' 'FIRMWAREDIR=${nonarch_base_libdir}/firmware' install-nodedup || \
> >          oe_runmake 'DESTDIR=${D}' 'FIRMWAREDIR=${nonarch_base_libdir}/firmware' install
> >          cp GPL-2 LICEN[CS]E.* WHENCE ${D}${nonarch_base_libdir}/firmware/
> 
> 20231030 builds fine without rdfind, and doesn't have install-nodedup
> target in its Makefile because that was only introduced yesterday:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/Makefile?id=4124f8f928d51a1437e2fd2636b20d540edc2836
> 
> So what is the purpose of this patch? I understand that we would need
> to use install-nodedup in lockstep with updating to newer
> linux-firmware versions, but until then it's not needed. No?

You are right, even though rdfind is used in 20231030, it doesn't report en error.
Error reporting was introduced with commit cf8315ded9b4
("Ensure rdfind is installed") which comes in 20231111.

So you are right, it will actually be only required before update
of linux-firmware to even newer version.

> In general, || in shell scripts should be avoided because it ignores
> the reason for the failure (ignoring some fails may be ok, ignoring
> all fails is not ok, as it will cause silent regressions).

20231111 will be problematic because rdfind will be required, but there is
no make target yet. My patch is at least falling back to "make install",
so it supports both 20231030 and versions after 20231111. It's not ignoring
the errors, falling back to "|| make install" will eventually fail the whole
build.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

On Tue, 28 Nov 2023 at 14:50, Sverdlin, Alexander
<alexander.sverdlin@siemens.com> wrote:
> > In general, || in shell scripts should be avoided because it ignores
> > the reason for the failure (ignoring some fails may be ok, ignoring
> > all fails is not ok, as it will cause silent regressions).
>
> 20231111 will be problematic because rdfind will be required, but there is
> no make target yet. My patch is at least falling back to "make install",
> so it supports both 20231030 and versions after 20231111. It's not ignoring
> the errors, falling back to "|| make install" will eventually fail the whole
> build.

So once someone sends an update to 20231111, they should also backport
the patch that introduces the new make target, and use that target.
And if there's an update to an even newer version, then the new target
can be used without any patching. But for now, I think nothing needs
to be done?

The problem with || is this: imagine that you have specified A || B,
and you introduced A in addition to B, assuming some specific reason X
that A can fail. What if it starts failing for some other reason Y?
Then the fall-through to B may be valid, or it may not, and whatever
had caused Y needs to be fixed some other way than executing B. This
fall-through will happen quietly and no one is going to notice it, and
that's where the regression is.

Alex

Hi Alex!

On Tue, 2023-11-28 at 15:00 +0100, Alexander Kanavin wrote:
> On Tue, 28 Nov 2023 at 14:50, Sverdlin, Alexander
> <alexander.sverdlin@siemens.com> wrote:
> > > In general, || in shell scripts should be avoided because it ignores
> > > the reason for the failure (ignoring some fails may be ok, ignoring
> > > all fails is not ok, as it will cause silent regressions).
> > 
> > 20231111 will be problematic because rdfind will be required, but there is
> > no make target yet. My patch is at least falling back to "make install",
> > so it supports both 20231030 and versions after 20231111. It's not ignoring
> > the errors, falling back to "|| make install" will eventually fail the whole
> > build.
> 
> So once someone sends an update to 20231111, they should also backport
> the patch that introduces the new make target, and use that target.
> And if there's an update to an even newer version, then the new target
> can be used without any patching. But for now, I think nothing needs
> to be done?

It would be the case if this particular patch gets accepted.
That is the whole point. Distros need adaptation for the new "solution" in
linux-firmware, default behaviour has been changed.

> The problem with || is this: imagine that you have specified A || B,
> and you introduced A in addition to B, assuming some specific reason X
> that A can fail. What if it starts failing for some other reason Y?
> Then the fall-through to B may be valid, or it may not, and whatever
> had caused Y needs to be fixed some other way than executing B. This
> fall-through will happen quietly and no one is going to notice it, and
> that's where the regression is.

Generic case is clear, in this particular case I believe falling back to
"make install" is always valid if "make install-nodedup" fails.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

On Tue, 28 Nov 2023 at 15:32, Sverdlin, Alexander
<alexander.sverdlin@siemens.com> wrote:
> > So once someone sends an update to 20231111, they should also backport
> > the patch that introduces the new make target, and use that target.
> > And if there's an update to an even newer version, then the new target
> > can be used without any patching. But for now, I think nothing needs
> > to be done?
>
> It would be the case if this particular patch gets accepted.
> That is the whole point. Distros need adaptation for the new "solution" in
> linux-firmware, default behaviour has been changed.

I do not understand why this change is needed, if linux-firmware
version is not being changed at the same time. It would merely try a
non-existing make rule and fall back to the one already there.

For the future version updates, the correct fixes are those that I
have described. The rule to use is deterministic because the recipe is
using an exact upstream version, and we don't need to try them one
after another.

Alex

Hi Alex!

On Tue, 2023-11-28 at 15:00 +0100, Alexander Kanavin wrote:
> On Tue, 28 Nov 2023 at 14:50, Sverdlin, Alexander
> <alexander.sverdlin@siemens.com> wrote:
> > > In general, || in shell scripts should be avoided because it ignores
> > > the reason for the failure (ignoring some fails may be ok, ignoring
> > > all fails is not ok, as it will cause silent regressions).
> > 
> > 20231111 will be problematic because rdfind will be required, but there is
> > no make target yet. My patch is at least falling back to "make install",
> > so it supports both 20231030 and versions after 20231111. It's not ignoring
> > the errors, falling back to "|| make install" will eventually fail the whole
> > build.
> 
> So once someone sends an update to 20231111, they should also backport
> the patch that introduces the new make target, and use that target.
> And if there's an update to an even newer version, then the new target
> can be used without any patching. But for now, I think nothing needs
> to be done?
> 
> The problem with || is this: imagine that you have specified A || B,
> and you introduced A in addition to B, assuming some specific reason X
> that A can fail. What if it starts failing for some other reason Y?
> Then the fall-through to B may be valid, or it may not, and whatever
> had caused Y needs to be fixed some other way than executing B. This
> fall-through will happen quietly and no one is going to notice it, and
> that's where the regression is.

Let's ignore the patch for the moment. You are right, 20231030 update is
OK, next updates can introduce new make target without fallback and avoid
the || operator.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com