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

Fri Jan 25 23:19:16 UTC 2019

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

SRCREV = d00d1a04cc2366d1a5f143b84b9f507f8bd32c44

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.

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

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