Re: [PATCH 1/2] bitbake: fetch2/git: Add sanity check for SHA validity of tag

[Martin Jansa <martin.jansa@gmail.com>]

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

On Tue, Jan 07, 2014 at 03:29:22AM +0000, zhenhua.luo@freescale.com wrote:
> It is a simple way to add "nobranch" option to skip the SHA validity check. I have posted a patch, please review. 
> 
> http://patches.openembedded.org/patch/64197/

The v2 looks good and it's already merged :).

> > > > Then we can add sanity check that when tag= and SRCREV are both used
> > > > than SRCREV should point to SHA-1 of annotated tag or dereferrenced
> > tag.
> > > [Luo Zhenhua-B19537] I submitted another patch to adjust the revision
> > definition priority(http://patches.openembedded.org/patch/63703/).
> > > 	SHA define priority sequence.
> > > 	a) a source revision if SHA is specified by SRCREV
> > > 	b) a source revision if revision is specified in SRC_URI
> > > 	c) latest revision if SRCREV="AUTOINC"
> > > 	d) None if not specified
> > >
> > > 	When tag is defined in SRC_URI, there are three SHA definition
> > scenarios:
> > > 	* SRCREV is set to SHA corresponding to the tag, commit
> > corresponding
> > > to the tag will be used
> > 
> > This is OK, but you cannot check that it really corresponds and show
> > warning if not, because it could be now allowed variant with older SHA as
> > bellow.

Be aware that for this to work correctly you need to run
"git fetch --tags" or equivalent, because with lightweight tags you can
have repo like this:

SHA-1
A123  <- tag-1.0
B123
C123  <- master HEAD

You're building C123 or tag-1.0 when C123 revision already exists, so
fetcher creates clone including all 3 SHA-1s, it creates tarball with
checkout and puts it on PREMIRROR.

Someone in upstream adds tag-1.1 pointing to B123

SHA-1
A123  <- tag-1.0
B123  <- tag-1.1
C123  <- master HEAD

Someone changes recipe to use:
SRCREV = "B123"
SRC_URI = "git://foo;tag=tag-1.1"

Current fetcher starts by checking if "B123" SHA-1 exists in checkout
in downloads directory (or even downloaded from PREMIRROR) and it
returns yes, so it doesn't try to update it, but then if you try to
check that B123 corresponds with "tag-1.1" it fails, because tag-1.1
doesn't exist yet in current checkout/premirror.

With annotated tags it's not problem because every new tag has new
SHA-1, so fetcher always updates the checkout first when checking for
new tag.

> > > 	* SRCREV is set to an older SHA in the tag, the older commit in the
> > > tag will be used
> > 
> > This one is IMHO a bit confusing, because people can notice
> > SRC_URI=.*;tag=foo
> > 
> > and then they don't notice SRCREV in the recipe (or don't expect it to be
> > older commit in foo tag) and they just assume that tag=foo means the tip
> > of the tag will be used in build.
> > 
> > In most cases such commit is also included in some branch and using just
> > SRC_URI=.*;branch=foo + SRCREV would be less confusing.
> > 
> > So I would show warning in this case.
> > 
> > In very few exceptions (if any) where you really want SRCREV not included
> > in any branch and included in some tag, but not corresponding to that tag
> > you would use SRC_URI=.*;nobranch + SRCREV

I think that with nobranch patch now merged we should show warning when
this case happens, people shouldn't use tag parameter when they don't
want exactly that tag.

> > > 	* SRCREV is not set, commit corresponding to the tag will be used.
> > > 	Does above implementation make sense? Or any other better method?
> > 
> > We're doing something similar
> > https://github.com/openwebos/meta-
> > webos/blob/master/classes/webos_enhanced_submissions.bbclass
> > with the advantage that we can say that all our components which inherit
> > this class have to use annotated tags (with lightweight tags you can use
> > SRCREV corresponding with tag which exists only in remote repository, but
> > isn't in your downloads/premirror version, even when the SRCREV exists
> > already - annotated tag has always new SRCREV so fetcher will always
> > update the repo and we don't need to use git ls-remote to verify that
> > SRCREV is matching the tag.
> > 
> > > > > Signed-off-by: Zhenhua Luo <zhenhua.luo@freescale.com>
> > > > > ---
> > > > >  lib/bb/fetch2/git.py | 29 +++++++++++++++++++++++++++--
> > > > >  1 file changed, 27 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py index
> > > > > bd107db..1c2d5d3 100644
> > > > > --- a/lib/bb/fetch2/git.py
> > > > > +++ b/lib/bb/fetch2/git.py
> > > > > @@ -116,6 +116,15 @@ class Git(FetchMethod):
> > > > >              ud.branches[name] = branch
> > > > >              ud.unresolvedrev[name] = branch
> > > > >
> > > > > +        tags = ud.parm.get("tag", "").split(',')
> > > > > +        if len(tags) != len(ud.names):
> > > > > +            raise bb.fetch2.ParameterError("The number of name
> > > > > + and tag
> > > > parameters is not balanced", ud.url)
> > > > > +        ud.tags = {}
> > > > > +        for name in ud.names:
> > > > > +            tag = tags[ud.names.index(name)]
> > > > > +            ud.tags[name] = tag
> > > > > +            ud.unresolvedrev[name] = tag
> > > > > +
> > > > >          ud.basecmd = data.getVar("FETCHCMD_git", d, True) or "git"
> > > > >
> > > > >          ud.write_tarballs =
> > > > > ((data.getVar("BB_GENERATE_MIRROR_TARBALLS", d, True) or "0") !=
> > > > > "0")
> > > > or ud.rebaseable @@ -218,7 +227,10 @@ class Git(FetchMethod):
> > > > >          os.chdir(ud.clonedir)
> > > > >          for name in ud.names:
> > > > >              if not self._contains_ref(ud, d, name):
> > > > > -                raise bb.fetch2.FetchError("Unable to find
> > revision %s
> > > > in branch %s even from upstream" % (ud.revisions[name],
> > > > ud.branches[name]))
> > > > > +                if ud.tags[name]:
> > > > > +                    raise bb.fetch2.FetchError("Unable to find
> > > > revision %s in tag %s even from upstream" % (ud.revisions[name],
> > > > ud.tags[name]))
> > > > > +                else:
> > > > > +                    raise bb.fetch2.FetchError("Unable to find
> > > > > + revision %s in branch %s even from upstream" %
> > > > > + (ud.revisions[name],
> > > > > + ud.branches[name]))
> > > > >
> > > > >      def build_mirror_data(self, ud, d):
> > > > >          # Generate a mirror tarball if needed @@ -288,6 +300,18
> > > > > @@ class Git(FetchMethod):
> > > > >          return True
> > > > >
> > > > >      def _contains_ref(self, ud, d, name):
> > > > > +        if len(ud.tags[name]) != 0:
> > > > > +            cmd =  "%s tag --contains %s --list %s 2> /dev/null |
> > > > > + wc -
> > > > l" % (
> > > > > +                ud.basecmd, ud.revisions[name], ud.tags[name])
> > > > > +            try:
> > > > > +                output = runfetchcmd(cmd, d, quiet=True)
> > > > > +            except bb.fetch2.FetchError:
> > > > > +                return False
> > > > > +            if len(output.split()) > 1:
> > > > > +                raise bb.fetch2.FetchError("The command '%s' gave
> > > > output with more then 1 line unexpectedly, output: '%s'" % (cmd,
> > > > output))
> > > > > +            else:
> > > > > +                return output.split()[0] != "0"
> > > > > +
> > > > >          cmd =  "%s branch --contains %s --list %s 2> /dev/null |
> > > > > wc -
> > > > l" % (
> > > > >              ud.basecmd, ud.revisions[name], ud.branches[name])
> > > > >          try:
> > > > > @@ -296,7 +320,8 @@ class Git(FetchMethod):
> > > > >              return False
> > > > >          if len(output.split()) > 1:
> > > > >              raise bb.fetch2.FetchError("The command '%s' gave
> > > > > output
> > > > with more then 1 line unexpectedly, output: '%s'" % (cmd, output))
> > > > > -        return output.split()[0] != "0"
> > > > > +        else:
> > > > > +            return output.split()[0] != "0"
> > > > >
> > > > >      def _revision_key(self, ud, d, name):
> > > > >          """
> > > > > --
> > > > > 1.8.4.2
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > bitbake-devel mailing list
> > > > > bitbake-devel@lists.openembedded.org
> > > > > http://lists.openembedded.org/mailman/listinfo/bitbake-devel
> > > >
> > > > --
> > > > Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com
> > 
> > --
> > Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

-- 
Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]