[PATCH 0/9] Always use the url specified in the recipe as a base for the git shallow tarball naming

[PATCH 0/9] Always use the url specified in the recipe as a base for the git shallow tarball naming

[Urs Fässler]

The existing behavior was to use the url where the repository was cloned from.
It happened that the tarball was not found when a mirror rewrite rule was
active.

We now use the url specified in the recipe to name the shallow tarball. It fixes
that the tarball is not found under certain conditions. In addition, the naming
is independent of network/server failure and the mapping of the name is easier
to understand.

Urs Fässler (9):
  fetch2/git: add tests to verify naming of download directories
  fetch2/git: add tests to capture existing behavior wrt. naming of git
    shallow tarball
  fetch2/git: only use relevant checks for shallow tarball unpack
  tests/data: extract LogRecord into own file
  fetch2/git: throw error when no up to date sources were found during
    unpack
  fetch2: declare urldata_init in base class
  fetch2: provide original url in addition to the mirrored url to
    FetchData.__init__ and FetchMethod.urldata_init
  fetch2/git: move generation of git source name into own method
  fetch2/git: name the shallow tarball according to the url specified in
    the recipe rather than the mirrored url

 lib/bb/fetch2/__init__.py  |  17 +++--
 lib/bb/fetch2/bzr.py       |   2 +-
 lib/bb/fetch2/clearcase.py |   2 +-
 lib/bb/fetch2/cvs.py       |   2 +-
 lib/bb/fetch2/git.py       |  56 +++++++++------
 lib/bb/fetch2/gitannex.py  |   4 +-
 lib/bb/fetch2/hg.py        |   2 +-
 lib/bb/fetch2/local.py     |   2 +-
 lib/bb/fetch2/npm.py       |   2 +-
 lib/bb/fetch2/osc.py       |   2 +-
 lib/bb/fetch2/perforce.py  |   2 +-
 lib/bb/fetch2/repo.py      |   2 +-
 lib/bb/fetch2/s3.py        |   2 +-
 lib/bb/fetch2/sftp.py      |   2 +-
 lib/bb/fetch2/ssh.py       |   2 +-
 lib/bb/fetch2/svn.py       |   2 +-
 lib/bb/fetch2/wget.py      |   2 +-
 lib/bb/tests/data.py       |  25 +------
 lib/bb/tests/fetch.py      | 142 +++++++++++++++++++++++++++++++++++++
 lib/bb/tests/logrecord.py  |  51 +++++++++++++
 20 files changed, 258 insertions(+), 65 deletions(-)
 create mode 100644 lib/bb/tests/logrecord.py

-- 
2.18.0

[PATCH 1/9] fetch2/git: add tests to verify naming of download directories

[Urs Fässler]

The mapping of the URLs to the local directory is not obvious. For easier
understanding, we add this tests to explicit showing the mapping.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/tests/fetch.py | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 4af0ac55..5bce1bf8 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -463,6 +463,58 @@ class MirrorUriTest(FetcherTest):
                                 'https://BBBB/B/B/B/bitbake/bitbake-1.0.tar.gz',
                                 'http://AAAA/A/A/A/B/B/bitbake/bitbake-1.0.tar.gz'])
 
+
+class GitDownloadDirectoryNamingTest(FetcherTest):
+    def setUp(self):
+        super(GitDownloadDirectoryNamingTest, self).setUp()
+        self.__recipe_url = "git://git.openembedded.org/bitbake"
+        self.__recipe_dir = "git.openembedded.org.bitbake"
+        self.__mirror_url = "git://github.com/openembedded/bitbake.git"
+        self.__mirror_dir = "github.com.openembedded.bitbake.git"
+
+        self.d.setVar('SRCREV', '82ea737a0b42a8b53e11c9cde141e9e9c0bd8c40')
+        self.d.setVar("PREMIRRORS", self.__recipe_url + " " + self.__mirror_url + " \n")
+
+    @skipIfNoNetwork()
+    def test_that_directory_is_named_after_recipe_url_when_no_mirroring_is_used(self):
+        fetcher = bb.fetch.Fetch([self.__mirror_url], self.d)
+
+        fetcher.download()
+
+        dir = os.listdir(self.dldir + "/git2")
+        self.assertIn(self.__mirror_dir, dir)
+
+    @skipIfNoNetwork()
+    def test_that_directory_exists_for_mirrored_url_and_recipe_url_when_mirroring_is_used(self):
+        fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
+
+        fetcher.download()
+
+        dir = os.listdir(self.dldir + "/git2")
+        self.assertIn(self.__mirror_dir, dir)
+        self.assertIn(self.__recipe_dir, dir)
+
+    @skipIfNoNetwork()
+    def test_that_recipe_directory_is_link_to_mirrored_directory_when_mirroring_is_used(self):
+        fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
+
+        fetcher.download()
+
+        path = os.readlink(self.dldir + "/git2/" + self.__recipe_dir)
+        self.assertTrue(path.endswith("/" + self.__mirror_dir))
+
+    @skipIfNoNetwork()
+    def test_that_recipe_directory_is_link_to_mirrored_directory_when_mirroring_is_used_and_the_mirrored_directory_already_exists(self):
+        fetcher = bb.fetch.Fetch([self.__mirror_url], self.d)
+        fetcher.download()
+        fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
+
+        fetcher.download()
+
+        path = os.readlink(self.dldir + "/git2/" + self.__recipe_dir)
+        self.assertTrue(path.endswith("/" + self.__mirror_dir))
+
+
 class FetcherLocalTest(FetcherTest):
     def setUp(self):
         def touch(fn):
-- 
2.18.0

[PATCH 2/9] fetch2/git: add tests to capture existing behavior wrt. naming of git shallow tarball

[Urs Fässler]

The mapping of the URLs to the local shallow tarballs is not obvious. For
easier understanding, we add this tests to explicit showing the mapping.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/tests/fetch.py | 49 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 5bce1bf8..83cc4f6c 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -515,6 +515,55 @@ class GitDownloadDirectoryNamingTest(FetcherTest):
         self.assertTrue(path.endswith("/" + self.__mirror_dir))
 
 
+class GitShallowTarballNamingTest(FetcherTest):
+    def setUp(self):
+        super(GitShallowTarballNamingTest, self).setUp()
+        self.__recipe_url = "git://git.openembedded.org/bitbake"
+        self.__recipe_tarball = "gitshallow_git.openembedded.org.bitbake_82ea737-1_master.tar.gz"
+        self.__mirror_url = "git://github.com/openembedded/bitbake.git"
+        self.__mirror_tarball = "gitshallow_github.com.openembedded.bitbake.git_82ea737-1_master.tar.gz"
+
+        self.d.setVar('BB_GIT_SHALLOW', '1')
+        self.d.setVar('BB_GENERATE_SHALLOW_TARBALLS', '1')
+        self.d.setVar('SRCREV', '82ea737a0b42a8b53e11c9cde141e9e9c0bd8c40')
+
+    def __setup_mirror_rewrite(self):
+        self.d.setVar("PREMIRRORS", self.__recipe_url + " " + self.__mirror_url + " \n")
+
+    @skipIfNoNetwork()
+    def test_that_the_tarball_is_named_after_recipe_url_when_no_mirroring_is_used(self):
+        fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
+
+        fetcher.download()
+
+        dir = os.listdir(self.dldir)
+        self.assertIn(self.__recipe_tarball, dir)
+
+    @skipIfNoNetwork()
+    def test_that_the_mirror_tarball_is_created_when_mirroring_is_used(self):
+        self.__setup_mirror_rewrite()
+        fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
+
+        fetcher.download()
+
+        dir = os.listdir(self.dldir)
+        self.assertIn(self.__mirror_tarball, dir)
+
+    @skipIfNoNetwork()
+    def test_that_the_mirror_tarball_still_exists_when_mirroring_is_used_and_the_mirrored_tarball_already_exists(self):
+        self.__setup_mirror_rewrite()
+        fetcher = bb.fetch.Fetch([self.__mirror_url], self.d)
+        fetcher.download()
+        bb.utils.prunedir(self.dldir + '/git2')
+        bb.utils.prunedir(self.unpackdir)
+        fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
+
+        fetcher.download()
+
+        dir = os.listdir(self.dldir)
+        self.assertIn(self.__mirror_tarball, dir)
+
+
 class FetcherLocalTest(FetcherTest):
     def setUp(self):
         def touch(fn):
-- 
2.18.0

[PATCH 3/9] fetch2/git: only use relevant checks for shallow tarball unpack

[Urs Fässler]

Some checks in need_update do not make sense in the unpack step. The
relevant checks for the unpack check are extracted into
__has_up_to_date_clonedir which is used in the unpack step.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/git.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 612aac43..3364bbf9 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -299,17 +299,22 @@ class Git(FetchMethod):
         return ud.clonedir
 
     def need_update(self, ud, d):
-        if not os.path.exists(ud.clonedir):
+        if not self.__has_up_to_date_clonedir(ud, d):
             return True
-        for name in ud.names:
-            if not self._contains_ref(ud, d, name, ud.clonedir):
-                return True
         if ud.shallow and ud.write_shallow_tarballs and not os.path.exists(ud.fullshallow):
             return True
         if ud.write_tarballs and not os.path.exists(ud.fullmirror):
             return True
         return False
 
+    def __has_up_to_date_clonedir(self, ud, d):
+        if not os.path.exists(ud.clonedir):
+            return False
+        for name in ud.names:
+            if not self._contains_ref(ud, d, name, ud.clonedir):
+                return False
+        return True
+
     def try_premirror(self, ud, d):
         # If we don't do this, updating an existing checkout with only premirrors
         # is not possible
@@ -472,7 +477,7 @@ class Git(FetchMethod):
         if os.path.exists(destdir):
             bb.utils.prunedir(destdir)
 
-        if ud.shallow and self.need_update(ud, d):
+        if ud.shallow and not self.__has_up_to_date_clonedir(ud, d):
             bb.utils.mkdirhier(destdir)
             runfetchcmd("tar -xzf %s" % ud.fullshallow, d, workdir=destdir)
         else:
-- 
2.18.0

[PATCH 4/9] tests/data: extract LogRecord into own file

[Urs Fässler]

LogRecord is a mechanism to capture the log output. It is used in the
data tests. It is moved into its own file to be used by other tests.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/tests/data.py      | 25 +------------------
 lib/bb/tests/logrecord.py | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 24 deletions(-)
 create mode 100644 lib/bb/tests/logrecord.py

diff --git a/lib/bb/tests/data.py b/lib/bb/tests/data.py
index a4a9dd30..e8121a21 100644
--- a/lib/bb/tests/data.py
+++ b/lib/bb/tests/data.py
@@ -24,30 +24,7 @@ import unittest
 import bb
 import bb.data
 import bb.parse
-import logging
-
-class LogRecord():
-    def __enter__(self):
-        logs = []
-        class LogHandler(logging.Handler):
-            def emit(self, record):
-                logs.append(record)
-        logger = logging.getLogger("BitBake")
-        handler = LogHandler()
-        self.handler = handler
-        logger.addHandler(handler)
-        return logs
-    def __exit__(self, type, value, traceback):
-        logger = logging.getLogger("BitBake")
-        logger.removeHandler(self.handler)
-        return
-
-def logContains(item, logs):
-    for l in logs:
-        m = l.getMessage()
-        if item in m:
-            return True
-    return False
+from bb.tests.logrecord import LogRecord, logContains
 
 class DataExpansions(unittest.TestCase):
     def setUp(self):
diff --git a/lib/bb/tests/logrecord.py b/lib/bb/tests/logrecord.py
new file mode 100644
index 00000000..2693e4ff
--- /dev/null
+++ b/lib/bb/tests/logrecord.py
@@ -0,0 +1,51 @@
+# ex:ts=4:sw=4:sts=4:et
+# -*- tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*-
+#
+# BitBake Tests for the Data Store (data.py/data_smart.py)
+#
+# Copyright (C) 2010 Chris Larson
+# Copyright (C) 2012 Richard Purdie
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+
+import logging
+
+
+class LogRecord():
+    def __enter__(self):
+        logs = []
+
+        class LogHandler(logging.Handler):
+            def emit(self, record):
+                logs.append(record)
+
+        logger = logging.getLogger("BitBake")
+        handler = LogHandler()
+        self.handler = handler
+        logger.addHandler(handler)
+        return logs
+
+    def __exit__(self, type, value, traceback):
+        logger = logging.getLogger("BitBake")
+        logger.removeHandler(self.handler)
+        return
+
+
+def logContains(item, logs):
+    for l in logs:
+        m = l.getMessage()
+        if item in m:
+            return True
+    return False
-- 
2.18.0

[PATCH 5/9] fetch2/git: throw error when no up to date sources were found during unpack

[Urs Fässler]

Check if the fullshallow tarball exists before unpacking it. If no
source to unpack is found, an error is thrown. The readability of the
logic to decide which source should be used is increased.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/git.py  |  6 ++++--
 lib/bb/tests/fetch.py | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 3364bbf9..13b9b9d8 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -477,11 +477,13 @@ class Git(FetchMethod):
         if os.path.exists(destdir):
             bb.utils.prunedir(destdir)
 
-        if ud.shallow and not self.__has_up_to_date_clonedir(ud, d):
+        if self.__has_up_to_date_clonedir(ud, d):
+            runfetchcmd("%s clone %s %s/ %s" % (ud.basecmd, ud.cloneflags, ud.clonedir, destdir), d)
+        elif ud.shallow and os.path.exists(ud.fullshallow):
             bb.utils.mkdirhier(destdir)
             runfetchcmd("tar -xzf %s" % ud.fullshallow, d, workdir=destdir)
         else:
-            runfetchcmd("%s clone %s %s/ %s" % (ud.basecmd, ud.cloneflags, ud.clonedir, destdir), d)
+            bb.fatal("no up to date source found")
 
         repourl = self._get_repo_url(ud)
         runfetchcmd("%s remote set-url origin %s" % (ud.basecmd, repourl), d, workdir=destdir)
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 83cc4f6c..fedc3b73 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -28,6 +28,7 @@ import os
 from bb.fetch2 import URI
 from bb.fetch2 import FetchMethod
 import bb
+from bb.tests.logrecord import LogRecord, logContains
 
 def skipIfNoNetwork():
     if os.environ.get("BB_SKIP_NETTESTS") == "yes":
@@ -1678,3 +1679,29 @@ class GitShallowTest(FetcherTest):
         self.assertNotEqual(orig_revs, revs)
         self.assertRefs(['master', 'origin/master'])
         self.assertRevCount(orig_revs - 1758)
+
+    def test_that_unpack_throws_an_error_when_the_git_clone_nor_shallow_tarball_exist(self):
+        self.add_empty_file('a')
+        fetcher, ud = self.fetch()
+        bb.utils.remove(self.gitdir, recurse=True)
+        bb.utils.remove(self.dldir, recurse=True)
+
+        with LogRecord() as logs:
+            with self.assertRaises(Exception) as context:
+                fetcher.unpack(self.d.getVar('WORKDIR'))
+
+            self.assertTrue(logContains("no up to date source found", logs))
+
+    @skipIfNoNetwork()
+    def test_that_unpack_does_work_when_using_git_shallow_tarball_and_mirror_rewrite_rules(self):
+        self.d.setVar('SRCREV', 'e5939ff608b95cdd4d0ab0e1935781ab9a276ac0')
+        self.d.setVar('BB_GIT_SHALLOW', '1')
+        self.d.setVar('BB_GENERATE_SHALLOW_TARBALLS', '1')
+        self.d.setVar('PREMIRRORS', 'git://git.yoctoproject.org/.* git://git.yoctoproject.org/git/PATH;protocol=https \n')
+
+        fetcher = bb.fetch.Fetch(["git://git.yoctoproject.org/fstests"], self.d)
+        fetcher.download()
+        fetcher.unpack(self.unpackdir)
+
+        dir = os.listdir(self.unpackdir + "/git/")
+        self.assertIn("fstests.doap", dir)
-- 
2.18.0

[PATCH 6/9] fetch2: declare urldata_init in base class

[Urs Fässler]

Declare urldata_init in FetchMethod since it is implemented by every
fetcher anyway. Always call urldata_init in FetchData init.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/__init__.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index a83526a5..c8653d62 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1259,8 +1259,7 @@ class FetchData(object):
             logger.warning('Consider updating %s recipe to use "protocol" not "proto" in SRC_URI.', d.getVar('PN'))
             self.parm["protocol"] = self.parm.get("proto", None)
 
-        if hasattr(self.method, "urldata_init"):
-            self.method.urldata_init(self, d)
+        self.method.urldata_init(self, d)
 
         if "localpath" in self.parm:
             # if user sets localpath for file, use it instead.
@@ -1349,6 +1348,12 @@ class FetchMethod(object):
 
         return True
 
+    def urldata_init(self, ud, d):
+        """
+        init method specific variable within url data
+        """
+        pass
+
     def recommends_checksum(self, urldata):
         """
         Is the backend on where checksumming is recommended (should warnings
-- 
2.18.0

[PATCH 7/9] fetch2: provide original url in addition to the mirrored url to FetchData.__init__ and FetchMethod.urldata_init

[Urs Fässler]

When a mirror rewrite rule is used, it can happen that the downloaded
file is named differently depending on the used mirror. In preparation
to a consistent naming, we provide the original url to the fetcher.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/__init__.py  | 12 +++++++-----
 lib/bb/fetch2/bzr.py       |  2 +-
 lib/bb/fetch2/clearcase.py |  2 +-
 lib/bb/fetch2/cvs.py       |  2 +-
 lib/bb/fetch2/git.py       |  2 +-
 lib/bb/fetch2/gitannex.py  |  4 ++--
 lib/bb/fetch2/hg.py        |  2 +-
 lib/bb/fetch2/local.py     |  2 +-
 lib/bb/fetch2/npm.py       |  2 +-
 lib/bb/fetch2/osc.py       |  2 +-
 lib/bb/fetch2/perforce.py  |  2 +-
 lib/bb/fetch2/repo.py      |  2 +-
 lib/bb/fetch2/s3.py        |  2 +-
 lib/bb/fetch2/sftp.py      |  2 +-
 lib/bb/fetch2/ssh.py       |  2 +-
 lib/bb/fetch2/svn.py       |  2 +-
 lib/bb/fetch2/wget.py      |  2 +-
 17 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index c8653d62..61009e43 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -932,7 +932,7 @@ def build_mirroruris(origud, mirrors, ld):
                 localmirrors.remove(line)
 
                 try:
-                    newud = FetchData(newuri, ld)
+                    newud = FetchData(newuri, ld, origud.url)
                     newud.setup_localpath(ld)
                 except bb.fetch2.BBFetchException as e:
                     logger.debug(1, "Mirror fetch failure for url %s (original url: %s)" % (newuri, origud.url))
@@ -1202,7 +1202,7 @@ class FetchData(object):
     """
     A class which represents the fetcher state for a given URI.
     """
-    def __init__(self, url, d, localonly = False):
+    def __init__(self, url, d, original_url = None, localonly = False):
         # localpath is the location of a downloaded result. If not set, the file is local.
         self.donestamp = None
         self.needdonestamp = True
@@ -1213,6 +1213,8 @@ class FetchData(object):
         self.basename = None
         self.basepath = None
         (self.type, self.host, self.path, self.user, self.pswd, self.parm) = decodeurl(d.expand(url))
+        if not original_url:
+            original_url = url
         self.date = self.getSRCDate(d)
         self.url = url
         if not self.user and "user" in self.parm:
@@ -1259,7 +1261,7 @@ class FetchData(object):
             logger.warning('Consider updating %s recipe to use "protocol" not "proto" in SRC_URI.', d.getVar('PN'))
             self.parm["protocol"] = self.parm.get("proto", None)
 
-        self.method.urldata_init(self, d)
+        self.method.urldata_init(self, d, original_url)
 
         if "localpath" in self.parm:
             # if user sets localpath for file, use it instead.
@@ -1348,7 +1350,7 @@ class FetchMethod(object):
 
         return True
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init method specific variable within url data
         """
@@ -1594,7 +1596,7 @@ class Fetch(object):
         for url in urls:
             if url not in self.ud:
                 try:
-                    self.ud[url] = FetchData(url, d, localonly)
+                    self.ud[url] = FetchData(url, d, localonly = localonly)
                 except NonLocalMethod:
                     if localonly:
                         self.ud[url] = None
diff --git a/lib/bb/fetch2/bzr.py b/lib/bb/fetch2/bzr.py
index 658502f9..4be73320 100644
--- a/lib/bb/fetch2/bzr.py
+++ b/lib/bb/fetch2/bzr.py
@@ -36,7 +36,7 @@ class Bzr(FetchMethod):
     def supports(self, ud, d):
         return ud.type in ['bzr']
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init bzr specific variable within url data
         """
diff --git a/lib/bb/fetch2/clearcase.py b/lib/bb/fetch2/clearcase.py
index 3a6573d0..ef1db138 100644
--- a/lib/bb/fetch2/clearcase.py
+++ b/lib/bb/fetch2/clearcase.py
@@ -84,7 +84,7 @@ class ClearCase(FetchMethod):
     def debug(self, msg):
         logger.debug(1, "ClearCase: %s", msg)
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init ClearCase specific variable within url data
         """
diff --git a/lib/bb/fetch2/cvs.py b/lib/bb/fetch2/cvs.py
index 0e0a3196..7c5296fb 100644
--- a/lib/bb/fetch2/cvs.py
+++ b/lib/bb/fetch2/cvs.py
@@ -42,7 +42,7 @@ class Cvs(FetchMethod):
         """
         return ud.type in ['cvs']
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         if not "module" in ud.parm:
             raise MissingParameterError("module", ud.url)
         ud.module = ud.parm["module"]
diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 13b9b9d8..7f7951f7 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -141,7 +141,7 @@ class Git(FetchMethod):
     def supports_checksum(self, urldata):
         return False
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init git specific variable within url data
         so that the git method like latest_revision() can work
diff --git a/lib/bb/fetch2/gitannex.py b/lib/bb/fetch2/gitannex.py
index a9b69caa..187e6b2e 100644
--- a/lib/bb/fetch2/gitannex.py
+++ b/lib/bb/fetch2/gitannex.py
@@ -33,8 +33,8 @@ class GitANNEX(Git):
         """
         return ud.type in ['gitannex']
 
-    def urldata_init(self, ud, d):
-        super(GitANNEX, self).urldata_init(ud, d)
+    def urldata_init(self, ud, d, original_url):
+        super(GitANNEX, self).urldata_init(ud, d, original_url)
         if ud.shallow:
             ud.shallow_extra_refs += ['refs/heads/git-annex', 'refs/heads/synced/*']
 
diff --git a/lib/bb/fetch2/hg.py b/lib/bb/fetch2/hg.py
index 936d0431..5fe08e80 100644
--- a/lib/bb/fetch2/hg.py
+++ b/lib/bb/fetch2/hg.py
@@ -50,7 +50,7 @@ class Hg(FetchMethod):
         """ 
         return False
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init hg specific variable within url data
         """
diff --git a/lib/bb/fetch2/local.py b/lib/bb/fetch2/local.py
index a114ac12..9e1b918c 100644
--- a/lib/bb/fetch2/local.py
+++ b/lib/bb/fetch2/local.py
@@ -39,7 +39,7 @@ class Local(FetchMethod):
         """
         return urldata.type in ['file']
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         # We don't set localfile as for this fetcher the file is already local!
         ud.decodedurl = urllib.parse.unquote(ud.url.split("://")[1].split(";")[0])
         ud.basename = os.path.basename(ud.decodedurl)
diff --git a/lib/bb/fetch2/npm.py b/lib/bb/fetch2/npm.py
index 408dfc3d..cab792d4 100644
--- a/lib/bb/fetch2/npm.py
+++ b/lib/bb/fetch2/npm.py
@@ -60,7 +60,7 @@ class Npm(FetchMethod):
         bb.utils.remove(ud.pkgdatadir, True)
         bb.utils.remove(ud.fullmirror, False)
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init NPM specific variable within url data
         """
diff --git a/lib/bb/fetch2/osc.py b/lib/bb/fetch2/osc.py
index 6c60456b..18dd6047 100644
--- a/lib/bb/fetch2/osc.py
+++ b/lib/bb/fetch2/osc.py
@@ -25,7 +25,7 @@ class Osc(FetchMethod):
         """
         return ud.type in ['osc']
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         if not "module" in ud.parm:
             raise MissingParameterError('module', ud.url)
 
diff --git a/lib/bb/fetch2/perforce.py b/lib/bb/fetch2/perforce.py
index 903a8e61..3a15754a 100644
--- a/lib/bb/fetch2/perforce.py
+++ b/lib/bb/fetch2/perforce.py
@@ -37,7 +37,7 @@ class Perforce(FetchMethod):
         """ Check to see if a given url can be fetched with perforce. """
         return ud.type in ['p4']
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         Initialize perforce specific variables within url data.  If P4CONFIG is
         provided by the env, use it.  If P4PORT is specified by the recipe, use
diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py
index 8c7e8185..c56df7e1 100644
--- a/lib/bb/fetch2/repo.py
+++ b/lib/bb/fetch2/repo.py
@@ -37,7 +37,7 @@ class Repo(FetchMethod):
         """
         return ud.type in ["repo"]
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         We don"t care about the git rev of the manifests repository, but
         we do care about the manifest to use.  The default is "default".
diff --git a/lib/bb/fetch2/s3.py b/lib/bb/fetch2/s3.py
index 16292886..e00f7072 100644
--- a/lib/bb/fetch2/s3.py
+++ b/lib/bb/fetch2/s3.py
@@ -47,7 +47,7 @@ class S3(FetchMethod):
     def recommends_checksum(self, urldata):
         return True
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         if 'downloadfilename' in ud.parm:
             ud.basename = ud.parm['downloadfilename']
         else:
diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
index 81884a6a..f68cbca1 100644
--- a/lib/bb/fetch2/sftp.py
+++ b/lib/bb/fetch2/sftp.py
@@ -78,7 +78,7 @@ class SFTP(FetchMethod):
     def recommends_checksum(self, urldata):
         return True
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         if 'protocol' in ud.parm and ud.parm['protocol'] == 'git':
             raise bb.fetch2.ParameterError(
                 "Invalid protocol - if you wish to fetch from a " +
diff --git a/lib/bb/fetch2/ssh.py b/lib/bb/fetch2/ssh.py
index 6047ee41..4f9e0853 100644
--- a/lib/bb/fetch2/ssh.py
+++ b/lib/bb/fetch2/ssh.py
@@ -77,7 +77,7 @@ class SSH(FetchMethod):
     def supports_checksum(self, urldata):
         return False
 
-    def urldata_init(self, urldata, d):
+    def urldata_init(self, urldata, d, original_url):
         if 'protocol' in urldata.parm and urldata.parm['protocol'] == 'git':
             raise bb.fetch2.ParameterError(
                 "Invalid protocol - if you wish to fetch from a git " +
diff --git a/lib/bb/fetch2/svn.py b/lib/bb/fetch2/svn.py
index ed70bcf8..be0e9eeb 100644
--- a/lib/bb/fetch2/svn.py
+++ b/lib/bb/fetch2/svn.py
@@ -42,7 +42,7 @@ class Svn(FetchMethod):
         """
         return ud.type in ['svn']
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         """
         init svn specific variable within url data
         """
diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
index 8f505b6d..bd2a0c6b 100644
--- a/lib/bb/fetch2/wget.py
+++ b/lib/bb/fetch2/wget.py
@@ -74,7 +74,7 @@ class Wget(FetchMethod):
     def recommends_checksum(self, urldata):
         return True
 
-    def urldata_init(self, ud, d):
+    def urldata_init(self, ud, d, original_url):
         if 'protocol' in ud.parm:
             if ud.parm['protocol'] == 'git':
                 raise bb.fetch2.ParameterError("Invalid protocol - if you wish to fetch from a git repository using http, you need to instead use the git:// prefix with protocol=http", ud.url)
-- 
2.18.0

[PATCH 8/9] fetch2/git: move generation of git source name into own method

[Urs Fässler]

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/git.py | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 7f7951f7..f9e31d2b 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -248,17 +248,7 @@ class Git(FetchMethod):
                     ud.unresolvedrev[name] = ud.revisions[name]
                 ud.revisions[name] = self.latest_revision(ud, d, name)
 
-        gitsrcname = '%s%s' % (ud.host.replace(':', '.'), ud.path.replace('/', '.').replace('*', '.'))
-        if gitsrcname.startswith('.'):
-            gitsrcname = gitsrcname[1:]
-
-        # for rebaseable git repo, it is necessary to keep mirror tar ball
-        # per revision, so that even the revision disappears from the
-        # upstream repo in the future, the mirror will remain intact and still
-        # contains the revision
-        if ud.rebaseable:
-            for name in ud.names:
-                gitsrcname = gitsrcname + '_' + ud.revisions[name]
+        gitsrcname = self.__build_git_source_name(ud.host, ud.path, ud.rebaseable, ud.names, ud.revisions)
 
         dl_dir = d.getVar("DL_DIR")
         gitdir = d.getVar("GITDIR") or (dl_dir + "/git2")
@@ -295,6 +285,22 @@ class Git(FetchMethod):
             ud.fullshallow = os.path.join(dl_dir, ud.shallowtarball)
             ud.mirrortarballs.insert(0, ud.shallowtarball)
 
+    @staticmethod
+    def __build_git_source_name(host, path, rebaseable, names, revisions):
+        gitsrcname = '%s%s' % (host.replace(':', '.'), path.replace('/', '.').replace('*', '.'))
+        if gitsrcname.startswith('.'):
+            gitsrcname = gitsrcname[1:]
+
+        # for rebaseable git repo, it is necessary to keep mirror tar ball
+        # per revision, so that even the revision disappears from the
+        # upstream repo in the future, the mirror will remain intact and still
+        # contains the revision
+        if rebaseable:
+            for name in names:
+                gitsrcname = gitsrcname + '_' + revisions[name]
+
+        return gitsrcname
+
     def localpath(self, ud, d):
         return ud.clonedir
 
-- 
2.18.0

[PATCH 9/9] fetch2/git: name the shallow tarball according to the url specified in the recipe rather than the mirrored url

[Urs Fässler]

Unpacking a git shallow tarball does not work when using mirror rewrite
rules that alter the path of the remote url. With this patch we always
use to remote url from the recipe to generate the local shallow tarball
name.

Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
 lib/bb/fetch2/git.py  |  7 +++++--
 lib/bb/tests/fetch.py | 26 ++++++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index f9e31d2b..c1fec614 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -79,7 +79,7 @@ import subprocess
 import tempfile
 import bb
 import bb.progress
-from   bb.fetch2 import FetchMethod
+from   bb.fetch2 import FetchMethod, decodeurl
 from   bb.fetch2 import runfetchcmd
 from   bb.fetch2 import logger
 
@@ -259,7 +259,10 @@ class Git(FetchMethod):
         ud.fullmirror = os.path.join(dl_dir, mirrortarball)
         ud.mirrortarballs = [mirrortarball]
         if ud.shallow:
-            tarballname = gitsrcname
+            _, original_host, original_path, _, _, _ = decodeurl(d.expand(original_url))
+            tarballname = self.__build_git_source_name(original_host, original_path, ud.rebaseable, ud.names,
+                                                       ud.revisions)
+
             if ud.bareclone:
                 tarballname = "%s_bare" % tarballname
 
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index fedc3b73..6b7df874 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -522,7 +522,6 @@ class GitShallowTarballNamingTest(FetcherTest):
         self.__recipe_url = "git://git.openembedded.org/bitbake"
         self.__recipe_tarball = "gitshallow_git.openembedded.org.bitbake_82ea737-1_master.tar.gz"
         self.__mirror_url = "git://github.com/openembedded/bitbake.git"
-        self.__mirror_tarball = "gitshallow_github.com.openembedded.bitbake.git_82ea737-1_master.tar.gz"
 
         self.d.setVar('BB_GIT_SHALLOW', '1')
         self.d.setVar('BB_GENERATE_SHALLOW_TARBALLS', '1')
@@ -532,7 +531,7 @@ class GitShallowTarballNamingTest(FetcherTest):
         self.d.setVar("PREMIRRORS", self.__recipe_url + " " + self.__mirror_url + " \n")
 
     @skipIfNoNetwork()
-    def test_that_the_tarball_is_named_after_recipe_url_when_no_mirroring_is_used(self):
+    def test_that_the_recipe_tarball_is_created_when_no_mirroring_is_used(self):
         fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
 
         fetcher.download()
@@ -541,17 +540,17 @@ class GitShallowTarballNamingTest(FetcherTest):
         self.assertIn(self.__recipe_tarball, dir)
 
     @skipIfNoNetwork()
-    def test_that_the_mirror_tarball_is_created_when_mirroring_is_used(self):
+    def test_that_the_recipe_tarball_is_created_when_mirroring_is_used(self):
         self.__setup_mirror_rewrite()
         fetcher = bb.fetch.Fetch([self.__recipe_url], self.d)
 
         fetcher.download()
 
         dir = os.listdir(self.dldir)
-        self.assertIn(self.__mirror_tarball, dir)
+        self.assertIn(self.__recipe_tarball, dir)
 
     @skipIfNoNetwork()
-    def test_that_the_mirror_tarball_still_exists_when_mirroring_is_used_and_the_mirrored_tarball_already_exists(self):
+    def test_that_the_recipe_tarball_is_created_when_mirroring_is_used_and_the_mirrored_tarball_already_exists(self):
         self.__setup_mirror_rewrite()
         fetcher = bb.fetch.Fetch([self.__mirror_url], self.d)
         fetcher.download()
@@ -562,7 +561,7 @@ class GitShallowTarballNamingTest(FetcherTest):
         fetcher.download()
 
         dir = os.listdir(self.dldir)
-        self.assertIn(self.__mirror_tarball, dir)
+        self.assertIn(self.__recipe_tarball, dir)
 
 
 class FetcherLocalTest(FetcherTest):
@@ -1705,3 +1704,18 @@ class GitShallowTest(FetcherTest):
 
         dir = os.listdir(self.unpackdir + "/git/")
         self.assertIn("fstests.doap", dir)
+
+    @skipIfNoNetwork()
+    def test_that_unpack_uses_the_git_shallow_tarball_when_using_mirror_rewrite_rules_and_the_git_clone_does_not_exist(self):
+        self.d.setVar('SRCREV', 'e5939ff608b95cdd4d0ab0e1935781ab9a276ac0')
+        self.d.setVar('BB_GIT_SHALLOW', '1')
+        self.d.setVar('BB_GENERATE_SHALLOW_TARBALLS', '1')
+        self.d.setVar('PREMIRRORS', 'git://git.yoctoproject.org/.* git://git.yoctoproject.org/git/PATH;protocol=https \n')
+        fetcher = bb.fetch.Fetch(["git://git.yoctoproject.org/fstests"], self.d)
+        fetcher.download()
+        bb.utils.remove(self.dldir + '/git2', recurse=True)
+
+        fetcher.unpack(self.unpackdir)
+
+        dir = os.listdir(self.unpackdir + "/git/")
+        self.assertIn("fstests.doap", dir)
-- 
2.18.0

Re: [PATCH 0/9] Always use the url specified in the recipe as a base for the git shallow tarball naming

[Richard Purdie]

On Mon, 2018-07-23 at 17:42 +0200, Urs Fässler wrote:
> The existing behavior was to use the url where the repository was
> cloned from.
> It happened that the tarball was not found when a mirror rewrite rule
> was active.
> 
> We now use the url specified in the recipe to name the shallow
> tarball. It fixes that the tarball is not found under certain
> conditions. In addition, the naming is independent of network/server
> failure and the mapping of the name is easier to understand.

I'm not sure why but throughout this patch series you've used function
names and variables prefixed with "__". We don't do this with any of
our other code so it seems out of keeping with the rest of the coding
style.

With regard to the function "__has_up_to_date_clonedir" you added, its
very unclear why some checks would go in that function and some checks
would go in the other. The commit description helps understand it a bit
more but that doesn't help the function naming or someone looking at
the code in future.

Finally, I'm still not convinced that passing around the original url
and forcing the original url naming into any mirroring code is the
right solution to the problem. I said that at the start and looking at
this code change, I'm still not convinced this is right. Part of the
reason I continue to believe that is you just added a N*N testing
problem to the fetcher where the fetchers now behave differently
depending on two urls passed in rather than just one :(.

Cheers,

Richard

Re: [PATCH 0/9] Always use the url specified in the recipe as a base for the git shallow tarball naming

[Urs Fässler]

Thank you for your feedback.

On Mon, 2018-07-23 at 23:00 +0100, Richard Purdie wrote:
> On Mon, 2018-07-23 at 17:42 +0200, Urs Fässler wrote:
> > The existing behavior was to use the url where the repository was
> > cloned from.
> > It happened that the tarball was not found when a mirror rewrite
> > rule
> > was active.
> > 
> > We now use the url specified in the recipe to name the shallow
> > tarball. It fixes that the tarball is not found under certain
> > conditions. In addition, the naming is independent of
> > network/server
> > failure and the mapping of the name is easier to understand.
> 
> I'm not sure why but throughout this patch series you've used
> function
> names and variables prefixed with "__". We don't do this with any of
> our other code so it seems out of keeping with the rest of the coding
> style.

I will fix that.
There are methods in the git fetcher (same file) that are prefixed with
one underscore. Is this the way to go or do you prefer no prefixing
underscores at all?

> With regard to the function "__has_up_to_date_clonedir" you added,
> its
> very unclear why some checks would go in that function and some
> checks
> would go in the other. The commit description helps understand it a
> bit
> more but that doesn't help the function naming or someone looking at
> the code in future.

I will fix that too.
The difficulty with this one is, that the use of "need_update" is most
certainly a misuse in this context. I can improve the patches to make
the misuse more clear, but it changes only a bit on the end result.

I am planning to resend only the patches that handle the cases when the
sources to unpack are not found (up to Patch 5/9 in this series) first.
The patches to solve the root of problem will follow separately.

> Finally, I'm still not convinced that passing around the original url
> and forcing the original url naming into any mirroring code is the
> right solution to the problem. I said that at the start and looking
> at
> this code change, I'm still not convinced this is right. Part of the
> reason I continue to believe that is you just added a N*N testing
> problem to the fetcher where the fetchers now behave differently
> depending on two urls passed in rather than just one :(.

Maybe it helps when I describe our use case and problem.

Our Yocto build machines have no access over the git protocol to the
servers. We use mirror rewrite rules to use git over https to access
the servers. Some server provide the git over http repository under a
different URL as when accessed over the git protocol. Since the URL
where we cloned the repository from is used to name the tarball, we end
up with different names of the tarballs depending of the availability
of some servers.

To be able to build our Image years later we archive the generated
tarballs. One way to store them is to use Amazon S3 which does not
natively support symlinks. To be able to access the tarballs from S3
over http, we use a mirror rewrite rule.

At the moment we have different scenarios where it fails, all of them a
bit difficult to reproduce. The common root of the problems is, that
the tarballs are not found because they are searched with the wrong
name.

I support your point that the same things should be solved with the
same mechanism. But in our case, the tarballs are not the same thing as
the local git clones. From our view as Bitbake user, the local git
clones are Bitbake internals whereas the tarballs are artifacts we get
out of Bitbake and use with third-party systems.

As a Bitbake user, I expect that the name of the tarball only depends
on the URL I specify in the recipe. I certainly do not expect that the
name is different depending on the availability of some servers since
this is a transport layer issue.

I think both solutions (symlinking tarballs and using recipe URL for
naming) solve the problem equally. We prefer the recipe URL solution
for better compatibility with third party systems.

But then I see that the symlink solution is the one that nicely fits
into Bitbake. I have to check if this is a feasible way to go for us. 

Cheers
Urs

Re: [PATCH 0/9] Always use the url specified in the recipe as a base for the git shallow tarball naming

[Urs Fässler]

Hi Richard,
I have improved my patches but like to have your Input before I commit
them. Please see my questions/reasoning below.

Thanks
Urs

On Wed, 2018-07-25 at 10:57 +0200, Urs Fässler wrote:
> Thank you for your feedback.
> 
> On Mon, 2018-07-23 at 23:00 +0100, Richard Purdie wrote:
> > On Mon, 2018-07-23 at 17:42 +0200, Urs Fässler wrote:
> > > The existing behavior was to use the url where the repository was
> > > cloned from.
> > > It happened that the tarball was not found when a mirror rewrite
> > > rule
> > > was active.
> > > 
> > > We now use the url specified in the recipe to name the shallow
> > > tarball. It fixes that the tarball is not found under certain
> > > conditions. In addition, the naming is independent of
> > > network/server
> > > failure and the mapping of the name is easier to understand.
> > 
> > I'm not sure why but throughout this patch series you've used
> > function
> > names and variables prefixed with "__". We don't do this with any
> > of
> > our other code so it seems out of keeping with the rest of the
> > coding
> > style.
> 
> I will fix that.
> There are methods in the git fetcher (same file) that are prefixed
> with
> one underscore. Is this the way to go or do you prefer no prefixing
> underscores at all?
> 
> > With regard to the function "__has_up_to_date_clonedir" you added,
> > its
> > very unclear why some checks would go in that function and some
> > checks
> > would go in the other. The commit description helps understand it a
> > bit
> > more but that doesn't help the function naming or someone looking
> > at
> > the code in future.
> 
> I will fix that too.
> The difficulty with this one is, that the use of "need_update" is
> most
> certainly a misuse in this context. I can improve the patches to make
> the misuse more clear, but it changes only a bit on the end result.
> 
> I am planning to resend only the patches that handle the cases when
> the
> sources to unpack are not found (up to Patch 5/9 in this series)
> first.
> The patches to solve the root of problem will follow separately.
> 
> > Finally, I'm still not convinced that passing around the original
> > url
> > and forcing the original url naming into any mirroring code is the
> > right solution to the problem. I said that at the start and looking
> > at
> > this code change, I'm still not convinced this is right. Part of
> > the
> > reason I continue to believe that is you just added a N*N testing
> > problem to the fetcher where the fetchers now behave differently
> > depending on two urls passed in rather than just one :(.
> 
> Maybe it helps when I describe our use case and problem.
> 
> Our Yocto build machines have no access over the git protocol to the
> servers. We use mirror rewrite rules to use git over https to access
> the servers. Some server provide the git over http repository under a
> different URL as when accessed over the git protocol. Since the URL
> where we cloned the repository from is used to name the tarball, we
> end
> up with different names of the tarballs depending of the availability
> of some servers.
> 
> To be able to build our Image years later we archive the generated
> tarballs. One way to store them is to use Amazon S3 which does not
> natively support symlinks. To be able to access the tarballs from S3
> over http, we use a mirror rewrite rule.
> 
> At the moment we have different scenarios where it fails, all of them
> a
> bit difficult to reproduce. The common root of the problems is, that
> the tarballs are not found because they are searched with the wrong
> name.
> 
> I support your point that the same things should be solved with the
> same mechanism. But in our case, the tarballs are not the same thing
> as
> the local git clones. From our view as Bitbake user, the local git
> clones are Bitbake internals whereas the tarballs are artifacts we
> get
> out of Bitbake and use with third-party systems.
> 
> As a Bitbake user, I expect that the name of the tarball only depends
> on the URL I specify in the recipe. I certainly do not expect that
> the
> name is different depending on the availability of some servers since
> this is a transport layer issue.
> 
> I think both solutions (symlinking tarballs and using recipe URL for
> naming) solve the problem equally. We prefer the recipe URL solution
> for better compatibility with third party systems.
> 
> But then I see that the symlink solution is the one that nicely fits
> into Bitbake. I have to check if this is a feasible way to go for
> us. 
> 
> Cheers
> Urs
> 

Re: [PATCH 0/9] Always use the url specified in the recipe as a base for the git shallow tarball naming

[Urs Fässler]

ping

On Fri, 2018-08-03 at 08:48 +0200, Urs Fässler wrote:
> Hi Richard,
> I have improved my patches but like to have your Input before I
> commit
> them. Please see my questions/reasoning below.
> 
> Thanks
> Urs
> 
> On Wed, 2018-07-25 at 10:57 +0200, Urs Fässler wrote:
> > Thank you for your feedback.
> > 
> > On Mon, 2018-07-23 at 23:00 +0100, Richard Purdie wrote:
> > > On Mon, 2018-07-23 at 17:42 +0200, Urs Fässler wrote:
> > > > The existing behavior was to use the url where the repository
> > > > was
> > > > cloned from.
> > > > It happened that the tarball was not found when a mirror
> > > > rewrite
> > > > rule
> > > > was active.
> > > > 
> > > > We now use the url specified in the recipe to name the shallow
> > > > tarball. It fixes that the tarball is not found under certain
> > > > conditions. In addition, the naming is independent of
> > > > network/server
> > > > failure and the mapping of the name is easier to understand.
> > > 
> > > I'm not sure why but throughout this patch series you've used
> > > function
> > > names and variables prefixed with "__". We don't do this with any
> > > of
> > > our other code so it seems out of keeping with the rest of the
> > > coding
> > > style.
> > 
> > I will fix that.
> > There are methods in the git fetcher (same file) that are prefixed
> > with
> > one underscore. Is this the way to go or do you prefer no prefixing
> > underscores at all?
> > 
> > > With regard to the function "__has_up_to_date_clonedir" you
> > > added,
> > > its
> > > very unclear why some checks would go in that function and some
> > > checks
> > > would go in the other. The commit description helps understand it
> > > a
> > > bit
> > > more but that doesn't help the function naming or someone looking
> > > at
> > > the code in future.
> > 
> > I will fix that too.
> > The difficulty with this one is, that the use of "need_update" is
> > most
> > certainly a misuse in this context. I can improve the patches to
> > make
> > the misuse more clear, but it changes only a bit on the end result.
> > 
> > I am planning to resend only the patches that handle the cases when
> > the
> > sources to unpack are not found (up to Patch 5/9 in this series)
> > first.
> > The patches to solve the root of problem will follow separately.
> > 
> > > Finally, I'm still not convinced that passing around the original
> > > url
> > > and forcing the original url naming into any mirroring code is
> > > the
> > > right solution to the problem. I said that at the start and
> > > looking
> > > at
> > > this code change, I'm still not convinced this is right. Part of
> > > the
> > > reason I continue to believe that is you just added a N*N testing
> > > problem to the fetcher where the fetchers now behave differently
> > > depending on two urls passed in rather than just one :(.
> > 
> > Maybe it helps when I describe our use case and problem.
> > 
> > Our Yocto build machines have no access over the git protocol to
> > the
> > servers. We use mirror rewrite rules to use git over https to
> > access
> > the servers. Some server provide the git over http repository under
> > a
> > different URL as when accessed over the git protocol. Since the URL
> > where we cloned the repository from is used to name the tarball, we
> > end
> > up with different names of the tarballs depending of the
> > availability
> > of some servers.
> > 
> > To be able to build our Image years later we archive the generated
> > tarballs. One way to store them is to use Amazon S3 which does not
> > natively support symlinks. To be able to access the tarballs from
> > S3
> > over http, we use a mirror rewrite rule.
> > 
> > At the moment we have different scenarios where it fails, all of
> > them
> > a
> > bit difficult to reproduce. The common root of the problems is,
> > that
> > the tarballs are not found because they are searched with the wrong
> > name.
> > 
> > I support your point that the same things should be solved with the
> > same mechanism. But in our case, the tarballs are not the same
> > thing
> > as
> > the local git clones. From our view as Bitbake user, the local git
> > clones are Bitbake internals whereas the tarballs are artifacts we
> > get
> > out of Bitbake and use with third-party systems.
> > 
> > As a Bitbake user, I expect that the name of the tarball only
> > depends
> > on the URL I specify in the recipe. I certainly do not expect that
> > the
> > name is different depending on the availability of some servers
> > since
> > this is a transport layer issue.
> > 
> > I think both solutions (symlinking tarballs and using recipe URL
> > for
> > naming) solve the problem equally. We prefer the recipe URL
> > solution
> > for better compatibility with third party systems.
> > 
> > But then I see that the symlink solution is the one that nicely
> > fits
> > into Bitbake. I have to check if this is a feasible way to go for
> > us. 
> > 
> > Cheers
> > Urs
> >