On Friday, September 4, 2020 at 12:28:24 AM UTC+5:30 Henning Schild wrote:

> Hi Vijay, 
>
> sorry i never really finished the review on this one. I hope the 
> maintainer will take action and look into it. I would be happy to look 
> at parts again but currently do not have the time to review further. 
>
>
No problem Hennnig. I will implement your initial review comments and send
out a new series. Hoping that other reviewers would be able to have a look 
at that.
Please chime in when you find time.

Thanks,
Vijai Kumar K

Henning 
>
> On Thu, 3 Sep 2020 04:43:13 -0700 (PDT) 
> "vijaikumar....@gmail.com" <vijaikumar....@gmail.com> wrote: 
>
> > Ping 
> > 
> > On Thursday, June 11, 2020 at 5:14:38 PM UTC+5:30 
> > vijaikumar....@gmail.com wrote: 
> > 
> > > Hi Henning, 
> > > 
> > > Did you get a chance to review the rest of the series? 
> > > 
> > > Thanks, 
> > > Vijai Kumar K 
> > > 
> > > On Fri, May 8, 2020 at 11:14 AM vijai kumar 
> > > <vijaikumar....@gmail.com> wrote: 
> > > > 
> > > > 
> > > > 
> > > > 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}' \ 
> > > >>> > > 
> > > >>> > > 
> > > >>> > 
> > > >>> 
> > > > -- 
> > > > You received this message because you are subscribed to the 
> > > > Google 
> > > Groups "isar-users" group. 
> > > > To unsubscribe from this group and stop receiving emails from it, 
> > > > send 
> > > an email to isar-users+...@googlegroups.com. 
> > > > To view this discussion on the web visit 
> > > 
> https://groups.google.com/d/msgid/isar-users/f6faa39e-fc03-46e0-99e0-6b08d09a8d4b%40googlegroups.com 
> > > . 
> > > 
> > 
>
>