On Wednesday, April 29, 2020 at 11:49:41 AM UTC+5:30, Henning Schild wrote:
Am Wed, 22 Apr 2020 02:57:13 -0700 (PDT)
schrieb vijai kumar <vijaikumar...@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?

We are mounting DEBSRCDIR onto the image rootfs and then downloading
the deb srcs on-to that. Once that is done, we are unmounting it. The lock 
makes sure that there is no race condition between mounts and unmounts.
Not seen such races but there could be a situation where in the first builds unmount
is called after the second builds mount check.

On an alternate way, we could just mount DL_DIR in rootfs_do_mounts and take care
of the cleanup in rootfs_finalize. That way we can avoid this additional mount.

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.


As I see, there are only two writers who write to DEBSRCDIR.
1. The post process caching function from this series.
2.  Fetch case using SRC_URI=apt://

Most of the package sources are fetched via postprocess. And with lock in
place no two deb-src caching takes place at the same time.

For fetch case using SRC_URI=apt://, say Package X.

Assume there are two multiconfig images A and B both include
the recipe which provides Package X. In that case when image A is in postprocess
deb-src caching, Package X source would already be available in DEBSRCDIR.
If multiconfig image B is fetching package X when image A is in postprocess
accessing it, there would be no issue, since apt-get source download-only does not
re-download the package.




> > 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?

Yes.
 
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.

I did an offline build(with BB_NO_NETWORK set) with this series and it seems
to work fine.

Thanks,
Vijai Kumar K


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}' \  
> >
> >  
>