Re: [PATCH v4 2/2] meta: cache deb srcs as part of postprocessing

[Henning Schild <henning.schild@siemens.com>]

On Thu, 16 Apr 2020 21:27:54 +0530
vijai kumar <vijaikumar.kanagarajan@gmail.com> wrote:

> On Wed, Apr 15, 2020 at 11:50 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > On Wed, 15 Apr 2020 17:59:12 +0530
> > vijai kumar <vijaikumar.kanagarajan@gmail.com> wrote:
> >  
> > > On Wed, Apr 8, 2020 at 6:00 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > >
> > > > Am Wed, 8 Apr 2020 16:07:15 +0530
> > > > schrieb vijai kumar <vijaikumar.kanagarajan@gmail.com>:
> > > >  
> > > > > On Wed, Apr 8, 2020 at 3:34 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> > > > > >
> > > > > > Am Fri, 3 Apr 2020 18:35:51 +0530
> > > > > > schrieb Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>:
> > > > > >  
> > > > > > > Collect the deb sources of the corresponding deb binaries
> > > > > > > cached in DEBDIR as part of postprocess for those to be
> > > > > > > later included into the final base-apt by do_cache.
> > > > > > >
> > > > > > > Signed-off-by: Vijai Kumar K
> > > > > > > <Vijaikumar_Kanagarajan@mentor.com> ---
> > > > > > >  meta/classes/image.bbclass  |  2 +-
> > > > > > >  meta/classes/rootfs.bbclass | 46
> > > > > > > +++++++++++++++++++++++++++++++++++++ 2 files changed, 47
> > > > > > > insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/meta/classes/image.bbclass
> > > > > > > b/meta/classes/image.bbclass index 9fa58f8..1c7a527 100644
> > > > > > > --- a/meta/classes/image.bbclass
> > > > > > > +++ b/meta/classes/image.bbclass
> > > > > > > @@ -60,7 +60,7 @@ image_do_mounts() {
> > > > > > >  }
> > > > > > >
> > > > > > >  ROOTFSDIR = "${IMAGE_ROOTFS}"
> > > > > > > -ROOTFS_FEATURES += "copy-package-cache
> > > > > > > clean-package-cache generate-manifest" +ROOTFS_FEATURES
> > > > > > > += "copy-package-cache clean-package-cache
> > > > > > > generate-manifest cache-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 8bb003d..7bfdfc9
> > > > > > > 100644 --- a/meta/classes/rootfs.bbclass +++
> > > > > > > b/meta/classes/rootfs.bbclass @@ -201,6 +201,52 @@
> > > > > > > rootfs_generate_manifest () {
> > > > > > > ${ROOTFS_MANIFEST_DEPLOY_DIR}/"${PF}".manifest }
> > > > > > >
> > > > > > > +ROOTFS_POSTPROCESS_COMMAND +=
> > > > > > > "${@bb.utils.contains('ROOTFS_FEATURES', 'cache-deb-src',
> > > > > > > 'cache_deb_src', '', d)}" +cache_deb_src() {
> > > > > > > +    if [ "${ISAR_USE_CACHED_BASE_REPO}" = "1" ]; then
> > > > > > > +        return 0
> > > > > > > +    fi  
> > > > > >
> > > > > > Should the source packages not all end up in the cache, so
> > > > > > they can and probably should be fetched from there.  
> > > > >
> > > > > Sorry. But I am not able to understand this. Can you please
> > > > > explain it again?  
> > > >
> > > > A first build without the cache will fetch all sources and drop
> > > > them into "${DEBSRCDIR}"/"${DISTRO}", just the the apt://
> > > > fetcher does. A second build with an enabled cache will place
> > > > all those src-pkgs in base-apt (see populate_base_apt
> > > > repo_add_srcpackage loop).
> > > >
> > > > So a second run of this function here should be able to fetch
> > > > all those srcs-pkgs from base-apt. And it would be a good idea
> > > > to actually do that to prove that everything is available
> > > > offline.
> > > >
> > > > Note that for real offline BB_NO_NETWORK would be required. And
> > > > that "guard" should still be able to download from base-apt.
> > > > Thinking about it again ... i think you do not need the guard.
> > > > If all src-pkgs are available offline this function will never
> > > > access the internet, if it still tries the invalid proxy
> > > > "guard" from isar_export_proxies will trigger.
> > > >
> > > > I think it boils down to removing the
> > > > [ "${ISAR_USE_CACHED_BASE_REPO}" = "1" ] && exit 0
> > > > and passing the ci offline/cache test
> > > >  
> > > > > >  
> > > > > > > +    mkdir -p "${DEBSRCDIR}"/"${DISTRO}"
> > > > > > > +
> > > > > > > +    sudo -s <<'EOSUDO'
> > > > > > > +    cp -L /etc/resolv.conf '${ROOTFSDIR}/etc'
> > > > > > > +    mkdir -p '${ROOTFSDIR}/deb-src'
> > > > > > > +    mountpoint -q '${ROOTFSDIR}/deb-src' || \
> > > > > > > +    mount --bind '${DEBSRCDIR}' '${ROOTFSDIR}/deb-src'
> > > > > > > +EOSUDO
> > > > > > > +
> > > > > > > +    sudo -E chroot ${ROOTFSDIR} /usr/bin/apt-get update  
> > > >
> > > > Why is that in here? Doing this in the image is not allowed,
> > > > only for isar-apt!  
> > >
> > > Hi Henning,
> > >
> > > I am sorry. But why is it not allowed? Am I missing any side
> > > effects of this call?  
> >
> > Thanks for asking, please keep doing that when things are unclear.  
> 
> Sure. Definitely.
> 
> >
> > An "update" stores a copy of the "view on the repo world" in the
> > image. It is essentially a copy of the Packages.gz or Sources.gz of
> > all repos. That information changes over time on the servers, while
> > they still (hopefully) offer to download packages referenced in
> > older version of those indexes.
> >
> > Isar relies on that. It fetches all indexes exactly once and later
> > downloads packages found in the cached versions. Once you update an
> > index the "view of the world" moves away from "the state of the
> > image".
> >
> > On a living debian system you would always upgrade packages after
> > update-ing the indexes. In an "installer" - like Isar - you
> > probably do not want those dynamics.
> >
> > So in order to keep "the state of the image" and "the view of the
> > world" in sync we never "apt-get update" ... except for isar-apt
> > which is a repo we can/do control.
> >
> > If a build takes a really long time, there is a slim chance that we
> > can not actually fetch packages found in our old indexes because
> > upstream does not provide them anymore. I have not seen real
> > evidence of that potential problem. It could however manifest if we
> > have a long running build ... arm without cross ... and do
> > additional fetches in postinst ... like you are implementing.
> > But whatever you can not fetch in the end, is probably not worth
> > fetching because it is not what was used to construct your image.  
> 
> Thank you for the explanation Henning. It would be good if this
> information is documented somewhere. ;)

Excellent idea! How about you rewrite it in your own words and send a
patch. I will review that. Why you and your own words ... well you will
be the first test audience to make sure its good docs.

I mean that partly as a joke and partly serious, patches are really
welcome.

Henning

> Best,
> Vijai Kumar K
> 
> 
> >
> > Henning
> >  
> > > Thanks,
> > > Vijai Kumar K
> > >  
> > > >  
> > > > > > > +    find "${DEBDIR}"/"${DISTRO}" -name '*\.deb' | while
> > > > > > > read package; do  
> > > >
> > > > You are reading this without grabbing the lock. In multiconfig
> > > > other images might be filling that directory as you read it.
> > > > And you might be calling dpkg-deb on half copied files.
> > > >
> > > > Try deb_dl_dir_import and looping over /var/cache/apt/archives/
> > > > ... in which case you will find yourself dealing with isar-apt
> > > > packages that you need to skip.
> > > > In fact you should use the manifest as input to not download
> > > > packages installed in other images with the same distro but
> > > > without the feature.
> > > >
> > > > Yeahh multiconfig!
> > > >
> > > > 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.
> > > > > > > +        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
> > > > > > > +
> > > > > > > +        sudo -E chroot --userspec=$( id -u ):$( id -g )
> > > > > > > ${ROOTFSDIR} \
> > > > > > > +            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 "${DISTRO}" "${src}"
> > > > > > > "${version}"
> > > > > > > +    done  
> > > > > >
> > > > > > Looks like we are going online without proxy configuration
> > > > > > here. It also needs a BB_NO_NETWORK guard.  
> > > > >
> > > > > Will take care of that.
> > > > >  
> > > > > >
> > > > > > And i would suggest to generate the list of things you want
> > > > > > to fetch, factor out the fetcher from dpkg-base and reuse is
> > > > > > instead of copying it.  
> > > > >
> > > > > Sure. I will have a look into how I can reuse that part.
> > > > >  
> > > > > >
> > > > > > And i would personally like a new series of patches to be
> > > > > > sent without "in-reply-to". Maybe its my client but i find
> > > > > > these deeply nested threads very hard to follow.  
> > > > >
> > > > > No Problem. Will send the next series separately.
> > > > >
> > > > > Thanks,
> > > > > Vijai Kumar K
> > > > >  
> > > > > >
> > > > > > Henning
> > > > > >  
> > > > > > > +    sudo -s <<'EOSUDO'
> > > > > > > +    mountpoint -q '${ROOTFSDIR}/deb-src' && \
> > > > > > > +    umount -l ${ROOTFSDIR}/deb-src
> > > > > > > +    rm -rf '${ROOTFSDIR}/etc/resolv.conf'
> > > > > > > +EOSUDO
> > > > > > > +}
> > > > > > > +
> > > > > > >  do_rootfs_postprocess[vardeps] =
> > > > > > > "${ROOTFS_POSTPROCESS_COMMAND}" python
> > > > > > > do_rootfs_postprocess() { # Take care that its correctly
> > > > > > > mounted:  
> > > > > >  
> > > >  
> >