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

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

[liu.ming50]

From: Ming Liu <liu.ming50@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@toradex.com>
Signed-off-by: Ming Liu <liu.ming50@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:
-- 
2.7.4

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Richard Purdie]

On Fri, 2019-01-25 at 15:41 +0100, liu.ming50@gmail.com wrote:
> From: Ming Liu <liu.ming50@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@toradex.com>
> Signed-off-by: Ming Liu <liu.ming50@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?

Have you seen this problem against the current master code?

Cheers,

Richard

Re: [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@gmail.com wrote:
>> From: Ming Liu <liu.ming50@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@toradex.com>
>> Signed-off-by: Ming Liu <liu.ming50@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

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Mark Hatle]

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.

(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@gmail.com wrote:
>>> From: Ming Liu <liu.ming50@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@toradex.com>
>>> Signed-off-by: Ming Liu <liu.ming50@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
> 
> 

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Mark Hatle]

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@gmail.com wrote:
>>>> From: Ming Liu <liu.ming50@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@toradex.com>
>>>> Signed-off-by: Ming Liu <liu.ming50@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
>>
>>
> 

Re: [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@gmail.com wrote:
>>>>> From: Ming Liu <liu.ming50@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@toradex.com>
>>>>> Signed-off-by: Ming Liu <liu.ming50@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
>>>
>>>
>>

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Ming Liu]

[-- Attachment #1: Type: text/plain, Size: 7928 bytes --]

Hi, Mark:

Thanks for the explanation, my understanding was wrong, sorry the noise.

//Ming Liu

Stefan Agner <stefan@agner.ch> 於 2019年1月26日 週六 上午11:15寫道：

> [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@gmail.com wrote:
> >>>>> From: Ming Liu <liu.ming50@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@toradex.com>
> >>>>> Signed-off-by: Ming Liu <liu.ming50@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
> >>>
> >>>
> >>
>

[-- Attachment #2: Type: text/html, Size: 11024 bytes --]

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Patrick Vacek]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Sat, Jan 26, 2019 at 11:15 AM Stefan Agner <stefan@agner.ch> wrote:

> > 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.
>
Sorry, I must be missing something. What might be wrong with the recipe? Is
there more that needs to be specified than a SRCREV and a branch? As far as
I understand git, it keeps track of the submodules, so I don't know what
other information there would be to provide. The reason this particular
case is difficult is because there are recursive submodules. When we build
aktualizr, we have to be careful to specify that. See
https://github.com/advancedtelematic/aktualizr#building:

git submodule update --init --recursive

Thanks,
Patrick

[-- Attachment #2: Type: text/html, Size: 1261 bytes --]

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Ming Liu]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

Hi, Mark:

I could reproduce the error reported by Stefan, on the tip:
...
commit 8c8ecec2a722bc2885e2648d41ac8df07bdf660d
Author: Mark Hatle <mark.hatle@windriver.com>
Date:   Wed Jan 23 10:28:18 2019 -0500

    gitsmy.py: Fix unpack of submodules of submodules
...


seems GitSM does not handle recursive submodules.

in my aktualizr/1.0+gitAUTOINC+d00d1a04cc-7/git/.git/config:
...
[submodule "tests/tuf-test-vectors"]
        url =
/mnt/oe/downloads/git2/github.com.advancedtelematic.tuf-test-vectors.
[submodule "partial/extern/isotp-c"]
        url =
/mnt/oe/downloads/git2/github.com.advancedtelematic.isotp-c.git
[submodule "third_party/HdrHistogram_c"]
        url = /mnt/oe/downloads/git2/github.com.HdrHistogram.HdrHistogram_c
[submodule "third_party/googletest"]
        url = /mnt/oe/downloads/git2/github.com.google.googletest.git
...

in which, /mnt/oe/downloads/git2/github.com.advancedtelematic.isotp-c.git
has its own submodule,

aktualizr/1.0+gitAUTOINC+d00d1a04cc-7/git/partial/extern/isotp-c/.gitmodules
...
[submodule "deps/bitfield-c"]
        path = deps/bitfield-c
        url = https://github.com/advancedtelematic/bitfield-c.git
...

but that recursive submodule is fetched but not unpacked.

the best,
thank you

Patrick Vacek <patrickvacek@gmail.com> 於 2019年1月26日 週六 下午1:07寫道：

> On Sat, Jan 26, 2019 at 11:15 AM Stefan Agner <stefan@agner.ch> wrote:
>
>> > 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.
>>
> Sorry, I must be missing something. What might be wrong with the recipe?
> Is there more that needs to be specified than a SRCREV and a branch? As far
> as I understand git, it keeps track of the submodules, so I don't know what
> other information there would be to provide. The reason this particular
> case is difficult is because there are recursive submodules. When we build
> aktualizr, we have to be careful to specify that. See
> https://github.com/advancedtelematic/aktualizr#building:
>
> git submodule update --init --recursive
>
> Thanks,
> Patrick
>

[-- Attachment #2: Type: text/html, Size: 3736 bytes --]

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Richard Purdie]

On Sat, 2019-01-26 at 13:07 +0100, Patrick Vacek wrote:
> On Sat, Jan 26, 2019 at 11:15 AM Stefan Agner <stefan@agner.ch>
> wrote:
> > > 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.
> 
> Sorry, I must be missing something. What might be wrong with the
> recipe? Is there more that needs to be specified than a SRCREV and a
> branch? As far as I understand git, it keeps track of the submodules,
> so I don't know what other information there would be to provide. The
> reason this particular case is difficult is because there are
> recursive submodules. When we build aktualizr, we have to be careful
> to specify that. See 
> https://github.com/advancedtelematic/aktualizr#building:
> 
> git submodule update --init --recursive

The problem is the checkout from the old version of the fetcher was
broken, it incorrectly checked out that SRCREV. The recipe was written
to build that broken checkout.

We've fixed the fetcher and Mark is saying that it now checks out the
correct thing (as compared with a git checkout outside of the build
system, independent of the fetcher).

The recipe assumed the old broken code so now fails to build. The
recipe therefore needs to change to adapt to the now correct source
code.

Cheers,

Richard

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Stefan Agner]

On 26.01.2019 14:35, Richard Purdie wrote:
> On Sat, 2019-01-26 at 13:07 +0100, Patrick Vacek wrote:
>> On Sat, Jan 26, 2019 at 11:15 AM Stefan Agner <stefan@agner.ch>
>> wrote:
>> > > 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.
>>
>> Sorry, I must be missing something. What might be wrong with the
>> recipe? Is there more that needs to be specified than a SRCREV and a
>> branch? As far as I understand git, it keeps track of the submodules,
>> so I don't know what other information there would be to provide. The
>> reason this particular case is difficult is because there are
>> recursive submodules. When we build aktualizr, we have to be careful
>> to specify that. See
>> https://github.com/advancedtelematic/aktualizr#building:
>>
>> git submodule update --init --recursive
> 
> The problem is the checkout from the old version of the fetcher was
> broken, it incorrectly checked out that SRCREV. The recipe was written
> to build that broken checkout.
> 
> We've fixed the fetcher and Mark is saying that it now checks out the
> correct thing (as compared with a git checkout outside of the build
> system, independent of the fetcher).
> 
> The recipe assumed the old broken code so now fails to build. The
> recipe therefore needs to change to adapt to the now correct source
> code.

I was not able to reproduce Marks findings.

In particular:

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

That wasn't the case for me, the old code checked out 4c5904bf just
fine.

However, I think Ming Liu's recent findings point out the culprit.

The submodule isotp-c requires a submodule on its own, and the new code
seems not to unpack that.

--
Stefan

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Mark Hatle]

On 1/26/19 7:50 AM, Stefan Agner wrote:
> On 26.01.2019 14:35, Richard Purdie wrote:
>> On Sat, 2019-01-26 at 13:07 +0100, Patrick Vacek wrote:
>>> On Sat, Jan 26, 2019 at 11:15 AM Stefan Agner <stefan@agner.ch>
>>> wrote:
>>>>> 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.
>>>
>>> Sorry, I must be missing something. What might be wrong with the
>>> recipe? Is there more that needs to be specified than a SRCREV and a
>>> branch? As far as I understand git, it keeps track of the submodules,
>>> so I don't know what other information there would be to provide. The
>>> reason this particular case is difficult is because there are
>>> recursive submodules. When we build aktualizr, we have to be careful
>>> to specify that. See
>>> https://github.com/advancedtelematic/aktualizr#building:
>>>
>>> git submodule update --init --recursive
>>
>> The problem is the checkout from the old version of the fetcher was
>> broken, it incorrectly checked out that SRCREV. The recipe was written
>> to build that broken checkout.
>>
>> We've fixed the fetcher and Mark is saying that it now checks out the
>> correct thing (as compared with a git checkout outside of the build
>> system, independent of the fetcher).
>>
>> The recipe assumed the old broken code so now fails to build. The
>> recipe therefore needs to change to adapt to the now correct source
>> code.
> 
> I was not able to reproduce Marks findings.
> 
> In particular:
> 
>> 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.
> 
> That wasn't the case for me, the old code checked out 4c5904bf just
> fine.
> 
> However, I think Ming Liu's recent findings point out the culprit.
> 
> The submodule isotp-c requires a submodule on its own, and the new code
> seems not to unpack that.

i see what you are saying, but for some reason the version of git on my machine
(using the steps I mentioned in the original email) for some reason did not pull
down the recursive git submodule, which would explain why my diff didn't find
it.  (It's pretty clear to me that even within git itself, some submodule
operations have been broken in various versions in the past.. my guess is that
I'm hitting one of those errant behaviors locally.)

If I manually go to the isotp-c directory and run git init ; git submodule
update it -then- downloads it.  (So apparently the --recursive argument isn't in
some versions.  My system has 1.8.3.1 on it BTW.)

Looking at this quickly, I suspect I know what the problem is:

        if not ud.bareclone and self.process_submodules(ud, ud.destdir,
unpack_submodules, d):

if ud.bareclone is set, then python will optimize and not even run the
process_submodules which is what does the unpack.  This is set for recursive
submodule parsing.

I'm getting ready to board a plane in a few hours.  So it'll be a day or so
before I can get back to this.  I think I have a fix, see:

http://git.openembedded.org/bitbake-contrib/commit/?h=mgh/gitsm&id=ce5bb0056e6c510855811a26afb95073daf90f2a

If you see this and can test it... I can formally send it for review if it's
resolved the issues when I get to my destination.

--Mark

Re: [PATCH V2] fetch2/gitsm: fix config file concurrent update race

[Ming Liu]

[-- Attachment #1: Type: text/plain, Size: 4086 bytes --]

Hi, Mark:

I have done a quick test, seems the problem fixed with this patch, the
steps are as follows:
```
bitbake aktualizr -c cleanall
bitbake aktualizr
```
build succeeded.

the best,
thank you


Mark Hatle <mark.hatle@windriver.com> 於 2019年1月27日 週日 上午2:19寫道：

> On 1/26/19 7:50 AM, Stefan Agner wrote:
> > On 26.01.2019 14:35, Richard Purdie wrote:
> >> On Sat, 2019-01-26 at 13:07 +0100, Patrick Vacek wrote:
> >>> On Sat, Jan 26, 2019 at 11:15 AM Stefan Agner <stefan@agner.ch>
> >>> wrote:
> >>>>> 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.
> >>>
> >>> Sorry, I must be missing something. What might be wrong with the
> >>> recipe? Is there more that needs to be specified than a SRCREV and a
> >>> branch? As far as I understand git, it keeps track of the submodules,
> >>> so I don't know what other information there would be to provide. The
> >>> reason this particular case is difficult is because there are
> >>> recursive submodules. When we build aktualizr, we have to be careful
> >>> to specify that. See
> >>> https://github.com/advancedtelematic/aktualizr#building:
> >>>
> >>> git submodule update --init --recursive
> >>
> >> The problem is the checkout from the old version of the fetcher was
> >> broken, it incorrectly checked out that SRCREV. The recipe was written
> >> to build that broken checkout.
> >>
> >> We've fixed the fetcher and Mark is saying that it now checks out the
> >> correct thing (as compared with a git checkout outside of the build
> >> system, independent of the fetcher).
> >>
> >> The recipe assumed the old broken code so now fails to build. The
> >> recipe therefore needs to change to adapt to the now correct source
> >> code.
> >
> > I was not able to reproduce Marks findings.
> >
> > In particular:
> >
> >> 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.
> >
> > That wasn't the case for me, the old code checked out 4c5904bf just
> > fine.
> >
> > However, I think Ming Liu's recent findings point out the culprit.
> >
> > The submodule isotp-c requires a submodule on its own, and the new code
> > seems not to unpack that.
>
> i see what you are saying, but for some reason the version of git on my
> machine
> (using the steps I mentioned in the original email) for some reason did
> not pull
> down the recursive git submodule, which would explain why my diff didn't
> find
> it.  (It's pretty clear to me that even within git itself, some submodule
> operations have been broken in various versions in the past.. my guess is
> that
> I'm hitting one of those errant behaviors locally.)
>
> If I manually go to the isotp-c directory and run git init ; git submodule
> update it -then- downloads it.  (So apparently the --recursive argument
> isn't in
> some versions.  My system has 1.8.3.1 on it BTW.)
>
> Looking at this quickly, I suspect I know what the problem is:
>
>         if not ud.bareclone and self.process_submodules(ud, ud.destdir,
> unpack_submodules, d):
>
> if ud.bareclone is set, then python will optimize and not even run the
> process_submodules which is what does the unpack.  This is set for
> recursive
> submodule parsing.
>
> I'm getting ready to board a plane in a few hours.  So it'll be a day or so
> before I can get back to this.  I think I have a fix, see:
>
>
> http://git.openembedded.org/bitbake-contrib/commit/?h=mgh/gitsm&id=ce5bb0056e6c510855811a26afb95073daf90f2a
>
> If you see this and can test it... I can formally send it for review if
> it's
> resolved the issues when I get to my destination.
>
> --Mark
>

[-- Attachment #2: Type: text/html, Size: 5380 bytes --]