[bitbake-devel] [RESEND RFC PATCH] gitsm: Control submodule recursion

Wed Aug 28 15:32:08 UTC 2019

On 8/28/19 9:38 AM, Andrew Jeffery wrote:
> Hi Mark,
> 
> On Wed, 28 Aug 2019, at 23:30, Mark Hatle wrote:
>> On 8/26/19 7:03 PM, Andrew Jeffery wrote:
>>> There exist some use-cases where repositories related by submodules do
>>> not require submodules be recursively initialised despite needing the
>>> first layer of submodules to be fetched. Enable this usecase in gitsm by
>>> implementing a `recurse=X` SRC_URI parameter. The current implementation
>>> of the recurse parameter defaults to enabling exhaustive recursion to
>>> match the current behaviour of gitsm, with the one alternative being no
>>> recursion. Enough rope is provided to allow specific recursion depths to
>>> be implemented if necessary via the same interface. Values currently
>>> unsupported for the recurse parameter raise a ValueError to ensure
>>> forward compatibility.
>>
>> I was thinking about this patch and I'm not sure it's needed.  There are ways to
>> do this without using gitsm.
>>
>> If you don't want recursion, then why are you specifying 'gitsm' at all? 
> 
> As far as I'm aware the git fetcher doesn't handle submodules at all. If I
> have a misunderstanding there and this patch is unnecessary then that's
> excellent, because I hate it :D

Yes, that is the point.  Since it doesn't handle submodules, there it won't
recurse and you are fine.

For example:

SRC_URI =
"gitsm://github.com/bus1/dbus-broker;protocol=git;rev=fc874afa0992d0c75ec25acb43d344679f0ee7d2"

This will result in the following submodules:

> Submodule 'edgelet/hsm-sys/azure-iot-hsm-c/deps/azure-c-shared-utility' (https://github.com/Azure/azure-c-shared-utility.git) registered for path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared'
> Submodule 'edgelet/hsm-sys/azure-iot-hsm-c/deps/azure-utpm-c' (https://github.com/Azure/azure-utpm-c.git) registered for path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm'
> Submodule 'testtools/ctest' (https://github.com/Azure/azure-ctest.git) registered for path 'testtools/ctest'
> Submodule 'testtools/testrunner' (https://github.com/Azure/azure-c-testrunnerswitcher.git) registered for path 'testtools/testrunner'
> Submodule 'testtools/umock-c' (https://github.com/Azure/umock-c.git) registered for path 'testtools/umock-c'
> Submodule 'deps/azure-ctest' (https://github.com/Azure/azure-ctest.git) registered for path 'deps/ctest'
> Submodule 'deps/testrunner' (https://github.com/Azure/azure-c-testrunnerswitcher.git) registered for path 'deps/testrunner'
> Submodule 'deps/c-utility' (https://github.com/Azure/azure-c-shared-utility.git) registered for path 'deps/c-utility'
> Submodule 'testtools/ctest' (https://github.com/Azure/azure-ctest.git) registered for path 'testtools/ctest'
> Submodule 'testtools/testrunner' (https://github.com/Azure/azure-c-testrunnerswitcher.git) registered for path 'testtools/testrunner'
> Submodule 'testtools/umock-c' (https://github.com/Azure/umock-c.git) registered for path 'testtools/umock-c'
> Submodule 'deps/azure-ctest' (https://github.com/Azure/azure-ctest.git) registered for path 'deps/ctest'
> Submodule 'deps/testrunner' (https://github.com/Azure/azure-c-testrunnerswitcher.git) registered for path 'deps/testrunner'

With the following commits:

> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared': checked out '4deb950a4154e9baa39c87d75dd323dd58e239b7'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared/testtools/ctest': checked out 'b1d09aff8212f1fb62ca9d6014cb3509d8b5559a'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared/testtools/testrunner': checked out '721daafd8c34616f77a7697210470c0227dca20b'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared/testtools/umock-c': checked out 'c2c0f18d785ba85fef213302266f520c64201bb1'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared/testtools/umock-c/deps/ctest': checked out 'b1d09aff8212f1fb62ca9d6014cb3509d8b5559a'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared/testtools/umock-c/deps/testrunner': checked out '721daafd8c34616f77a7697210470c0227dca20b'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm': checked out 'a82e758bfcea806167e19d9dd94c2e1cc223aaee'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm/deps/c-utility': checked out '4deb950a4154e9baa39c87d75dd323dd58e239b7'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm/deps/c-utility/testtools/ctest': checked out 'b1d09aff8212f1fb62ca9d6014cb3509d8b5559a'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm/deps/c-utility/testtools/testrunner': checked out '721daafd8c34616f77a7697210470c0227dca20b'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm/deps/c-utility/testtools/umock-c': checked out 'c2c0f18d785ba85fef213302266f520c64201bb1'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm/deps/c-utility/testtools/umock-c/deps/ctest': checked out 'b1d09aff8212f1fb62ca9d6014cb3509d8b5559a'
> Submodule path 'edgelet/hsm-sys/azure-iot-hsm-c/deps/utpm/deps/c-utility/testtools/umock-c/deps/testrunner': checked out '721daafd8c34616f77a7697210470c0227dca20b'

If we clone this WITHOUT gitsm, it would be done as, and only want a subset, we
could do:

> 
> SRC_URI = "git://github.com/bus1/dbus-broker;protocol=git;rev=fc874afa0992d0c75ec25acb43d344679f0ee7d2 \
>            git://github.com/Azure/azure-c-shared-utility.git;protocol=git;subpath=edgelet/hsm-sys/azure-iot-hsm-c/deps/c-shared;rev=4deb950a4154e9baa39c87d75dd323dd58e239b7 \
>            "

>> The
>> 'git' fetcher won't recurse and can be used to pull down a subset of what would
>> have been a recursive fetch.  The 'subpath' parameter can even be used to
>> combine the items in the same way as the gitsm fetcher if really needed.
> 
> Can you expand a bit more on how I'd do this (examples?), or point me in
> the right direction with documentation? There don't appear to be any
> pointers in the git:// fetcher section of the mega manual.
> 
> Just for clarity, the repository structure this patch is trying to cater for
> looks like:
> 
>       +---->C
>       |
>       +---->D
>       +
> A+--->B+--->E
>       +
>       +---->F
>       |
>       +---->G
> 
> We have a recipe for A, whose repository has a submodule B, and B has
> several submodules C, D, E, F, G. Repositories C-G are large and actually
> irrelevant for the purpose of what we're doing in A, but the content of B
> is required for A to build.
> 
> I hate the patch because the scenario seems like such a corner-case,
> but here we are with a real-world use-case.
> 
>>
>> (A few more comments below, as more of a technical review)
>>
>>> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
>>> ---
>>>  lib/bb/fetch2/gitsm.py | 34 ++++++++++++++++++++++++++++------
>>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/bb/fetch2/gitsm.py b/lib/bb/fetch2/gitsm.py
>>> index c622771d21aa..5a8426b486e3 100644
>>> --- a/lib/bb/fetch2/gitsm.py
>>> +++ b/lib/bb/fetch2/gitsm.py
>>> @@ -6,7 +6,15 @@ after cloning.
>>>  
>>>  SRC_URI = "gitsm://<see Git fetcher for syntax>"
>>>  
>>> -See the Git fetcher, git://, for usage documentation.
>>> +Supported SRC_URI options are:
>>> +
>>> +- recurse
>>> +    Control recursive nature of submodule initialisation.
>>> +
>>> +    The default value is -1, which will recursively initialise all submodules.
>>> +    Set to 0 to disable recursiion.
>>> +
>>> +See the Git fetcher, git://, for further usage documentation.
>>>  
>>>  NOTE: Switching a SRC_URI from "git://" to "gitsm://" requires a clean of your recipe.
>>>  
>>> @@ -26,6 +34,15 @@ from   bb.fetch2 import logger
>>>  from   bb.fetch2 import Fetch
>>>  from   bb.fetch2 import BBFetchException
>>>  
>>> +def should_recurse(ud):
>>> +    recurse = ud.parm.get("recurse", "-1")
>>> +
>>> +    # Change this condition if we end up supporting specific maximum depths
>>> +    if recurse in ( "-1", "0" ):
>>> +        return recurse == "-1"
>>> +
>>> +    raise ValueError("Recursion control value not supported: %s" % (recurse))
>>> +
>>>  class GitSM(Git):
>>>      def supports(self, ud, d):
>>>          """
>>> @@ -95,25 +112,28 @@ class GitSM(Git):
>>>          for module in submodules:
>>>              # Translate the module url into a SRC_URI
>>>  
>>> +            recurse = should_recurse(ud)
>>>              if "://" in uris[module]:
>>>                  # Properly formated URL already
>>>                  proto = uris[module].split(':', 1)[0]
>>> -                url = uris[module].replace('%s:' % proto, 'gitsm:', 1)
>>> +                desired = "gitsm:" if recurse else "git:"
>>> +                url = uris[module].replace('%s:' % proto, desired, 1)
>>
>> Above 'desired' is used, below 'scheme' is used.  You should use the same
>> variable for both, and set it before the first 'if'.  To me 'scheme' is a better
>> variable.
> 
> Yep, will address this if we go forward with the approach.
> 
> Thanks for the feedback!
> 
> Andrew
> 