From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6816610675337461760 Date: Wed, 22 Apr 2020 03:20:48 -0700 (PDT) From: vijai kumar To: isar-users Message-Id: <9e5b92cb-127a-4ccb-903f-403b41401482@googlegroups.com> In-Reply-To: <20200422092516.37031839@md1za8fc.ad001.siemens.net> References: <20200417093040.15130-1-Vijaikumar_Kanagarajan@mentor.com> <20200417093040.15130-6-Vijaikumar_Kanagarajan@mentor.com> <20200422092516.37031839@md1za8fc.ad001.siemens.net> Subject: Re: [PATCH v5 05/13] deb-dl-dir: Download files only belonging to the current image MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_Part_2996_981316600.1587550849045" X-Google-Token: EIC1gPUFIAImqDFOkLY0 X-Google-IP: 192.94.34.34 X-TUID: UFteCv/MmQvl ------=_Part_2996_981316600.1587550849045 Content-Type: multipart/alternative; boundary="----=_Part_2997_1099684260.1587550849045" ------=_Part_2997_1099684260.1587550849045 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On Wednesday, April 22, 2020 at 12:55:19 PM UTC+5:30, Henning Schild wrote: > > On Fri, 17 Apr 2020 15:00:32 +0530 > Vijai Kumar K > wrote: > > > Avoid downloading deb-srcs for debs cached from other image builds. > > One way to ensure that is to see if the package is present in the dpkg > > status file. > > Not sure i understand what it is doing/avoiding. Maybe you can go into > more detail with an example. > We are looping over the entries in DEBDIR/deb, It would contain packages from all former builds. To make sure we take only those are used in the image, we check the status file. While handling this I had a thought why not to use the status file as the source directly, but P7 in this series changed that. Not all is available in status file, only those which are installed. There might be chances that something is removed later on like the localepurge in meta/classes/image-locales-extension.bbclass. Those does not get captured in the status file or the manifest and hence the dpkg log check in P7. > > Signed-off-by: Vijai Kumar K > > > --- > > meta/classes/deb-dl-dir.bbclass | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/meta/classes/deb-dl-dir.bbclass > > b/meta/classes/deb-dl-dir.bbclass index 9399741..b3f4842 100644 > > --- a/meta/classes/deb-dl-dir.bbclass > > +++ b/meta/classes/deb-dl-dir.bbclass > > @@ -5,6 +5,15 @@ > > > > inherit repository > > > > +check_in_rootfs() { > > I am confused by the logic again. And maybe the name. We already have > repo_contains_package which suggests a "boolean" answer, should this > one be "rootfs_contains_package"? Or maybe "has_package_installed" > I have to admit, on seeing this back, it is a bit confusing, I will fix these naming. > > > + local package="$( dpkg-deb --show --showformat '${Package}' > > "${1}" )" > > + local output="$( grep -hs "^Package: ${package}" \ > > + "${IMAGE_ROOTFS}"/var/lib/dpkg/status \ > > + "${BUILDCHROOT_HOST_DIR}"/var/lib/dpkg/status \ > > + "${BUILDCHROOT_TARGET_DIR}"/var/lib/dpkg/status )" > > I think you should chroot in and ask "dpkg -i", doing this kind of > guessing on the outside is error-prone. > > > + [ -z "${output}" ] && return 1 || return 0 > > Again confused, in shell boolean "0" means true and anything else means > false. > Sorry. Confusing indeed. I will use the below. Thanks. > > return [ -n "${output}" ] > > > +} > > + > > debsrc_download() { > > export rootfs="$1" > > export rootfs_distro="$2" > > @@ -18,6 +27,7 @@ debsrc_download() { > > mount --bind "${DEBSRCDIR}" "${rootfs}/deb-src" > > EOSUDO > > find "${rootfs}/var/cache/apt/archives/" -maxdepth 1 -type f > > -iname '*\.deb' | while read package; do > > + check_in_rootfs "${package}" || continue > > This reads like you skip the download when you found package to _not_ > be "in_rootfs". > Sorry that it is confusing. I will fix these to be more clear in the next series. > > Henning > > > 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. > > ------=_Part_2997_1099684260.1587550849045 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable


On Wednesday, April 22, 2020 at 12:55:19 PM UTC+5:= 30, Henning Schild wrote:
On Fr= i, 17 Apr 2020 15:00:32 +0530
Vijai Kumar K <vijaikumar...@gmail.com> wrote:

> Avoid downloading deb-srcs for debs cached from other image builds= .
> One way to ensure that is to see if the package is present in the = dpkg
> status file.

Not sure i understand what it is doing/avoiding. Maybe you can go into
more detail with an example.

We are looping over the entries in DEB= DIR/deb, It would contain packages from
all former builds. To mak= e sure we take only those are used in the image, we check
the sta= tus file. While handling this I had a thought why not to use the status fil= e as the
source directly, but P7 in this series changed that. Not= all is available in status file,=C2=A0
only those which are inst= alled. There might be chances that something is removed later
on = like the localepurge in meta/classes/image-locales-extension.bbclass. Those= does not
get captured in the status file or the manifest and hen= ce the dpkg log check in P7.


> Signed-off-by: Vijai Kumar K <Vijaikumar_...@mentor.com&g= t;
> ---
> =C2=A0meta/classes/deb-dl-dir.bbclass | 10 ++++++++++
> =C2=A01 file changed, 10 insertions(+)
>=20
> diff --git a/meta/classes/deb-dl-dir.bbclass
> b/meta/classes/deb-dl-dir.bbclass index 9399741..b3f4842 1006= 44
> --- a/meta/classes/deb-dl-dir.bbclass
> +++ b/meta/classes/deb-dl-dir.bbclass
> @@ -5,6 +5,15 @@
> =C2=A0
> =C2=A0inherit repository
>
> +check_in_rootfs() {

I am confused by the logic again. And maybe the name. We already have
repo_contains_package which suggests a "boolean" answer, shou= ld this
one be "rootfs_contains_package"? Or maybe "has_package_= installed"

I have to admit, on seeing this back, = it is a bit confusing, I will fix these naming.
=C2=A0

> + =C2=A0 =C2=A0local package=3D"$( dpkg-deb --show --showform= at '${Package}'
> "${1}" )"
> + =C2=A0 =C2=A0local output=3D"$( grep -hs "^Package: ${= package}" \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"${IMAGE_ROOTFS}&q= uot;/var/lib/dpkg/status \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"${BUILDCHROOT_HOS= T_DIR}"/var/lib/dpkg/status \
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"${BUILDCHROOT_TAR= GET_DIR}"/var/lib/dpkg/status )"

I think you should chroot in and ask "dpkg -i", doing this ki= nd of
guessing on the outside is error-prone.

> + =C2=A0 =C2=A0[ -z "${output}" ] && return 1 ||= return 0

Again confused, in shell boolean "0" means true and anything = else means
false.

Sorry. Confusing indeed. I will use th= e below. Thanks.
=C2=A0

return [ -n "${output}" ]

> +}
> +
> =C2=A0debsrc_download() {
> =C2=A0 =C2=A0 =C2=A0export rootfs=3D"$1"
> =C2=A0 =C2=A0 =C2=A0export rootfs_distro=3D"$2"
> @@ -18,6 +27,7 @@ debsrc_download() {
> =C2=A0 =C2=A0 =C2=A0mount --bind "${DEBSRCDIR}" "${= rootfs}/deb-src"
> =C2=A0EOSUDO
> =C2=A0 =C2=A0 =C2=A0find "${rootfs}/var/cache/apt/archiv= es/" -maxdepth 1 -type f
> -iname '*\.deb' | while read package; do
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0check_in_rootfs "${package}"= ; || continue

This reads like you skip the download when you found package to _not_
be "in_rootfs".

Sorry that it is confusing. I will fix= these to be more clear in the next series.
=C2=A0

Henning

> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0local src=3D"$( 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.

------=_Part_2997_1099684260.1587550849045-- ------=_Part_2996_981316600.1587550849045--