| Message ID | 20220912073221.16601-1-mikko.rapeli@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] common-tasks.rst: remove SRC_URI:append from examples | expand |
Hi Mikko, On 9/12/22 09:32, Mikko Rapeli wrote: > Using SRC_URI:append without recipe, machine or architecture > specific limitations makes the :append'ed text unremovable > and thus users and custom layers can not change the variable > anymore. This makes it hard to e.g. override SRC_URI completely > in a bbappend to update the full recipe to a newer version. > Thus common, reusable layers which users are meant to re-use and > customize should not use SRC_URI:append but SRC_URI += instead. > > Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org> > --- > documentation/dev-manual/common-tasks.rst | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/documentation/dev-manual/common-tasks.rst b/documentation/dev-manual/common-tasks.rst > index b08a55331d..3112f9b893 100644 > --- a/documentation/dev-manual/common-tasks.rst > +++ b/documentation/dev-manual/common-tasks.rst > @@ -2577,7 +2577,7 @@ chapter of the BitBake User Manual. > > S = "${WORKDIR}/postfix-${PV}" > CFLAGS += "-DNO_ASM" > - SRC_URI:append = " file://fixup.patch" > + CFLAGS:append = " --enable-important-feature" > > - *Functions:* Functions provide a series of actions to be performed. > You usually use functions to override the default implementation of a > @@ -2708,19 +2708,20 @@ in the BitBake User Manual. > to existing variables. This operator does not add any additional > space. Also, the operator is applied after all the ``+=``, and ``=+`` > operators have been applied and after all ``=`` assignments have > - occurred. > + occurred. This means that if ``:append`` is used, that text can not be > + removed. > Pedantic: it can, with :remove. One should try really hard to not use it though. The rest is fine. Cheers, Quentin
Mikko, thanks for the patch series. Quentin, thanks for the review. Indeed, I think that the description of the commit should be improved too. See below. On 9/19/22 16:04, Quentin Schulz via lists.openembedded.org wrote: > Hi Mikko, > > On 9/12/22 09:32, Mikko Rapeli wrote: >> Using SRC_URI:append without recipe, machine or architecture >> specific limitations makes the :append'ed text unremovable >> and thus users and custom layers can not change the variable >> anymore. This makes it hard to e.g. override SRC_URI completely >> in a bbappend to update the full recipe to a newer version. >> Thus common, reusable layers which users are meant to re-use and >> customize should not use SRC_URI:append but SRC_URI += instead. What the following text instead? Using SRC_URI:append without recipe, machine or architecture specific limitations makes the :append'ed text more difficult to override than if the "+=" operator was used. This makes it hard for example to override SRC_URI completely in a bbappend to update the full recipe to a newer version. Thus common, reusable layers which users are meant to re-use and customize should not use SRC_URI:append but SRC_URI += instead. >> >> Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org> >> --- >> documentation/dev-manual/common-tasks.rst | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/documentation/dev-manual/common-tasks.rst >> b/documentation/dev-manual/common-tasks.rst >> index b08a55331d..3112f9b893 100644 >> --- a/documentation/dev-manual/common-tasks.rst >> +++ b/documentation/dev-manual/common-tasks.rst >> @@ -2577,7 +2577,7 @@ chapter of the BitBake User Manual. >> S = "${WORKDIR}/postfix-${PV}" >> CFLAGS += "-DNO_ASM" >> - SRC_URI:append = " file://fixup.patch" >> + CFLAGS:append = " --enable-important-feature" Didn't you mean + SRC_URI += "file://fixup.patch" instead here? >> - *Functions:* Functions provide a series of actions to be >> performed. >> You usually use functions to override the default implementation >> of a >> @@ -2708,19 +2708,20 @@ in the BitBake User Manual. >> to existing variables. This operator does not add any additional >> space. Also, the operator is applied after all the ``+=``, and >> ``=+`` >> operators have been applied and after all ``=`` assignments have >> - occurred. >> + occurred. This means that if ``:append`` is used, that text can >> not be >> + removed. > > Pedantic: it can, with :remove. One should try really hard to not use > it though. What about... "This means that if ``:append`` is used in a recipe, it cannot only be overridden by another layer using the special ``:remove`` operator, which in turn will prevent further layers from adding it back." I'm not very happy with this wording, because ":remove" hasn't been introduced at this stage in this section. Maybe we should add a link to https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#conditional-syntax-overrides Any thoughts? Cheers Michael.
Hi, On Mon, 19 Sept 2022 at 23:35, Michael Opdenacker <michael.opdenacker@bootlin.com> wrote: > > Mikko, thanks for the patch series. > Quentin, thanks for the review. > > Indeed, I think that the description of the commit should be improved > too. See below. > > On 9/19/22 16:04, Quentin Schulz via lists.openembedded.org wrote: > > Hi Mikko, > > > > On 9/12/22 09:32, Mikko Rapeli wrote: > >> Using SRC_URI:append without recipe, machine or architecture > >> specific limitations makes the :append'ed text unremovable > >> and thus users and custom layers can not change the variable > >> anymore. This makes it hard to e.g. override SRC_URI completely > >> in a bbappend to update the full recipe to a newer version. > >> Thus common, reusable layers which users are meant to re-use and > >> customize should not use SRC_URI:append but SRC_URI += instead. > > > What the following text instead? > > Using SRC_URI:append without recipe, machine or architecture specific > limitations makes the :append'ed text more difficult to override than if > the "+=" operator was used. This makes it hard for example to override > SRC_URI completely in a bbappend to update the full recipe to a newer > version. Thus common, reusable layers which users are meant to re-use > and customize should not use SRC_URI:append but SRC_URI += instead. Ok, will send a v2 with this. > >> > >> Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org> > >> --- > >> documentation/dev-manual/common-tasks.rst | 9 +++++---- > >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/documentation/dev-manual/common-tasks.rst > >> b/documentation/dev-manual/common-tasks.rst > >> index b08a55331d..3112f9b893 100644 > >> --- a/documentation/dev-manual/common-tasks.rst > >> +++ b/documentation/dev-manual/common-tasks.rst > >> @@ -2577,7 +2577,7 @@ chapter of the BitBake User Manual. > >> S = "${WORKDIR}/postfix-${PV}" > >> CFLAGS += "-DNO_ASM" > >> - SRC_URI:append = " file://fixup.patch" > >> + CFLAGS:append = " --enable-important-feature" > > > Didn't you mean > + SRC_URI += "file://fixup.patch" > > instead here? No. The sample code shows how to use variables so it needs an :append example but SRC_URI is not good use for :append's. Hence I changed it to CFLAGS:append. > >> - *Functions:* Functions provide a series of actions to be > >> performed. > >> You usually use functions to override the default implementation > >> of a > >> @@ -2708,19 +2708,20 @@ in the BitBake User Manual. > >> to existing variables. This operator does not add any additional > >> space. Also, the operator is applied after all the ``+=``, and > >> ``=+`` > >> operators have been applied and after all ``=`` assignments have > >> - occurred. > >> + occurred. This means that if ``:append`` is used, that text can > >> not be > >> + removed. > > > > Pedantic: it can, with :remove. One should try really hard to not use > > it though. > > > What about... > > "This means that if ``:append`` is used in a recipe, it cannot only be > overridden by another layer using the special ``:remove`` operator, > which in turn will prevent further layers from adding it back." > > I'm not very happy with this wording, because ":remove" hasn't been > introduced at this stage in this section. Maybe we should add a link > to https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#conditional-syntax-overrides > > Any thoughts? I'll send v2 with this version. I'd like to get rid of all SRC_URI:append's in documentation since that bad practice already ended in various recipes in poky master and kirkstone branches. Cheers, -Mikko
diff --git a/documentation/dev-manual/common-tasks.rst b/documentation/dev-manual/common-tasks.rst index b08a55331d..3112f9b893 100644 --- a/documentation/dev-manual/common-tasks.rst +++ b/documentation/dev-manual/common-tasks.rst @@ -2577,7 +2577,7 @@ chapter of the BitBake User Manual. S = "${WORKDIR}/postfix-${PV}" CFLAGS += "-DNO_ASM" - SRC_URI:append = " file://fixup.patch" + CFLAGS:append = " --enable-important-feature" - *Functions:* Functions provide a series of actions to be performed. You usually use functions to override the default implementation of a @@ -2708,19 +2708,20 @@ in the BitBake User Manual. to existing variables. This operator does not add any additional space. Also, the operator is applied after all the ``+=``, and ``=+`` operators have been applied and after all ``=`` assignments have - occurred. + occurred. This means that if ``:append`` is used, that text can not be + removed. The following example shows the space being explicitly added to the start to ensure the appended value is not merged with the existing value:: - SRC_URI:append = " file://fix-makefile.patch" + CFLAGS:append = " --enable-important-feature" You can also use the ``:append`` operator with overrides, which results in the actions only being performed for the specified target or machine:: - SRC_URI:append:sh4 = " file://fix-makefile.patch" + CFLAGS:append:sh4 = " --enable-important-sh4-specific-feature" - *Prepending (:prepend):* Use the ``:prepend`` operator to prepend values to existing variables. This operator does not add any
Using SRC_URI:append without recipe, machine or architecture specific limitations makes the :append'ed text unremovable and thus users and custom layers can not change the variable anymore. This makes it hard to e.g. override SRC_URI completely in a bbappend to update the full recipe to a newer version. Thus common, reusable layers which users are meant to re-use and customize should not use SRC_URI:append but SRC_URI += instead. Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org> --- documentation/dev-manual/common-tasks.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)