Re: [PATCHv4 22/26] meta: deb-dl-dir: do not cache debs from isar-apt

[Henning Schild <henning.schild@siemens.com>]

On Tue, 4 Feb 2020 06:14:00 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 04.02.20 06:10, Su, Bao Cheng (RC-CN DI FA R&D SW) wrote:
> > Still exists.
> > 
> > Under `build/tmp/work/`:
> > 
> > $ grep "command not found" . -r --include="log.do_*"
> > ./isar-arm64/isar-bootstrap-target/1.0-r0/temp/log.do_bootstrap.329:/bin/bash: 
> > line 5: repo_contains_package: command not found
> > ./isar-arm64/isar-bootstrap-target/1.0-r0/temp/log.do_bootstrap.329:/bin/bash: 
> > line 5: repo_contains_package: command not found
> > ./isar-arm64/isar-bootstrap-target/1.0-r0/temp/log.do_bootstrap.329:/bin/bash: 
> > line 5: repo_contains_package: command not found
> > ./isar-arm64/isar-bootstrap-target/1.0-r0/temp/log.do_bootstrap.329:/bin/bash: 
> > line 5: repo_contains_package: command not found
> > 
> > $ grep "command not found" . -r --include="log.do_*" | wc -l
> > 1596
> > 
> > This is only a half-way build, for a full build, there are more
> > occurrences.  
> 
> And if I read the code correctly, the issue only prevents an 
> optimization, not the correct functioning of the code. Thus, when
> fixing this, building should be even faster :).

No, it is putting packages we built into the dl-cache where they really
do not belong. Having them in there is harmful because it will most
likely cause issues when rebuilding, checksum changes on
pre-downloaded files ... something that can not happen with real
downloads. But something that is probably common in Isar, when people
do not bump PV on every change.

Henning

> Thanks,
> Jan
> 
> > 
> > With best regards
> > ------------------------------------------------------------------------
> > *发件人：* Su, Bao Cheng (RC-CN DI FA R&D SW)
> > *发送时间：* 2020年2月4日星期二 10:11
> > *收件人：* Schild, Henning (CT RDA IOT SES-DE); Kiszka, Jan (CT RDA
> > IOT SES-DE)
> > *抄送：* isar-users@googlegroups.com; Vijai Kumar K
> > *主题：* Re: [PATCHv4 22/26] meta: deb-dl-dir: do not cache debs
> > from isar-apt
> > 
> > after building, grep "command not found" or "repo_contains_package"
> > in the log.do_* file.
> > 
> > I met this problem on an early version of v4 round patch, not sure
> > if still exists on the latest version. will run a test build
> > against the latest patch to check.
> > 
> > Sorry for mailing via phone, due to coronavirus, not convenient for
> > me to use laptop emails.
> > 
> > With best regards
> > ------------------------------------------------------------------------
> > *发件人：* Schild, Henning (CT RDA IOT SES-DE)
> > *发送时间：* 2020年2月4日星期二 02:59
> > *收件人：* Kiszka, Jan (CT RDA IOT SES-DE)
> > *抄送：* isar-users@googlegroups.com; Su, Bao Cheng (RC-CN DI FA
> > R&D SW); Vijai Kumar K
> > *主题：* Re: [PATCHv4 22/26] meta: deb-dl-dir: do not cache debs
> > from isar-apt
> > 
> > Am Mon, 3 Feb 2020 19:27:01 +0100
> > schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> >   
> >> Am Mon, 3 Feb 2020 18:20:11 +0100
> >> schrieb Jan Kiszka <jan.kiszka@siemens.com>:
> >>   
> >> > On 31.01.20 15:29, [ext] Henning Schild wrote:    
> >> > > From: Henning Schild <henning.schild@siemens.com>
> >> > > 
> >> > > Packages from isar-apt are not downloaded from the outside and
> >> > > should not be cached.
> >> > > 
> >> > > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >> > > ---
> >> > >   meta/classes/deb-dl-dir.bbclass | 14 ++++++++++----
> >> > >   1 file changed, 10 insertions(+), 4 deletions(-)
> >> > > 
> >> > > diff --git a/meta/classes/deb-dl-dir.bbclass
> >> > > b/meta/classes/deb-dl-dir.bbclass index ab4b1f09..f9699603
> >> > > 100644 --- a/meta/classes/deb-dl-dir.bbclass
> >> > > +++ b/meta/classes/deb-dl-dir.bbclass
> >> > > @@ -3,8 +3,10 @@
> >> > >   #
> >> > >   # SPDX-License-Identifier: MIT
> >> > >   
> >> > > +inherit repository
> >> > > +
> >> > >   deb_dl_dir_import() {
> >> > > -    export pc="${DEBDIR}/${DISTRO}"
> >> > > +    export pc="${DEBDIR}/${DISTRO}/"
> >> > >       export rootfs="${1}"
> >> > >       [ ! -d "${pc}" ] && return 0
> >> > >       flock -s "${pc}".lock -c ' \
> >> > > @@ -14,12 +16,16 @@ deb_dl_dir_import() {
> >> > >   }
> >> > >   
> >> > >   deb_dl_dir_export() {
> >> > > -    export pc="${DEBDIR}/${DISTRO}"
> >> > > +    export pc="${DEBDIR}/${DISTRO}/"
> >> > >       export rootfs="${1}"
> >> > >       mkdir -p "${pc}"
> >> > >       flock "${pc}".lock -c ' \
> >> > > -        sudo find "${rootfs}"/var/cache/apt/archives/ -type f
> >> > > -iname '*\.deb' \
> >> > > -            -exec cp -f '{}' "${pc}" \;
> >> > > +        find "${rootfs}"/var/cache/apt/archives/ -type f
> >> > > -iname '*\.deb' |\
> >> > > +        while read p; do
> >> > > +             repo_contains_package
> >> > > "${REPO_ISAR_DIR}"/"${DISTRO}" "${p}" && \      
> >> > 
> >> > repo_contains_package may not be found inside the flock shell
> >> > context, as Bao Cheng noticed out. He suggests the pattern    
> >> 
> >> That is possible indeed. I remember that i struggled re-using the
> >> function since i did not want to code it twice.
> >> 
> >> But to be honest i do not understand the problem with the given
> >> description. Bao Cheng please go into more detail.  
> > 
> > This is a hot code-path and if it is _very_ broken that would be
> > very visible.
> > 
> > Just tried a build and am looking at the download cache and isar-apt
> > 
> > find tmp/deploy/isar-apt/ -iname *hello*
> > tmp/deploy/isar-apt/apt/debian-buster/pool/main/libh/libhello
> > tmp/deploy/isar-apt/apt/debian-buster/pool/main/libh/libhello/libhello-dbgsym_0.1_amd64.deb
> > tmp/deploy/isar-apt/apt/debian-buster/pool/main/libh/libhello/libhello-dev_0.1_amd64.deb
> > tmp/deploy/isar-apt/apt/debian-buster/pool/main/libh/libhello/libhello_0.1_amd64.deb
> > ....
> > tmp/deploy/isar-apt/apt/debian-buster/pool/main/h/hello-isar/hello-isar_0.3_amd64.deb
> > 
> > Ok so we have the hello stuff in isar-apt, the own package as well
> > as the rebuild upstream (also own).
> > 
> > And nothing in the download cache on hello -> expected for the image
> > find downloads/deb/ -iname *hello*
> > <nothing>
> > 
> > Meaning the filtering worked in my case.
> > 
> > And the caching of all required outside stuff works as well, since
> > the offline rebuild works.
> > 
> > Caching and filtering are the two main aspects of that code.
> > 
> > Please make sure to give feedback for sure, i do not want such a
> > unclear description slowing down the merge even more.
> > 
> > Henning
> >   
> >> Maybe the filtering indeed does not work, i will try that on a
> >> simple test now. And it might be a good idea to make sure that
> >> code gets a "set -e".
> >> 
> >> Henning
> >>   
> >> > (flock 8
> >> >   ...
> >> > ) 8>${LOCKFILE}
> >> >
> >> > Bao Check, maybe you can describe how you noticed.
> >> > 
> >> > Jan
> >> >     
> >> > > +                 continue
> >> > > +             sudo cp -f "${p}" "${pc}"
> >> > > +        done
> >> > >           sudo chown -R $(id -u):$(id -g) "${pc}"
> >> > >       '
> >> > >   }
> >> > >       
> >> >     
> >>   
> >   
>