From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6816610675337461760 Date: Thu, 7 May 2020 22:44:05 -0700 (PDT) From: vijai kumar To: isar-users Message-Id: In-Reply-To: <2043181c-a74f-4bfa-a8dd-36916d87c962@googlegroups.com> References: <20200417093040.15130-1-Vijaikumar_Kanagarajan@mentor.com> <20200417093040.15130-4-Vijaikumar_Kanagarajan@mentor.com> <20200422090639.645bf93b@md1za8fc.ad001.siemens.net> <20200429081937.7e6a059d@md1za8fc.ad001.siemens.net> <2043181c-a74f-4bfa-a8dd-36916d87c962@googlegroups.com> Subject: Re: [PATCH v5 03/13] meta: cache deb srcs as part of postprocessing MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_Part_287_468093218.1588916645921" X-Google-Token: EKXj0_UFInIbkoqJ8F00 X-Google-IP: 192.94.34.34 X-TUID: biZJaPc00/Ln ------=_Part_287_468093218.1588916645921 Content-Type: multipart/alternative; boundary="----=_Part_288_1721550343.1588916645922" ------=_Part_288_1721550343.1588916645922 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 : >> >> > 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 > wrote: >> > > >> > > > Collect the deb sources of the corresponding deb binaries cached >> > > > in DEBDIR as part of image postprocess. >> > > > >> > > > Signed-off-by: Vijai Kumar K > > > > > --- >> > > > 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}' \ >> > > >> > > >> > >> >> ------=_Part_288_1721550343.1588916645922 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable


On Monday, May 4, 2020 at 2:12:38 PM UTC+5:30, vij= ai kumar wrote:


On Wednesday, April 29, 2020 at 11:49:41 AM UTC+5:30, Henning Sch= ild 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 Schi= ld
> wrote:
> >
> > On Fri, 17 Apr 2020 15:00:30 +0530=20
> > Vijai Kumar K <vijaikumar...@gmail.com <javascri= pt:>> wrote:=20
> > =C2=A0
> > > Collect the deb sources of the corresponding deb binarie= s cached=20
> > > in DEBDIR as part of image postprocess.=20
> > >=20
> > > Signed-off-by: Vijai Kumar K <Vijaikumar_...@mento= r.com
> > > <javascript:>> ---=20
> > > =C2=A0meta/classes/deb-dl-dir.bbclass | 39=20
> > > +++++++++++++++++++++++++++++++++ meta/classes/imag= e.bbclass
> > > | 2 +- meta/classes/rootfs.bbclass =C2=A0 =C2=A0 | =C2= =A08 +++++++=20
> > > =C2=A03 files changed, 48 insertions(+), 1 deletion(-)= =20
> > >=20
> > > diff --git a/meta/classes/deb-dl-dir.bbclass=20
> > > b/meta/classes/deb-dl-dir.bbclass index 29a3d67..47= 2b9fe 100644=20
> > > --- a/meta/classes/deb-dl-dir.bbclass=20
> > > +++ b/meta/classes/deb-dl-dir.bbclass=20
> > > @@ -5,6 +5,45 @@=20
> > > =C2=A0=20
> > > =C2=A0inherit repository=20
> > > =C2=A0=20
> > > +debsrc_download() {=20
> > > + =C2=A0 =C2=A0export rootfs=3D"$1"=20
> > > + =C2=A0 =C2=A0export rootfs_distro=3D"$2"=20
> > > + =C2=A0 =C2=A0mkdir -p "${DEBSRCDIR}"/"$= {rootfs_distro}"=20
> > > + =C2=A0 =C2=A0( flock 9 =C2=A0=20
> >
> > I think you can grab that lock only for the actual writes, an= d keep
> > the generation of the list out of the critical section.=20
> > =C2=A0
>=20
> To note, this lock also guards the mount part.=20

Any why does that need to be under the lock? What is the essence of the
lock anyways?

We are mounting DEBSRCDIR onto the ima= ge rootfs and then downloading
the deb srcs on-to that. Once that= is done, we are unmounting it. The lock=C2=A0
makes sure that th= ere is no race condition between mounts and unmounts.
Not seen su= ch 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.=C2=A0
The lock may not be nee= ded for mounts.=C2=A0


On an alternate way, we co= uld just mount DL_DIR in rootfs_do_mounts and take care
of the cl= eanup in rootfs_finalize. That way we can avoid this additional mount.=C2= =A0

As fa= r 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 t= o DEBSRCDIR.
1. The post process caching function from this serie= s.
2.=C2=A0 Fetch case using SRC_URI=3Dapt://

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

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

Assume there are two multiconfig images A and B= both include
the recipe which provides Package X. In that case w= hen image A is in postprocess
deb-src caching, Package X source w= ould 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
<= div>re-download the package.




> > Note that i played with this "flock 9" syntax inste= ad of what is
> > used in deb-dl-dir, it did not work as expected. Probably bec= ause
> > it will be many shells and 9 is a different fd in all of them= .=20
> > =C2=A0
>=20
> Interesting. Works as expected here. =C2=A0If we still need to swi= tch the
> syntax to be sure, we could.

So you did try multiconfig and two or more writers never ran at the
same time?

Yes.
=C2=A0
In deb-dl-dir there is exclusive writer lockin= g and shared reader
locking, maybe that is why i decided against "flock 9".

> > + =C2=A0 =C2=A0set -e =C2=A0=20
> > > + =C2=A0 =C2=A0printenv | grep -q BB_VERBOSE_LOGS &&= amp; set -x=20
> > > + =C2=A0 =C2=A0sudo -E -s <<'EOSUDO'=20
> > > + =C2=A0 =C2=A0mkdir -p "${rootfs}/deb-src"=20
> > > + =C2=A0 =C2=A0mountpoint -q "${rootfs}/deb-src&quo= t; || \=20
> > > + =C2=A0 =C2=A0mount --bind "${DEBSRCDIR}" &qu= ot;${rootfs}/deb-src"=20
> > > +EOSUDO=20
> > > + =C2=A0 =C2=A0find "${rootfs}/var/cache/apt/a= rchives/" -maxdepth 1 -type f=20
> > > -iname '*\.deb' | while read package; do=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0local src=3D"$( dpkg-d= eb --show --showformat '${Source}'=20
> > > "${package}" )"=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0# If the binary package ver= sion and source package
> > > version are different, then the=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0# source package version wi= ll be present inside "()" of
> > > the Source field. =C2=A0=20
> >
> > dpkg-query(1)=20
> >
> > dpkg-deb --show --showformat '${source:Version}'=20
> > dpkg-deb --show --showformat '${source:Upstream-Version}&= #39;=20
> >
> > might help to write this cleaner.=20
> > =C2=A0
>=20
> Thanks. Will use this.
> =C2=A0
>=20
> > =C2=A0
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0local version=3D"$( ec= ho "$src" | cut -sd "(" -f2 | cut
> > > -sd ")" -f1 )"=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if [ -z ${version} ]; then= =20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0version=3D&qu= ot;$( dpkg-deb --show --showformat
> > > '${Version}' "${package}" )"=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0fi=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0# Now strip any version inf= ormation that might be
> > > available.=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0src=3D"$( echo "$= src" | cut -d' ' -f1 )"=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0# If there is no source fie= ld, then the source package
> > > has the same name as the=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0# binary package.=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if [ -z "${src}" = ];then=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0src=3D"$= ( dpkg-deb --show --showformat '${Package}'=20
> > > "${package}" )"=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0fi =C2=A0=20
> >
> > I still could not find those proxies that all downloading fun= ctions=20
> > need in their env.=20
> > =C2=A0
>=20
> From what I see, the rootfs class from where this is called, alrea= dy
> takes care of this
>=20
> https://github.com/ilbers/isar/blob/next/meta/class= es/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 wi= ll learn
how you feature and that switch work together.

I did an offline build(with BB_NO_NETW= ORK set) with this series and it seems
to work fine.
Thanks,
Vijai Kumar K


Henning

>=20
> >
> > Henning=20
> > =C2=A0
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0sudo -E chroot --userspec= =3D$( id -u ):$( id -g )
> > > ${rootfs} \=20
> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sh -c ' m= kdir -p "/deb-src/${1}/${2}" && cd=20
> > > "/deb-src/${1}/${2}" && apt-get -y --d= ownload-only --only-source=20
> > > source "$2"=3D"$3" ' download-sr= c "${rootfs_distro}" "${src}"=20
> > > "${version}"=20
> > > + =C2=A0 =C2=A0done=20
> > > + =C2=A0 =C2=A0sudo -E -s <<'EOSUDO'=20
> > > + =C2=A0 =C2=A0mountpoint -q "${rootfs}/deb-src&quo= t; && \=20
> > > + =C2=A0 =C2=A0umount -l "${rootfs}/deb-src"= =20
> > > + =C2=A0 =C2=A0rm -rf "${rootfs}/deb-src"=20
> > > +EOSUDO=20
> > > + =C2=A0 =C2=A0) 9>"${DEBSRCDIR}/${rootfs_d= istro}.lock"=20
> > > +}=20
> > > +=20
> > > =C2=A0deb_dl_dir_import() {=20
> > > =C2=A0 =C2=A0 =C2=A0export pc=3D"${DEBDIR}/${2}&quo= t;=20
> > > =C2=A0 =C2=A0 =C2=A0export rootfs=3D"${1}"=20
> > > diff --git a/meta/classes/image.bbclass
> > > b/meta/classes/image.bbclass index 6b04c0a..fcaebd6 1006= 44=20
> > > --- a/meta/classes/image.bbclass=20
> > > +++ b/meta/classes/image.bbclass=20
> > > @@ -63,7 +63,7 @@ image_do_mounts() {=20
> > > =C2=A0}=20
> > > =C2=A0=20
> > > =C2=A0ROOTFSDIR =3D "${IMAGE_ROOTFS}"=20
> > > -ROOTFS_FEATURES +=3D "clean-package-cache generate= -manifest"=20
> > > +ROOTFS_FEATURES +=3D "clean-package-cache generate= -manifest=20
> > > cach-deb-src" ROOTFS_PACKAGES +=3D "${IMAGE_PR= EINSTALL}=20
> > > ${IMAGE_INSTALL}" ROOTFS_MANIFEST_DEPLOY_DIR ?=3D
> > > "${DEPLOY_DIR_IMAGE}"=20
> > > diff --git a/meta/classes/rootfs.bbclass
> > > b/meta/classes/rootfs.bbclass index cee358c..ee57989 100= 644=20
> > > --- a/meta/classes/rootfs.bbclass=20
> > > +++ b/meta/classes/rootfs.bbclass=20
> > > @@ -185,6 +185,14 @@ python do_rootfs_install() {=20
> > > =C2=A0}=20
> > > =C2=A0addtask rootfs_install before do_rootfs_postproces= s after
> > > do_unpack=20
> > > +ROOTFS_POSTPROCESS_COMMAND +=3D=20
> > > "${@bb.utils.contains('ROOTFS_FEATURES'= ;, 'cache-deb-src',=20
> > > 'cache_deb_src', '', d)}" +cache_de= b_src() {=20
> > > + =C2=A0 =C2=A0rootfs_install_resolvconf=20
> > > + =C2=A0 =C2=A0deb_dl_dir_import ${ROOTFSDIR} ${ROOTFS_D= ISTRO}=20
> > > + =C2=A0 =C2=A0debsrc_download ${ROOTFSDIR} ${ROOTFS_DIS= TRO}=20
> > > + =C2=A0 =C2=A0rootfs_install_clean_files=20
> > > +}=20
> > > +=20
> > > =C2=A0ROOTFS_POSTPROCESS_COMMAND +=3D=20
> > > "${@bb.utils.contains('ROOTFS_FEATURES'= ;, 'clean-package-cache',=20
> > > 'rootfs_postprocess_clean_package_cache', &= #39;', d)}"=20
> > > rootfs_postprocess_clean_package_cache() { sudo -E = chroot=20
> > > '${ROOTFSDIR}' \ =C2=A0=20
> >
> > =C2=A0
>=20

------=_Part_288_1721550343.1588916645922-- ------=_Part_287_468093218.1588916645921--