| Message ID | 20250126132005.1540374-1-sst@poczta.fm |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] kernel-yocto: fix merge_config.sh flag set when KBUILD_DEFCONFIG is used | expand |
Slawomir Stepien via lists.openembedded.org schrieb am So 26. Jan, 13:43 (+0100): > Based on the documentation of KCONFIG_MODE[1], when we use "in-tree" > defconfig (the KBUILD_DEFCONFIG is set), then the alldefconfig mode > should be used with merge_config.sh. > > This commit fixes the logic behind setting the flag by checking if the > provided defconfig file exists in the linux source tree. > > [1] https://docs.yoctoproject.org/ref-manual/variables.html#term-KCONFIG_MODE > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > --- > meta/classes-recipe/kernel-yocto.bbclass | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/meta/classes-recipe/kernel-yocto.bbclass b/meta/classes-recipe/kernel-yocto.bbclass > index c45abf6ddc..ffda41ffc9 100644 > --- a/meta/classes-recipe/kernel-yocto.bbclass > +++ b/meta/classes-recipe/kernel-yocto.bbclass > @@ -113,6 +113,14 @@ def get_dirs_with_fragments(d): > > return " ".join(extrafiles) > > +kbuild_defconfig_exists() { > + if [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ]; then > + return 0 > + fi > + > + return 1 Hi, you don't need the branching logic here. The function always returns the last return value. So kbuild_defconfig_exists() { [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ] } is enough. Is it really worth to have a single function for this command? Maybe, it's more useful to define a variable S_KBUILD_DEFCONFIG_PATH and replace all occurrences of `${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}` by this. But even this might not help so much. > if [ -n "${KBUILD_DEFCONFIG}" ]; then > - if [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ]; then > + if $(kbuild_defconfig_exists); then I think you don't want the $(), because you don't want the output of the command, but the return value. So `if kbuild_defconfig_exists; then` > if [ -f "${UNPACKDIR}/defconfig" ]; then > # If the two defconfig's are different, warn that we overwrote the > # one already placed in UNPACKDIR > @@ -478,8 +486,12 @@ do_kernel_configme() { > config_flags="" > ;; > *) > - if [ -f ${UNPACKDIR}/defconfig ]; then > - config_flags="-n" > + if $(kbuild_defconfig_exists); then Is it save here to assume KBUILD_DEFCONFIG is set. This is checked in the code fragment above. > + config_flags="" > + else > + if [ -f ${UNPACKDIR}/defconfig ]; then You can use `elif [ …` to reduce the nesting. > + config_flags="-n" > + fi > fi > ;; > esac > -- > 2.48.1 > Bye, Jörg
On Sun, Jan 26, 2025 at 8:20 AM Slawomir Stepien via lists.openembedded.org <sst=poczta.fm@lists.openembedded.org> wrote: > Based on the documentation of KCONFIG_MODE[1], when we use "in-tree" > defconfig (the KBUILD_DEFCONFIG is set), then the alldefconfig mode > should be used with merge_config.sh. > > Please make sure to copy me directly on any kernel-yocto changes. > This commit fixes the logic behind setting the flag by checking if the > provided defconfig file exists in the linux source tree. There's no sense referencing the documentation here, since it was written long after the code and matches the logic in the class, not the other way around. Meaning it would be the documentation that is wrong, not the code if there's a mismatch. > [1] > https://docs.yoctoproject.org/ref-manual/variables.html#term-KCONFIG_MODE > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > --- > meta/classes-recipe/kernel-yocto.bbclass | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/meta/classes-recipe/kernel-yocto.bbclass > b/meta/classes-recipe/kernel-yocto.bbclass > index c45abf6ddc..ffda41ffc9 100644 > --- a/meta/classes-recipe/kernel-yocto.bbclass > +++ b/meta/classes-recipe/kernel-yocto.bbclass > @@ -113,6 +113,14 @@ def get_dirs_with_fragments(d): > > return " ".join(extrafiles) > > +kbuild_defconfig_exists() { > + if [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ]; then > + return 0 > + fi > + > + return 1 > +} > + > do_kernel_metadata() { > set +e > > @@ -155,7 +163,7 @@ do_kernel_metadata() { > # precendence. > # > if [ -n "${KBUILD_DEFCONFIG}" ]; then > - if [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ]; > then > + if $(kbuild_defconfig_exists); then > replacing this with a function call doesn't make much sense. See below for why. > if [ -f "${UNPACKDIR}/defconfig" ]; then > # If the two defconfig's are different, > warn that we overwrote the > # one already placed in UNPACKDIR > @@ -478,8 +486,12 @@ do_kernel_configme() { > config_flags="" > ;; > *) > - if [ -f ${UNPACKDIR}/defconfig ]; then > - config_flags="-n" > + if $(kbuild_defconfig_exists); then > + config_flags="" > + else > + if [ -f ${UNPACKDIR}/defconfig ]; then > + config_flags="-n" > + fi > This is only a sanity fall back, and was something I added only when pushed for it. There should be no mod setting / changing based on logic at this point in the build. Since we really can't be sure what type of defconfig is what or if it has somehow been patched or modified in an intervening task / step. There's 10+ years of logic that sits on top of the current behaviour. Anything that relies on a specific type of processing should explicitly set the KCONFIG_MODE variable in their kernel recipe / where the defconfig variable is set. It might make sense to just set the KCONFIG_MODE in the do_kernel_metadata routine when the in-tree defconfig is copied and then just use it here. Then just delete the entirety of the *) case, since we don't need a fallback (the default of alldefconfig will just occur since the config_flags aren't set to -n). But we should not be looking into the tree and making a secondary decision here based on the source of the defconfig. Bruce > fi > ;; > esac > -- > 2.48.1 > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#210289): > https://lists.openembedded.org/g/openembedded-core/message/210289 > Mute This Topic: https://lists.openembedded.org/mt/110821216/1050810 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [ > bruce.ashfield@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- > >
diff --git a/meta/classes-recipe/kernel-yocto.bbclass b/meta/classes-recipe/kernel-yocto.bbclass index c45abf6ddc..ffda41ffc9 100644 --- a/meta/classes-recipe/kernel-yocto.bbclass +++ b/meta/classes-recipe/kernel-yocto.bbclass @@ -113,6 +113,14 @@ def get_dirs_with_fragments(d): return " ".join(extrafiles) +kbuild_defconfig_exists() { + if [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ]; then + return 0 + fi + + return 1 +} + do_kernel_metadata() { set +e @@ -155,7 +163,7 @@ do_kernel_metadata() { # precendence. # if [ -n "${KBUILD_DEFCONFIG}" ]; then - if [ -f "${S}/arch/${ARCH}/configs/${KBUILD_DEFCONFIG}" ]; then + if $(kbuild_defconfig_exists); then if [ -f "${UNPACKDIR}/defconfig" ]; then # If the two defconfig's are different, warn that we overwrote the # one already placed in UNPACKDIR @@ -478,8 +486,12 @@ do_kernel_configme() { config_flags="" ;; *) - if [ -f ${UNPACKDIR}/defconfig ]; then - config_flags="-n" + if $(kbuild_defconfig_exists); then + config_flags="" + else + if [ -f ${UNPACKDIR}/defconfig ]; then + config_flags="-n" + fi fi ;; esac
Based on the documentation of KCONFIG_MODE[1], when we use "in-tree" defconfig (the KBUILD_DEFCONFIG is set), then the alldefconfig mode should be used with merge_config.sh. This commit fixes the logic behind setting the flag by checking if the provided defconfig file exists in the linux source tree. [1] https://docs.yoctoproject.org/ref-manual/variables.html#term-KCONFIG_MODE Signed-off-by: Slawomir Stepien <sst@poczta.fm> --- meta/classes-recipe/kernel-yocto.bbclass | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)