[bitbake-devel][RFC][PATCH] bitbake: fetch2: correct PREMIRROR URI

["Scott Weaver" <weaverjs@gmail.com>]

When downloadfilename is defined in a recipe's SRC_URI and a PREMIRROR is also
defined for that URI, the downloadfilename is appended to the mirror URI and it
should not be appended.

Three new fetch bitbake-selftest tests were added to test downloadfilename
scenarios.

[YOCTO #13039]

Signed-off-by: Scott Weaver <weaverjs@gmail.com>
---

This fix is for BZ13039.

downloadfilename is defined as "the filename used when storing the downloaded
file" under the definition of SRC_URI [1].
This definition appears to be echoed in 44.3.2 where it says downloadfilename
"allows the name of the downloaded file to be specified" [2]. Although, the
second definition isn't as clear and could be interpreted to mean that the
name of the file to be downloaded can be specified. Which, if intentional,
would not match the first definition.

I believe that the first definition is the correct definition of
downloadfilename.

I've added three new tests to bitbake-selftest's fetch testsuite.

Executed against the master branch, the following new tests fail (see patch):

test_fetch_premirror_specify_downloadfilename_regex_uri() fails because
the fetcher attempts to download the downloadfilename.

test_fetch_premirror_specify_downloadfilename_specific_uri() was added
to address BZ13039 and is expected to fail.

These tests pass when executed with this patch applied.

However, this change breaks the test that I have commented out in the patch
which is executed by MirrorUriTest.test_urireplace.
I don't believe this is a valid test because a mirror is normally the location
where an artifact can be found and should not specify the filename itself.

I recognize that there might be someone using PREMIRRORS in this way
given that a test was written for this use case and maybe this is a valid
definition of a mirror but this is what leads me to this RFC.

I would like your opinion on my interpretation of downloadfilename and if you
think that it's okay to remove the commented out test. Thus, any use of
PREMIRRORS in this way will now fail because this patch assumes that a mirror
only defines the location and would not include the filename.

[1] https://www.yoctoproject.org/docs/latest/mega-manual/mega-manual.html#var-SRC_URI
[2] https://www.yoctoproject.org/docs/latest/mega-manual/mega-manual.html#http-ftp-fetcher


 bitbake/lib/bb/fetch2/__init__.py | 19 ++++++++++++++++---
 bitbake/lib/bb/tests/fetch.py     | 26 ++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/bitbake/lib/bb/fetch2/__init__.py b/bitbake/lib/bb/fetch2/__init__.py
index 914fa5c024..01e0d1becf 100644
--- a/bitbake/lib/bb/fetch2/__init__.py
+++ b/bitbake/lib/bb/fetch2/__init__.py
@@ -466,8 +466,8 @@ def uri_replace(ud, uri_find, uri_replace, replacements, d, mirrortarball=None):
                     # Kill parameters, they make no sense for mirror tarballs
                     uri_decoded[5] = {}
                 elif ud.localpath and ud.method.supports_checksum(ud):
-                    basename = os.path.basename(ud.localpath)
-                if basename and not result_decoded[loc].endswith(basename):
+                    basename = os.path.basename(uri_decoded[loc])
+                if basename and result_decoded[loc].endswith('/'):
                     result_decoded[loc] = os.path.join(result_decoded[loc], basename)
         else:
             return None
@@ -527,12 +527,25 @@ def fetcher_compare_revisions(d):
     headrevs = dict(bb.persist_data.persist('BB_URI_HEADREVS', d))
     return headrevs != bb.fetch2.saved_headrevs
 
+def mirror_fixup(mirrors):
+    # Ensure that the mirror uri contains a terminating directory separator.
+    # Git mirrors shall not be modified.
+    for m in mirrors:
+        if not re.search("^git:", m[1]):
+            if not ';' in m[1]:
+                m[1] = os.path.join(m[1], "")
+            else:
+                s = m[1].split(';', 1)
+                uri = os.path.join(s[0], "")
+                m[1] = "%s;%s" % (uri, s[1])
+    return mirrors
+
 def mirror_from_string(data):
     mirrors = (data or "").replace('\\n',' ').split()
     # Split into pairs
     if len(mirrors) % 2 != 0:
         bb.warn('Invalid mirror data %s, should have paired members.' % data)
-    return list(zip(*[iter(mirrors)]*2))
+    return mirror_fixup([list(m) for m in zip(*[iter(mirrors)]*2)])
 
 def verify_checksum(ud, d, precomputed={}):
     """
diff --git a/bitbake/lib/bb/tests/fetch.py b/bitbake/lib/bb/tests/fetch.py
index 9291ce4a06..7b6d253186 100644
--- a/bitbake/lib/bb/tests/fetch.py
+++ b/bitbake/lib/bb/tests/fetch.py
@@ -395,6 +395,7 @@ class FetcherTest(unittest.TestCase):
 
 class MirrorUriTest(FetcherTest):
 
+    # (<src_uri>, <mirror_regex>, <mirror>):<expected_result>
     replaceuris = {
         ("git://git.invalid.infradead.org/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/.*", "http://somewhere.org/somedir/") 
             : "http://somewhere.org/somedir/git2_git.invalid.infradead.org.mtd-utils.git.tar.gz",
@@ -412,8 +413,8 @@ class MirrorUriTest(FetcherTest):
             : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
         ("http://somewhere.org/somedir1/somedir2/somefile_1.2.3.tar.gz", "http://.*/.*", "http://somewhere2.org/somedir3") 
             : "http://somewhere2.org/somedir3/somefile_1.2.3.tar.gz",
-        ("http://somewhere.org/somedir1/somefile_1.2.3.tar.gz", "http://somewhere.org/somedir1/somefile_1.2.3.tar.gz", "http://somewhere2.org/somedir3/somefile_1.2.3.tar.gz") 
-            : "http://somewhere2.org/somedir3/somefile_1.2.3.tar.gz",
+        #("http://somewhere.org/somedir1/somefile_1.2.3.tar.gz", "http://somewhere.org/somedir1/somefile_1.2.3.tar.gz", "http://somewhere2.org/somedir3/somefile_1.2.3.tar.gz")
+        #    : "http://somewhere2.org/somedir3/somefile_1.2.3.tar.gz",
         ("http://www.apache.org/dist/subversion/subversion-1.7.1.tar.bz2", "http://www.apache.org/dist", "http://archive.apache.org/dist")
             : "http://archive.apache.org/dist/subversion/subversion-1.7.1.tar.bz2",
         ("http://www.apache.org/dist/subversion/subversion-1.7.1.tar.bz2", "http://.*/.*", "file:///somepath/downloads/")
@@ -866,6 +867,27 @@ class FetcherNetworkTest(FetcherTest):
         fetcher.download()
         self.assertEqual(os.path.getsize(self.dldir + "/bitbake-1.0.tar.gz"), 57749)
 
+    @skipIfNoNetwork()
+    def test_fetch_specify_downloadfilename(self):
+        fetcher = bb.fetch.Fetch(["http://downloads.yoctoproject.org/releases/bitbake/bitbake-1.0.tar.gz;downloadfilename=bitbake-v1.0.0.tar.gz"], self.d)
+        fetcher.download()
+        self.assertEqual(os.path.getsize(self.dldir + "/bitbake-v1.0.0.tar.gz"), 57749)
+
+    @skipIfNoNetwork()
+    def test_fetch_premirror_specify_downloadfilename_regex_uri(self):
+        self.d.setVar("PREMIRRORS", "http://.*/.* http://downloads.yoctoproject.org/releases/bitbake/")
+        fetcher = bb.fetch.Fetch(["http://invalid.yoctoproject.org/releases/bitbake/bitbake-1.0.tar.gz;downloadfilename=bitbake-v1.0.0.tar.gz"], self.d)
+        fetcher.download()
+        self.assertEqual(os.path.getsize(self.dldir + "/bitbake-v1.0.0.tar.gz"), 57749)
+
+    @skipIfNoNetwork()
+    # BZ13039
+    def test_fetch_premirror_specify_downloadfilename_specific_uri(self):
+        self.d.setVar("PREMIRRORS", "http://invalid.yoctoproject.org/releases/bitbake http://downloads.yoctoproject.org/releases/bitbake")
+        fetcher = bb.fetch.Fetch(["http://invalid.yoctoproject.org/releases/bitbake/bitbake-1.0.tar.gz;downloadfilename=bitbake-v1.0.0.tar.gz"], self.d)
+        fetcher.download()
+        self.assertEqual(os.path.getsize(self.dldir + "/bitbake-v1.0.0.tar.gz"), 57749)
+
     @skipIfNoNetwork()
     def gitfetcher(self, url1, url2):
         def checkrevision(self, fetcher):
-- 
2.25.1