From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6816610675337461760 Date: Thu, 3 Sep 2020 04:43:13 -0700 (PDT) From: "vijaikumar....@gmail.com" To: isar-users Message-Id: In-Reply-To: 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_834_122695421.1599133393635" X-TUID: z+0p9LjAj+9g ------=_Part_834_122695421.1599133393635 Content-Type: multipart/alternative; boundary="----=_Part_835_516254853.1599133393635" ------=_Part_835_516254853.1599133393635 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 > 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 : > >>> > >>> > 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}' \ > >>> > > > >>> > > > >>> > > >>> > > -- > > 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 > . > ------=_Part_835_516254853.1599133393635 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit 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.
------=_Part_835_516254853.1599133393635-- ------=_Part_834_122695421.1599133393635--