[PATCH 0/2] bitbake: fetch2: Add sanity check for SHA validity of tag and adjust SHA define order

[PATCH 0/2] bitbake: fetch2: Add sanity check for SHA validity of tag and adjust SHA define order

[Zhenhua Luo]

* Add the sanity check for SHA valididy when tag is defined in SRC_URI,
the check is useful for rebased git tree in which the referred commit is not valid
in branch and is saved in tag.

* To allign with above change, the priority of revision needs to be adjusted as
following, otherwise SHA defined by SRCREV will be overriden by rev/tag defined
by SRC_URI.
  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

Zhenhua Luo (2):
  bitbake: fetch2/git: Add sanity check for SHA validity of tag
  bitbake: fetch2: adjust the priority of revision definition

 lib/bb/fetch2/__init__.py | 19 ++++++++++---------
 lib/bb/fetch2/git.py      | 29 +++++++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 11 deletions(-)

-- 
1.8.4.2

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

[Zhenhua Luo]

The change add the sanity check for SHA valididy when tag is defined in SRC_URI,
the check is useful for rebased git tree in which the referred commit is not valid
in branch and is saved in 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

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

[Martin Jansa]

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

On Tue, Dec 24, 2013 at 04:06:32PM +0800, Zhenhua Luo wrote:
> The change add the sanity check for SHA valididy when tag is defined in SRC_URI,
> the check is useful for rebased git tree in which the referred commit is not valid
> in branch and is saved in tag.

I've tested this patch with corner case reported in:
http://lists.openembedded.org/pipermail/openembedded-core/2013-December/087486.html

and now I can "fix" yajl recipe fetch just by adding "tag=1.0.12" into the
SRC_URI.

But it's still a bit confusing for SRCREVs which are accessible from
some tag, but aren't corresponding to that tag directly, people will
assume that tag=foo in SRC_URI is really what will be used, but instead
some older SRCREV can be used.

Maybe we need 3rd option to prevent default "master" branch and handle
SRCREVs not included in any branch and not matching any tag differently?

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.

> 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

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

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

[zhenhua.luo]

> -----Original Message-----
> From: bitbake-devel-bounces@lists.openembedded.org [mailto:bitbake-devel-
> bounces@lists.openembedded.org] On Behalf Of Martin Jansa
> Sent: Friday, January 03, 2014 9:44 PM
> 
> On Tue, Dec 24, 2013 at 04:06:32PM +0800, Zhenhua Luo wrote:
> > The change add the sanity check for SHA valididy when tag is defined
> > in SRC_URI, the check is useful for rebased git tree in which the
> > referred commit is not valid in branch and is saved in tag.
> 
> I've tested this patch with corner case reported in:
> http://lists.openembedded.org/pipermail/openembedded-core/2013-
> December/087486.html
> 
> and now I can "fix" yajl recipe fetch just by adding "tag=1.0.12" into
> the SRC_URI.
> 
> But it's still a bit confusing for SRCREVs which are accessible from some
> tag, but aren't corresponding to that tag directly, people will assume
> that tag=foo in SRC_URI is really what will be used, but instead some
> older SRCREV can be used.
> 
> Maybe we need 3rd option to prevent default "master" branch and handle
> SRCREVs not included in any branch and not matching any tag differently?
[Luo Zhenhua-B19537] Do you mean adding a new option to skip the validity check of SHA?

> 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
	* SRCREV is set to an older SHA in the tag, the older commit in the tag will be used
	* SRCREV is not set, commit corresponding to the tag will be used. 
	Does above implementation make sense? Or any other better method? 


Best Regards,

Zhenhua

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

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

[Martin Jansa]

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

On Mon, Jan 06, 2014 at 04:25:00AM +0000, zhenhua.luo@freescale.com wrote:
> > -----Original Message-----
> > From: bitbake-devel-bounces@lists.openembedded.org [mailto:bitbake-devel-
> > bounces@lists.openembedded.org] On Behalf Of Martin Jansa
> > Sent: Friday, January 03, 2014 9:44 PM
> > 
> > On Tue, Dec 24, 2013 at 04:06:32PM +0800, Zhenhua Luo wrote:
> > > The change add the sanity check for SHA valididy when tag is defined
> > > in SRC_URI, the check is useful for rebased git tree in which the
> > > referred commit is not valid in branch and is saved in tag.
> > 
> > I've tested this patch with corner case reported in:
> > http://lists.openembedded.org/pipermail/openembedded-core/2013-
> > December/087486.html
> > 
> > and now I can "fix" yajl recipe fetch just by adding "tag=1.0.12" into
> > the SRC_URI.
> > 
> > But it's still a bit confusing for SRCREVs which are accessible from some
> > tag, but aren't corresponding to that tag directly, people will assume
> > that tag=foo in SRC_URI is really what will be used, but instead some
> > older SRCREV can be used.
> > 
> > Maybe we need 3rd option to prevent default "master" branch and handle
> > SRCREVs not included in any branch and not matching any tag differently?
> [Luo Zhenhua-B19537] Do you mean adding a new option to skip the validity check of SHA?

Yes, I was thinking about something like nobranch parameter which will
explicitly say that selected SRCREV is known to be not included in any
branch or any tag.

In most cases it shouldn't be needed but still such corner cases exist
(e.g. rebased repos).
 
> > 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.

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

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

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

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

[zhenhua.luo]

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/


Best Regards,

Zhenhua

> -----Original Message-----
> From: Martin Jansa [mailto:martin.jansa@gmail.com]
> Sent: Monday, January 06, 2014 5:13 PM
> To: Luo Zhenhua-B19537
> Cc: Guo Chunrong-B40290; bitbake-devel@lists.openembedded.org; Yu
> Zongchun-B40527; Schmitt Richard-B43082; Liu Ting-B28495
> Subject: Re: [bitbake-devel] [PATCH 1/2] bitbake: fetch2/git: Add sanity
> check for SHA validity of tag
> 
> On Mon, Jan 06, 2014 at 04:25:00AM +0000, zhenhua.luo@freescale.com wrote:
> > > -----Original Message-----
> > > From: bitbake-devel-bounces@lists.openembedded.org
> > > [mailto:bitbake-devel- bounces@lists.openembedded.org] On Behalf Of
> > > Martin Jansa
> > > Sent: Friday, January 03, 2014 9:44 PM
> > >
> > > On Tue, Dec 24, 2013 at 04:06:32PM +0800, Zhenhua Luo wrote:
> > > > The change add the sanity check for SHA valididy when tag is
> > > > defined in SRC_URI, the check is useful for rebased git tree in
> > > > which the referred commit is not valid in branch and is saved in
> tag.
> > >
> > > I've tested this patch with corner case reported in:
> > > http://lists.openembedded.org/pipermail/openembedded-core/2013-
> > > December/087486.html
> > >
> > > and now I can "fix" yajl recipe fetch just by adding "tag=1.0.12"
> > > into the SRC_URI.
> > >
> > > But it's still a bit confusing for SRCREVs which are accessible from
> > > some tag, but aren't corresponding to that tag directly, people will
> > > assume that tag=foo in SRC_URI is really what will be used, but
> > > instead some older SRCREV can be used.
> > >
> > > Maybe we need 3rd option to prevent default "master" branch and
> > > handle SRCREVs not included in any branch and not matching any tag
> differently?
> > [Luo Zhenhua-B19537] Do you mean adding a new option to skip the
> validity check of SHA?
> 
> Yes, I was thinking about something like nobranch parameter which will
> explicitly say that selected SRCREV is known to be not included in any
> branch or any tag.
> 
> In most cases it shouldn't be needed but still such corner cases exist
> (e.g. rebased repos).
> 
> > > 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.
> 
> > 	* 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
> 
> > 	* 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

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

[Martin Jansa]

[-- 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 --]

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

[zhenhua.luo]

> -----Original Message-----
> From: Martin Jansa [mailto:martin.jansa@gmail.com]
> Sent: Tuesday, January 07, 2014 10:47 PM
> 
> 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 :).
> 
> > > > 	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.
[Luo Zhenhua-B19537] Thanks for the explanation. 

> > > > 	* 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.
[Luo Zhenhua-B19537] You point is to show warning when SRCREV and tag are set, meanwhile SRCREV is not corresponding to the tag, right?


Best Regards,

Zhenhua

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

[Martin Jansa]

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

On Wed, Jan 08, 2014 at 10:21:53AM +0000, zhenhua.luo@freescale.com wrote:
> > -----Original Message-----
> > From: Martin Jansa [mailto:martin.jansa@gmail.com]
> > Sent: Tuesday, January 07, 2014 10:47 PM
> > 
> > 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 :).
> > 
> > > > > 	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.
> [Luo Zhenhua-B19537] Thanks for the explanation. 
> 
> > > > > 	* 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.
> [Luo Zhenhua-B19537] You point is to show warning when SRCREV and tag are set, meanwhile SRCREV is not corresponding to the tag, right?

Yes

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

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

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

[Richard Purdie]

On Wed, 2014-01-08 at 12:32 +0100, Martin Jansa wrote:
> > [Luo Zhenhua-B19537] You point is to show warning when SRCREV and tag are set, meanwhile SRCREV is not corresponding to the tag, right?
> 
> Yes

We have several different issues around and the moment and I'm worried
some cases are going to get lost. We really need a list of them along
with the planed fix. As I see it we have:

a) conflicting SRCREV and tag/rev url parameters

Plan: bb.error on this case asking the user to say what they want

b) dereferencing of tags when checking existance of commits

Plan: Add ^{} to ls-remote command to allow dereferencing

c) tags may get resolved to incorrect tags in sub directories

Plan: Need to anchor tag/branch search expression


What else are we missing?


Ideally we could do with examples of these in the fetcher test code
too...

Cheers,

Richard

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

[Richard Purdie]

On Wed, 2014-01-08 at 12:43 +0000, Richard Purdie wrote:
> On Wed, 2014-01-08 at 12:32 +0100, Martin Jansa wrote:
> > > [Luo Zhenhua-B19537] You point is to show warning when SRCREV and tag are set, meanwhile SRCREV is not corresponding to the tag, right?
> > 
> > Yes
> 
> We have several different issues around and the moment and I'm worried
> some cases are going to get lost. We really need a list of them along
> with the planed fix. As I see it we have:
> 
> a) conflicting SRCREV and tag/rev url parameters
> 
> Plan: bb.error on this case asking the user to say what they want
> 
> b) dereferencing of tags when checking existance of commits
> 
> Plan: Add ^{} to ls-remote command to allow dereferencing
> 
> c) tags may get resolved to incorrect tags in sub directories
> 
> Plan: Need to anchor tag/branch search expression
> 
> 
> What else are we missing?
> 
> 
> Ideally we could do with examples of these in the fetcher test code
> too...

FWIW I have fixed the above issues with the patchset that I've sent out
(and is in master-next). Are there any remaining issues?

Cheers,

Richard

[PATCH 2/2] bitbake: fetch2: adjust the priority of revision definition

[Zhenhua Luo]

The sanity check for commit validity has been added for branch, to support the
commit included in tag rather than branch, the commit validity check should be
supported for tag as well.

To allign with above change, the priority of revision needs to be adjusted as
following, otherwise SHA defined by SRCREV will be overriden by rev/tag defined
by SRC_URI.

New SHA check order:
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

Signed-off-by: Zhenhua Luo <zhenhua.luo@freescale.com>
---
 lib/bb/fetch2/__init__.py | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 8fdf59c..4c5c2e8 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -862,17 +862,12 @@ def try_mirrors(d, origud, mirrors, check = False):
 def srcrev_internal_helper(ud, d, name):
     """
     Return:
-        a) a source revision if specified
-        b) latest revision if SRCREV="AUTOINC"
-        c) None if not specified
+        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
     """
 
-    if 'rev' in ud.parm:
-        return ud.parm['rev']
-
-    if 'tag' in ud.parm:
-        return ud.parm['tag']
-
     rev = None
     pn = d.getVar("PN", True)
     if name != '':
@@ -883,6 +878,12 @@ def srcrev_internal_helper(ud, d, name):
         rev = d.getVar("SRCREV_pn-%s" % pn, True)
     if not rev:
         rev = d.getVar("SRCREV", True)
+    if not rev:
+        if 'rev' in ud.parm:
+            return ud.parm['rev']
+    if not rev:
+        if 'tag' in ud.parm:
+            return ud.parm['tag']
     if rev == "INVALID":
         var = "SRCREV_pn-%s" % pn
         if name != '':
-- 
1.8.4.2

Re: [PATCH 0/2] bitbake: fetch2: Add sanity check for SHA validity of tag and adjust SHA define order

[zhenhua.luo]

Any comment on the patch to support SHA validate of tag?


Best Regards,

Zhenhua

> -----Original Message-----
> From: Zhenhua Luo [mailto:zhenhua.luo@freescale.com]
> Sent: Tuesday, December 24, 2013 4:07 PM
> To: bitbake-devel@lists.openembedded.org
> Cc: Liu Ting-B28495; Yu Zongchun-B40527; Guo Chunrong-B40290; Luo
> Zhenhua-B19537
> Subject: [PATCH 0/2] bitbake: fetch2: Add sanity check for SHA validity
> of tag and adjust SHA define order
> 
> * Add the sanity check for SHA valididy when tag is defined in SRC_URI,
> the check is useful for rebased git tree in which the referred commit is
> not valid in branch and is saved in tag.
> 
> * To allign with above change, the priority of revision needs to be
> adjusted as following, otherwise SHA defined by SRCREV will be overriden
> by rev/tag defined by SRC_URI.
>   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
> 
> Zhenhua Luo (2):
>   bitbake: fetch2/git: Add sanity check for SHA validity of tag
>   bitbake: fetch2: adjust the priority of revision definition
> 
>  lib/bb/fetch2/__init__.py | 19 ++++++++++---------
>  lib/bb/fetch2/git.py      | 29 +++++++++++++++++++++++++++--
>  2 files changed, 37 insertions(+), 11 deletions(-)
> 
> --
> 1.8.4.2
>