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

[Stefan Agner]

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