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.

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