diff mbox series

u-boot.inc: WORKDIR -> UNPACKDIR transition

Message ID 20240623213254.3215897-1-leon.anavi@konsulko.com
State Under Review
Headers show
Series u-boot.inc: WORKDIR -> UNPACKDIR transition | expand

Commit Message

Leon Anavi June 23, 2024, 9:32 p.m. UTC
Replace references of WORKDIR with UNPACKDIR for U-Boot script
and variable UBOOT_ENV_BINARY, for example for boot.scr.

Signed-off-by: Leon Anavi <leon.anavi@konsulko.com>
---
 meta/recipes-bsp/u-boot/u-boot.inc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Richard Purdie June 26, 2024, 1:20 p.m. UTC | #1
On Mon, 2024-06-24 at 00:32 +0300, Leon Anavi via lists.openembedded.org wrote:
> Replace references of WORKDIR with UNPACKDIR for U-Boot script
> and variable UBOOT_ENV_BINARY, for example for boot.scr.
> 
> Signed-off-by: Leon Anavi <leon.anavi@konsulko.com>
> ---
>  meta/recipes-bsp/u-boot/u-boot.inc | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/recipes-bsp/u-boot/u-boot.inc b/meta/recipes-bsp/u-boot/u-boot.inc
> index 45d700fbdd..9a91a0e3d8 100644
> --- a/meta/recipes-bsp/u-boot/u-boot.inc
> +++ b/meta/recipes-bsp/u-boot/u-boot.inc
> @@ -92,7 +92,7 @@ do_compile () {
>  
>      if [ -n "${UBOOT_ENV}" ] && [ "${UBOOT_ENV_SUFFIX}" = "scr" ]
>      then
> -        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${WORKDIR}/${UBOOT_ENV_SRC} ${WORKDIR}/${UBOOT_ENV_BINARY}
> +        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${UNPACKDIR}/${UBOOT_ENV_SRC} ${UNPACKDIR}/${UBOOT_ENV_BINARY}
>      fi
>  }


If that command is writing a file, the written file should remain in
WORKDIR, not UNPACKDIR. I suspect the SRC should probably therefore be
changed but not the BINARY?

>  
> @@ -188,7 +188,7 @@ do_install () {
>  
>      if [ -n "${UBOOT_ENV}" ]
>      then
> -        install -m 644 ${WORKDIR}/${UBOOT_ENV_BINARY} ${D}/boot/${UBOOT_ENV_IMAGE}
> +        install -m 644 ${UNPACKDIR}/${UBOOT_ENV_BINARY} ${D}/boot/${UBOOT_ENV_IMAGE}
>          ln -sf ${UBOOT_ENV_IMAGE} ${D}/boot/${UBOOT_ENV_BINARY}
>      fi
>  
> @@ -324,7 +324,7 @@ do_deploy () {
>  
>      if [ -n "${UBOOT_ENV}" ]
>      then
> -        install -m 644 ${WORKDIR}/${UBOOT_ENV_BINARY} ${DEPLOYDIR}/${UBOOT_ENV_IMAGE}
> +        install -m 644 ${UNPACKDIR}/${UBOOT_ENV_BINARY} ${DEPLOYDIR}/${UBOOT_ENV_IMAGE}
>          ln -sf ${UBOOT_ENV_IMAGE} ${DEPLOYDIR}/${UBOOT_ENV_BINARY}
>          ln -sf ${UBOOT_ENV_IMAGE} ${DEPLOYDIR}/${UBOOT_ENV_SYMLINK}
>      fi

Which would then make these unneeded?

Cheers,

Richard
Quentin Schulz June 26, 2024, 1:34 p.m. UTC | #2
Hi Richard,

On 6/26/24 3:20 PM, Richard Purdie via lists.openembedded.org wrote:
> On Mon, 2024-06-24 at 00:32 +0300, Leon Anavi via lists.openembedded.org wrote:
>> Replace references of WORKDIR with UNPACKDIR for U-Boot script
>> and variable UBOOT_ENV_BINARY, for example for boot.scr.
>>
>> Signed-off-by: Leon Anavi <leon.anavi@konsulko.com>
>> ---
>>   meta/recipes-bsp/u-boot/u-boot.inc | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/meta/recipes-bsp/u-boot/u-boot.inc b/meta/recipes-bsp/u-boot/u-boot.inc
>> index 45d700fbdd..9a91a0e3d8 100644
>> --- a/meta/recipes-bsp/u-boot/u-boot.inc
>> +++ b/meta/recipes-bsp/u-boot/u-boot.inc
>> @@ -92,7 +92,7 @@ do_compile () {
>>   
>>       if [ -n "${UBOOT_ENV}" ] && [ "${UBOOT_ENV_SUFFIX}" = "scr" ]
>>       then
>> -        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${WORKDIR}/${UBOOT_ENV_SRC} ${WORKDIR}/${UBOOT_ENV_BINARY}
>> +        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${UNPACKDIR}/${UBOOT_ENV_SRC} ${UNPACKDIR}/${UBOOT_ENV_BINARY}
>>       fi
>>   }
> 
> 
> If that command is writing a file, the written file should remain in
> WORKDIR, not UNPACKDIR. I suspect the SRC should probably therefore be
> changed but not the BINARY?

WORKDIR still isn't cleaned between builds of a recipe I assume? While 
migrating to UNPACKDIR gets rid of patches that were once in SRC_URI but 
aren't anymore (or other files for that matter), using WORKDIR as a 
temporary directory between tasks is going to be an issue depending on 
how the other tasks are going to behave. If the logic using the file is 
the same as the logic for creating the file then it shouldn't be an 
issue. But if the logic using the file is just checking whether the file 
exists, then using WORKDIR is an issue since it won't be cleaned between 
builds of the same recipe.

Maybe we should go for ${B} instead?

Cheers,
Quentin
Richard Purdie June 26, 2024, 2:09 p.m. UTC | #3
On Wed, 2024-06-26 at 15:34 +0200, Quentin Schulz wrote:
> Hi Richard,
> 
> On 6/26/24 3:20 PM, Richard Purdie via lists.openembedded.org wrote:
> > On Mon, 2024-06-24 at 00:32 +0300, Leon Anavi via lists.openembedded.org wrote:
> > > Replace references of WORKDIR with UNPACKDIR for U-Boot script
> > > and variable UBOOT_ENV_BINARY, for example for boot.scr.
> > > 
> > > Signed-off-by: Leon Anavi <leon.anavi@konsulko.com>
> > > ---
> > >   meta/recipes-bsp/u-boot/u-boot.inc | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/meta/recipes-bsp/u-boot/u-boot.inc b/meta/recipes-bsp/u-boot/u-boot.inc
> > > index 45d700fbdd..9a91a0e3d8 100644
> > > --- a/meta/recipes-bsp/u-boot/u-boot.inc
> > > +++ b/meta/recipes-bsp/u-boot/u-boot.inc
> > > @@ -92,7 +92,7 @@ do_compile () {
> > >   
> > >       if [ -n "${UBOOT_ENV}" ] && [ "${UBOOT_ENV_SUFFIX}" = "scr" ]
> > >       then
> > > -        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${WORKDIR}/${UBOOT_ENV_SRC} ${WORKDIR}/${UBOOT_ENV_BINARY}
> > > +        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${UNPACKDIR}/${UBOOT_ENV_SRC} ${UNPACKDIR}/${UBOOT_ENV_BINARY}
> > >       fi
> > >   }
> > 
> > 
> > If that command is writing a file, the written file should remain in
> > WORKDIR, not UNPACKDIR. I suspect the SRC should probably therefore be
> > changed but not the BINARY?
> 
> WORKDIR still isn't cleaned between builds of a recipe I assume? While 
> migrating to UNPACKDIR gets rid of patches that were once in SRC_URI but 
> aren't anymore (or other files for that matter), using WORKDIR as a 
> temporary directory between tasks is going to be an issue depending on 
> how the other tasks are going to behave. If the logic using the file is 
> the same as the logic for creating the file then it shouldn't be an 
> issue. But if the logic using the file is just checking whether the file 
> exists, then using WORKDIR is an issue since it won't be cleaned between 
> builds of the same recipe.

Correct.

> Maybe we should go for ${B} instead?

${B} would probably be an improvement, yes.

Cheers,

Richard
Leon Anavi June 27, 2024, 8:18 a.m. UTC | #4
Hi Richard, Quentin,

On 26.06.24 г. 17:09 ч., Richard Purdie wrote:
> On Wed, 2024-06-26 at 15:34 +0200, Quentin Schulz wrote:
>> Hi Richard,
>>
>> On 6/26/24 3:20 PM, Richard Purdie via lists.openembedded.org wrote:
>>> On Mon, 2024-06-24 at 00:32 +0300, Leon Anavi via lists.openembedded.org wrote:
>>>> Replace references of WORKDIR with UNPACKDIR for U-Boot script
>>>> and variable UBOOT_ENV_BINARY, for example for boot.scr.
>>>>
>>>> Signed-off-by: Leon Anavi<leon.anavi@konsulko.com>
>>>> ---
>>>>    meta/recipes-bsp/u-boot/u-boot.inc | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/meta/recipes-bsp/u-boot/u-boot.inc b/meta/recipes-bsp/u-boot/u-boot.inc
>>>> index 45d700fbdd..9a91a0e3d8 100644
>>>> --- a/meta/recipes-bsp/u-boot/u-boot.inc
>>>> +++ b/meta/recipes-bsp/u-boot/u-boot.inc
>>>> @@ -92,7 +92,7 @@ do_compile () {
>>>>    
>>>>        if [ -n "${UBOOT_ENV}" ] && [ "${UBOOT_ENV_SUFFIX}" = "scr" ]
>>>>        then
>>>> -        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${WORKDIR}/${UBOOT_ENV_SRC} ${WORKDIR}/${UBOOT_ENV_BINARY}
>>>> +        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${UNPACKDIR}/${UBOOT_ENV_SRC} ${UNPACKDIR}/${UBOOT_ENV_BINARY}
>>>>        fi
>>>>    }
>>>
>>> If that command is writing a file, the written file should remain in
>>> WORKDIR, not UNPACKDIR. I suspect the SRC should probably therefore be
>>> changed but not the BINARY?
>> WORKDIR still isn't cleaned between builds of a recipe I assume? While
>> migrating to UNPACKDIR gets rid of patches that were once in SRC_URI but
>> aren't anymore (or other files for that matter), using WORKDIR as a
>> temporary directory between tasks is going to be an issue depending on
>> how the other tasks are going to behave. If the logic using the file is
>> the same as the logic for creating the file then it shouldn't be an
>> issue. But if the logic using the file is just checking whether the file
>> exists, then using WORKDIR is an issue since it won't be cleaned between
>> builds of the same recipe.
> Correct.
>
>> Maybe we should go for ${B} instead?
> ${B} would probably be an improvement, yes.

Thank you for the valuable feedback. Following your suggestions I 
modified the patch to use B for UBOOT_ENV_BINARY. I sent v2 of the patch 
a few minutes ago. Best regards, Leon

>
> Cheers,
>
> Richard
>
>
diff mbox series

Patch

diff --git a/meta/recipes-bsp/u-boot/u-boot.inc b/meta/recipes-bsp/u-boot/u-boot.inc
index 45d700fbdd..9a91a0e3d8 100644
--- a/meta/recipes-bsp/u-boot/u-boot.inc
+++ b/meta/recipes-bsp/u-boot/u-boot.inc
@@ -92,7 +92,7 @@  do_compile () {
 
     if [ -n "${UBOOT_ENV}" ] && [ "${UBOOT_ENV_SUFFIX}" = "scr" ]
     then
-        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${WORKDIR}/${UBOOT_ENV_SRC} ${WORKDIR}/${UBOOT_ENV_BINARY}
+        ${UBOOT_MKIMAGE} -C none -A ${UBOOT_ARCH} -T script -d ${UNPACKDIR}/${UBOOT_ENV_SRC} ${UNPACKDIR}/${UBOOT_ENV_BINARY}
     fi
 }
 
@@ -188,7 +188,7 @@  do_install () {
 
     if [ -n "${UBOOT_ENV}" ]
     then
-        install -m 644 ${WORKDIR}/${UBOOT_ENV_BINARY} ${D}/boot/${UBOOT_ENV_IMAGE}
+        install -m 644 ${UNPACKDIR}/${UBOOT_ENV_BINARY} ${D}/boot/${UBOOT_ENV_IMAGE}
         ln -sf ${UBOOT_ENV_IMAGE} ${D}/boot/${UBOOT_ENV_BINARY}
     fi
 
@@ -324,7 +324,7 @@  do_deploy () {
 
     if [ -n "${UBOOT_ENV}" ]
     then
-        install -m 644 ${WORKDIR}/${UBOOT_ENV_BINARY} ${DEPLOYDIR}/${UBOOT_ENV_IMAGE}
+        install -m 644 ${UNPACKDIR}/${UBOOT_ENV_BINARY} ${DEPLOYDIR}/${UBOOT_ENV_IMAGE}
         ln -sf ${UBOOT_ENV_IMAGE} ${DEPLOYDIR}/${UBOOT_ENV_BINARY}
         ln -sf ${UBOOT_ENV_IMAGE} ${DEPLOYDIR}/${UBOOT_ENV_SYMLINK}
     fi