[bitbake-devel] [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Stefan Agner]

[adding Patrick, he is often working on the recipe]

On 26.01.2019 00:19, Mark Hatle wrote:
> On 1/25/19 1:53 PM, Mark Hatle wrote:
>> Is the repository you are using public?  If so, give me the URL and commit ID
>> for the main repository and I'll get it added to the test suite.
> 
> I reread this and went back into the prior info from you.  It appears the
> repository you are talking about is:
> 
> gitsm://github.com/advancedtelematic/aktualizr;branch=master

Yes that is the repository.

> 
> SRCREV = d00d1a04cc2366d1a5f143b84b9f507f8bd32c44

And the hash I was using.

> 
> 
> I manually checked this out doing the following:
> 
> git clone git://github.com/advancedtelematic/aktualizr
> git checkout d00d1a04cc2366d1a5f143b84b9f507f8bd32c44
> git submodule init
> git submodule update --recursive
> 
> 
> I then ran the same thing using the gitsm fetcher.  Using:
> 
> 
> gitsm://github.com/advancedtelematic/aktualizr;branch=master;protocol=git;rev=d00d1a04cc2366d1a5f143b84b9f507f8bd32c44
> 
> Comparing the resulting system, they are nearly identical.  All of the checked
> out sources are -exactly- the same.
> 
> There are some minor differences in the .git directory, but none of them affect
> what is actually checked out.
> 
> Output of:
> 
>    diff -urN 'gitsm version' aktualizr -x .git
> 
> is identical
> 
> comparing the git directory is slightly different, but all within the
> expectations.
> 
> So I do not see anything that the gitsm fetcher is doing incorrectly
> at this point.
> 
> You should run the same type of comparison and see if you are getting the same
> results.
> 
> 
> BTW I went back to bitbake commit c1fcc46e2498, and repeated the experiment.
> 
> This shows that a significant part of was checked out incorrectly.
> 
> For instance, the submodule partial/extern/isotp-c:
> 
> In the current version is checked out on
> 4c5904bff3ed83ec471c8586b5faed62898d0224.  (Which matches the manual
> git behavior.)
> 
> While the gitsm fetcher in the older system checkedout commit
> 614fe48fe55aeea98b49804b7e31ef2d304f62de.  Which is incorrect.
> 

Yeah this would explain the behavior I am seeing.

> 
> So it seems to be that something in the recipe was expecting semi-broken
> behavior.. and its actually a recipe problem.

Agreed, in this case we need to fix the recipe.

Thanks for looking into it and sorry for the noise!

--
Stefan

> 
> --Mark
> 
>> (For testing what I've been doing is checking out with raw git and then with the
>> gitsm fetcher.. comparing the difference and then adding those specific things
>> as regressions checks.)
>>
>> If it's not public, then you'll likely need to do that yourself.  Compare the
>> results and try to figure out what the difference is.  Once you have that, it
>> may be more obvious what the problems.  (Comparison should include the .git
>> contents as well..)
>>
>> --Mark
>>
>> On 1/25/19 1:24 PM, Stefan Agner wrote:
>>> On 25.01.2019 16:08, Richard Purdie wrote:
>>>> On Fri, 2019-01-25 at 15:41 +0100, liu.ming50 at gmail.com wrote:
>>>>> From: Ming Liu <liu.ming50 at gmail.com>
>>>>>
>>>>> A following issue was observed with gitsm:
>>>>>> git -c core.fsyncobjectfiles=0 config submodule... failed with exit code 255, output:
>>>>>> error: could not lock config file config: File exists
>>>>>
>>>>> Since all git submodules share a config file and Git creates a lock
>>>>> file (.git/config.lock) to synchronize access to it, but Git only tries
>>>>> exactly once and returns a error if it's already locked. This leads to
>>>>> a race condition if there are many 'git config submodule' run in
>>>>> different processes.
>>>>>
>>>>> It could be fixed by introducing a bitbake file lock to protect config
>>>>> file from concurrent update from submodules.
>>>>>
>>>>> Reported-by: Stefan Agner <stefan.agner at toradex.com>
>>>>> Signed-off-by: Ming Liu <liu.ming50 at gmail.com>
>>>>> ---
>>>>>  lib/bb/fetch2/gitsm.py | 16 ++++++++++++----
>>>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/lib/bb/fetch2/gitsm.py b/lib/bb/fetch2/gitsm.py
>>>>> index f45546b..9465acb 100644
>>>>> --- a/lib/bb/fetch2/gitsm.py
>>>>> +++ b/lib/bb/fetch2/gitsm.py
>>>>> @@ -183,11 +183,19 @@ class GitSM(Git):
>>>>>
>>>>>              local_path = newfetch.localpath(url)
>>>>>
>>>>> -            # Correct the submodule references to the local download version...
>>>>> -            runfetchcmd("%(basecmd)s config submodule.%(module)s.url %(url)s" % {'basecmd': ud.basecmd, 'module': module, 'url' : local_path}, d, workdir=ud.destdir)
>>>>> +            # Protects the config file from concurrent updates by submodules
>>>>> +            # e.g. 'git config submodule' run in different processes may lead
>>>>> +            # to a race condition.
>>>>> +            lf = bb.utils.lockfile(os.path.join(ud.destdir, "gitsm-config.lock"))
>>>>>
>>>>> -            if ud.shallow:
>>>>> -                runfetchcmd("%(basecmd)s config submodule.%(module)s.shallow true" % {'basecmd': ud.basecmd, 'module': module}, d, workdir=ud.destdir)
>>>>> +            try:
>>>>> +                # Correct the submodule references to the local download version...
>>>>> +                runfetchcmd("%(basecmd)s config submodule.%(module)s.url %(url)s" % {'basecmd': ud.basecmd, 'module': module, 'url' : local_path}, d, workdir=ud.destdir)
>>>>> +
>>>>> +                if ud.shallow:
>>>>> +                    runfetchcmd("%(basecmd)s config submodule.%(module)s.shallow true" % {'basecmd': ud.basecmd, 'module': module}, d, workdir=ud.destdir)
>>>>> +            finally:
>>>>> +                bb.utils.unlockfile(lf)
>>>>>
>>>>>              # Ensure the submodule repository is NOT set to bare, since we're checking it out...
>>>>>              try:
>>>>
>>>> I still don't see why this is necessary after Mark's recent changes.
>>>> These files are in ud.destdir which in OE terms is WORKDIR. Only one
>>>> fetch happens against WORKDIR at a time? Therefore how is this racing?
>>>
>>> The git config race we only observed before the recent changes by Mark.
>>>
>>> If those changes have the potential to fix this issue, we should test
>>> those first and hold on with this patch.
>>>
>>>>
>>>> Have you seen this problem against the current master code?
>>>
>>> Master as of yesterday (or Wednesday) was not able to fetch the project
>>> (aktualizr).
>>>
>>> Even with the latest 2 fixes I am not able to build the (very same)
>>> recipe anymore, it ends up in an error during do_configure, presumably
>>> due to missing submodules:
>>>
>>>
>>> | -- Configuring done
>>> | CMake Error at partial/CMakeLists.txt:116 (add_library):
>>> |   Cannot find source file:
>>> |
>>>
>>> |     extern/isotp-c/deps/bitfield-c/src/bitfield/8byte.c
>>>
>>> |
>>>
>>> |   Tried extensions .c .C .c++ .cc .cpp .cxx .cu .m .M .mm .h .hh .h++
>>> .hm
>>> |   .hpp .hxx .in .txx
>>> |
>>> |
>>> | CMake Error at partial/CMakeLists.txt:116 (add_library):
>>> |   No SOURCES given to target: isotp
>>> |
>>>
>>> Using c1fcc46e2498 ("bitbake: Fix Deprecated warnings from regexs")
>>> works fine.
>>>
>>> --
>>> Stefan
>>>
>>>
>>