| Message ID | SJ0PR22MB2795A6612E85C6D7B2D5CAB7FAEA2@SJ0PR22MB2795.namprd22.prod.outlook.com |
|---|---|
| State | Accepted, archived |
| Commit | 21cfc7ae9a19f39ac8904e1c3466e7e499ac523f |
| Headers | show |
| Series | fetch2/svn: Fix mirroring issue with svn | expand |
On Tue, 2024-05-21 at 07:51 +0000, Kari Sivonen via lists.openembedded.org wrote: > > There was bug in first fix of svn mirror fix for [YOCTO #15473] > > > svn to svn mirror fixed in first commit but it broke svn to other mirrors > This commit fix those other mirrors back to original way of working > --- > lib/bb/fetch2/__init__.py | 10 ++++++++++ > lib/bb/tests/fetch.py | 5 +++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py > index 5bf2c4b8c..cfe6d5025 100644 > --- a/lib/bb/fetch2/__init__.py > +++ b/lib/bb/fetch2/__init__.py > @@ -472,6 +472,16 @@ def uri_replace(ud, uri_find, uri_replace, replacements, d, mirrortarball=None): > uri_find_decoded[5] = {} > elif ud.localpath and ud.method.supports_checksum(ud): > basename = os.path.basename(ud.localpath) > + elif ud.localpath and uri_decoded[0] == "svn" and uri_replace_decoded[0] != "svn": > + #When added svn fetcher that supports_checksum return false > + #all mirrors for svn skip all basename sets. That is ok for svn -> svn mirrors > + #but other mirrors not work anymore. Example you want use tar balls for svn from https server > + #This elif will catch cases where svn mirros gone before svn mirror fix > + #This is basically same than first if branch except no mirrortarball set. Can it some how set for svn uris? > + basename = os.path.basename(ud.localpath) > + # Kill parameters, they make no sense for mirror tarballs > + uri_decoded[5] = {} > + uri_find_decoded[5] = {} > if basename: > uri_basename = os.path.basename(uri_decoded[loc]) > # Prefix with a slash as a sentinel in case > diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py > index ed7a39a72..934c19e0e 100644 > --- a/lib/bb/tests/fetch.py > +++ b/lib/bb/tests/fetch.py > @@ -512,7 +512,8 @@ class MirrorUriTest(FetcherTest): > "git://someserver.org/bitbake git://git.openembedded.org/bitbake " \ > "https://.*/.* file:///someotherpath/downloads/ " \ > "http://.*/.* file:///someotherpath/downloads/ " \ > - "svn://svn.server1.com/ svn://svn.server2.com/" > + "svn://svn.server1.com/ svn://svn.server2.com/ " \ > + "svn://svn.server1.com/ file:///someotherpath/downloads/ " > > def test_urireplace(self): > self.d.setVar("FILESPATH", ".") > @@ -541,7 +542,7 @@ class MirrorUriTest(FetcherTest): > fetcher = bb.fetch.FetchData("svn://svn.server1.com/isource/svnroot/reponame/tags/tagname;module=path_in_tagnamefolder;protocol=https;rev=2", self.d) > mirrors = bb.fetch2.mirror_from_string(self.mirrorvar) > uris, uds = bb.fetch2.build_mirroruris(fetcher, mirrors, self.d) > - self.assertEqual(uris, ['svn://svn.server2.com/isource/svnroot/reponame/tags/tagname;module=path_in_tagnamefolder;protocol=https;rev=2']) > + self.assertEqual(uris, ['svn://svn.server2.com/isource/svnroot/reponame/tags/tagname;module=path_in_tagnamefolder;protocol=https;rev=2', 'file:///someotherpath/downloads/isource/svnroot/reponame/tags/path_in_tagnamefolder_svn.server1.com_.isource.svnroot.reponame.tags.tagname_2_1.tar.gz']) > > def test_mirror_of_mirror(self): > # Test if mirror of a mirror works I took the original patch since it seemed to make sense and was simple with a test case. This patch is adding fetcher specific conditionals into the core code and this always worries me. You'll note we don't have many, if any of these cases currently. That makes me wonder if there isn't a better way to solve this somehow. It also makes me wonder if the original change was correct :/. I certainly don't like the if block above being added. Cheers, Richard
I think original change was correct in way that it fix svn to svn mirroring. Now I also added handling of svn to other mirroring. I saw there is already if clause which would do the correct thing if mirrortarball is set. But I am not sure where it can set and what all and where it will affect if is set. That why I set svn specific if else there that no any other place need to change. I also added test case for svn to file mirroring. Maybe it can be tested with this new test without any svn changes if svn to file mirroring works like with this change. -Kari
diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py index 5bf2c4b8c..cfe6d5025 100644 --- a/lib/bb/fetch2/__init__.py +++ b/lib/bb/fetch2/__init__.py @@ -472,6 +472,16 @@ def uri_replace(ud, uri_find, uri_replace, replacements, d, mirrortarball=None): uri_find_decoded[5] = {} elif ud.localpath and ud.method.supports_checksum(ud): basename = os.path.basename(ud.localpath) + elif ud.localpath and uri_decoded[0] == "svn" and uri_replace_decoded[0] != "svn": + #When added svn fetcher that supports_checksum return false + #all mirrors for svn skip all basename sets. That is ok for svn -> svn mirrors + #but other mirrors not work anymore. Example you want use tar balls for svn from https server + #This elif will catch cases where svn mirros gone before svn mirror fix + #This is basically same than first if branch except no mirrortarball set. Can it some how set for svn uris? + basename = os.path.basename(ud.localpath) + # Kill parameters, they make no sense for mirror tarballs + uri_decoded[5] = {} + uri_find_decoded[5] = {} if basename: uri_basename = os.path.basename(uri_decoded[loc]) # Prefix with a slash as a sentinel in case diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py index ed7a39a72..934c19e0e 100644 --- a/lib/bb/tests/fetch.py +++ b/lib/bb/tests/fetch.py @@ -512,7 +512,8 @@ class MirrorUriTest(FetcherTest): "git://someserver.org/bitbake git://git.openembedded.org/bitbake " \ "https://.*/.* file:///someotherpath/downloads/ " \ "http://.*/.* file:///someotherpath/downloads/ " \ - "svn://svn.server1.com/ svn://svn.server2.com/" + "svn://svn.server1.com/ svn://svn.server2.com/ " \ + "svn://svn.server1.com/ file:///someotherpath/downloads/ " def test_urireplace(self): self.d.setVar("FILESPATH", ".") @@ -541,7 +542,7 @@ class MirrorUriTest(FetcherTest): fetcher = bb.fetch.FetchData("svn://svn.server1.com/isource/svnroot/reponame/tags/tagname;module=path_in_tagnamefolder;protocol=https;rev=2", self.d) mirrors = bb.fetch2.mirror_from_string(self.mirrorvar) uris, uds = bb.fetch2.build_mirroruris(fetcher, mirrors, self.d) - self.assertEqual(uris, ['svn://svn.server2.com/isource/svnroot/reponame/tags/tagname;module=path_in_tagnamefolder;protocol=https;rev=2']) + self.assertEqual(uris, ['svn://svn.server2.com/isource/svnroot/reponame/tags/tagname;module=path_in_tagnamefolder;protocol=https;rev=2', 'file:///someotherpath/downloads/isource/svnroot/reponame/tags/path_in_tagnamefolder_svn.server1.com_.isource.svnroot.reponame.tags.tagname_2_1.tar.gz']) def test_mirror_of_mirror(self): # Test if mirror of a mirror works