Re: [PATCH] bitbake: fetch/git: Fix usehead for non-default names

Re: [PATCH] bitbake: fetch/git: Fix usehead for non-default names

[Joey Degges]

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

Hello all,

I noticed that you've each had a hand in the usehead feature since it was
introduced. Any thoughts on this patch? Is this the right forum to submit
such a change?

This patch is intended to allow the usehead feature to work for URLs with
non-default names.

Best,
Joey

On Mon, Dec 14, 2020 at 1:43 AM Joey Degges <jdegges@gmail.com> wrote:

> The usehead url parameter for git repositories causes bitbake to use
> whatever commit the repository HEAD is pointing to if the repository
> happens to have the name 'default'. This is the default name so in many
> cases it works just fine, but if a different name is specified with the
> url parameter 'name=newName' then it will fail to parse the recipe with
> an error along the lines of:
>
> ERROR: ExpansionError during parsing /path/to/my/recipe.bb
> Traceback (most recent call last):
>   File "/path/to/poky/bitbake/lib/bb/fetch2/git.py", line 235, in
> Git.urldata_init:
>     >        ud.setup_revisions(d)
>   File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1302, in
> FetchData.setup_revisions:
>              for name in self.names:
>     >            self.revisions[name] = srcrev_internal_helper(self, d,
> name)
>   File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1167, in
> srcrev_internal_helper(name='newName'):
>          if srcrev == "AUTOINC":
>     >        srcrev = ud.method.latest_revision(ud, d, name)
>   File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1562, in
> Git.latest_revision(name='newName'):
>              except KeyError:
>     >            revs[key] = rev = self._latest_revision(ud, d, name)
>                  return rev
>   File "/path/to/poky/bitbake/lib/bb/fetch2/git.py", line 650, in
> Git._latest_revision(name='newName'):
>              raise bb.fetch2.FetchError("Unable to resolve '%s' in
> upstream git repository in git ls-remote output for %s" % \
>     >            (ud.unresolvedrev[name], ud.host+ud.path))
> bb.data_smart.ExpansionError: Failure expanding variable SRCPV, expression
> was ${@bb.fetch2.get_srcrev(d)} which triggered exception FetchError:
> Fetcher failure: Unable to resolve 'master' in upstream git repository in
> git ls-remote output for /path/to/local/git/repo
>
> Let's fix this by setting the unresolved rev of _all_ repository names
> to 'HEAD' when the usehead url parameter is specified.
>
> This change preserves existing behavior that allows usehead to be
> overridden by a valid looking revision if one happens to be specified
> instead of AUTOREV.
>
> Signed-off-by: Joey Degges <jdegges@gmail.com>
> ---
>  lib/bb/fetch2/git.py | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index 490d57fb..df9538a6 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -220,7 +220,12 @@ class Git(FetchMethod):
>              ud.shallow = False
>
>          if ud.usehead:
> -            ud.unresolvedrev['default'] = 'HEAD'
> +            # When usehead is set let's associate 'HEAD' with the
> unresolved
> +            # rev of this repository. This will get resolved into a
> revision
> +            # later. If an actual revision happens to have also been
> provided
> +            # then this setting will be overridden.
> +            for name in ud.names:
> +                ud.unresolvedrev[name] = 'HEAD'
>
>          ud.basecmd = d.getVar("FETCHCMD_git") or "git -c
> core.fsyncobjectfiles=0"
>
> --
> 2.24.3 (Apple Git-128)
>
>

[-- Attachment #2: Type: text/html, Size: 4498 bytes --]

Re: [PATCH] bitbake: fetch/git: Fix usehead for non-default names

[Richard Purdie]

On Mon, 2021-01-04 at 09:12 -0800, Joey Degges wrote:
> I noticed that you've each had a hand in the usehead feature since it
> was introduced. Any thoughts on this patch? Is this the right forum
> to submit such a change?
> 
> This patch is intended to allow the usehead feature to work for URLs
> with non-default names.

The patch looks reasonable to me, I did ask on the mailing list in
reply to it whether you could extend our tests to ensure if didn't
regress again?

Cheers,

Richard

Re: [PATCH] bitbake: fetch/git: Fix usehead for non-default names

[Joey Degges]

[-- Attachment #1.1: Type: text/plain, Size: 341 bytes --]

Hi Richard,

Thanks for the quick reply. I must have missed your response from back in December--still getting used to this forum.

Happy to update the test cases. Please find the new set of patches attached. This message and attachments were sent through the forum website so hopefully everything is properly formatted.

Best,
Joey

[-- Attachment #1.2: Type: text/html, Size: 369 bytes --]

[-- Attachment #2: 0001-bitbake-tests-fetch-Organize-usehead-tests-by-net-re.patch --]
[-- Type: application/octet-stream, Size: 3883 bytes --]

From 7386ada8e3be5fa9e6777fee608fb3123e79be70 Mon Sep 17 00:00:00 2001
From: Joey Degges <jdegges@gmail.com>
Date: Mon, 4 Jan 2021 13:49:41 -0800
Subject: [PATCH 1/4] bitbake: tests/fetch: Organize usehead tests by net
 requirements

Move the local only usehead test to the FetcherLocalTest class so it
will be run when BB_SKIP_NETTESTS=yes since it does not require
network access.

Rename the usehead tests to better match the new organization.

Signed-off-by: Joey Degges <jdegges@gmail.com>
---
 lib/bb/tests/fetch.py | 55 +++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index da17d7f2..1323ac2c 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -654,6 +654,32 @@ class FetcherLocalTest(FetcherTest):
         with self.assertRaises(bb.fetch2.UnpackError):
             self.fetchUnpack(['file://a;subdir=/bin/sh'])
 
+    def test_local_gitfetch_usehead(self):
+        # Create dummy local Git repo
+        src_dir = tempfile.mkdtemp(dir=self.tempdir,
+                                   prefix='gitfetch_localusehead_')
+        src_dir = os.path.abspath(src_dir)
+        bb.process.run("git init", cwd=src_dir)
+        bb.process.run("git commit --allow-empty -m'Dummy commit'",
+                       cwd=src_dir)
+        # Use other branch than master
+        bb.process.run("git checkout -b my-devel", cwd=src_dir)
+        bb.process.run("git commit --allow-empty -m'Dummy commit 2'",
+                       cwd=src_dir)
+        stdout = bb.process.run("git rev-parse HEAD", cwd=src_dir)
+        orig_rev = stdout[0].strip()
+
+        # Fetch and check revision
+        self.d.setVar("SRCREV", "AUTOINC")
+        url = "git://" + src_dir + ";protocol=file;usehead=1"
+        fetcher = bb.fetch.Fetch([url], self.d)
+        fetcher.download()
+        fetcher.unpack(self.unpackdir)
+        stdout = bb.process.run("git rev-parse HEAD",
+                                cwd=os.path.join(self.unpackdir, 'git'))
+        unpack_rev = stdout[0].strip()
+        self.assertEqual(orig_rev, unpack_rev)
+
 class FetcherNoNetworkTest(FetcherTest):
     def setUp(self):
         super().setUp()
@@ -844,34 +870,7 @@ class FetcherNetworkTest(FetcherTest):
         self.assertRaises(bb.fetch.FetchError, self.gitfetcher, url1, url2)
 
     @skipIfNoNetwork()
-    def test_gitfetch_localusehead(self):
-        # Create dummy local Git repo
-        src_dir = tempfile.mkdtemp(dir=self.tempdir,
-                                   prefix='gitfetch_localusehead_')
-        src_dir = os.path.abspath(src_dir)
-        bb.process.run("git init", cwd=src_dir)
-        bb.process.run("git commit --allow-empty -m'Dummy commit'",
-                       cwd=src_dir)
-        # Use other branch than master
-        bb.process.run("git checkout -b my-devel", cwd=src_dir)
-        bb.process.run("git commit --allow-empty -m'Dummy commit 2'",
-                       cwd=src_dir)
-        stdout = bb.process.run("git rev-parse HEAD", cwd=src_dir)
-        orig_rev = stdout[0].strip()
-
-        # Fetch and check revision
-        self.d.setVar("SRCREV", "AUTOINC")
-        url = "git://" + src_dir + ";protocol=file;usehead=1"
-        fetcher = bb.fetch.Fetch([url], self.d)
-        fetcher.download()
-        fetcher.unpack(self.unpackdir)
-        stdout = bb.process.run("git rev-parse HEAD",
-                                cwd=os.path.join(self.unpackdir, 'git'))
-        unpack_rev = stdout[0].strip()
-        self.assertEqual(orig_rev, unpack_rev)
-
-    @skipIfNoNetwork()
-    def test_gitfetch_remoteusehead(self):
+    def test_gitfetch_usehead(self):
         url = "git://git.openembedded.org/bitbake;usehead=1"
         self.assertRaises(bb.fetch.ParameterError, self.gitfetcher, url, url)
 
-- 
2.24.3 (Apple Git-128)


[-- Attachment #3: 0002-bitbake-tests-fetch-Document-behavior-of-test_gitfet.patch --]
[-- Type: application/octet-stream, Size: 1256 bytes --]

From 2ce2ddbd49d1f3070db2c9081609abe9ef29282d Mon Sep 17 00:00:00 2001
From: Joey Degges <jdegges@gmail.com>
Date: Mon, 4 Jan 2021 21:08:42 -0800
Subject: [PATCH 2/4] bitbake: tests/fetch: Document behavior of
 test_gitfetch_usehead

The test `test_gitfetch_usehead' exercises a way to override the usehead
feature by setting SRCREV. It may not be obvious that this is what is
being exercised here so let's add some comments to document the expected
behavior.

Signed-off-by: Joey Degges <jdegges@gmail.com>
---
 lib/bb/tests/fetch.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 1323ac2c..971c613d 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -871,6 +871,10 @@ class FetcherNetworkTest(FetcherTest):
 
     @skipIfNoNetwork()
     def test_gitfetch_usehead(self):
+        # Since self.gitfetcher() sets SRCREV we expect this to override
+        # `usehead=1' and instead fetch the specified SRCREV. See
+        # test_local_gitfetch_usehead() for a positive use of the usehead
+        # feature.
         url = "git://git.openembedded.org/bitbake;usehead=1"
         self.assertRaises(bb.fetch.ParameterError, self.gitfetcher, url, url)
 
-- 
2.24.3 (Apple Git-128)


[-- Attachment #4: 0003-bitbake-tests-fetch-Test-usehead-with-a-non-default-.patch --]
[-- Type: application/octet-stream, Size: 3340 bytes --]

From 92b8754c78a002da11951eb552d840f1c77e4190 Mon Sep 17 00:00:00 2001
From: Joey Degges <jdegges@gmail.com>
Date: Mon, 4 Jan 2021 21:14:55 -0800
Subject: [PATCH 3/4] bitbake: tests/fetch: Test usehead with a non-default
 name

Add tests for fetching a URL with the usehead parameter set and a
non-default name set. We currently expect the local version of this test
to fail since there is a bug in the usehead implementation that breaks
for non-default names.

Signed-off-by: Joey Degges <jdegges@gmail.com>
---
 lib/bb/tests/fetch.py | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 971c613d..64cc9fa7 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -680,6 +680,38 @@ class FetcherLocalTest(FetcherTest):
         unpack_rev = stdout[0].strip()
         self.assertEqual(orig_rev, unpack_rev)
 
+    def test_local_gitfetch_usehead_withname(self):
+        # Create dummy local Git repo
+        src_dir = tempfile.mkdtemp(dir=self.tempdir,
+                                   prefix='gitfetch_localusehead_')
+        src_dir = os.path.abspath(src_dir)
+        bb.process.run("git init", cwd=src_dir)
+        bb.process.run("git commit --allow-empty -m'Dummy commit'",
+                       cwd=src_dir)
+        # Use other branch than master
+        bb.process.run("git checkout -b my-devel", cwd=src_dir)
+        bb.process.run("git commit --allow-empty -m'Dummy commit 2'",
+                       cwd=src_dir)
+        stdout = bb.process.run("git rev-parse HEAD", cwd=src_dir)
+        orig_rev = stdout[0].strip()
+
+        # Fetch and check revision
+        self.d.setVar("SRCREV", "AUTOINC")
+        url = "git://" + src_dir + ";protocol=file;usehead=1;name=newName"
+        try:
+            fetcher = bb.fetch.Fetch([url], self.d)
+        except Exception:
+            # TODO: We currently expect this test to fail. Drop the try and
+            # assert when usehead has been fixed.
+            return
+        self.assertEqual(1, 0)
+        fetcher.download()
+        fetcher.unpack(self.unpackdir)
+        stdout = bb.process.run("git rev-parse HEAD",
+                                cwd=os.path.join(self.unpackdir, 'git'))
+        unpack_rev = stdout[0].strip()
+        self.assertEqual(orig_rev, unpack_rev)
+
 class FetcherNoNetworkTest(FetcherTest):
     def setUp(self):
         super().setUp()
@@ -878,6 +910,15 @@ class FetcherNetworkTest(FetcherTest):
         url = "git://git.openembedded.org/bitbake;usehead=1"
         self.assertRaises(bb.fetch.ParameterError, self.gitfetcher, url, url)
 
+    @skipIfNoNetwork()
+    def test_gitfetch_usehead_withname(self):
+        # Since self.gitfetcher() sets SRCREV we expect this to override
+        # `usehead=1' and instead fetch the specified SRCREV. See
+        # test_local_gitfetch_usehead() for a positive use of the usehead
+        # feature.
+        url = "git://git.openembedded.org/bitbake;usehead=1;name=newName"
+        self.assertRaises(bb.fetch.ParameterError, self.gitfetcher, url, url)
+
     @skipIfNoNetwork()
     def test_gitfetch_finds_local_tarball_for_mirrored_url_when_previous_downloaded_by_the_recipe_url(self):
         recipeurl = "git://git.openembedded.org/bitbake"
-- 
2.24.3 (Apple Git-128)


[-- Attachment #5: 0004-bitbake-fetch-git-Fix-usehead-for-non-default-names.patch --]
[-- Type: application/octet-stream, Size: 4180 bytes --]

From cb4955576818e644746426d0a559ec91fcdaacd2 Mon Sep 17 00:00:00 2001
From: Joey Degges <jdegges@gmail.com>
Date: Mon, 14 Dec 2020 08:30:02 +0000
Subject: [PATCH 4/4] bitbake: fetch/git: Fix usehead for non-default names

The usehead url parameter for git repositories causes bitbake to use
whatever commit the repository HEAD is pointing to if the repository
happens to have the name 'default'. This is the default name so in many
cases it works just fine, but if a different name is specified with the
url parameter 'name=newName' then it will fail to parse the recipe with
an error along the lines of:

ERROR: ExpansionError during parsing /path/to/my/recipe.bb
Traceback (most recent call last):
  File "/path/to/poky/bitbake/lib/bb/fetch2/git.py", line 235, in Git.urldata_init:
    >        ud.setup_revisions(d)
  File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1302, in FetchData.setup_revisions:
             for name in self.names:
    >            self.revisions[name] = srcrev_internal_helper(self, d, name)
  File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1167, in srcrev_internal_helper(name='newName'):
         if srcrev == "AUTOINC":
    >        srcrev = ud.method.latest_revision(ud, d, name)
  File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1562, in Git.latest_revision(name='newName'):
             except KeyError:
    >            revs[key] = rev = self._latest_revision(ud, d, name)
                 return rev
  File "/path/to/poky/bitbake/lib/bb/fetch2/git.py", line 650, in Git._latest_revision(name='newName'):
             raise bb.fetch2.FetchError("Unable to resolve '%s' in upstream git repository in git ls-remote output for %s" % \
    >            (ud.unresolvedrev[name], ud.host+ud.path))
bb.data_smart.ExpansionError: Failure expanding variable SRCPV, expression was ${@bb.fetch2.get_srcrev(d)} which triggered exception FetchError: Fetcher failure: Unable to resolve 'master' in upstream git repository in git ls-remote output for /path/to/local/git/repo

Let's fix this by setting the unresolved rev of _all_ repository names
to 'HEAD' when the usehead url parameter is specified. Update the
currently failing test, test_local_gitfetch_usehead_withname, to now
expect success.

This change preserves existing behavior that allows usehead to be
overridden by a valid looking revision if one happens to be specified
instead of AUTOREV.

Signed-off-by: Joey Degges <jdegges@gmail.com>
---
 lib/bb/fetch2/git.py  | 7 ++++++-
 lib/bb/tests/fetch.py | 8 +-------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 490d57fb..df9538a6 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -220,7 +220,12 @@ class Git(FetchMethod):
             ud.shallow = False
 
         if ud.usehead:
-            ud.unresolvedrev['default'] = 'HEAD'
+            # When usehead is set let's associate 'HEAD' with the unresolved
+            # rev of this repository. This will get resolved into a revision
+            # later. If an actual revision happens to have also been provided
+            # then this setting will be overridden.
+            for name in ud.names:
+                ud.unresolvedrev[name] = 'HEAD'
 
         ud.basecmd = d.getVar("FETCHCMD_git") or "git -c core.fsyncobjectfiles=0"
 
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 64cc9fa7..1452d761 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -698,13 +698,7 @@ class FetcherLocalTest(FetcherTest):
         # Fetch and check revision
         self.d.setVar("SRCREV", "AUTOINC")
         url = "git://" + src_dir + ";protocol=file;usehead=1;name=newName"
-        try:
-            fetcher = bb.fetch.Fetch([url], self.d)
-        except Exception:
-            # TODO: We currently expect this test to fail. Drop the try and
-            # assert when usehead has been fixed.
-            return
-        self.assertEqual(1, 0)
+        fetcher = bb.fetch.Fetch([url], self.d)
         fetcher.download()
         fetcher.unpack(self.unpackdir)
         stdout = bb.process.run("git rev-parse HEAD",
-- 
2.24.3 (Apple Git-128)

[PATCH] bitbake: fetch/git: Fix usehead for non-default names

[jdegges]

The usehead url parameter for git repositories causes bitbake to use
whatever commit the repository HEAD is pointing to if the repository
happens to have the name 'default'. This is the default name so in many
cases it works just fine, but if a different name is specified with the
url parameter 'name=newName' then it will fail to parse the recipe with
an error along the lines of:

ERROR: ExpansionError during parsing /path/to/my/recipe.bb
Traceback (most recent call last):
  File "/path/to/poky/bitbake/lib/bb/fetch2/git.py", line 235, in Git.urldata_init:
    >        ud.setup_revisions(d)
  File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1302, in FetchData.setup_revisions:
             for name in self.names:
    >            self.revisions[name] = srcrev_internal_helper(self, d, name)
  File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1167, in srcrev_internal_helper(name='newName'):
         if srcrev == "AUTOINC":
    >        srcrev = ud.method.latest_revision(ud, d, name)
  File "/path/to/poky/bitbake/lib/bb/fetch2/__init__.py", line 1562, in Git.latest_revision(name='newName'):
             except KeyError:
    >            revs[key] = rev = self._latest_revision(ud, d, name)
                 return rev
  File "/path/to/poky/bitbake/lib/bb/fetch2/git.py", line 650, in Git._latest_revision(name='newName'):
             raise bb.fetch2.FetchError("Unable to resolve '%s' in upstream git repository in git ls-remote output for %s" % \
    >            (ud.unresolvedrev[name], ud.host+ud.path))
bb.data_smart.ExpansionError: Failure expanding variable SRCPV, expression was ${@bb.fetch2.get_srcrev(d)} which triggered exception FetchError: Fetcher failure: Unable to resolve 'master' in upstream git repository in git ls-remote output for /path/to/local/git/repo

Let's fix this by setting the unresolved rev of _all_ repository names
to 'HEAD' when the usehead url parameter is specified.

This change preserves existing behavior that allows usehead to be
overridden by a valid looking revision if one happens to be specified
instead of AUTOREV.

Signed-off-by: Joey Degges <jdegges@gmail.com>
---
 lib/bb/fetch2/git.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 490d57fb..df9538a6 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -220,7 +220,12 @@ class Git(FetchMethod):
             ud.shallow = False
 
         if ud.usehead:
-            ud.unresolvedrev['default'] = 'HEAD'
+            # When usehead is set let's associate 'HEAD' with the unresolved
+            # rev of this repository. This will get resolved into a revision
+            # later. If an actual revision happens to have also been provided
+            # then this setting will be overridden.
+            for name in ud.names:
+                ud.unresolvedrev[name] = 'HEAD'
 
         ud.basecmd = d.getVar("FETCHCMD_git") or "git -c core.fsyncobjectfiles=0"
 
-- 
2.24.3 (Apple Git-128)