Re: [PATCH v5 03/13] meta: cache deb srcs as part of postprocessing

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

Am Wed, 22 Apr 2020 02:57:13 -0700 (PDT)
schrieb vijai kumar <vijaikumar.kanagarajan@gmail.com>:

> On Wednesday, April 22, 2020 at 12:36:42 PM UTC+5:30, Henning Schild
> wrote:
> >
> > On Fri, 17 Apr 2020 15:00:30 +0530 
> > Vijai Kumar K <vijaikumar...@gmail.com <javascript:>> wrote: 
> >  
> > > Collect the deb sources of the corresponding deb binaries cached 
> > > in DEBDIR as part of image postprocess. 
> > > 
> > > Signed-off-by: Vijai Kumar K <Vijaikumar_...@mentor.com
> > > <javascript:>> --- 
> > >  meta/classes/deb-dl-dir.bbclass | 39 
> > > +++++++++++++++++++++++++++++++++ meta/classes/image.bbclass
> > > | 2 +- meta/classes/rootfs.bbclass     |  8 +++++++ 
> > >  3 files changed, 48 insertions(+), 1 deletion(-) 
> > > 
> > > diff --git a/meta/classes/deb-dl-dir.bbclass 
> > > b/meta/classes/deb-dl-dir.bbclass index 29a3d67..472b9fe 100644 
> > > --- a/meta/classes/deb-dl-dir.bbclass 
> > > +++ b/meta/classes/deb-dl-dir.bbclass 
> > > @@ -5,6 +5,45 @@ 
> > >   
> > >  inherit repository 
> > >   
> > > +debsrc_download() { 
> > > +    export rootfs="$1" 
> > > +    export rootfs_distro="$2" 
> > > +    mkdir -p "${DEBSRCDIR}"/"${rootfs_distro}" 
> > > +    ( flock 9   
> >
> > I think you can grab that lock only for the actual writes, and keep
> > the generation of the list out of the critical section. 
> >  
> 
> To note, this lock also guards the mount part. 

Any why does that need to be under the lock? What is the essence of the
lock anyways?
As far as i understand there are multiple writers potentially creating
the same files in DEBSRCDIR. If that is a problem we also need locking
in do_apt_fetch. While one multiconfig image is in your postprocess,
another might still be fetching that same sources.

> > Note that i played with this "flock 9" syntax instead of what is
> > used in deb-dl-dir, it did not work as expected. Probably because
> > it will be many shells and 9 is a different fd in all of them. 
> >  
> 
> Interesting. Works as expected here.  If we still need to switch the
> syntax to be sure, we could.

So you did try multiconfig and two or more writers never ran at the
same time?
In deb-dl-dir there is exclusive writer locking and shared reader
locking, maybe that is why i decided against "flock 9".

> > +    set -e   
> > > +    printenv | grep -q BB_VERBOSE_LOGS && set -x 
> > > +    sudo -E -s <<'EOSUDO' 
> > > +    mkdir -p "${rootfs}/deb-src" 
> > > +    mountpoint -q "${rootfs}/deb-src" || \ 
> > > +    mount --bind "${DEBSRCDIR}" "${rootfs}/deb-src" 
> > > +EOSUDO 
> > > +    find "${rootfs}/var/cache/apt/archives/" -maxdepth 1 -type f 
> > > -iname '*\.deb' | while read package; do 
> > > +        local src="$( dpkg-deb --show --showformat '${Source}' 
> > > "${package}" )" 
> > > +        # If the binary package version and source package
> > > version are different, then the 
> > > +        # source package version will be present inside "()" of
> > > the Source field.   
> >
> > dpkg-query(1) 
> >
> > dpkg-deb --show --showformat '${source:Version}' 
> > dpkg-deb --show --showformat '${source:Upstream-Version}' 
> >
> > might help to write this cleaner. 
> >  
> 
> Thanks. Will use this.
>  
> 
> >  
> > > +        local version="$( echo "$src" | cut -sd "(" -f2 | cut
> > > -sd ")" -f1 )" 
> > > +        if [ -z ${version} ]; then 
> > > +            version="$( dpkg-deb --show --showformat
> > > '${Version}' "${package}" )" 
> > > +        fi 
> > > +        # Now strip any version information that might be
> > > available. 
> > > +        src="$( echo "$src" | cut -d' ' -f1 )" 
> > > +        # If there is no source field, then the source package
> > > has the same name as the 
> > > +        # binary package. 
> > > +        if [ -z "${src}" ];then 
> > > +            src="$( dpkg-deb --show --showformat '${Package}' 
> > > "${package}" )" 
> > > +        fi   
> >
> > I still could not find those proxies that all downloading functions 
> > need in their env. 
> >  
> 
> From what I see, the rootfs class from where this is called, already
> takes care of this
> 
> https://github.com/ilbers/isar/blob/next/meta/classes/rootfs.bbclass#L22

I see. It might be a good idea to run a local test with BB_NO_NETWORK.
That will show whether the proxies really "arrive" and you will learn
how you feature and that switch work together.

Henning

> 
> >
> > Henning 
> >  
> > > +        sudo -E chroot --userspec=$( id -u ):$( id -g )
> > > ${rootfs} \ 
> > > +            sh -c ' mkdir -p "/deb-src/${1}/${2}" && cd 
> > > "/deb-src/${1}/${2}" && apt-get -y --download-only --only-source 
> > > source "$2"="$3" ' download-src "${rootfs_distro}" "${src}" 
> > > "${version}" 
> > > +    done 
> > > +    sudo -E -s <<'EOSUDO' 
> > > +    mountpoint -q "${rootfs}/deb-src" && \ 
> > > +    umount -l "${rootfs}/deb-src" 
> > > +    rm -rf "${rootfs}/deb-src" 
> > > +EOSUDO 
> > > +    ) 9>"${DEBSRCDIR}/${rootfs_distro}.lock" 
> > > +} 
> > > + 
> > >  deb_dl_dir_import() { 
> > >      export pc="${DEBDIR}/${2}" 
> > >      export rootfs="${1}" 
> > > diff --git a/meta/classes/image.bbclass
> > > b/meta/classes/image.bbclass index 6b04c0a..fcaebd6 100644 
> > > --- a/meta/classes/image.bbclass 
> > > +++ b/meta/classes/image.bbclass 
> > > @@ -63,7 +63,7 @@ image_do_mounts() { 
> > >  } 
> > >   
> > >  ROOTFSDIR = "${IMAGE_ROOTFS}" 
> > > -ROOTFS_FEATURES += "clean-package-cache generate-manifest" 
> > > +ROOTFS_FEATURES += "clean-package-cache generate-manifest 
> > > cach-deb-src" ROOTFS_PACKAGES += "${IMAGE_PREINSTALL} 
> > > ${IMAGE_INSTALL}" ROOTFS_MANIFEST_DEPLOY_DIR ?=
> > > "${DEPLOY_DIR_IMAGE}" 
> > > diff --git a/meta/classes/rootfs.bbclass
> > > b/meta/classes/rootfs.bbclass index cee358c..ee57989 100644 
> > > --- a/meta/classes/rootfs.bbclass 
> > > +++ b/meta/classes/rootfs.bbclass 
> > > @@ -185,6 +185,14 @@ python do_rootfs_install() { 
> > >  } 
> > >  addtask rootfs_install before do_rootfs_postprocess after
> > > do_unpack 
> > > +ROOTFS_POSTPROCESS_COMMAND += 
> > > "${@bb.utils.contains('ROOTFS_FEATURES', 'cache-deb-src', 
> > > 'cache_deb_src', '', d)}" +cache_deb_src() { 
> > > +    rootfs_install_resolvconf 
> > > +    deb_dl_dir_import ${ROOTFSDIR} ${ROOTFS_DISTRO} 
> > > +    debsrc_download ${ROOTFSDIR} ${ROOTFS_DISTRO} 
> > > +    rootfs_install_clean_files 
> > > +} 
> > > + 
> > >  ROOTFS_POSTPROCESS_COMMAND += 
> > > "${@bb.utils.contains('ROOTFS_FEATURES', 'clean-package-cache', 
> > > 'rootfs_postprocess_clean_package_cache', '', d)}" 
> > > rootfs_postprocess_clean_package_cache() { sudo -E chroot 
> > > '${ROOTFSDIR}' \   
> >
> >  
>