Re: [PATCH] Map git objects to buildchroot

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

Am Mon, 20 Nov 2017 18:54:23 +0300
schrieb Alexander Smirnov <asmirnov@ilbers.de>:

> On 11/20/2017 06:32 PM, Henning Schild wrote:
> > Am Mon, 20 Nov 2017 18:06:41 +0300
> > schrieb Alexander Smirnov <asmirnov@ilbers.de>:
> >   
> >> Hi Christian,
> >>
> >> The patch above has 2 major issues:
> >>
> >> 1. It mounts ${GITDIR} to buildchroot during package building. This
> >> variable is static, so it's the same for each recipe. This means
> >> that if several packages are building in parallel, the folder
> >> ${BUILDCHROOT_DIR}/${GITDIR} could be mounted/unmounted while there
> >> is active I/O operation by another package. To be honest, I don't
> >> know what could happen, but believe in worst case.  
> > 
> > True, that must not happen!
> >   
> >> 2. Using host configuration in buildchroot could lead to some
> >> problems. Lets consider the following example:
> >>   - User override ${DL_DIR} to point it to local cache, let's
> >> say: /home/builder/XXX
> >>   - Isar uses  the following path to build package
> >> YYY: /home/builder/YYY
> >>   - If XXX equals YYY, you will probably experience some problems,
> >> because your source tree will be overriden by incorect mount.
> >>
> >> So I'd see the more generic solution like patch below:
> >>
> >> 8<--
> >>
> >> dpkg-base: Map git objects to buildchroot
> >>
> >> This patch mounts git repository to buildchroot into the respective
> >> folder:
> >>
> >>   /git/$REPO_NAME
> >>
> >> Also it updates git alternative to correctly point to objects in
> >> buildchroot.
> >>
> >> Signed-off-by: Alexander Smirnov <asmirnov@ilbers.de>
> >> ---
> >>   meta/classes/dpkg-base.bbclass | 13 +++++++++++++
> >>   1 file changed, 13 insertions(+)
> >>
> >> diff --git a/meta/classes/dpkg-base.bbclass
> >> b/meta/classes/dpkg-base.bbclass index 35af6d5..147cfce 100644
> >> --- a/meta/classes/dpkg-base.bbclass
> >> +++ b/meta/classes/dpkg-base.bbclass
> >> @@ -19,12 +19,25 @@ dpkg_runbuild() {
> >>   
> >>   # Wrap the function dpkg_runbuild with the bind mount for
> >> buildroot do_build() {
> >> +    if [ -d ${WORKDIR}/git/.git ]; then
> >> +        OBJ_PATH=$(cat
> >> ${WORKDIR}/git/.git/objects/info/alternates)
> >> +        REPO_PATH=$(dirname $OBJ_PATH)
> >> +        REPO_NAME=$(basename $REPO_PATH)
> >> +        sudo mkdir -p ${BUILDCHROOT_DIR}/git/$REPO_NAME
> >> +        sudo mount --bind $REPO_PATH
> >> ${BUILDCHROOT_DIR}/git/$REPO_NAME
> >> +        echo "/git/$REPO_NAME/objects" >
> >> ${WORKDIR}/git/.git/objects/info/alternates
> >> +    fi
> >> +
> >>       mkdir -p ${BUILDROOT}
> >>       sudo mount --bind ${WORKDIR} ${BUILDROOT}
> >>       _do_build_cleanup() {
> >>           ret=$?
> >>           sudo umount ${BUILDROOT} 2>/dev/null || true
> >>           sudo rmdir ${BUILDROOT} 2>/dev/null || true
> >> +        if [ -d ${WORKDIR}/git/.git ]; then
> >> +            sudo umount ${BUILDCHROOT_DIR}/git/$REPO_NAME
> >> +            sudo rm -rf ${BUILDCHROOT_DIR}/git/$REPO_NAME
> >> +        fi
> >>           (exit $ret) || bb_exit_handler
> >>       }
> >>       trap '_do_build_cleanup' EXIT  
> > 
> > I would not do this either. There should be one mount in
> > buildchroot.bb and one umount. Additional umounts could be allowed
> > in exception handlers, in case they can not call out to functions
> > from buildchroot.  
> 
> Sorry, didn't get your idea regarding one mount buildchroot.bb, what
> do you mean?

There should be one mount done from buildchroot.bb before all the
do_builds of package recipes start. And that should get umounted after
the last do_build is done.

> BTW: we always have option to clone git repository to workspace by 
> running in ${WORKDIR}/git:
> 
> $ git repack -a
> 
> in do_build() task. With this, no extra mount/umount logic is needed.

That sounds promising. But i would only do that if git is actually the
only download-tool that works with such "symlinks". If it is not the
whole dl-dir should get mounted, not just the git-subdir.

> We could consider this behavior for source 3.0 packages only by using 
> some flag in recipe.

I think that is something we can just do in any case. A flag would just
complicate things.

Henning

> Alex