Message ID | 20231128131954.337015-1-alexander.sverdlin@siemens.com |
---|---|
State | Deferred |
Headers | show |
Series | linux-firmware: use install-nodedup make target | expand |
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
diff --git a/meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb b/meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb index c0394b9b3b9..b2b700b926e 100644 --- a/meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb +++ b/meta/recipes-kernel/linux-firmware/linux-firmware_20231030.bb @@ -248,6 +248,9 @@ do_compile() { } 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/ }