[RFC PATCH 00/15] Make mirror replacement syntax explicit

[RFC PATCH 00/15] Make mirror replacement syntax explicit

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>


The mirror replacement syntax contains many implicit transformations.
The path of the URI always contains the base name of the downloaded
filename. This makes it impossible to rename or remove the base name of
the original path. It prevents upstream mirror for SRC_URIS with a
downloadfilename parameter. The base name of the downloaded filename
makes it impossible to use the download mirror for SRC_URIs with
subfolders in the downloadfilename parameter. Altogether the implicit
transformation complicates the understanding of the replacements.

This series adds an additional replacement named DOWNLOADFILENAME. This
replacement contains the relative filename of the downloaded file or
mirror archive for git and hg. This allows the user to explicitly define
the behavior. The usage is equivalent to the PATH replacement for the
sstate mirror from file to https scheme.

git://.*/.*  http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
https?://.*/.*  http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
file://.*  https://sstate.yoctoproject.org/all/PATH;downloadfilename=PATH

Without a replacement variable the mirror will use the same base name as
the origin SRC_URI. This allows the usage of private package manager
registry together with a downloadfilename parameter or the rename of the
base name.

https://registry.npmjs.org/  https://example.com/npm/registry/
https://example.com/example/1.0.0.tgz  https://example.com/example/example-1.0.0.tgz

The series adds heuristics to keep a backward compatibility to common
styles. Because of the ambiguity of the old style, it is advisable to
remove this compatibility sooner or later to avoid unexpected behavior.


Stefan Herbrechtsmeier (15):
  fetch2: remove unnecessary expand function calls
  fetch2: local: use path variable
  fetch2: remove unnecessary unquote
  fetch2: ssh: use common localpath handling
  fetch2: clearcase: remove double DL_DIR from localfile
  fetch2: remove basepath from FetchData
  fetch2: remove basename from FetchData
  fetch2: use localpath instead of localfile
  fetch2: make DOWNLOADFILENAME and PATH explicit in mirrors
  tests: fetch: unify style of replaceuris in MirrorUriTest
  tests: fetch: fix nonsensical replaceuris in MirrorUriTest
  tests: fetch: reenable replaceuris in MirrorUriTest
  tests: fetch: comment and add replaceuris in MirrorUriTest
  tests: fetch: add npm mirrors replaceuris in MirrorUriTest
  tests: fetch: fix nonsensical mirror uris in FetcherNetworkTest

 lib/bb/fetch2/__init__.py  | 62 ++++++++++++++++++++------------------
 lib/bb/fetch2/az.py        |  8 ++---
 lib/bb/fetch2/clearcase.py |  2 --
 lib/bb/fetch2/gcp.py       |  8 -----
 lib/bb/fetch2/local.py     |  8 ++---
 lib/bb/fetch2/npm.py       |  2 +-
 lib/bb/fetch2/repo.py      |  2 +-
 lib/bb/fetch2/s3.py        |  7 -----
 lib/bb/fetch2/sftp.py      | 10 +-----
 lib/bb/fetch2/ssh.py       |  3 +-
 lib/bb/fetch2/wget.py      | 11 +------
 lib/bb/tests/fetch.py      | 62 ++++++++++++++++++++++++++++++++------
 12 files changed, 98 insertions(+), 87 deletions(-)

-- 
2.39.5

[RFC PATCH 01/15] fetch2: remove unnecessary expand function calls

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

The fetch data class already expands the type, host, path, user, pswd
and parm variables. The fetcher classes already expand the localfile
variable. The getVar function expands the returned string per default.
Remove unnecessary expand function calls to simplify the code.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/__init__.py | 4 ++--
 lib/bb/fetch2/az.py       | 5 +++--
 lib/bb/fetch2/gcp.py      | 2 +-
 lib/bb/fetch2/npm.py      | 2 +-
 lib/bb/fetch2/s3.py       | 2 +-
 lib/bb/fetch2/sftp.py     | 2 +-
 lib/bb/fetch2/wget.py     | 4 ++--
 7 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index de36f06bf..c77155a1e 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1182,7 +1182,7 @@ def trusted_network(d, url):
     if bb.utils.to_boolean(d.getVar("BB_NO_NETWORK")):
         return True
 
-    pkgname = d.expand(d.getVar('PN', False))
+    pkgname = d.getVar('PN')
     trusted_hosts = None
     if pkgname:
         trusted_hosts = d.getVarFlag('BB_ALLOWED_NETWORKS', pkgname, False)
@@ -1817,7 +1817,7 @@ class Fetch(object):
             self.ud[url] = FetchData(url, self.d)
 
         self.ud[url].setup_localpath(self.d)
-        return self.d.expand(self.ud[url].localpath)
+        return self.ud[url].localpath
 
     def localpaths(self):
         """
diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
index 3ccc594c2..346124a8b 100644
--- a/lib/bb/fetch2/az.py
+++ b/lib/bb/fetch2/az.py
@@ -66,11 +66,12 @@ class Az(Wget):
         else:
             azuri = '%s%s%s' % ('https://', ud.host, ud.path)
 
+        dldir = d.getVar("DL_DIR")
         if os.path.exists(ud.localpath):
             # file exists, but we didnt complete it.. trying again.
-            fetchcmd += d.expand(" -c -P ${DL_DIR} '%s'" % azuri)
+            fetchcmd += " -c -P %s '%s'" % (dldir, azuri)
         else:
-            fetchcmd += d.expand(" -P ${DL_DIR} '%s'" % azuri)
+            fetchcmd += " -P %s '%s'" % (dldir, azuri)
 
         try:
             self._runwget(ud, d, fetchcmd, False)
diff --git a/lib/bb/fetch2/gcp.py b/lib/bb/fetch2/gcp.py
index 2ee9ed219..268267b7a 100644
--- a/lib/bb/fetch2/gcp.py
+++ b/lib/bb/fetch2/gcp.py
@@ -46,7 +46,7 @@ class GCP(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = d.expand(urllib.parse.unquote(ud.basename))
+        ud.localfile = urllib.parse.unquote(ud.basename)
 
     def get_gcp_client(self):
         from google.cloud import storage
diff --git a/lib/bb/fetch2/npm.py b/lib/bb/fetch2/npm.py
index ac76d64cd..c09f05044 100644
--- a/lib/bb/fetch2/npm.py
+++ b/lib/bb/fetch2/npm.py
@@ -166,7 +166,7 @@ class Npm(FetchMethod):
         # Using the 'downloadfilename' parameter as local filename
         # or the npm package name.
         if "downloadfilename" in ud.parm:
-            ud.localfile = npm_localfile(d.expand(ud.parm["downloadfilename"]))
+            ud.localfile = npm_localfile(ud.parm["downloadfilename"])
         else:
             ud.localfile = npm_localfile(ud.package, ud.version)
 
diff --git a/lib/bb/fetch2/s3.py b/lib/bb/fetch2/s3.py
index 6b8ffd535..fa5292dfd 100644
--- a/lib/bb/fetch2/s3.py
+++ b/lib/bb/fetch2/s3.py
@@ -77,7 +77,7 @@ class S3(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = d.expand(urllib.parse.unquote(ud.basename))
+        ud.localfile = urllib.parse.unquote(ud.basename)
 
         ud.basecmd = d.getVar("FETCHCMD_s3") or "/usr/bin/env aws s3"
 
diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
index 7884cce94..45b6afb4a 100644
--- a/lib/bb/fetch2/sftp.py
+++ b/lib/bb/fetch2/sftp.py
@@ -77,7 +77,7 @@ class SFTP(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = d.expand(urllib.parse.unquote(ud.basename))
+        ud.localfile = urllib.parse.unquote(ud.basename)
 
     def download(self, ud, d):
         """Fetch urls"""
diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
index 198426065..7066d5e2c 100644
--- a/lib/bb/fetch2/wget.py
+++ b/lib/bb/fetch2/wget.py
@@ -78,9 +78,9 @@ class Wget(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = d.expand(urllib.parse.unquote(ud.basename))
+        ud.localfile = urllib.parse.unquote(ud.basename)
         if not ud.localfile:
-            ud.localfile = d.expand(urllib.parse.unquote(ud.host + ud.path).replace("/", "."))
+            ud.localfile = urllib.parse.unquote(ud.host + ud.path).replace("/", ".")
 
         self.basecmd = d.getVar("FETCHCMD_wget") or "/usr/bin/env wget -t 2 -T 100"
 
-- 
2.39.5

[RFC PATCH 02/15] fetch2: local: use path variable

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Use the path variable from the fetch data instead of decoding the path
manually from the plain unexpanded url.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/__init__.py | 2 +-
 lib/bb/fetch2/local.py    | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index c77155a1e..f282ecf17 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1271,7 +1271,7 @@ def get_checksum_file_list(d):
             found = False
             paths = ud.method.localfile_searchpaths(ud, d)
             for f in paths:
-                pth = ud.decodedurl
+                pth = ud.path
                 if os.path.exists(f):
                     found = True
                 filelist.append(f + ":" + str(os.path.exists(f)))
diff --git a/lib/bb/fetch2/local.py b/lib/bb/fetch2/local.py
index 7d7668110..fda56a564 100644
--- a/lib/bb/fetch2/local.py
+++ b/lib/bb/fetch2/local.py
@@ -29,11 +29,10 @@ class Local(FetchMethod):
 
     def urldata_init(self, ud, d):
         # 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)
-        ud.basepath = ud.decodedurl
+        ud.basename = os.path.basename(ud.path)
+        ud.basepath = ud.path
         ud.needdonestamp = False
-        if "*" in ud.decodedurl:
+        if "*" in ud.path:
             raise bb.fetch2.ParameterError("file:// urls using globbing are no longer supported. Please place the files in a directory and reference that instead.", ud.url)
         return
 
@@ -48,7 +47,7 @@ class Local(FetchMethod):
         Return the local filename of a given url assuming a successful fetch.
         """
         searched = []
-        path = urldata.decodedurl
+        path = urldata.path
         newpath = path
         if path[0] == "/":
             logger.debug2("Using absolute %s" % (path))
-- 
2.39.5

[RFC PATCH 03/15] fetch2: remove unnecessary unquote

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

The URI path is already unquoted. Remove the unnecessary unquote
function calls for URI path values.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/gcp.py  | 2 +-
 lib/bb/fetch2/s3.py   | 2 +-
 lib/bb/fetch2/sftp.py | 2 +-
 lib/bb/fetch2/wget.py | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/bb/fetch2/gcp.py b/lib/bb/fetch2/gcp.py
index 268267b7a..86546d40b 100644
--- a/lib/bb/fetch2/gcp.py
+++ b/lib/bb/fetch2/gcp.py
@@ -46,7 +46,7 @@ class GCP(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = urllib.parse.unquote(ud.basename)
+        ud.localfile = ud.basename
 
     def get_gcp_client(self):
         from google.cloud import storage
diff --git a/lib/bb/fetch2/s3.py b/lib/bb/fetch2/s3.py
index fa5292dfd..22c053813 100644
--- a/lib/bb/fetch2/s3.py
+++ b/lib/bb/fetch2/s3.py
@@ -77,7 +77,7 @@ class S3(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = urllib.parse.unquote(ud.basename)
+        ud.localfile = ud.basename
 
         ud.basecmd = d.getVar("FETCHCMD_s3") or "/usr/bin/env aws s3"
 
diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
index 45b6afb4a..bee71a0d0 100644
--- a/lib/bb/fetch2/sftp.py
+++ b/lib/bb/fetch2/sftp.py
@@ -77,7 +77,7 @@ class SFTP(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = urllib.parse.unquote(ud.basename)
+        ud.localfile = ud.basename
 
     def download(self, ud, d):
         """Fetch urls"""
diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
index 7066d5e2c..6cb728ab4 100644
--- a/lib/bb/fetch2/wget.py
+++ b/lib/bb/fetch2/wget.py
@@ -78,9 +78,9 @@ class Wget(FetchMethod):
         else:
             ud.basename = os.path.basename(ud.path)
 
-        ud.localfile = urllib.parse.unquote(ud.basename)
+        ud.localfile = ud.basename
         if not ud.localfile:
-            ud.localfile = urllib.parse.unquote(ud.host + ud.path).replace("/", ".")
+            ud.localfile = ud.host + ud.path.replace("/", ".")
 
         self.basecmd = d.getVar("FETCHCMD_wget") or "/usr/bin/env wget -t 2 -T 100"
 
-- 
2.39.5

[RFC PATCH 04/15] fetch2: ssh: use common localpath handling

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/ssh.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/bb/fetch2/ssh.py b/lib/bb/fetch2/ssh.py
index 0cbb2a6f2..2a0f2cb44 100644
--- a/lib/bb/fetch2/ssh.py
+++ b/lib/bb/fetch2/ssh.py
@@ -73,8 +73,7 @@ class SSH(FetchMethod):
         path = m.group('path')
         path = urllib.parse.unquote(path)
         host = m.group('host')
-        urldata.localpath = os.path.join(d.getVar('DL_DIR'),
-                os.path.basename(os.path.normpath(path)))
+        urldata.localfile = os.path.basename(os.path.normpath(path))
 
     def download(self, urldata, d):
         dldir = d.getVar('DL_DIR')
-- 
2.39.5

[RFC PATCH 05/15] fetch2: clearcase: remove double DL_DIR from localfile

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/clearcase.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/bb/fetch2/clearcase.py b/lib/bb/fetch2/clearcase.py
index 2b3bd7069..17500daf9 100644
--- a/lib/bb/fetch2/clearcase.py
+++ b/lib/bb/fetch2/clearcase.py
@@ -130,8 +130,6 @@ class ClearCase(FetchMethod):
         self.debug("configspecfile  = %s" % ud.configspecfile)
         self.debug("localfile       = %s" % ud.localfile)
 
-        ud.localfile = os.path.join(d.getVar("DL_DIR"), ud.localfile)
-
     def _build_ccase_command(self, ud, command):
         """
         Build up a commandline based on ud
-- 
2.39.5

[RFC PATCH 06/15] fetch2: remove basepath from FetchData

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Remove the basepath variable from the FetchData class. The variable is
never used in the class because the only user of the variable also sets
the needdonestamp variable to false. The basepath variable is useless
because it contains the content of the path variable.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/__init__.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index f282ecf17..f4cf25cfd 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1305,7 +1305,6 @@ class FetchData(object):
         self.lockfile = None
         self.mirrortarballs = []
         self.basename = None
-        self.basepath = None
         (self.type, self.host, self.path, self.user, self.pswd, self.parm) = decodeurl(d.expand(url))
         self.date = self.getSRCDate(d)
         self.url = url
@@ -1378,8 +1377,8 @@ class FetchData(object):
             basepath = self.localpath
         elif self.localpath:
             basepath = dldir + os.sep + os.path.basename(self.localpath)
-        elif self.basepath or self.basename:
-            basepath = dldir + os.sep + (self.basepath or self.basename)
+        elif self.basename:
+            basepath = dldir + os.sep + self.basename
         else:
             bb.fatal("Can't determine lock path for url %s" % url)
 
-- 
2.39.5

[RFC PATCH 07/15] fetch2: remove basename from FetchData

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Remove the basename variable from the FetchData class. The variable
contains the same information as the localfile variable and is mainly
used by duplicated code. Consolidate the duplicated code inside the
FetchData class.

The variable name is also misleading because it doesn’t contain the
basename of a file.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/__init__.py | 14 ++++++++++----
 lib/bb/fetch2/gcp.py      |  8 --------
 lib/bb/fetch2/local.py    |  1 -
 lib/bb/fetch2/s3.py       |  7 -------
 lib/bb/fetch2/sftp.py     |  7 -------
 lib/bb/fetch2/wget.py     |  9 ---------
 6 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index f4cf25cfd..2328a8fe3 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1304,7 +1304,6 @@ class FetchData(object):
         self.localpath = None
         self.lockfile = None
         self.mirrortarballs = []
-        self.basename = None
         (self.type, self.host, self.path, self.user, self.pswd, self.parm) = decodeurl(d.expand(url))
         self.date = self.getSRCDate(d)
         self.url = url
@@ -1360,10 +1359,17 @@ class FetchData(object):
 
         self.ignore_checksums = False
 
+        if not self.localfile and self.needdonestamp:
+            if 'downloadfilename' in self.parm:
+                self.localfile = self.parm['downloadfilename']
+            else:
+                self.localfile = os.path.basename(self.path)
+            if not os.path.basename(self.localfile):
+                self.localfile = self.host + self.path.replace("/", ".")
+
         if "localpath" in self.parm:
             # if user sets localpath for file, use it instead.
             self.localpath = self.parm["localpath"]
-            self.basename = os.path.basename(self.localpath)
         elif self.localfile:
             self.localpath = self.method.localpath(self, d)
 
@@ -1377,8 +1383,8 @@ class FetchData(object):
             basepath = self.localpath
         elif self.localpath:
             basepath = dldir + os.sep + os.path.basename(self.localpath)
-        elif self.basename:
-            basepath = dldir + os.sep + self.basename
+        elif self.localfile:
+            basepath = dldir + os.sep + self.localfile
         else:
             bb.fatal("Can't determine lock path for url %s" % url)
 
diff --git a/lib/bb/fetch2/gcp.py b/lib/bb/fetch2/gcp.py
index 86546d40b..62715f746 100644
--- a/lib/bb/fetch2/gcp.py
+++ b/lib/bb/fetch2/gcp.py
@@ -40,14 +40,6 @@ class GCP(FetchMethod):
     def recommends_checksum(self, urldata):
         return True
 
-    def urldata_init(self, ud, d):
-        if 'downloadfilename' in ud.parm:
-            ud.basename = ud.parm['downloadfilename']
-        else:
-            ud.basename = os.path.basename(ud.path)
-
-        ud.localfile = ud.basename
-
     def get_gcp_client(self):
         from google.cloud import storage
         self.gcp_client = storage.Client(project=None)
diff --git a/lib/bb/fetch2/local.py b/lib/bb/fetch2/local.py
index fda56a564..0e9357968 100644
--- a/lib/bb/fetch2/local.py
+++ b/lib/bb/fetch2/local.py
@@ -29,7 +29,6 @@ class Local(FetchMethod):
 
     def urldata_init(self, ud, d):
         # We don't set localfile as for this fetcher the file is already local!
-        ud.basename = os.path.basename(ud.path)
         ud.basepath = ud.path
         ud.needdonestamp = False
         if "*" in ud.path:
diff --git a/lib/bb/fetch2/s3.py b/lib/bb/fetch2/s3.py
index 22c053813..2cf181442 100644
--- a/lib/bb/fetch2/s3.py
+++ b/lib/bb/fetch2/s3.py
@@ -72,13 +72,6 @@ class S3(FetchMethod):
         return True
 
     def urldata_init(self, ud, d):
-        if 'downloadfilename' in ud.parm:
-            ud.basename = ud.parm['downloadfilename']
-        else:
-            ud.basename = os.path.basename(ud.path)
-
-        ud.localfile = ud.basename
-
         ud.basecmd = d.getVar("FETCHCMD_s3") or "/usr/bin/env aws s3"
 
     def download(self, ud, d):
diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
index bee71a0d0..2a2a70a1b 100644
--- a/lib/bb/fetch2/sftp.py
+++ b/lib/bb/fetch2/sftp.py
@@ -72,13 +72,6 @@ class SFTP(FetchMethod):
                 "git repository using ssh, you need to use the " +
                 "git:// prefix with protocol=ssh", ud.url)
 
-        if 'downloadfilename' in ud.parm:
-            ud.basename = ud.parm['downloadfilename']
-        else:
-            ud.basename = os.path.basename(ud.path)
-
-        ud.localfile = ud.basename
-
     def download(self, ud, d):
         """Fetch urls"""
 
diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
index 6cb728ab4..161c66bea 100644
--- a/lib/bb/fetch2/wget.py
+++ b/lib/bb/fetch2/wget.py
@@ -73,15 +73,6 @@ class Wget(FetchMethod):
             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)
 
-        if 'downloadfilename' in ud.parm:
-            ud.basename = ud.parm['downloadfilename']
-        else:
-            ud.basename = os.path.basename(ud.path)
-
-        ud.localfile = ud.basename
-        if not ud.localfile:
-            ud.localfile = ud.host + ud.path.replace("/", ".")
-
         self.basecmd = d.getVar("FETCHCMD_wget") or "/usr/bin/env wget -t 2 -T 100"
 
         if ud.type == 'ftp' or ud.type == 'ftps':
-- 
2.39.5

[RFC PATCH 08/15] fetch2: use localpath instead of localfile

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Use localpath variable instead of localfile and DL_DIR variable.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/az.py   | 3 +--
 lib/bb/fetch2/repo.py | 2 +-
 lib/bb/fetch2/sftp.py | 3 +--
 lib/bb/fetch2/wget.py | 2 +-
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
index 346124a8b..1f0f1337a 100644
--- a/lib/bb/fetch2/az.py
+++ b/lib/bb/fetch2/az.py
@@ -49,8 +49,7 @@ class Az(Wget):
         fetchcmd = self.basecmd + ' --retry-connrefused --waitretry=5'
 
         # We need to provide a localpath to avoid wget using the SAS
-        # ud.localfile either has the downloadfilename or ud.path
-        localpath = os.path.join(d.getVar("DL_DIR"), ud.localfile)
+        localpath = ud.localpath
         bb.utils.mkdirhier(os.path.dirname(localpath))
         fetchcmd += " -O %s" % shlex.quote(localpath)
 
diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py
index fa4cb8149..5c0edf1f2 100644
--- a/lib/bb/fetch2/repo.py
+++ b/lib/bb/fetch2/repo.py
@@ -46,7 +46,7 @@ class Repo(FetchMethod):
     def download(self, ud, d):
         """Fetch url"""
 
-        if os.access(os.path.join(d.getVar("DL_DIR"), ud.localfile), os.R_OK):
+        if os.access(ud.localpath, os.R_OK):
             logger.debug("%s already exists (or was stashed). Skipping repo init / sync.", ud.localpath)
             return
 
diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
index 2a2a70a1b..b88dc5a28 100644
--- a/lib/bb/fetch2/sftp.py
+++ b/lib/bb/fetch2/sftp.py
@@ -82,8 +82,7 @@ class SFTP(FetchMethod):
             port = '-P %d' % urlo.port
             urlo.port = None
 
-        dldir = d.getVar('DL_DIR')
-        lpath = os.path.join(dldir, ud.localfile)
+        lpath = ud.localpath
 
         user = ''
         if urlo.userinfo:
diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
index 161c66bea..1194f0e7a 100644
--- a/lib/bb/fetch2/wget.py
+++ b/lib/bb/fetch2/wget.py
@@ -95,7 +95,7 @@ class Wget(FetchMethod):
         fetchcmd = self.basecmd
 
         dldir = os.path.realpath(d.getVar("DL_DIR"))
-        localpath = os.path.join(dldir, ud.localfile) + ".tmp"
+        localpath = ud.localpath + ".tmp"
         bb.utils.mkdirhier(os.path.dirname(localpath))
         fetchcmd += " -O %s" % shlex.quote(localpath)
 
-- 
2.39.5

Re: [bitbake-devel] [RFC PATCH 08/15] fetch2: use localpath instead of localfile

[Richard Purdie]

On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> 
> Use localpath variable instead of localfile and DL_DIR variable.
> 
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
> 
>  lib/bb/fetch2/az.py   | 3 +--
>  lib/bb/fetch2/repo.py | 2 +-
>  lib/bb/fetch2/sftp.py | 3 +--
>  lib/bb/fetch2/wget.py | 2 +-
>  4 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
> index 346124a8b..1f0f1337a 100644
> --- a/lib/bb/fetch2/az.py
> +++ b/lib/bb/fetch2/az.py
> @@ -49,8 +49,7 @@ class Az(Wget):
>          fetchcmd = self.basecmd + ' --retry-connrefused --waitretry=5'
>  
>          # We need to provide a localpath to avoid wget using the SAS
> -        # ud.localfile either has the downloadfilename or ud.path
> -        localpath = os.path.join(d.getVar("DL_DIR"), ud.localfile)
> +        localpath = ud.localpath
>          bb.utils.mkdirhier(os.path.dirname(localpath))
>          fetchcmd += " -O %s" % shlex.quote(localpath)
>  
> diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py
> index fa4cb8149..5c0edf1f2 100644
> --- a/lib/bb/fetch2/repo.py
> +++ b/lib/bb/fetch2/repo.py
> @@ -46,7 +46,7 @@ class Repo(FetchMethod):
>      def download(self, ud, d):
>          """Fetch url"""
>  
> -        if os.access(os.path.join(d.getVar("DL_DIR"), ud.localfile), os.R_OK):
> +        if os.access(ud.localpath, os.R_OK):
>              logger.debug("%s already exists (or was stashed). Skipping repo init / sync.", ud.localpath)
>              return
>  
> diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
> index 2a2a70a1b..b88dc5a28 100644
> --- a/lib/bb/fetch2/sftp.py
> +++ b/lib/bb/fetch2/sftp.py
> @@ -82,8 +82,7 @@ class SFTP(FetchMethod):
>              port = '-P %d' % urlo.port
>              urlo.port = None
>  
> -        dldir = d.getVar('DL_DIR')
> -        lpath = os.path.join(dldir, ud.localfile)
> +        lpath = ud.localpath
>  
>          user = ''
>          if urlo.userinfo:
> diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
> index 161c66bea..1194f0e7a 100644
> --- a/lib/bb/fetch2/wget.py
> +++ b/lib/bb/fetch2/wget.py
> @@ -95,7 +95,7 @@ class Wget(FetchMethod):
>          fetchcmd = self.basecmd
>  
>          dldir = os.path.realpath(d.getVar("DL_DIR"))
> -        localpath = os.path.join(dldir, ud.localfile) + ".tmp"
> +        localpath = ud.localpath + ".tmp"
>          bb.utils.mkdirhier(os.path.dirname(localpath))
>          fetchcmd += " -O %s" % shlex.quote(localpath)
>  
> 


I put the first 8 patches of this series into a test branch and ran it
against the autobuilder. It fails to parse, unable to find files :(.

https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/stdio
https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/errors

That suggests there is some breaking change in here unfortunately.

Cheers,

Richard

Re: [bitbake-devel] [RFC PATCH 08/15] fetch2: use localpath instead of localfile

[Stefan Herbrechtsmeier]

Am 06.02.2025 um 16:26 schrieb Richard Purdie:
> On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Use localpath variable instead of localfile and DL_DIR variable.
>>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>> ---
>>
>>   lib/bb/fetch2/az.py   | 3 +--
>>   lib/bb/fetch2/repo.py | 2 +-
>>   lib/bb/fetch2/sftp.py | 3 +--
>>   lib/bb/fetch2/wget.py | 2 +-
>>   4 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
>> index 346124a8b..1f0f1337a 100644
>> --- a/lib/bb/fetch2/az.py
>> +++ b/lib/bb/fetch2/az.py
>> @@ -49,8 +49,7 @@ class Az(Wget):
>>           fetchcmd = self.basecmd + ' --retry-connrefused --waitretry=5'
>>   
>>           # We need to provide a localpath to avoid wget using the SAS
>> -        # ud.localfile either has the downloadfilename or ud.path
>> -        localpath = os.path.join(d.getVar("DL_DIR"), ud.localfile)
>> +        localpath = ud.localpath
>>           bb.utils.mkdirhier(os.path.dirname(localpath))
>>           fetchcmd += " -O %s" % shlex.quote(localpath)
>>   
>> diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py
>> index fa4cb8149..5c0edf1f2 100644
>> --- a/lib/bb/fetch2/repo.py
>> +++ b/lib/bb/fetch2/repo.py
>> @@ -46,7 +46,7 @@ class Repo(FetchMethod):
>>       def download(self, ud, d):
>>           """Fetch url"""
>>   
>> -        if os.access(os.path.join(d.getVar("DL_DIR"), ud.localfile), os.R_OK):
>> +        if os.access(ud.localpath, os.R_OK):
>>               logger.debug("%s already exists (or was stashed). Skipping repo init / sync.", ud.localpath)
>>               return
>>   
>> diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
>> index 2a2a70a1b..b88dc5a28 100644
>> --- a/lib/bb/fetch2/sftp.py
>> +++ b/lib/bb/fetch2/sftp.py
>> @@ -82,8 +82,7 @@ class SFTP(FetchMethod):
>>               port = '-P %d' % urlo.port
>>               urlo.port = None
>>   
>> -        dldir = d.getVar('DL_DIR')
>> -        lpath = os.path.join(dldir, ud.localfile)
>> +        lpath = ud.localpath
>>   
>>           user = ''
>>           if urlo.userinfo:
>> diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
>> index 161c66bea..1194f0e7a 100644
>> --- a/lib/bb/fetch2/wget.py
>> +++ b/lib/bb/fetch2/wget.py
>> @@ -95,7 +95,7 @@ class Wget(FetchMethod):
>>           fetchcmd = self.basecmd
>>   
>>           dldir = os.path.realpath(d.getVar("DL_DIR"))
>> -        localpath = os.path.join(dldir, ud.localfile) + ".tmp"
>> +        localpath = ud.localpath + ".tmp"
>>           bb.utils.mkdirhier(os.path.dirname(localpath))
>>           fetchcmd += " -O %s" % shlex.quote(localpath)
>>   
>>
>
> I put the first 8 patches of this series into a test branch and ran it
> against the autobuilder. It fails to parse, unable to find files :(.

Thanks for the test.

> https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/stdio
> https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/errors
>
> That suggests there is some breaking change in here unfortunately.

I assume the @ in the filename is interpreter as username separator . I 
will look into it and add a test to the selftest.

Re: [bitbake-devel] [RFC PATCH 08/15] fetch2: use localpath instead of localfile

[Stefan Herbrechtsmeier]

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


Am 06.02.2025 um 17:21 schrieb Stefan Herbrechtsmeier via 
lists.openembedded.org:
> Am 06.02.2025 um 16:26 schrieb Richard Purdie:
>> On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via 
>> lists.openembedded.org wrote:
>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>
>>> Use localpath variable instead of localfile and DL_DIR variable.
>>>
>>> Signed-off-by: Stefan Herbrechtsmeier 
>>> <stefan.herbrechtsmeier@weidmueller.com>
>>> ---
>>>
>>>   lib/bb/fetch2/az.py   | 3 +--
>>>   lib/bb/fetch2/repo.py | 2 +-
>>>   lib/bb/fetch2/sftp.py | 3 +--
>>>   lib/bb/fetch2/wget.py | 2 +-
>>>   4 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
>>> index 346124a8b..1f0f1337a 100644
>>> --- a/lib/bb/fetch2/az.py
>>> +++ b/lib/bb/fetch2/az.py
>>> @@ -49,8 +49,7 @@ class Az(Wget):
>>>           fetchcmd = self.basecmd + ' --retry-connrefused 
>>> --waitretry=5'
>>>             # We need to provide a localpath to avoid wget using the 
>>> SAS
>>> -        # ud.localfile either has the downloadfilename or ud.path
>>> -        localpath = os.path.join(d.getVar("DL_DIR"), ud.localfile)
>>> +        localpath = ud.localpath
>>>           bb.utils.mkdirhier(os.path.dirname(localpath))
>>>           fetchcmd += " -O %s" % shlex.quote(localpath)
>>>   diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py
>>> index fa4cb8149..5c0edf1f2 100644
>>> --- a/lib/bb/fetch2/repo.py
>>> +++ b/lib/bb/fetch2/repo.py
>>> @@ -46,7 +46,7 @@ class Repo(FetchMethod):
>>>       def download(self, ud, d):
>>>           """Fetch url"""
>>>   -        if os.access(os.path.join(d.getVar("DL_DIR"), 
>>> ud.localfile), os.R_OK):
>>> +        if os.access(ud.localpath, os.R_OK):
>>>               logger.debug("%s already exists (or was stashed). 
>>> Skipping repo init / sync.", ud.localpath)
>>>               return
>>>   diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
>>> index 2a2a70a1b..b88dc5a28 100644
>>> --- a/lib/bb/fetch2/sftp.py
>>> +++ b/lib/bb/fetch2/sftp.py
>>> @@ -82,8 +82,7 @@ class SFTP(FetchMethod):
>>>               port = '-P %d' % urlo.port
>>>               urlo.port = None
>>>   -        dldir = d.getVar('DL_DIR')
>>> -        lpath = os.path.join(dldir, ud.localfile)
>>> +        lpath = ud.localpath
>>>             user = ''
>>>           if urlo.userinfo:
>>> diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
>>> index 161c66bea..1194f0e7a 100644
>>> --- a/lib/bb/fetch2/wget.py
>>> +++ b/lib/bb/fetch2/wget.py
>>> @@ -95,7 +95,7 @@ class Wget(FetchMethod):
>>>           fetchcmd = self.basecmd
>>>             dldir = os.path.realpath(d.getVar("DL_DIR"))
>>> -        localpath = os.path.join(dldir, ud.localfile) + ".tmp"
>>> +        localpath = ud.localpath + ".tmp"
>>>           bb.utils.mkdirhier(os.path.dirname(localpath))
>>>           fetchcmd += " -O %s" % shlex.quote(localpath)
>>>
>>
>> I put the first 8 patches of this series into a test branch and ran it
>> against the autobuilder. It fails to parse, unable to find files :(.
>
> Thanks for the test.
>
>> https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/stdio 
>>
>> https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/errors 
>>
>>
>> That suggests there is some breaking change in here unfortunately.
>
> I assume the @ in the filename is interpreter as username separator . 
> I will look into it and add a test to the selftest.

We have two solutions to decode an URI with different behavior:
class URI - urllib.parse.urlparse(uri)
def decodeurl - re.compile(...).match(uri)

Additionally the encode of an URI object and the encodeuri function have 
different behavior. The class use the unquoted and the function the 
quoted path. Is this behavior intended or could we use one implementation?

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

Re: [bitbake-devel] [RFC PATCH 08/15] fetch2: use localpath instead of localfile

[Richard Purdie]

On Fri, 2025-02-07 at 09:05 +0100, Stefan Herbrechtsmeier wrote:
>  
> 
>  
>  
> Am 06.02.2025 um 17:21 schrieb Stefan Herbrechtsmeier via
> lists.openembedded.org:
>  
>  
> > Am 06.02.2025 um 16:26 schrieb Richard Purdie: 
> >  
> > > On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via
> > > lists.openembedded.org wrote: 
> > >  
> > > > From: Stefan Herbrechtsmeier
> > > > <stefan.herbrechtsmeier@weidmueller.com> 
> > > >  
> > > >  Use localpath variable instead of localfile and DL_DIR
> > > > variable. 
> > > >  
> > > >  Signed-off-by: Stefan Herbrechtsmeier
> > > > <stefan.herbrechtsmeier@weidmueller.com> 
> > > >  --- 
> > > >  
> > > >    lib/bb/fetch2/az.py   | 3 +-- 
> > > >    lib/bb/fetch2/repo.py | 2 +- 
> > > >    lib/bb/fetch2/sftp.py | 3 +-- 
> > > >    lib/bb/fetch2/wget.py | 2 +- 
> > > >    4 files changed, 4 insertions(+), 6 deletions(-) 
> > > >  
> > > >  diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py 
> > > >  index 346124a8b..1f0f1337a 100644 
> > > >  --- a/lib/bb/fetch2/az.py 
> > > >  +++ b/lib/bb/fetch2/az.py 
> > > >  @@ -49,8 +49,7 @@ class Az(Wget): 
> > > >            fetchcmd = self.basecmd + ' --retry-connrefused --
> > > > waitretry=5' 
> > > >              # We need to provide a localpath to avoid wget
> > > > using the SAS 
> > > >  -        # ud.localfile either has the downloadfilename or
> > > > ud.path 
> > > >  -        localpath = os.path.join(d.getVar("DL_DIR"),
> > > > ud.localfile) 
> > > >  +        localpath = ud.localpath 
> > > >            bb.utils.mkdirhier(os.path.dirname(localpath)) 
> > > >            fetchcmd += " -O %s" % shlex.quote(localpath) 
> > > >    diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py 
> > > >  index fa4cb8149..5c0edf1f2 100644 
> > > >  --- a/lib/bb/fetch2/repo.py 
> > > >  +++ b/lib/bb/fetch2/repo.py 
> > > >  @@ -46,7 +46,7 @@ class Repo(FetchMethod): 
> > > >        def download(self, ud, d): 
> > > >            """Fetch url""" 
> > > >    -        if os.access(os.path.join(d.getVar("DL_DIR"),
> > > > ud.localfile), os.R_OK): 
> > > >  +        if os.access(ud.localpath, os.R_OK): 
> > > >                logger.debug("%s already exists (or was
> > > > stashed). Skipping repo init / sync.", ud.localpath) 
> > > >                return 
> > > >    diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py 
> > > >  index 2a2a70a1b..b88dc5a28 100644 
> > > >  --- a/lib/bb/fetch2/sftp.py 
> > > >  +++ b/lib/bb/fetch2/sftp.py 
> > > >  @@ -82,8 +82,7 @@ class SFTP(FetchMethod): 
> > > >                port = '-P %d' % urlo.port 
> > > >                urlo.port = None 
> > > >    -        dldir = d.getVar('DL_DIR') 
> > > >  -        lpath = os.path.join(dldir, ud.localfile) 
> > > >  +        lpath = ud.localpath 
> > > >              user = '' 
> > > >            if urlo.userinfo: 
> > > >  diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py 
> > > >  index 161c66bea..1194f0e7a 100644 
> > > >  --- a/lib/bb/fetch2/wget.py 
> > > >  +++ b/lib/bb/fetch2/wget.py 
> > > >  @@ -95,7 +95,7 @@ class Wget(FetchMethod): 
> > > >            fetchcmd = self.basecmd 
> > > >              dldir = os.path.realpath(d.getVar("DL_DIR")) 
> > > >  -        localpath = os.path.join(dldir, ud.localfile) +
> > > > ".tmp" 
> > > >  +        localpath = ud.localpath + ".tmp" 
> > > >            bb.utils.mkdirhier(os.path.dirname(localpath)) 
> > > >            fetchcmd += " -O %s" % shlex.quote(localpath) 
> > > >    
> > > >  
> > >  
> > >  I put the first 8 patches of this series into a test branch and
> > > ran it 
> > >  against the autobuilder. It fails to parse, unable to find files
> > > :(. 
> > >  
> >  
> >  Thanks for the test. 
> >  
> >  
> > > https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/stdio
> > > https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/errors
> > >  
> > >  That suggests there is some breaking change in here
> > > unfortunately. 
> > >  
> >  
> >  I assume the @ in the filename is interpreter as username
> > separator . I will look into it and add a test to the selftest.
>  
> We have two solutions to decode an URI with different behavior:
>  class URI - urllib.parse.urlparse(uri)
>  def decodeurl - re.compile(...).match(uri)


decodeurl is older and predates urllib existing. People have tried to
switch things over and use urllib where possible but as you say, the
behaviour is different. I don't remember what the differences are.

>   Additionally the encode of an URI object and the encodeuri function
> have different behavior. The class use the unquoted and the function
> the quoted path. Is this behavior intended or could we use one
> implementation?


I suspect we've been preserving the old behaviour so it was intended if
a bit strange. It is hard to comment on what we could do without an
idea of what it would break and what the differences are.

Cheers,

Richard

Re: [bitbake-devel] [RFC PATCH 08/15] fetch2: use localpath instead of localfile

[Stefan Herbrechtsmeier]

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

Am 07.02.2025 um 09:10 schrieb Richard Purdie:
> On Fri, 2025-02-07 at 09:05 +0100, Stefan Herbrechtsmeier wrote:
>> Am 06.02.2025 um 17:21 schrieb Stefan Herbrechtsmeier via
>> lists.openembedded.org:
>>   
>>   
>>> Am 06.02.2025 um 16:26 schrieb Richard Purdie:
>>>   
>>>> On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via
>>>> lists.openembedded.org wrote:
>>>>   
>>>>> From: Stefan Herbrechtsmeier
>>>>> <stefan.herbrechtsmeier@weidmueller.com> 
>>>>>   
>>>>>   Use localpath variable instead of localfile and DL_DIR
>>>>> variable.
>>>>>   
>>>>>   Signed-off-by: Stefan Herbrechtsmeier
>>>>> <stefan.herbrechtsmeier@weidmueller.com> 
>>>>>   ---
>>>>>   
>>>>>     lib/bb/fetch2/az.py   | 3 +--
>>>>>     lib/bb/fetch2/repo.py | 2 +-
>>>>>     lib/bb/fetch2/sftp.py | 3 +--
>>>>>     lib/bb/fetch2/wget.py | 2 +-
>>>>>     4 files changed, 4 insertions(+), 6 deletions(-)
>>>>>   
>>>>>   diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
>>>>>   index 346124a8b..1f0f1337a 100644
>>>>>   --- a/lib/bb/fetch2/az.py
>>>>>   +++ b/lib/bb/fetch2/az.py
>>>>>   @@ -49,8 +49,7 @@ class Az(Wget):
>>>>>             fetchcmd = self.basecmd + ' --retry-connrefused --
>>>>> waitretry=5'
>>>>>               # We need to provide a localpath to avoid wget
>>>>> using the SAS
>>>>>   -        # ud.localfile either has the downloadfilename or
>>>>> ud.path
>>>>>   -        localpath = os.path.join(d.getVar("DL_DIR"),
>>>>> ud.localfile)
>>>>>   +        localpath = ud.localpath
>>>>>             bb.utils.mkdirhier(os.path.dirname(localpath))
>>>>>             fetchcmd += " -O %s" % shlex.quote(localpath)
>>>>>     diff --git a/lib/bb/fetch2/repo.py b/lib/bb/fetch2/repo.py
>>>>>   index fa4cb8149..5c0edf1f2 100644
>>>>>   --- a/lib/bb/fetch2/repo.py
>>>>>   +++ b/lib/bb/fetch2/repo.py
>>>>>   @@ -46,7 +46,7 @@ class Repo(FetchMethod):
>>>>>         def download(self, ud, d):
>>>>>             """Fetch url"""
>>>>>     -        if os.access(os.path.join(d.getVar("DL_DIR"),
>>>>> ud.localfile), os.R_OK):
>>>>>   +        if os.access(ud.localpath, os.R_OK):
>>>>>                 logger.debug("%s already exists (or was
>>>>> stashed). Skipping repo init / sync.", ud.localpath)
>>>>>                 return
>>>>>     diff --git a/lib/bb/fetch2/sftp.py b/lib/bb/fetch2/sftp.py
>>>>>   index 2a2a70a1b..b88dc5a28 100644
>>>>>   --- a/lib/bb/fetch2/sftp.py
>>>>>   +++ b/lib/bb/fetch2/sftp.py
>>>>>   @@ -82,8 +82,7 @@ class SFTP(FetchMethod):
>>>>>                 port = '-P %d' % urlo.port
>>>>>                 urlo.port = None
>>>>>     -        dldir = d.getVar('DL_DIR')
>>>>>   -        lpath = os.path.join(dldir, ud.localfile)
>>>>>   +        lpath = ud.localpath
>>>>>               user = ''
>>>>>             if urlo.userinfo:
>>>>>   diff --git a/lib/bb/fetch2/wget.py b/lib/bb/fetch2/wget.py
>>>>>   index 161c66bea..1194f0e7a 100644
>>>>>   --- a/lib/bb/fetch2/wget.py
>>>>>   +++ b/lib/bb/fetch2/wget.py
>>>>>   @@ -95,7 +95,7 @@ class Wget(FetchMethod):
>>>>>             fetchcmd = self.basecmd
>>>>>               dldir = os.path.realpath(d.getVar("DL_DIR"))
>>>>>   -        localpath = os.path.join(dldir, ud.localfile) +
>>>>> ".tmp"
>>>>>   +        localpath = ud.localpath + ".tmp"
>>>>>             bb.utils.mkdirhier(os.path.dirname(localpath))
>>>>>             fetchcmd += " -O %s" % shlex.quote(localpath)
>>>>>     
>>>>>   
>>>>   
>>>>   I put the first 8 patches of this series into a test branch and
>>>> ran it
>>>>   against the autobuilder. It fails to parse, unable to find files
>>>> :(.
>>>>   
>>>   
>>>   Thanks for the test.
>>>   
>>>   
>>>> https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/stdio
>>>> https://autobuilder.yoctoproject.org/valkyrie/#/builders/29/builds/960/steps/13/logs/errors
>>>>   
>>>>   That suggests there is some breaking change in here
>>>> unfortunately.
>>>>   
>>>   
>>>   I assume the @ in the filename is interpreter as username
>>> separator . I will look into it and add a test to the selftest.
>>   
>> We have two solutions to decode an URI with different behavior:
>>   class URI - urllib.parse.urlparse(uri)
>>   def decodeurl - re.compile(...).match(uri)
>
> decodeurl is older and predates urllib existing. People have tried to
> switch things over and use urllib where possible but as you say, the
> behaviour is different. I don't remember what the differences are.
>
>>    Additionally the encode of an URI object and the encodeuri function
>> have different behavior. The class use the unquoted and the function
>> the quoted path. Is this behavior intended or could we use one
>> implementation?
>
> I suspect we've been preserving the old behaviour so it was intended if
> a bit strange. It is hard to comment on what we could do without an
> idea of what it would break and what the differences are.

I have add a patch to fix the problem and an optimization to remove 
duplicated code.

Is it possible to test the changes on the build server?

Regards
   Stefan

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

[RFC PATCH 09/15] fetch2: make DOWNLOADFILENAME and PATH explicit in mirrors

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Make the DOWNLOADFILENAME and PATH replacement explicit in the mirrors
to support upstream and download mirrors. An upstream mirror expects the
original PATH whereas a download mirror requires the DOWNLOADFILENAME.

Don’t replace the last part of the PATH with the basename of the
downloaded filename. The downloadfilename parameter may contain
subfolders and a mirror may require the original PATH.

Add a preprocessing of the mirror replacement to support common
inexplicit use cases.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/fetch2/__init__.py | 41 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 2328a8fe3..7ec1b4b20 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -429,6 +429,21 @@ def uri_replace(ud, uri_find, uri_replace, replacements, d, mirrortarball=None):
     uri_decoded = list(decodeurl(ud.url))
     uri_find_decoded = list(decodeurl(uri_find))
     uri_replace_decoded = list(decodeurl(uri_replace))
+
+    # Fix mirror lines without PATH, BASENAME or DOWNLOADFILENAME
+    if (uri_find_decoded[0] == "file" and ".*" in uri_find_decoded[2]
+            and all({k not in uri_replace_decoded[2]
+                     for k in {"PATH", "BASENAME"}})):
+        uri_replace_decoded[2] = os.path.join(uri_replace_decoded[2], "PATH")
+    elif ((uri_replace_decoded[0].startswith("http")
+                or uri_replace_decoded[0] == "file")
+            and ((uri_decoded[0] != uri_replace_decoded[0] and mirrortarball)
+                    or ".*" in uri_find_decoded[2])
+            and all({k not in uri_replace_decoded[2]
+                     for k in {"PATH", "BASENAME", "DOWNLOADFILENAME"}})):
+        uri_replace_decoded[2] = os.path.join(uri_replace_decoded[2],
+                                              "DOWNLOADFILENAME")
+
     logger.debug2("For url %s comparing %s to %s" % (uri_decoded, uri_find_decoded, uri_replace_decoded))
     result_decoded = ['', '', '', '', '', {}]
     # 0 - type, 1 - host, 2 - path, 3 - user,  4- pswd, 5 - params
@@ -441,7 +456,10 @@ def uri_replace(ud, uri_find, uri_replace, replacements, d, mirrortarball=None):
             regexp += "$"
         if loc == 5:
             # Handle URL parameters
-            if i:
+            if uri_decoded[0] != uri_replace_decoded[0] and mirrortarball:
+                # Kill parameters, they make no sense for mirror tarballs
+                result_decoded[5] = {}
+            elif i:
                 # Any specified URL parameters must match
                 for k in uri_find_decoded[loc]:
                     if uri_decoded[loc][k] != uri_find_decoded[loc][k]:
@@ -462,26 +480,6 @@ def uri_replace(ud, uri_find, uri_replace, replacements, d, mirrortarball=None):
                     uri_replace_decoded[loc] = uri_replace_decoded[loc].replace(k, replacements[k])
                 #bb.note("%s %s %s" % (regexp, uri_replace_decoded[loc], uri_decoded[loc]))
                 result_decoded[loc] = re.sub(regexp, uri_replace_decoded[loc], uri_decoded[loc], count=1)
-            if loc == 2:
-                # Handle path manipulations
-                basename = None
-                if uri_decoded[0] != uri_replace_decoded[0] and mirrortarball:
-                    # If the source and destination url types differ, must be a mirrortarball mapping
-                    basename = os.path.basename(mirrortarball)
-                    # Kill parameters, they make no sense for mirror tarballs
-                    uri_decoded[5] = {}
-                    uri_find_decoded[5] = {}
-                elif ud.localpath and ud.method.supports_checksum(ud):
-                    basename = os.path.basename(ud.localpath)
-                if basename:
-                    uri_basename = os.path.basename(uri_decoded[loc])
-                    # Prefix with a slash as a sentinel in case
-                    # result_decoded[loc] does not contain one.
-                    path = "/" + result_decoded[loc]
-                    if uri_basename and basename != uri_basename and path.endswith("/" + uri_basename):
-                        result_decoded[loc] = path[1:-len(uri_basename)] + basename
-                    elif not path.endswith("/" + basename):
-                        result_decoded[loc] = os.path.join(path[1:], basename)
         else:
             return None
     result = encodeurl(result_decoded)
@@ -1000,6 +998,7 @@ def build_mirroruris(origud, mirrors, ld):
                 continue
 
             for tarball in tarballs:
+                replacements["DOWNLOADFILENAME"] = tarball or origud.localfile
                 newuri = uri_replace(ud, find, replace, replacements, ld, tarball)
                 if not newuri or newuri in uris or newuri == origud.url:
                     continue
-- 
2.39.5

[RFC PATCH 10/15] tests: fetch: unify style of replaceuris in MirrorUriTest

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/tests/fetch.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index d64717011..dbb787a7d 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -497,15 +497,18 @@ class MirrorUriTest(FetcherTest):
             : "git://user2@git.openembedded.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master;protocol=http",
         ("git://someserver.org/bitbake;tag=1234567890123456789012345678901234567890;protocol=git;branch=master", "git://someserver.org/bitbake", "git://someotherserver.org/bitbake;protocol=https")
             : "git://someotherserver.org/bitbake;tag=1234567890123456789012345678901234567890;protocol=https;branch=master",
-        ("gitsm://git.qemu.org/git/seabios.git/;protocol=https;name=roms/seabios;subpath=roms/seabios;bareclone=1;nobranch=1;rev=1234567890123456789012345678901234567890", "gitsm://.*/.*", "http://petalinux.xilinx.com/sswreleases/rel-v${XILINX_VER_MAIN}/downloads") : "http://petalinux.xilinx.com/sswreleases/rel-v%24%7BXILINX_VER_MAIN%7D/downloads/git2_git.qemu.org.git.seabios.git..tar.gz",
+        ("gitsm://git.qemu.org/git/seabios.git/;protocol=https;name=roms/seabios;subpath=roms/seabios;bareclone=1;nobranch=1;rev=1234567890123456789012345678901234567890", "gitsm://.*/.*", "http://petalinux.xilinx.com/sswreleases/rel-v${XILINX_VER_MAIN}/downloads")
+            : "http://petalinux.xilinx.com/sswreleases/rel-v%24%7BXILINX_VER_MAIN%7D/downloads/git2_git.qemu.org.git.seabios.git..tar.gz",
         ("https://somewhere.org/example/1.0.0/example;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/PATH")
             : "file:///mirror/example/1.0.0/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
         ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/some-example-1.0.0.tgz")
             : "file:///mirror/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
 
         #Renaming files doesn't work
-        #("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_2.3.4.tar.gz") : "http://somewhere2.org/somedir3/somefile_2.3.4.tar.gz"
-        #("file://sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache") : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
+        #("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_2.3.4.tar.gz")
+        #    : "http://somewhere2.org/somedir3/somefile_2.3.4.tar.gz"
+        #("file://sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache")
+        #    : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
     }
 
     mirrorvar = "http://.*/.* file:///somepath/downloads/ " \
-- 
2.39.5

[RFC PATCH 11/15] tests: fetch: fix nonsensical replaceuris in MirrorUriTest

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

A replacement inside a mirror URI with a PATH replacement should retain
the original path and shouldn't consider the downloadfilename parameter.

A replacement inside a mirror URI of any path should retain the
downloadfilename and shouldn't contain a single file.

A replacement inside a mirror URI for a folder requires a folder in the tested URI.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/tests/fetch.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index dbb787a7d..ec6d1c039 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -500,15 +500,15 @@ class MirrorUriTest(FetcherTest):
         ("gitsm://git.qemu.org/git/seabios.git/;protocol=https;name=roms/seabios;subpath=roms/seabios;bareclone=1;nobranch=1;rev=1234567890123456789012345678901234567890", "gitsm://.*/.*", "http://petalinux.xilinx.com/sswreleases/rel-v${XILINX_VER_MAIN}/downloads")
             : "http://petalinux.xilinx.com/sswreleases/rel-v%24%7BXILINX_VER_MAIN%7D/downloads/git2_git.qemu.org.git.seabios.git..tar.gz",
         ("https://somewhere.org/example/1.0.0/example;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/PATH")
-            : "file:///mirror/example/1.0.0/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
-        ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/some-example-1.0.0.tgz")
+            : "file:///mirror/example/1.0.0/example;downloadfilename=some-example-1.0.0.tgz",
+        ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/")
             : "file:///mirror/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
 
         #Renaming files doesn't work
         #("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_2.3.4.tar.gz")
         #    : "http://somewhere2.org/somedir3/somefile_2.3.4.tar.gz"
-        #("file://sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache")
-        #    : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
+        #("file://somedir/sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache")
+        #    : "file:///somewhere/1234/sstate-cache/somedir/sstate-xyz.tgz",
     }
 
     mirrorvar = "http://.*/.* file:///somepath/downloads/ " \
-- 
2.39.5

[RFC PATCH 12/15] tests: fetch: reenable replaceuris in MirrorUriTest

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

A replacement inside a mirror URI should support the rename of the PATH.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/tests/fetch.py | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index ec6d1c039..29920bd9c 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -473,6 +473,8 @@ class MirrorUriTest(FetcherTest):
             : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
         ("file://sstate-xyz.tgz", "file://.*", "file:///somewhere/1234/sstate-cache/")
             : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
+        ("file://somedir/sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache")
+            : "file:///somewhere/1234/sstate-cache/somedir/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")
@@ -491,6 +493,8 @@ class MirrorUriTest(FetcherTest):
             : "http://somewhere2.org/somefile_1.2.3.tar.gz",
         ("http://somewhere.org/somedir1/somedir2/somefile_1.2.3.tar.gz", "http://.*/.*", "http://somewhere2.org/")
             : "http://somewhere2.org/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_2.3.4.tar.gz")
+            : "http://somewhere2.org/somedir3/somefile_2.3.4.tar.gz",
         ("git://someserver.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master", "git://someserver.org/bitbake;branch=master", "git://git.openembedded.org/bitbake;protocol=http")
             : "git://git.openembedded.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master;protocol=http",
         ("git://user1@someserver.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master", "git://someserver.org/bitbake;branch=master", "git://user2@git.openembedded.org/bitbake;protocol=http")
@@ -503,12 +507,6 @@ class MirrorUriTest(FetcherTest):
             : "file:///mirror/example/1.0.0/example;downloadfilename=some-example-1.0.0.tgz",
         ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/")
             : "file:///mirror/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
-
-        #Renaming files doesn't work
-        #("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_2.3.4.tar.gz")
-        #    : "http://somewhere2.org/somedir3/somefile_2.3.4.tar.gz"
-        #("file://somedir/sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache")
-        #    : "file:///somewhere/1234/sstate-cache/somedir/sstate-xyz.tgz",
     }
 
     mirrorvar = "http://.*/.* file:///somepath/downloads/ " \
-- 
2.39.5

[RFC PATCH 13/15] tests: fetch: comment and add replaceuris in MirrorUriTest

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Comment the replaceuris in MirrorUriTest accordingly the test cases.

Add additional tests:
* Explicit PATH and DOWNLOADFILENAME replacement
* Rename of server and path
* Rename of downloaded filename
* Subfolder in downloadfilename parameter

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/tests/fetch.py | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 29920bd9c..37de4d3e8 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -459,8 +459,12 @@ class FetcherTest(unittest.TestCase):
 class MirrorUriTest(FetcherTest):
 
     replaceuris = {
+        # remote download mirror for git
         ("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",
+        ("git://git.invalid.infradead.org/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/.*", "http://somewhere.org/somedir/DOWNLOADFILENAME")
+            : "http://somewhere.org/somedir/git2_git.invalid.infradead.org.mtd-utils.git.tar.gz",
+        # remote git upstream mirror
         ("git://git.invalid.infradead.org/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/([^/]+/)*([^/]*)", "git://somewhere.org/somedir/\\2;protocol=http")
             : "git://somewhere.org/somedir/mtd-utils.git;tag=1234567890123456789012345678901234567890;protocol=http",
         ("git://git.invalid.infradead.org/foo/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/([^/]+/)*([^/]*)", "git://somewhere.org/somedir/\\2;protocol=http")
@@ -469,44 +473,77 @@ class MirrorUriTest(FetcherTest):
             : "git://somewhere.org/mtd-utils.git;tag=1234567890123456789012345678901234567890;protocol=http",
         ("git://someserver.org/bitbake;tag=1234567890123456789012345678901234567890", "git://someserver.org/bitbake", "git://git.openembedded.org/bitbake")
             : "git://git.openembedded.org/bitbake;tag=1234567890123456789012345678901234567890",
+        # local state mirror for file
         ("file://sstate-xyz.tgz", "file://.*", "file:///somewhere/1234/sstate-cache")
             : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
         ("file://sstate-xyz.tgz", "file://.*", "file:///somewhere/1234/sstate-cache/")
             : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
+        ("file://sstate-xyz.tgz", "file://.*", "file:///somewhere/1234/sstate-cache/PATH")
+            : "file:///somewhere/1234/sstate-cache/sstate-xyz.tgz",
         ("file://somedir/sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache")
             : "file:///somewhere/1234/sstate-cache/somedir/sstate-xyz.tgz",
+        ("file://somedir/sstate-xyz.tgz", "file://.*/.*", "file:///somewhere/1234/sstate-cache/PATH")
+            : "file:///somewhere/1234/sstate-cache/somedir/sstate-xyz.tgz",
+        # remote download mirror for http
         ("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/somedir2/somefile_1.2.3.tar.gz", "http://.*/.*", "http://somewhere2.org/somedir3/DOWNLOADFILENAME")
+            : "http://somewhere2.org/somedir3/somefile_1.2.3.tar.gz",
+        # remote upstream mirror for http with rename of server and path expect basename
         ("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",
+        # remote upstream mirror for http with rename of server and path inclusive basename
+        ("http://somewhere.org/somedir1/somefile_1.2.3.tar.gz", "http://somewhere.org/somedir1/somefile_1.2.3.tar.gz", "http://somewhere2.org/somedir3/somefile4_1.2.3.tar.gz")
+            : "http://somewhere2.org/somedir3/somefile4_1.2.3.tar.gz",
+        # remote upstream mirror
         ("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",
+        # local download mirror for http
         ("http://www.apache.org/dist/subversion/subversion-1.7.1.tar.bz2", "http://.*/.*", "file:///somepath/downloads/")
             : "file:///somepath/downloads/subversion-1.7.1.tar.bz2",
+        ("http://www.apache.org/dist/subversion/subversion-1.7.1.tar.bz2", "http://.*/.*", "file:///somepath/downloads/DOWNLOADFILENAME")
+            : "file:///somepath/downloads/subversion-1.7.1.tar.bz2",
+        # remote upstream mirror for git with BASENAME
         ("git://git.invalid.infradead.org/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/.*", "git://somewhere.org/somedir/BASENAME;protocol=http")
             : "git://somewhere.org/somedir/mtd-utils.git;tag=1234567890123456789012345678901234567890;protocol=http",
         ("git://git.invalid.infradead.org/foo/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/.*", "git://somewhere.org/somedir/BASENAME;protocol=http")
             : "git://somewhere.org/somedir/mtd-utils.git;tag=1234567890123456789012345678901234567890;protocol=http",
+        # remote upstream mirror for git with MIRRORNAME
         ("git://git.invalid.infradead.org/foo/mtd-utils.git;tag=1234567890123456789012345678901234567890", "git://.*/.*", "git://somewhere.org/somedir/MIRRORNAME;protocol=http")
             : "git://somewhere.org/somedir/git.invalid.infradead.org.foo.mtd-utils.git;tag=1234567890123456789012345678901234567890;protocol=http",
+        # remote download mirror for http
         ("http://somewhere.org/somedir1/somedir2/somefile_1.2.3.tar.gz", "http://.*/.*", "http://somewhere2.org")
             : "http://somewhere2.org/somefile_1.2.3.tar.gz",
         ("http://somewhere.org/somedir1/somedir2/somefile_1.2.3.tar.gz", "http://.*/.*", "http://somewhere2.org/")
             : "http://somewhere2.org/somefile_1.2.3.tar.gz",
+        ("http://somewhere.org/somedir1/somedir2/somefile_1.2.3.tar.gz", "http://.*/.*", "http://somewhere2.org/DOWNLOADFILENAME")
+            : "http://somewhere2.org/somefile_1.2.3.tar.gz",
+        # remote upstream mirror with rename of server and path inclusive basename
         ("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_2.3.4.tar.gz")
             : "http://somewhere2.org/somedir3/somefile_2.3.4.tar.gz",
+        # remote upstream mirror for git with other protocol
         ("git://someserver.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master", "git://someserver.org/bitbake;branch=master", "git://git.openembedded.org/bitbake;protocol=http")
             : "git://git.openembedded.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master;protocol=http",
         ("git://user1@someserver.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master", "git://someserver.org/bitbake;branch=master", "git://user2@git.openembedded.org/bitbake;protocol=http")
             : "git://user2@git.openembedded.org/bitbake;tag=1234567890123456789012345678901234567890;branch=master;protocol=http",
         ("git://someserver.org/bitbake;tag=1234567890123456789012345678901234567890;protocol=git;branch=master", "git://someserver.org/bitbake", "git://someotherserver.org/bitbake;protocol=https")
             : "git://someotherserver.org/bitbake;tag=1234567890123456789012345678901234567890;protocol=https;branch=master",
+        # remote download mirror for gitsm
         ("gitsm://git.qemu.org/git/seabios.git/;protocol=https;name=roms/seabios;subpath=roms/seabios;bareclone=1;nobranch=1;rev=1234567890123456789012345678901234567890", "gitsm://.*/.*", "http://petalinux.xilinx.com/sswreleases/rel-v${XILINX_VER_MAIN}/downloads")
             : "http://petalinux.xilinx.com/sswreleases/rel-v%24%7BXILINX_VER_MAIN%7D/downloads/git2_git.qemu.org.git.seabios.git..tar.gz",
+        ("gitsm://git.qemu.org/git/seabios.git/;protocol=https;name=roms/seabios;subpath=roms/seabios;bareclone=1;nobranch=1;rev=1234567890123456789012345678901234567890", "gitsm://.*/.*", "http://petalinux.xilinx.com/sswreleases/rel-v${XILINX_VER_MAIN}/downloads/DOWNLOADFILENAME")
+            : "http://petalinux.xilinx.com/sswreleases/rel-v%24%7BXILINX_VER_MAIN%7D/downloads/git2_git.qemu.org.git.seabios.git..tar.gz",
+        # local download mirror for https
         ("https://somewhere.org/example/1.0.0/example;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/PATH")
             : "file:///mirror/example/1.0.0/example;downloadfilename=some-example-1.0.0.tgz",
         ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/")
             : "file:///mirror/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
+        # local mirror for https with rename of download filename
+        ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/DOWNLOADFILENAME")
+            : "file:///mirror/some-example-1.0.0.tgz;downloadfilename=some-example-1.0.0.tgz",
+        # local mirror for https with subfolder in downloadfilename
+        ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=subdir/some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/sub")
+            : "file:///mirror/sub/subdir/some-example-1.0.0.tgz;downloadfilename=subdir/some-example-1.0.0.tgz",
     }
 
     mirrorvar = "http://.*/.* file:///somepath/downloads/ " \
-- 
2.39.5

[RFC PATCH 14/15] tests: fetch: add npm mirrors replaceuris in MirrorUriTest

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add replaceuris in MirrorUriTest for a npm upstream and download mirror.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/tests/fetch.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 37de4d3e8..5d247f6a9 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -544,6 +544,12 @@ class MirrorUriTest(FetcherTest):
         # local mirror for https with subfolder in downloadfilename
         ("https://somewhere.org/example-1.0.0.tgz;downloadfilename=subdir/some-example-1.0.0.tgz", "https://.*/.*", "file:///mirror/sub")
             : "file:///mirror/sub/subdir/some-example-1.0.0.tgz;downloadfilename=subdir/some-example-1.0.0.tgz",
+        # NPM Upstream Mirror
+        ("https://registry.npmjs.org/@node-rs/bcrypt/-/bcrypt-1.10.4.tgz;downloadfilename=npm2/@node-rs-bcrypt-1.10.4.tgz", "https://registry.npmjs.org/", "https://example.com/npm/")
+            : "https://example.com/npm/%40node-rs/bcrypt/-/bcrypt-1.10.4.tgz;downloadfilename=npm2/@node-rs-bcrypt-1.10.4.tgz",
+        # NPM Download Mirror
+        ("https://registry.npmjs.org/@node-rs/bcrypt/-/bcrypt-1.10.4.tgz;downloadfilename=npm2/@node-rs-bcrypt-1.10.4.tgz", "https://registry.npmjs.org/.*", "file:///downloads/DOWNLOADFILENAME")
+            : "file:///downloads/npm2/%40node-rs-bcrypt-1.10.4.tgz;downloadfilename=npm2/@node-rs-bcrypt-1.10.4.tgz",
     }
 
     mirrorvar = "http://.*/.* file:///somepath/downloads/ " \
-- 
2.39.5

[RFC PATCH 15/15] tests: fetch: fix nonsensical mirror uris in FetcherNetworkTest

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

A replacement inside a mirror URI for a local path should use a plain
replacement and shouldn’t use a placeholder without PATH or BASENAME
replacement.

A replacement inside a mirror URI for a remote path should use a
placeholder to match the whole path to replace it and shouldn’t depend
on the implementation which wrongly replace the base name with the base
name of the downloadfilename.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

 lib/bb/tests/fetch.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 5d247f6a9..3d8b932ac 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -1027,7 +1027,7 @@ class FetcherNetworkTest(FetcherTest):
     @skipIfNoNetwork()
     def test_fetch_file_mirror_of_mirror(self):
         self.d.setVar("FILESPATH", ".")
-        self.d.setVar("MIRRORS", "http://.*/.* file:///some1where/ file:///some1where/.* file://some2where/ file://some2where/.* https://downloads.yoctoproject.org/releases/bitbake")
+        self.d.setVar("MIRRORS", "http://.*/.* file:///some1where/ file:///some1where/ file://some2where/ file://some2where/ https://downloads.yoctoproject.org/releases/bitbake/")
         fetcher = bb.fetch.Fetch(["http://invalid.yoctoproject.org/releases/bitbake/bitbake-1.0.tar.gz"], self.d)
         os.mkdir(self.dldir + "/some2where")
         fetcher.download()
@@ -1056,7 +1056,7 @@ class FetcherNetworkTest(FetcherTest):
     @skipIfNoNetwork()
     # BZ13039
     def test_fetch_premirror_specify_downloadfilename_specific_uri(self):
-        self.d.setVar("PREMIRRORS", "http://invalid.yoctoproject.org/releases/bitbake https://downloads.yoctoproject.org/releases/bitbake")
+        self.d.setVar("PREMIRRORS", "http://invalid.yoctoproject.org/releases/bitbake/.* https://downloads.yoctoproject.org/releases/bitbake")
         fetcher = bb.fetch.Fetch(["http://invalid.yoctoproject.org/releases/bitbake/1.0.tar.gz;downloadfilename=bitbake-1.0.tar.gz"], self.d)
         fetcher.download()
         self.assertEqual(os.path.getsize(self.dldir + "/bitbake-1.0.tar.gz"), 57749)
-- 
2.39.5

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Richard Purdie]

On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
> The mirror replacement syntax contains many implicit transformations.
> The path of the URI always contains the base name of the downloaded
> filename. This makes it impossible to rename or remove the base name of
> the original path. It prevents upstream mirror for SRC_URIS with a
> downloadfilename parameter. The base name of the downloaded filename
> makes it impossible to use the download mirror for SRC_URIs with
> subfolders in the downloadfilename parameter. Altogether the implicit
> transformation complicates the understanding of the replacements.
> 
> This series adds an additional replacement named DOWNLOADFILENAME. This
> replacement contains the relative filename of the downloaded file or
> mirror archive for git and hg. This allows the user to explicitly define
> the behavior. The usage is equivalent to the PATH replacement for the
> sstate mirror from file to https scheme.
> 
> git://.*/.*  http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
> https?://.*/.*  http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
> file://.*  https://sstate.yoctoproject.org/all/PATH;downloadfilename=PATH
> 
> Without a replacement variable the mirror will use the same base name as
> the origin SRC_URI. This allows the usage of private package manager
> registry together with a downloadfilename parameter or the rename of the
> base name.
> 
> https://registry.npmjs.org/  https://example.com/npm/registry/
> https://example.com/example/1.0.0.tgz  https://example.com/example/example-1.0.0.tgz
> 
> The series adds heuristics to keep a backward compatibility to common
> styles. Because of the ambiguity of the old style, it is advisable to
> remove this compatibility sooner or later to avoid unexpected behavior.

Thanks for the patches, these look interesting with some good
improvements in there. A lot of the series looks like cleanups and
those look like good fixes to have. It may make sense to split this
series into two, the cleanups/fixes and the behaviour changes.

I'm not entirely "sold" on the naming of DOWNLOADFILENAME. You have to
think about this from the perspective of someone writing a MIRROR or
PREMIRROR entry - would they understand what that means vs some of the
other names?

I'm also a bit nervous about breaking compatibility with the older
syntaxes. It is unclear to me how or when  we'd detect/deprecate older
formats. I do agree we probably do need to remove some support for some
syntax and move to something new though as what we have is turning into
some kind of nightmare. FWIW this is really old code that in many ways
predates my involvement so probably 20+ years old.

I'll continue to give this some thought. If you could confirm which
patches are "cleanup" that would help though, we can see if we can get
those in a bit faster.

Cheers,

Richard

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Stefan Herbrechtsmeier]

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

Am 05.02.2025 um 11:34 schrieb Richard Purdie:
> On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
>> The mirror replacement syntax contains many implicit transformations.
>> The path of the URI always contains the base name of the downloaded
>> filename. This makes it impossible to rename or remove the base name of
>> the original path. It prevents upstream mirror for SRC_URIS with a
>> downloadfilename parameter. The base name of the downloaded filename
>> makes it impossible to use the download mirror for SRC_URIs with
>> subfolders in the downloadfilename parameter. Altogether the implicit
>> transformation complicates the understanding of the replacements.
>>
>> This series adds an additional replacement named DOWNLOADFILENAME. This
>> replacement contains the relative filename of the downloaded file or
>> mirror archive for git and hg. This allows the user to explicitly define
>> the behavior. The usage is equivalent to the PATH replacement for the
>> sstate mirror from file to https scheme.
>>
>> git://.*/.*http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
>> https?://.*/.*http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
>> file://.*  https://sstate.yoctoproject.org/all/PATH;downloadfilename=PATH
>>
>> Without a replacement variable the mirror will use the same base name as
>> the origin SRC_URI. This allows the usage of private package manager
>> registry together with a downloadfilename parameter or the rename of the
>> base name.
>>
>> https://registry.npmjs.org/  https://example.com/npm/registry/
>> https://example.com/example/1.0.0.tgz  https://example.com/example/example-1.0.0.tgz
>>
>> The series adds heuristics to keep a backward compatibility to common
>> styles. Because of the ambiguity of the old style, it is advisable to
>> remove this compatibility sooner or later to avoid unexpected behavior.
> Thanks for the patches, these look interesting with some good
> improvements in there. A lot of the series looks like cleanups and
> those look like good fixes to have. It may make sense to split this
> series into two, the cleanups/fixes and the behaviour changes.
>
> I'm not entirely "sold" on the naming of DOWNLOADFILENAME. You have to
> think about this from the perspective of someone writing a MIRROR or
> PREMIRROR entry - would they understand what that means vs some of the
> other names?

I’m open for suggestions. Even ARCHIVE or TARBALL are hard to understand 
because it is only a relative path on the download mirror. Alternative 
we can mark the lines as upstream or download mirror and give the 
replacement different meanings. The path could be the original PATH for 
an upstream mirror or the relative path of the downloaded file for the 
download mirror.

> I'm also a bit nervous about breaking compatibility with the older
> syntaxes. It is unclear to me how or when  we'd detect/deprecate older
> formats. I do agree we probably do need to remove some support for some
> syntax and move to something new though as what we have is turning into
> some kind of nightmare. FWIW this is really old code that in many ways
> predates my involvement so probably 20+ years old.

The only possibility is to mark the new format because it is impossible 
to know if a replacement want to keep the base name of the path.

As example the following replacement is used in the test:

file:///some1where/.* file://some2where/

The plain regular expressions replaces the complete path with a new path 
but the old code appends only the base name of the path. What was the 
desired behavior?

file:///some1where/ file://some2where/
file:///some1where/.* file://some2where/BASENAME_OF_DOWNLOADFILENAME

The main problem is that we have to keep the bugs if we keep the old 
behavior.

> I'll continue to give this some thought. If you could confirm which
> patches are "cleanup" that would help though, we can see if we can get
> those in a bit faster.

The first 8 patches are cleanups.

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

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Richard Purdie]

On Wed, 2025-02-05 at 13:12 +0100, Stefan Herbrechtsmeier wrote:
>  Am 05.02.2025 um 11:34 schrieb Richard Purdie:
>  On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
> > > The mirror replacement syntax contains many implicit transformations.
> > > The path of the URI always contains the base name of the downloaded
> > > filename. This makes it impossible to rename or remove the base name of
> > > the original path. It prevents upstream mirror for SRC_URIS with a
> > > downloadfilename parameter. The base name of the downloaded filename
> > > makes it impossible to use the download mirror for SRC_URIs with
> > > subfolders in the downloadfilename parameter. Altogether the implicit
> > > transformation complicates the understanding of the replacements.
> > > 
> > > This series adds an additional replacement named DOWNLOADFILENAME. This
> > > replacement contains the relative filename of the downloaded file or
> > > mirror archive for git and hg. This allows the user to explicitly define
> > > the behavior. The usage is equivalent to the PATH replacement for the
> > > sstate mirror from file to https scheme.
> > > 
> > > git://.*/.*  http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
> > > https?://.*/.*  http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
> > > file://.*  https://sstate.yoctoproject.org/all/PATH;downloadfilename=PATH
> > > 
> > > Without a replacement variable the mirror will use the same base name as
> > > the origin SRC_URI. This allows the usage of private package manager
> > > registry together with a downloadfilename parameter or the rename of the
> > > base name.
> > > 
> > > https://registry.npmjs.org/  https://example.com/npm/registry/
> > > https://example.com/example/1.0.0.tgz  https://example.com/example/example-1.0.0.tgz
> > > 
> > > The series adds heuristics to keep a backward compatibility to common
> > > styles. Because of the ambiguity of the old style, it is advisable to
> > > remove this compatibility sooner or later to avoid unexpected behavior.
> > >  
> > Thanks for the patches, these look interesting with some good
> > improvements in there. A lot of the series looks like cleanups and
> > those look like good fixes to have. It may make sense to split this
> > series into two, the cleanups/fixes and the behaviour changes.
> > 
> > I'm not entirely "sold" on the naming of DOWNLOADFILENAME. You have to
> > think about this from the perspective of someone writing a MIRROR or
> > PREMIRROR entry - would they understand what that means vs some of the
> > other names?
> >  
> I’m open for suggestions. Even ARCHIVE or TARBALL are hard to
> understand because it is only a relative path on the download mirror.
> Alternative we can mark the lines as upstream or download mirror and
> give the replacement different meanings. The path could be the
> original PATH for an upstream mirror or the relative path of the
> downloaded file for the download mirror.

I've been giving this topic some thought. One idea I wondered about was
to instead markup the mirror urls with how they're expected to work
with a new parameter. For example:

git://.*/.*  http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball

The possible options would be something like:

mirrortarball - mirror tarballs taken from DL_DIR
flattened - copy of DL_DIR so DL_DIR layout (maybe call it dldir?)
upstream - layout is the same as the upstream directory structure so a direct url replacement

If using a mirrortarball mirror url, we'd know to use the values from
urldata.mirrortarballs. We could add parameters to the fetcher to have
two parameters, one will be the DL_DIR path and the other would be the
upstream url path. One key question I have is how we might need to
shorted the url path for some mirror urls to add/remove a path prefix
in the mirroring.

I think that would cover most of our scenarios and make the mirror urls
much more useful/explicit.

It also would give us a migration path as the code can simply error if
it sees a mirror url without a mirrorformat parameter. This may mean we
could have a clean slate for the mirror urls and switch to one clear
format. I think we can make a case for such a breaking architecture
change.

To move it forward we'd need to work out which pieces of mirror syntax
we should drop, which mirror replacement strings we need (I'm not
convinced the current ones are right/useful). We'd probably need a some
proof of concept patches showing it in action and a proposal to the oe-
arch list explaining why the change was needed and what the change
would do/look like.

I think this can be done independently of the other fetcher/vendor
changing but should be able to work for the vendoring issues too.

Thoughts? I'm sure I'm missing things here :/.

Cheers,

Richard

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Stefan Herbrechtsmeier]

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

Am 20.02.2025 um 11:22 schrieb Richard Purdie via lists.openembedded.org:
> On Wed, 2025-02-05 at 13:12 +0100, Stefan Herbrechtsmeier wrote:
>>   Am 05.02.2025 um 11:34 schrieb Richard Purdie:
>>   On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
>>>> The mirror replacement syntax contains many implicit transformations.
>>>> The path of the URI always contains the base name of the downloaded
>>>> filename. This makes it impossible to rename or remove the base name of
>>>> the original path. It prevents upstream mirror for SRC_URIS with a
>>>> downloadfilename parameter. The base name of the downloaded filename
>>>> makes it impossible to use the download mirror for SRC_URIs with
>>>> subfolders in the downloadfilename parameter. Altogether the implicit
>>>> transformation complicates the understanding of the replacements.
>>>>
>>>> This series adds an additional replacement named DOWNLOADFILENAME. This
>>>> replacement contains the relative filename of the downloaded file or
>>>> mirror archive for git and hg. This allows the user to explicitly define
>>>> the behavior. The usage is equivalent to the PATH replacement for the
>>>> sstate mirror from file to https scheme.
>>>>
>>>> git://.*/.*http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
>>>> https?://.*/.*http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
>>>> file://.*  https://sstate.yoctoproject.org/all/PATH;downloadfilename=PATH
>>>>
>>>> Without a replacement variable the mirror will use the same base name as
>>>> the origin SRC_URI. This allows the usage of private package manager
>>>> registry together with a downloadfilename parameter or the rename of the
>>>> base name.
>>>>
>>>> https://registry.npmjs.org/  https://example.com/npm/registry/
>>>> https://example.com/example/1.0.0.tgz  https://example.com/example/example-1.0.0.tgz
>>>>
>>>> The series adds heuristics to keep a backward compatibility to common
>>>> styles. Because of the ambiguity of the old style, it is advisable to
>>>> remove this compatibility sooner or later to avoid unexpected behavior.
>>>>   
>>> Thanks for the patches, these look interesting with some good
>>> improvements in there. A lot of the series looks like cleanups and
>>> those look like good fixes to have. It may make sense to split this
>>> series into two, the cleanups/fixes and the behaviour changes.
>>>
>>> I'm not entirely "sold" on the naming of DOWNLOADFILENAME. You have to
>>> think about this from the perspective of someone writing a MIRROR or
>>> PREMIRROR entry - would they understand what that means vs some of the
>>> other names?
>>>   
>> I’m open for suggestions. Even ARCHIVE or TARBALL are hard to
>> understand because it is only a relative path on the download mirror.
>> Alternative we can mark the lines as upstream or download mirror and
>> give the replacement different meanings. The path could be the
>> original PATH for an upstream mirror or the relative path of the
>> downloaded file for the download mirror.
> I've been giving this topic some thought. One idea I wondered about was
> to instead markup the mirror urls with how they're expected to work
> with a new parameter. For example:
>
> git://.*/.*http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball
The ? could be problematic because it is the separator for the query. It 
is unlikely that the user really use this query parameter but it could 
complicate the code because we have to handle additional query parameters.
What does the "?mirrorformat=mirrortarball" mean? Will it work like a 
MIRRORTARBALL replacement?

How does a simple replacement should look like?

http://  https://

Because of the backward compatible this will replace the basename of the 
path.

> The possible options would be something like:
>
> mirrortarball - mirror tarballs taken from DL_DIR
> flattened - copy of DL_DIR so DL_DIR layout (maybe call it dldir?)
> upstream - layout is the same as the upstream directory structure so a direct url replacement
Do you think we have to handle the mirror tarball explicit? The mirror 
tarball is required for a scheme change.

> If using a mirrortarball mirror url, we'd know to use the values from
> urldata.mirrortarballs. We could add parameters to the fetcher to have
> two parameters, one will be the DL_DIR path and the other would be the
> upstream url path.
I don't understand where this is needed, because the mirror tarball and 
downloadfilename are used by different fetchers.

> One key question I have is how we might need to
> shorted the url path for some mirror urls to add/remove a path prefix
> in the mirroring.
What do you mean by this? The downloadfilename could contain a path 
without any problem after my change.

> I think that would cover most of our scenarios and make the mirror urls
> much more useful/explicit.
>
> It also would give us a migration path as the code can simply error if
> it sees a mirror url without a mirrorformat parameter. This may mean we
> could have a clean slate for the mirror urls and switch to one clear
> format. I think we can make a case for such a breaking architecture
> change.
Does this means we doesn't need a backward compatibility?

Alternative we could make some replacement mandatory if a wildcard is 
used to detect obsolete entries.

> To move it forward we'd need to work out which pieces of mirror syntax
> we should drop, which mirror replacement strings we need (I'm not
> convinced the current ones are right/useful). We'd probably need a some
> proof of concept patches showing it in action and a proposal to the oe-
> arch list explaining why the change was needed and what the change
> would do/look like.
>
> I think this can be done independently of the other fetcher/vendor
> changing but should be able to work for the vendoring issues too.
Sure. I can rework my patch as soon as we have an agreement.

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

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Richard Purdie]

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

On Thu, 2025-02-20 at 12:45 +0100, Stefan Herbrechtsmeier via
lists.openembedded.org wrote:
>  
> Am 20.02.2025 um 11:22 schrieb Richard Purdie via
> lists.openembedded.org:
> > On Wed, 2025-02-05 at 13:12 +0100, Stefan Herbrechtsmeier wrote:
> > >  Am 05.02.2025 um 11:34 schrieb Richard Purdie:
> > >  On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
> > > I’m open for suggestions. Even ARCHIVE or TARBALL are hard to
> > > understand because it is only a relative path on the download mirror.
> > > Alternative we can mark the lines as upstream or download mirror and
> > > give the replacement different meanings. The path could be the
> > > original PATH for an upstream mirror or the relative path of the
> > > downloaded file for the download mirror.
> > >  
> >  
> > I've been giving this topic some thought. One idea I wondered about was
> > to instead markup the mirror urls with how they're expected to work
> > with a new parameter. For example:
> > 
> > git://.*/.*  http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball
> >  
> The ? could be problematic because it is the separator for the query.
> It is unlikely that the user really use this query parameter but it
> could complicate the code because we have to handle additional query
> parameters.
> What does the "?mirrorformat=mirrortarball" mean? Will it work like a
> MIRRORTARBALL replacement?

The mirrorformat parameter would be used by the mirroring code itself
to understand how to handle the url. It would be dropped from the
modified url so is only therefore our code's use. If there are
additional parameters they would be passed through as they are now.

> How does a simple replacement should look like?
> 
> http://  https://
> 
> Because of the backward compatible this will replace the basename of
> the path.

It would depend how the mirror is laid out. Some mirrors flatten the
urls like DL_DIR is laid out, some potentially don't. The standard
usage would likely have a mirrorformat=dldir parameter added.


> > The possible options would be something like:
> > 
> > mirrortarball - mirror tarballs taken from DL_DIR
> > flattened - copy of DL_DIR so DL_DIR layout (maybe call it dldir?)
> > upstream - layout is the same as the upstream directory structure so a direct url replacement
> >  
> Do you think we have to handle the mirror tarball explicit? The
> mirror tarball is required for a scheme change.

If we do that, we can avoid having to guess at too many urls to test to
figure out a mirror format so I think it would be an improvement on
where we are today.

> > If using a mirrortarball mirror url, we'd know to use the values from
> > urldata.mirrortarballs. We could add parameters to the fetcher to have
> > two parameters, one will be the DL_DIR path and the other would be the
> > upstream url path.
> >  
> I don't understand where this is needed, because the mirror tarball
> and downloadfilename are used by different fetchers.

Please keep in mind that downloadfilename is pretty much a misfeature.
It was added as we couldn't control collisions inside dl_dir but it
creates all kind of other problems. I think we do need to handle that
problem case but it does then mean we have to indicate whether any
given mirror uses "dldir" or "upstream" names and paths.

> > One key question I have is how we might need to
> > shorted the url path for some mirror urls to add/remove a path prefix
> > in the mirroring.
> >  
> What do you mean by this? The downloadfilename could contain a path
> without any problem after my change.

See above, downloadfilename is not something I'm keen to promote and is
creating several of the problems we have by badly trying to hack extra
functionality onto the fetcher without thinking through all the issues
like mirroring.

This is about the fact that you could have:

htttps://some.server/some/deep/multi/level/path/

and a mirror of things there at:

htttps://some.server/shortpath/path/

so the mirror urls need to be able to map

htttps://some.server/some/deep/multi/level/path/a/b/c.tgz

to

htttps://some.server/shortpath/path/a/b/c.tgz

but also:

htttps://some.server/shortpath/path/c.tgz

or possibly

htttps://some.server/shortpath/path.d.tgz

if downloadfilename is in action.

> >  
> > I think that would cover most of our scenarios and make the mirror urls
> > much more useful/explicit.
> > 
> > It also would give us a migration path as the code can simply error if
> > it sees a mirror url without a mirrorformat parameter. This may mean we
> > could have a clean slate for the mirror urls and switch to one clear
> > format. I think we can make a case for such a breaking architecture
> > change.
> >  
> Does this means we doesn't need a backward compatibility?

If we can come up with sufficient justification to break things and can
detect and error correctly for old usage and get this signed off by the
TSC/community, potentially, yes.

> Alternative we could make some replacement mandatory if a wildcard is
> used to detect obsolete entries.

I don't understand that. 

I do think we have too many problems in the existing mirroring url
mapping and we probably need to rework this rather than try and pile
more patches into it and complicate it further.

The question is whether the proposal fixes the issues it needs to and
has enough simplification and benefit to justify making the change.

Cheers,

Richard

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

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Stefan Herbrechtsmeier]

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

Am 20.02.2025 um 13:21 schrieb Richard Purdie:
> On Thu, 2025-02-20 at 12:45 +0100, Stefan Herbrechtsmeier via 
> lists.openembedded.org wrote:
>> Am 20.02.2025 um 11:22 schrieb Richard Purdie via lists.openembedded.org:
>>> On Wed, 2025-02-05 at 13:12 +0100, Stefan Herbrechtsmeier wrote:
>>>>   Am 05.02.2025 um 11:34 schrieb Richard Purdie:
>>>>   On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
>>>> I’m open for suggestions. Even ARCHIVE or TARBALL are hard to
>>>> understand because it is only a relative path on the download mirror.
>>>> Alternative we can mark the lines as upstream or download mirror and
>>>> give the replacement different meanings. The path could be the
>>>> original PATH for an upstream mirror or the relative path of the
>>>> downloaded file for the download mirror.
>>> I've been giving this topic some thought. One idea I wondered about was
>>> to instead markup the mirror urls with how they're expected to work
>>> with a new parameter. For example:
>>> git://.*/.*http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball
>> The ? could be problematic because it is the separator for the query. 
>> It is unlikely that the user really use this query parameter but it 
>> could complicate the code because we have to handle additional query 
>> parameters.
>> What does the "?mirrorformat=mirrortarball" mean? Will it work like a 
>> MIRRORTARBALL replacement?
>
> The mirrorformat parameter would be used by the mirroring code itself 
> to understand how to handle the url.

What is the different to a MIRRORTARBALL replacement? The code will 
replace the word with the content.

> It would be dropped from the modified url so is only therefore our 
> code's use. If there are additional parameters they would be passed 
> through as they are now.
>
>> How does a simple replacement should look like?
>>
>> http://  https://
>>
>> Because of the backward compatible this will replace the basename of 
>> the path.
>
> It would depend how the mirror is laid out. Some mirrors flatten the 
> urls like DL_DIR is laid out, some potentially don't. The standard 
> usage would likely have a mirrorformat=dldir parameter added.

How does the user specify an entry that replace the http scheme with 
https and keeps everything else like it is (upstream mirror)?

>>> The possible options would be something like:
>>> mirrortarball - mirror tarballs taken from DL_DIR
>>> flattened - copy of DL_DIR so DL_DIR layout (maybe call it dldir?)
>>> upstream - layout is the same as the upstream directory structure so a direct url replacement
>> Do you think we have to handle the mirror tarball explicit? The 
>> mirror tarball is required for a scheme change.
>
> If we do that, we can avoid having to guess at too many urls to test 
> to figure out a mirror format so I think it would be an improvement on 
> where we are today.

Do you mean we will test if the URL have a mirrortarball and if not skip 
the entry?

>
>>> If using a mirrortarball mirror url, we'd know to use the values from
>>> urldata.mirrortarballs. We could add parameters to the fetcher to have
>>> two parameters, one will be the DL_DIR path and the other would be the
>>> upstream url path.
>> I don't understand where this is needed, because the mirror tarball 
>> and downloadfilename are used by different fetchers.
>
> Please keep in mind that downloadfilename is pretty much a misfeature. 
> It was added as we couldn't control collisions inside dl_dir but it 
> creates all kind of other problems. I think we do need to handle that 
> problem case but it does then mean we have to indicate whether any 
> given mirror uses "dldir" or "upstream" names and paths.

I don't understand the problem. The download mirror will use the 
downloadfilename or its default the basename of the localpath. The 
upstream mirror will use the path. The mirrortarball will use the 
mirrortarball. Why the fetcher need two parameters?

>>> One key question I have is how we might need to
>>> shorted the url path for some mirror urls to add/remove a path prefix
>>> in the mirroring.
>> What do you mean by this? The downloadfilename could contain a path 
>> without any problem after my change.
>
> See above, downloadfilename is not something I'm keen to promote and 
> is creating several of the problems we have by badly trying to hack 
> extra functionality onto the fetcher without thinking through all the 
> issues like mirroring.

What is the desired way to avoid name clashes? The package manager 
fetcher need an generic way to override the basename.

> This is about the fact that you could have:
>
> htttps://some.server/some/deep/multi/level/path/
>
> and a mirror of things there at:
>
> htttps://some.server/shortpath/path/
>
> so the mirror urls need to be able to map
>
> htttps://some.server/some/deep/multi/level/path/a/b/c.tgz

This could be solved if we use my proposed code.

> to
>
> htttps://some.server/shortpath/path/a/b/c.tgz

htttps://some.server/some/deep/multi/level/path/ 
htttps://some.server/shortpath/path/

> but also:
>
> htttps://some.server/shortpath/path/c.tgz

htttps://some.server/some/deep/multi/level/path/.* 
htttps://some.server/shortpath/path/DOWNLOADFILENAME

> or possibly
>
> htttps://some.server/shortpath/path.d.tgz

> if downloadfilename is in action.

Because the fallback for the downloadfilename is the basename the same 
entry works.

htttps://some.server/some/deep/multi/level/path/.* 
htttps://some.server/shortpath/DOWNLOADFILENAME

>>> I think that would cover most of our scenarios and make the mirror urls
>>> much more useful/explicit.
>>> It also would give us a migration path as the code can simply error if
>>> it sees a mirror url without a mirrorformat parameter. This may mean we
>>> could have a clean slate for the mirror urls and switch to one clear
>>> format. I think we can make a case for such a breaking architecture
>>> change.
>> Does this means we doesn't need a backward compatibility?
>
> If we can come up with sufficient justification to break things and 
> can detect and error correctly for old usage and get this signed off 
> by the TSC/community, potentially, yes.

Okay

>> Alternative we could make some replacement mandatory if a wildcard is 
>> used to detect obsolete entries.
>
> I don't understand that.

If we have a wildcard .* in the path we need to know how to replace it. 
This could be the PATH, BASENAME, DOWNLOADFILENAME, MIRRORARCHIVE or re 
group. But in case of the re group this could still be an old entry.

Do support all cases we need a fix prefix or delimiter:

r:http https

http#https
http|https

http?://.*/.*|http://downloads.yoctoproject.org/mirror/sources/DOWNLOADARCHIVE
git://.*/.*|http://downloads.yoctoproject.org/mirror/sources/MIRRORARCHIVE

> I do think we have too many problems in the existing mirroring url 
> mapping and we probably need to rework this rather than try and pile 
> more patches into it and complicate it further.
I have already rework it. If I can remove the backward compatibility and 
replace it with an error this would simplify the code. I only need a 
better name for the DOWNLOADFILENAME (DOWNLOADARCHIVE) and add the 
MIRRORARCHIV.

> The question is whether the proposal fixes the issues it needs to and 
> has enough simplification and benefit to justify making the change.
My patches support folders in the downloadfilename, upstream mirrors and 
renames. I have to rework the MIRROR strings but therefore the commented 
out tests work.

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

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Richard Purdie]

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

On Thu, 2025-02-20 at 18:37 +0100, Stefan Herbrechtsmeier wrote:
>  
> Am 20.02.2025 um 13:21 schrieb Richard Purdie:
> > On Thu, 2025-02-20 at 12:45 +0100, Stefan Herbrechtsmeier via
> > lists.openembedded.org wrote:
> > > Am 20.02.2025 um 11:22 schrieb Richard Purdie via
> > > lists.openembedded.org:
> > > > On Wed, 2025-02-05 at 13:12 +0100, Stefan Herbrechtsmeier wrote:
> > > > >  Am 05.02.2025 um 11:34 schrieb Richard Purdie:
> > > > >  
> > > > >  On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
> > > > >  
> > > > > I’m open for suggestions. Even ARCHIVE or TARBALL are hard to
> > > > >  
> > > > > understand because it is only a relative path on the download mirror.
> > > > >  
> > > > > Alternative we can mark the lines as upstream or download mirror and
> > > > >  
> > > > > give the replacement different meanings. The path could be the
> > > > >  
> > > > > original PATH for an upstream mirror or the relative path of the
> > > > >  
> > > > > downloaded file for the download mirror.
> > > > >  
> > > > >  
> > > > >  
> > > >  
> > > >  
> > > >  
> > > > I've been giving this topic some thought. One idea I wondered about was
> > > >  
> > > > to instead markup the mirror urls with how they're expected to work
> > > >  
> > > > with a new parameter. For example:
> > > >   
> > > > git://.*/.*  http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball
> > > >  
> > > >  
> > > >  
> > >  
> > > The ? could be problematic because it is the separator for the
> > > query. It is unlikely that the user really use this query
> > > parameter but it could complicate the code because we have to
> > > handle additional query parameters.
> > > What does the "?mirrorformat=mirrortarball" mean? Will it work
> > > like a MIRRORTARBALL replacement?
> > >  
> >  
> > 
> >  
> >  
> > The mirrorformat parameter would be used by the mirroring code
> > itself to understand how to handle the url.
> >  
>  
> What is the different to a MIRRORTARBALL replacement? The code will
> replace the word with the content.
Think about this from a usability perspective. We're struggling to even
work out good names for your proposal. Even if we work out the names, I
still don't think users are going to understand how to convert urls
into the new syntax.

The difference with my proposed format is that we're specifying it in a
way which I suspect users will better understand without needing to go
and read the docs every time. We're saying what we're configuring with
the "mirrorformat" key and then the value should be able to describe
the format. 

My proposal also gives us both a way to clearly detect when obsolete
formatting is used and a namespace mechanism to extend, with both being
in a way we can easily and clearly describe in the docs.

I appreciate with your proposal we can add more strings and we can add
docs about how to migrate but I suspect users aren't going to be as
readily/easily able to understand it.


> > It would be dropped from the modified url so is only therefore our
> > code's use. If there are additional parameters they would be passed
> > through as they are now.
> >  
> > 
> >  
> >  
> > >  
> > > How does a simple replacement should look like?
> > > 
> > > http://  https://
> > > 
> > > Because of the backward compatible this will replace the basename
> > > of the path.
> > >  
> >  
> > 
> >  
> >  
> > It would depend how the mirror is laid out. Some mirrors flatten
> > the urls like DL_DIR is laid out, some potentially don't. The
> > standard usage would likely have a mirrorformat=dldir parameter
> > added.
> >  
>  
> How does the user specify an entry that replace the http scheme with
> https and keeps everything else like it is (upstream mirror)?

http://.*/.* https://.*/.*?mirrorformat=upstream

We need to determine the best value for "upstream". I'd also like to
review whether the .* formatting is the best way to handle this if we
are going to change the format.

> > > > The possible options would be something like:
> > > >   
> > > > mirrortarball - mirror tarballs taken from DL_DIR
> > > >  
> > > > flattened - copy of DL_DIR so DL_DIR layout (maybe call it dldir?)
> > > >  
> > > > upstream - layout is the same as the upstream directory structure so a direct url replacement
> > > >  
> > > >  
> > > >  
> > >  
> > > Do you think we have to handle the mirror tarball explicit? The
> > > mirror tarball is required for a scheme change.
> > >  
> >  
> > 
> >  
> >  
> > If we do that, we can avoid having to guess at too many urls to
> > test to figure out a mirror format so I think it would be an
> > improvement on where we are today.
> >  
>  
> Do you mean we will test if the URL have a mirrortarball and if not
> skip the entry?
Correct.

> > > > If using a mirrortarball mirror url, we'd know to use the values from
> > > >  
> > > > urldata.mirrortarballs. We could add parameters to the fetcher to have
> > > >  
> > > > two parameters, one will be the DL_DIR path and the other would be the
> > > >  
> > > > upstream url path.
> > > >  
> > > >  
> > > >  
> > >  
> > > I don't understand where this is needed, because the mirror
> > > tarball and downloadfilename are used by different fetchers.
> > >  
> >  
> > 
> >  
> >  
> > Please keep in mind that downloadfilename is pretty much a
> > misfeature. It was added as we couldn't control collisions inside
> > dl_dir but it creates all kind of other problems. I think we do
> > need to handle that problem case but it does then mean we have to
> > indicate whether any given mirror uses "dldir" or "upstream" names
> > and paths.
> >  
>  
> I don't understand the problem. The download mirror will use the
> downloadfilename or its default the basename of the localpath. The
> upstream mirror will use the path. The mirrortarball will use the
> mirrortarball. Why the fetcher need two parameters?
You are trying to make downloadfilename a supported parameter of every
fetcher. I'm arguing that I wish we'd never added it at all and that
I'd rather not use it or encourage its use. I don't think you
understand the way the fetcher API was written/used and this is why
some of the patches are still on hold in master-next until I can
convince myself they are actually the right thing to do. There have
been too many other misunderstandings to give me confidence they're
going to do the right thing :(. Sadly, I just don't have the time do
the right level of review and everything else being asked of me.

> > > > One key question I have is how we might need to
> > > >  
> > > > shorted the url path for some mirror urls to add/remove a path prefix
> > > >  
> > > > in the mirroring.
> > > >  
> > > >  
> > > >  
> > >  
> > > What do you mean by this? The downloadfilename could contain a
> > > path without any problem after my change.
> > >  
> >  
> > 
> >  
> >  
> > See above, downloadfilename is not something I'm keen to promote
> > and is creating several of the problems we have by badly trying to
> > hack extra functionality onto the fetcher without thinking through
> > all the issues like mirroring.
> >  
>  
> What is the desired way to avoid name clashes? The package manager
> fetcher need an generic way to override the basename.
Why does it need that? Usually we've used the directory layout to avoid
problems where we can for example. I've been hoping we could do similar
here rather than use downloadfilename, which causes so many mirroring
issues in the first place.


> > > Alternative we could make some replacement mandatory if a
> > > wildcard is used to detect obsolete entries.
> > >  
> >  
> > 
> >  
> >  
> > I don't understand that.
> >  
>  
> If we have a wildcard .* in the path we need to know how to replace
> it. This could be the PATH, BASENAME, DOWNLOADFILENAME, MIRRORARCHIVE
> or re group. But in case of the re group this could still be an old
> entry.
I'm not entirely sure we want to keep all the different syntax. One
frustration I have with the current code is the way pattern matches are
restricted to that url component for example and I've wondered if we
could/should do something different instead, if we can make it simpler.

> Do support all cases we need a fix prefix or delimiter:
> 
> r:http https
>  
> http#https
> http|https
> 
> http?://.*/.*|
> http://downloads.yoctoproject.org/mirror/sources/DOWNLOADARCHIVE
> git://.*/.*|
> http://downloads.yoctoproject.org/mirror/sources/MIRRORARCHIVE
> 
>  
> >  
> > I do think we have too many problems in the existing mirroring url
> > mapping and we probably need to rework this rather than try and
> > pile more patches into it and complicate it further.
> >  
> I have already rework it. If I can remove the backward compatibility
> and replace it with an error this would simplify the code. I only
> need a better name for the DOWNLOADFILENAME (DOWNLOADARCHIVE) and add
> the MIRRORARCHIV.

> 
> >  
> > The question is whether the proposal fixes the issues it needs to
> > and has enough simplification and benefit to justify making the
> > change.
> >  
> My patches support folders in the downloadfilename, upstream mirrors
> and renames. I have to rework the MIRROR strings but therefore the
> commented out tests work.
> 


You've created a patch, yes. I don't think it improves usability though
and I think you're also pushing concepts like downloadfilename into
places we might not want to use them too. For me to merge patches like
these, there needs to be a sense of trust and shared understanding.
This simply isn't there, you're just saying your patches are fine as
they are, I disagree. I therefore worry we're at an impasse and are
going to struggle to move beyond this. That does make me quite sad.

Regards,

Richard




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

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Stefan Herbrechtsmeier]

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

Am 20.02.2025 um 23:00 schrieb Richard Purdie:
> On Thu, 2025-02-20 at 18:37 +0100, Stefan Herbrechtsmeier wrote:
>> Am 20.02.2025 um 13:21 schrieb Richard Purdie:
>>> On Thu, 2025-02-20 at 12:45 +0100, Stefan Herbrechtsmeier via 
>>> lists.openembedded.org wrote:
>>>> Am 20.02.2025 um 11:22 schrieb Richard Purdie via 
>>>> lists.openembedded.org:
>>>>> On Wed, 2025-02-05 at 13:12 +0100, Stefan Herbrechtsmeier wrote:
>>>>>>   Am 05.02.2025 um 11:34 schrieb Richard Purdie:
>>>>>>   On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
>>>>>> I’m open for suggestions. Even ARCHIVE or TARBALL are hard to
>>>>>> understand because it is only a relative path on the download mirror.
>>>>>> Alternative we can mark the lines as upstream or download mirror and
>>>>>> give the replacement different meanings. The path could be the
>>>>>> original PATH for an upstream mirror or the relative path of the
>>>>>> downloaded file for the download mirror.
>>>>> I've been giving this topic some thought. One idea I wondered about was
>>>>> to instead markup the mirror urls with how they're expected to work
>>>>> with a new parameter. For example:
>>>>> git://.*/.*http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball
>>>> The ? could be problematic because it is the separator for the 
>>>> query. It is unlikely that the user really use this query parameter 
>>>> but it could complicate the code because we have to handle 
>>>> additional query parameters.
>>>> What does the "?mirrorformat=mirrortarball" mean? Will it work like 
>>>> a MIRRORTARBALL replacement?
>>>
>>> The mirrorformat parameter would be used by the mirroring code 
>>> itself to understand how to handle the url.
>>
>> What is the different to a MIRRORTARBALL replacement? The code will 
>> replace the word with the content.
>>
> Think about this from a usability perspective. We're struggling to 
> even work out good names for your proposal. Even if we work out the names,
We need the good names in any case. The ? is misleading because it is 
part of a common URL.

> I still don't think users are going to understand how to convert urls 
> into the new syntax.

git://.*/.* 
http://downloads.yoctoproject.org/mirror/sources/?mirrorformat=mirrortarball
git://.*/.* http://downloads.yoctoproject.org/mirror/sources/MIRRORTARBALL
git://.*/.* http://downloads.yoctoproject.org/mirror/sources/{MIRRORTARBALL}

git://.*/.* 
http://downloads.yoctoproject.org/mirror/sources/download.cgi?filename=?mirrorformat=mirrortarball
git://.*/.* 
http://downloads.yoctoproject.org/mirror/sources/download.cgi?filename=MIRRORTARBALL
git://.*/.* 
http://downloads.yoctoproject.org/mirror/sources/download.cgi?filename={MIRRORTARBALL}

> The difference with my proposed format is that we're specifying it in 
> a way which I suspect users will better understand without needing to 
> go and read the docs every time. We're saying what we're configuring 
> with the "mirrorformat" key and then the value should be able to 
> describe the format.
In this case we should add a prefix or use a parameter

git://.*/.* scmmirror:http://downloads.yoctoproject.org/mirror/sources/
http?://.*/.* 
downloadmirror:https://downloads.yoctoproject.org/mirror/sources/
http://  plain:https://

git://.*/.* 
http://downloads.yoctoproject.org/mirror/sources/;mirrorformat=scm
http?://.*/.* 
downloadmirror:https://downloads.yoctoproject.org/mirror/sources/;mirrorformat=download
http:// https://;mirrorformat=plain

> My proposal also gives us both a way to clearly detect when obsolete 
> formatting is used and a namespace mechanism to extend, with both 
> being in a way we can easily and clearly describe in the docs.
But it makes it impossible to support arbitrary regular expressions.

https://a.com/b/c/d.tar https://x.com/y/z.tar
https://a.com/b/(.*) https://x.com/y/\1

> I appreciate with your proposal we can add more strings and we can add 
> docs about how to migrate but I suspect users aren't going to be as 
> readily/easily able to understand it.

The main difference is that in your case the user have to read the docs 
to learn the pre- or postfix and in my case it has to learn the placeholder.

In any case the user need to understand that there is a difference 
between the download and a plain mirror. Either he has to mark the entry 
and hope that the magic behind work for his use case or he has to place 
the placeholder at the correct position.

git://.*/.* https://downloads.abc.org/mirror/sources/download/MIRRORTARBALL
git://.*/.* 
https://downloads.abc.org/mirror/sources/download.cgi?filename=MIRRORTARBALL
git://(.*)/(.*) 
https://downloads.abc.org/mirror/sources/download/\1/\2/MIRRORTARBALL

>>> It would be dropped from the modified url so is only therefore our 
>>> code's use. If there are additional parameters they would be passed 
>>> through as they are now.
>>>
>>>
>>>> How does a simple replacement should look like?
>>>>
>>>> http://  https://
>>>>
>>>> Because of the backward compatible this will replace the basename 
>>>> of the path.
>>>
>>> It would depend how the mirror is laid out. Some mirrors flatten the 
>>> urls like DL_DIR is laid out, some potentially don't. The standard 
>>> usage would likely have a mirrorformat=dldir parameter added.
>>
>> How does the user specify an entry that replace the http scheme with 
>> https and keeps everything else like it is (upstream mirror)?
>>
>
> http://.*/.* https://.*/.*?mirrorformat=upstream
I assume you mean the following because this doesn't make sense.

http:// https://?mirrorformat=upstream

What happens if the user need to add the query parameter mirror=1:

http:// https://?mirrorformat=upstream?mirror=1

> We need to determine the best value for "upstream". I'd also like to 
> review whether the .* formatting is the best way to handle this if we 
> are going to change the format.
>
>>>>> The possible options would be something like:
>>>>> mirrortarball - mirror tarballs taken from DL_DIR
>>>>> flattened - copy of DL_DIR so DL_DIR layout (maybe call it dldir?)
>>>>> upstream - layout is the same as the upstream directory structure so a direct url replacement
>>>> Do you think we have to handle the mirror tarball explicit? The 
>>>> mirror tarball is required for a scheme change.
>>>
>>> If we do that, we can avoid having to guess at too many urls to test 
>>> to figure out a mirror format so I think it would be an improvement 
>>> on where we are today.
>>
>> Do you mean we will test if the URL have a mirrortarball and if not 
>> skip the entry?
>>
> Correct.
But doesn't the mirrortarball depends on the scheme and we doesn't use 
the entry in anyway? Do you have an example?


>>>>> If using a mirrortarball mirror url, we'd know to use the values from
>>>>> urldata.mirrortarballs. We could add parameters to the fetcher to have
>>>>> two parameters, one will be the DL_DIR path and the other would be the
>>>>> upstream url path.
>>>> I don't understand where this is needed, because the mirror tarball 
>>>> and downloadfilename are used by different fetchers.
>>>
>>> Please keep in mind that downloadfilename is pretty much a 
>>> misfeature. It was added as we couldn't control collisions inside 
>>> dl_dir but it creates all kind of other problems. I think we do need 
>>> to handle that problem case but it does then mean we have to 
>>> indicate whether any given mirror uses "dldir" or "upstream" names 
>>> and paths.
>>
>> I don't understand the problem. The download mirror will use the 
>> downloadfilename or its default the basename of the localpath. The 
>> upstream mirror will use the path. The mirrortarball will use the 
>> mirrortarball. Why the fetcher need two parameters?
>>
> You are trying to make downloadfilename a supported parameter of every 
> fetcher.

No, I simply harmonize the use of the parameter and remove the 
problematic assumption that the filename inside the download folder 
doesn't contain additional folders.

A lot of fetcher already support the downloadfilename. Why should I 
assume that this is wrong and not the desired way.

> I'm arguing that I wish we'd never added it at all and that I'd rather 
> not use it or encourage its use.
What is your alternative to handle name clashes?

At the moment it is unclear if the usage of the basename or the folders 
inside the downloadfilename are the bug.

The problem is, that even the encoded URI as filename doesn't ensure 
name clashes because some projects doesn't encode the version inside the 
URI. The information from the recipe like BP is also problematical.

> I don't think you understand the way the fetcher API was written/used
Based on the code I assume the code was used to implement a download 
mirror. Thereby it assume that the download folder use a flat list of 
files. The problem is that neither the usage of the downloadfilename nor 
the syntax for the MIRROR variables reflect this. The functions contains 
some magic inside it.

Based on the code I assume there is a need for folders inside the 
downloadfilename and for the support of upstream mirrors.

>
>
> and this is why some of the patches are still on hold in master-next 
> until I can convince myself they are actually the right thing to do. 
> There have been too many other misunderstandings to give me confidence 
> they're going to do the right thing :(. Sadly, I just don't have the 
> time do the right level of review and everything else being asked of me.

Again my problem is that I don't know the vision. I look at the code and 
develop a solution which looks reasonable for the different existing use 
cases.

>>>>> One key question I have is how we might need to
>>>>> shorted the url path for some mirror urls to add/remove a path prefix
>>>>> in the mirroring.
>>>> What do you mean by this? The downloadfilename could contain a path 
>>>> without any problem after my change.
>>>
>>> See above, downloadfilename is not something I'm keen to promote and 
>>> is creating several of the problems we have by badly trying to hack 
>>> extra functionality onto the fetcher without thinking through all 
>>> the issues like mirroring.
>>
>> What is the desired way to avoid name clashes? The package manager 
>> fetcher need an generic way to override the basename.
>>
> Why does it need that?

What do we do if the upstream only provide a single URI to get the last 
version?

> Usually we've used the directory layout to avoid problems where we can 
> for example. I've been hoping we could do similar here rather than use 
> downloadfilename, which causes so many mirroring issues in the first 
> place.

Usually? The wget fetcher only use the basename. The npm fetcher use the 
scope and basename. The crate fetcher the name and version together with 
a specific file extension. Only the gomod fetcher use the path.

It looks like the gomod fetcher only works because it use the path. How 
should I know that it is the only correct user of the downloadfilename?

The mirror code only use the basename of the downloadfilename / path. 
The download mirror only works if the entry keeps the upstream path.

>>>> Alternative we could make some replacement mandatory if a wildcard 
>>>> is used to detect obsolete entries.
>>>
>>> I don't understand that.
>>
>> If we have a wildcard .* in the path we need to know how to replace 
>> it. This could be the PATH, BASENAME, DOWNLOADFILENAME, MIRRORARCHIVE 
>> or re group. But in case of the re group this could still be an old 
>> entry.
>>
> I'm not entirely sure we want to keep all the different syntax. One 
> frustration I have with the current code is the way pattern matches 
> are restricted to that url component for example and I've wondered if 
> we could/should do something different instead, if we can make it simpler.

What is the purpose of the MIRROR. Should it be support any use case 
(regex) or should it only support specific use cases?

Maybe I could simple remove the url components. Until now I simple 
minimize the changes to simplify the review.

>> Do support all cases we need a fix prefix or delimiter:
>>
>> r:http https
>>
>> http#https
>> http|https
>>
>> http?://.*/.*|http://downloads.yoctoproject.org/mirror/sources/DOWNLOADARCHIVE
>> git://.*/.*|http://downloads.yoctoproject.org/mirror/sources/MIRRORARCHIVE
>>
>>
>>> I do think we have too many problems in the existing mirroring url 
>>> mapping and we probably need to rework this rather than try and pile 
>>> more patches into it and complicate it further.
>> I have already rework it. If I can remove the backward compatibility 
>> and replace it with an error this would simplify the code. I only 
>> need a better name for the DOWNLOADFILENAME (DOWNLOADARCHIVE) and add 
>> the MIRRORARCHIV.
>
>>
>>> The question is whether the proposal fixes the issues it needs to 
>>> and has enough simplification and benefit to justify making the change.
>> My patches support folders in the downloadfilename, upstream mirrors 
>> and renames. I have to rework the MIRROR strings but therefore the 
>> commented out tests work.
>>
>
>
> You've created a patch, yes. I don't think it improves usability 
> though and I think you're also pushing concepts like downloadfilename 
> into places we might not want to use them too.
Until now I don't know the desired solution. Now I think your desired 
solution is to mimic the upstream in the download folder and this is a 
requirement for the rework of the mirror code. This means we have to fix 
the exiting code first and should document this requirement.

> For me to merge patches like these, there needs to be a sense of trust 
> and shared understanding. This simply isn't there, you're just saying 
> your patches are fine as they are, I disagree.
No. I don't understand what problem your proposed solution like the "?" 
solve. The solutions comes with drawbacks and I want to understand the 
advantages which balance the disadvantage.

> I therefore worry we're at an impasse and are going to struggle to 
> move beyond this. That does make me quite sad.
We haven't an impasse. The problem is the code doesn't reflect your 
concepts and it's hard for me to identify them from your replies.

If I assume that the download folder should mimic the upstream we 
doesn't need the DOWNLOADARCHIVE because it should be the same as the PATH.

But I'm unsure if this works because the PATH isn't enough to unique 
identify a file and a path not always contain a file extension.

https://www.linuxtv.org/hg/dvb-apps/archive/3d43b280298c.tar.bz2;downloadfilename=${BPN}-3d43b280298c.tar.bz2
https://wrapdb.mesonbuild.com/v2/fmt_11.0.2-1/get_patch;downloadfilename=fmt_11.0.2-1_patch.zip
http://miniupnp.tuxfamily.org/files/download.php?file=${BP}.tar.gz;downloadfilename=${BP}.tar.gz
https://selenic.com/repo/${BPN}/archive/${HG_CHANGESET}.tar.bz2;downloadfilename=${BP}.tar.bz2
http://www.netlib.org/benchmark/dhry-c;downloadfilename=dhry-c.shar
https://sourceforge.net/p/giflib/code/ci/d54b45b0240d455bbaedee4be5203d2703e59967/tree/doc/giflib-logo.gif?format=raw;downloadfilename=giflib-logo.gif

Additionally the same PATH could be used on different hosts. If we 
remove the possibility for the user to override the filename inside the 
download folder we have to ensure that every URI leads to an other 
filename inside the download folder.

We need to generate a file path from the whole URI to append it to the 
download mirror URI.

I suspect we have to define the vision for the download folder first 
before we could discuses how the mirror entries should look like.

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

Re: [bitbake-devel] [RFC PATCH 00/15] Make mirror replacement syntax explicit

[Stefan Herbrechtsmeier]

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

Am 05.02.2025 um 13:12 schrieb Stefan Herbrechtsmeier via 
lists.openembedded.org:
> Am 05.02.2025 um 11:34 schrieb Richard Purdie:
>> On Wed, 2025-02-05 at 08:15 +0100, Stefan Herbrechtsmeier via lists.openembedded.org wrote:
>>> The mirror replacement syntax contains many implicit transformations.
>>> The path of the URI always contains the base name of the downloaded
>>> filename. This makes it impossible to rename or remove the base name of
>>> the original path. It prevents upstream mirror for SRC_URIS with a
>>> downloadfilename parameter. The base name of the downloaded filename
>>> makes it impossible to use the download mirror for SRC_URIs with
>>> subfolders in the downloadfilename parameter. Altogether the implicit
>>> transformation complicates the understanding of the replacements.
>>>
>>> This series adds an additional replacement named DOWNLOADFILENAME. This
>>> replacement contains the relative filename of the downloaded file or
>>> mirror archive for git and hg. This allows the user to explicitly define
>>> the behavior. The usage is equivalent to the PATH replacement for the
>>> sstate mirror from file to https scheme.
>>>
>>> git://.*/.*http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
>>> https?://.*/.*http://downloads.yoctoproject.org/mirror/sources/DOWNLOADFILENAME
>>> file://.*  https://sstate.yoctoproject.org/all/PATH;downloadfilename=PATH
>>>
>>> Without a replacement variable the mirror will use the same base name as
>>> the origin SRC_URI. This allows the usage of private package manager
>>> registry together with a downloadfilename parameter or the rename of the
>>> base name.
>>>
>>> https://registry.npmjs.org/  https://example.com/npm/registry/
>>> https://example.com/example/1.0.0.tgz  https://example.com/example/example-1.0.0.tgz
>>>
>>> The series adds heuristics to keep a backward compatibility to common
>>> styles. Because of the ambiguity of the old style, it is advisable to
>>> remove this compatibility sooner or later to avoid unexpected behavior.
>> Thanks for the patches, these look interesting with some good
>> improvements in there. A lot of the series looks like cleanups and
>> those look like good fixes to have. It may make sense to split this
>> series into two, the cleanups/fixes and the behaviour changes.
>>
>> I'm not entirely "sold" on the naming of DOWNLOADFILENAME. You have to
>> think about this from the perspective of someone writing a MIRROR or
>> PREMIRROR entry - would they understand what that means vs some of the
>> other names?
>
> I’m open for suggestions. Even ARCHIVE or TARBALL are hard to 
> understand because it is only a relative path on the download mirror. 
> Alternative we can mark the lines as upstream or download mirror and 
> give the replacement different meanings. The path could be the 
> original PATH for an upstream mirror or the relative path of the 
> downloaded file for the download mirror.
>
>> I'm also a bit nervous about breaking compatibility with the older
>> syntaxes. It is unclear to me how or when  we'd detect/deprecate older
>> formats. I do agree we probably do need to remove some support for some
>> syntax and move to something new though as what we have is turning into
>> some kind of nightmare. FWIW this is really old code that in many ways
>> predates my involvement so probably 20+ years old.
>
> The only possibility is to mark the new format because it is 
> impossible to know if a replacement want to keep the base name of the 
> path.
>
> As example the following replacement is used in the test:
>
> file:///some1where/.* file://some2where/
>
> The plain regular expressions replaces the complete path with a new 
> path but the old code appends only the base name of the path. What was 
> the desired behavior?
>
> file:///some1where/ file://some2where/
> file:///some1where/.* file://some2where/BASENAME_OF_DOWNLOADFILENAME
>
The BASENAME_OF_DOWNLOADFILENAME is wrong because the original is a 
local path and not a downloaded file:

file:///some1where/.* file://some2where/BASENAME

> The main problem is that we have to keep the bugs if we keep the old 
> behavior.
>
>> I'll continue to give this some thought. If you could confirm which
>> patches are "cleanup" that would help though, we can see if we can get
>> those in a bit faster.
>
> The first 8 patches are cleanups.
>

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