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

[vijai kumar <vijaikumar.kanagarajan@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8959 bytes --]



On Monday, May 4, 2020 at 2:12:38 PM UTC+5:30, vijai kumar wrote:
>
>
>
> 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.
>

Looking back, I don't think there will ever be a case like this for image 
rootfs. 
The lock may not be needed for mounts. 


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

[-- Attachment #1.2: Type: text/html, Size: 13506 bytes --]