Re: [PATCH] Map git objects to buildchroot

[Alexander Smirnov <asmirnov@ilbers.de>]

Hi,

On 11/21/2017 11:20 AM, Henning Schild wrote:
> 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.

Are you speaking about git folder mount only? Or you are speaking about 
all the possible mounts related to building process?

Alex

> 
>> 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
>