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

[Andrew Jeffery]

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

> 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