Why to make branch info is mandatory for non-master commit with latest bitbake

Why to make branch info is mandatory for non-master commit with latest bitbake

[zhenhua.luo]

Previously the branch name doesn't need to be defined when a non-master branch commit is referred in recipe, this has been changed in latest bitbake. 

Is this an intentional change? May I know the reason of the change if it is intentional?


Best Regards,

Zhenhua

Re: Why to make branch info is mandatory for non-master commit with latest bitbake

[Richard Purdie]

On Mon, 2013-12-23 at 06:41 +0000, zhenhua.luo@freescale.com wrote:
> Previously the branch name doesn't need to be defined when a
> non-master branch commit is referred in recipe, this has been changed
> in latest bitbake. 
> 
> Is this an intentional change? May I know the reason of the change if it is intentional?

It was intentional and was triggered by this change:

http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=f19546e02d3318ee69fd0c34e21aa97b74c987ec

which is a sanity test added to fix certain failure cases where a given
revision wasn't on a specified branch.

The bug was nasty since the fetcher was hitting the network in cases it
shouldn't have been when a branch wasn't specified.

Cheers,

Richard

Re: Why to make branch info is mandatory for non-master commit with latest bitbake

[zhenhua.luo]

Hi Richard, 

> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of Richard Purdie
> Sent: Monday, December 23, 2013 6:43 PM
> 
> On Mon, 2013-12-23 at 06:41 +0000, zhenhua.luo@freescale.com wrote:
> > Previously the branch name doesn't need to be defined when a
> > non-master branch commit is referred in recipe, this has been changed
> > in latest bitbake.
> >
> > Is this an intentional change? May I know the reason of the change if
> it is intentional?
> 
> It was intentional and was triggered by this change:
> 
> http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=f19546e02d3318ee69fd
> 0c34e21aa97b74c987ec
> 
> which is a sanity test added to fix certain failure cases where a given
> revision wasn't on a specified branch.
> 
> The bug was nasty since the fetcher was hitting the network in cases it
> shouldn't have been when a branch wasn't specified.
[Luo Zhenhua-B19537] I see, can bitbake be enhanced to support tag as well? Sometimes a commit might be contained by a tag rather than branch, e.g. tag created for rebased git tree. 


Best Regards,

Zhenhua

Re: Why to make branch info is mandatory for non-master commit with latest bitbake

[Detlev Zundel]

Hi Richard,

sorry to jump in so late, but I just realized that this "small" change
has some impact also on our ELDK recipies, so I would really like to
understand where the change comes from and why we couple a persistent
specification (commit ID) with a transient specification (branch name).
With all my previous git experience, this looks at least somewhat
strange.

> On Mon, 2013-12-23 at 06:41 +0000,
> zhenhua.luo@freescale.com wrote:
>> Previously the branch name doesn't need to be defined when a
>> non-master branch commit is referred in recipe, this has been changed
>> in latest bitbake. 
>> 
>> Is this an intentional change? May I know the reason of the change
>> if it is intentional?
>
> It was intentional and was triggered by this change:
>
> http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=f19546e02d3318ee69fd0c34e21aa97b74c987ec
>
> which is a sanity test added to fix certain failure cases where a given
> revision wasn't on a specified branch.
>
> The bug was nasty since the fetcher was hitting the network in cases it
> shouldn't have been when a branch wasn't specified.

I looked at the provided link but I'm not certain that I understand the
problem nor the fix for it.

As far as I can make out, the "failure mode" was likely a specified
commit ID that was not available in the local downloads.  As a
consequence, the fetcher then hit the network and tried to find this
(locally not existing) version in the upstream repository.  So very
likely, the "failure" was that a network access happened at all,
correct?  So the "failure" is much more a "we don't want network access
at all under certain circumstances".

If this is correct, then I see how the fix prevents this by effectively
limiting the selectable commit IDs to the _existing_ commits in one
branch.  But doesn't that prevent me from specifying a commit-id in
an upstream branch "later" then what I have available locally in that
old branch state?

I.e. should we rather find a way to say "no downloads are allowed at
all" rather than adding this workaround?

Of course we can fix all our recipies, but I'd really like to understand
why we are doing this.

Thanks!
  Detlev

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de

Re: Why to make branch info is mandatory for non-master commit with latest bitbake

[Richard Purdie]

On Fri, 2014-03-14 at 12:50 +0100, Detlev Zundel wrote:
> Hi Richard,
> 
> sorry to jump in so late, but I just realized that this "small" change
> has some impact also on our ELDK recipies, so I would really like to
> understand where the change comes from and why we couple a persistent
> specification (commit ID) with a transient specification (branch name).
> With all my previous git experience, this looks at least somewhat
> strange.
> 
> > On Mon, 2013-12-23 at 06:41 +0000,
> > zhenhua.luo@freescale.com wrote:
> >> Previously the branch name doesn't need to be defined when a
> >> non-master branch commit is referred in recipe, this has been changed
> >> in latest bitbake. 
> >> 
> >> Is this an intentional change? May I know the reason of the change
> >> if it is intentional?
> >
> > It was intentional and was triggered by this change:
> >
> > http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=f19546e02d3318ee69fd0c34e21aa97b74c987ec
> >
> > which is a sanity test added to fix certain failure cases where a given
> > revision wasn't on a specified branch.
> >
> > The bug was nasty since the fetcher was hitting the network in cases it
> > shouldn't have been when a branch wasn't specified.
> 
> I looked at the provided link but I'm not certain that I understand the
> problem nor the fix for it.
> 
> As far as I can make out, the "failure mode" was likely a specified
> commit ID that was not available in the local downloads.  As a
> consequence, the fetcher then hit the network and tried to find this
> (locally not existing) version in the upstream repository.  So very
> likely, the "failure" was that a network access happened at all,
> correct?  So the "failure" is much more a "we don't want network access
> at all under certain circumstances".

Imagine you set your recipe to some invalid SRCREV. It was possible that
the fetcher would notice it was missing and attempt to fetch it from
upstream (so far so good), then silently continue the build even though
it had been unable to find the revision.

I think we can agree that is bad, if it can't find something specified,
it should error. This is why we added the extra checks to ensure that
the revision really does exist after we fetch from upstream.

> If this is correct, then I see how the fix prevents this by effectively
> limiting the selectable commit IDs to the _existing_ commits in one
> branch.  But doesn't that prevent me from specifying a commit-id in
> an upstream branch "later" then what I have available locally in that
> old branch state?
> 
> I.e. should we rather find a way to say "no downloads are allowed at
> all" rather than adding this workaround?

This isn't a workaround, it fixed a very specific and nasty issue as
described above.

Cheers,

Richard

Re: Why to make branch info is mandatory for non-master commit with latest bitbake

[Richard Purdie]

On Mon, 2014-03-17 at 11:53 +0100, Detlev Zundel wrote:
> Hi Richard,
> 
> thanks for your answer.
> 
> > Imagine you set your recipe to some invalid SRCREV. It was possible that
> > the fetcher would notice it was missing and attempt to fetch it from
> > upstream (so far so good), then silently continue the build even though
> > it had been unable to find the revision.
> 
> But isn't that the real problem that there was no error in this case?

Yes, that is the real error. The change you pointed to adds in such a
check and raises an error.

> > I think we can agree that is bad, if it can't find something specified,
> > it should error. This is why we added the extra checks to ensure that
> > the revision really does exist after we fetch from upstream.
> 
> Of course I agree that this is bad, but isn't it possible to fix the
> "original problem", i.e. not erroring out after not finding a SRCREV
> even after fetching from upstream?  It is still unclear to me why we
> need to add another check (and another input data) if we agree on this
> error condition.  But maybe this isn't easy to implement?

This is exactly what the patch you pointed at does.

There were other changes to the git fetcher around this time and perhaps
your real issue is with one of the other changes rather than the one you
linked to?

I suspect you're more concerned that the branch name needs to be
specified correctly, i.e. this change:

http://git.yoctoproject.org/cgit.cgi/poky/commit/bitbake/lib/bb/fetch2/git.py?id=2354250a95eab484459f41f8715ae112295c2174

We have recipes that care quite deeply that the branch structure being
fetched matches what is in the remote tree. This includes branches that
may over time be merged with different names, we needed those updates
but they were not always being detected. The check for the revision
therefore wasn't enough, we needed to ensure it matched the branch/tag
being requested, not just that it was present.

I understand this change is a little disruptive but deterministic builds
are extremely important and we uncovered real issues with this fix.

If you are having a problem adapting a recipe please explain what the
problem is and we'll see if we can help figure out a solution.

Cheers,

Richard