| Message ID | 20230317081916.1857201-3-frederic.martinsons@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bitbake-devel,v2,1/3] fetch2: Add checksum capability for crate fetcher | expand |
Please cherry-pick this to Kirkstone and Langdale so that they can read updated recipes that use crate:// URIs with parameters. I assume it should be ok to cherry-pick part one of this series too, as long as you do not cherry-pick part two, as it would be a breaking change. //Peter > -----Original Message----- > From: bitbake-devel@lists.openembedded.org <bitbake-devel@lists.openembedded.org> On Behalf Of Frederic Martinsons > Sent: den 17 mars 2023 09:19 > To: bitbake-devel@lists.openembedded.org > Cc: Frederic Martinsons <frederic.martinsons@gmail.com> > Subject: [bitbake-devel][PATCH v2 3/3] crate.py: authorize crate url with parameters > > From: Frederic Martinsons <frederic.martinsons@gmail.com> > > This allow to have classic fetch parameters > (like destsuffix, sha256, name ...) not being > considered by crate fetcher itself (and so mess > up its download) > > Moreover, it allow to overload the name of the downloaded > crate (maybe usefull if there is a naming clash between > two crates coming from different repositories) > > Signed-off-by: Frederic Martinsons <frederic.martinsons@gmail.com> > --- > lib/bb/fetch2/crate.py | 9 ++++++--- > lib/bb/tests/fetch.py | 24 ++++++++++++++++++++++++ > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/lib/bb/fetch2/crate.py b/lib/bb/fetch2/crate.py > index f8367ed3..590dc9c1 100644 > --- a/lib/bb/fetch2/crate.py > +++ b/lib/bb/fetch2/crate.py > @@ -56,8 +56,10 @@ class Crate(Wget): > if len(parts) < 5: > raise bb.fetch2.ParameterError("Invalid URL: Must be crate://HOST/NAME/VERSION", ud.url) > > - # last field is version > - version = parts[len(parts) - 1] > + # version is expected to be the last token > + # but ignore possible url parameters which will be used > + # by the top fetcher class > + version, _, _ = parts[len(parts) -1].partition(";") > # second to last field is name > name = parts[len(parts) - 2] > # host (this is to allow custom crate registries to be specified > @@ -69,7 +71,8 @@ class Crate(Wget): > > ud.url = "https://%s/%s/%s/download" % (host, name, version) > ud.parm['downloadfilename'] = "%s-%s.crate" % (name, version) > - ud.parm['name'] = name > + if 'name' not in ud.parm: > + ud.parm['name'] = name > > logger.debug2("Fetching %s to %s" % (ud.url, ud.parm['downloadfilename'])) > > diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py > index fd089bc8..85cf25e7 100644 > --- a/lib/bb/tests/fetch.py > +++ b/lib/bb/tests/fetch.py > @@ -2423,6 +2423,30 @@ class CrateTest(FetcherTest): > self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/time-0.1.35/.cargo-checksum.json")) > self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/time-0.1.35/src/lib.rs")) > > + @skipIfNoNetwork() > + def test_crate_url_params(self): > + > + uri = "crate://crates.io/aho-corasick/0.7.20;name=aho-corasick-renamed" > + self.d.setVar('SRC_URI', uri) > + > + uris = self.d.getVar('SRC_URI').split() > + d = self.d > + > + fetcher = bb.fetch2.Fetch(uris, self.d) > + ud = fetcher.ud[fetcher.urls[0]] > + > + self.assertIn("name", ud.parm) > + self.assertEqual(ud.parm["name"], "aho-corasick-renamed") > + self.assertIn("downloadfilename", ud.parm) > + self.assertEqual(ud.parm["downloadfilename"], "aho-corasick-0.7.20.crate") > + > + fetcher.download() > + fetcher.unpack(self.tempdir) > + self.assertEqual(sorted(os.listdir(self.tempdir)), ['cargo_home', 'download' , 'unpacked']) > + self.assertEqual(sorted(os.listdir(self.tempdir + "/download")), ['aho-corasick-0.7.20.crate', 'aho-corasick-0.7.20.crate.done']) > + self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/aho-corasick-0.7.20/.cargo-checksum.json")) > + self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/aho-corasick-0.7.20/src/lib.rs")) > + > @skipIfNoNetwork() > def test_crate_incorrect_cksum(self): > uri = "crate://crates.io/aho-corasick/0.7.20" > -- > 2.34.1
On Thu, 2023-04-06 at 03:40 +0000, Peter Kjellerstedt wrote: > Please cherry-pick this to Kirkstone and Langdale so that they can read > updated recipes that use crate:// URIs with parameters. > > I assume it should be ok to cherry-pick part one of this series too, as > long as you do not cherry-pick part two, as it would be a breaking change. If we do this does this mean we're committing to forwards compatibility? I'm not sure I like that precedent... Cheers, Richard
> -----Original Message----- > From: Richard Purdie <richard.purdie@linuxfoundation.org> > Sent: den 6 april 2023 10:43 > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; Steve Sakoman > <steve@sakoman.com> > Cc: Frederic Martinsons <frederic.martinsons@gmail.com>; bitbake- > devel@lists.openembedded.org > Subject: Re: [bitbake-devel][PATCH v2 3/3] crate.py: authorize crate url > with parameters > > On Thu, 2023-04-06 at 03:40 +0000, Peter Kjellerstedt wrote: > > Please cherry-pick this to Kirkstone and Langdale so that they can read > > updated recipes that use crate:// URIs with parameters. > > > > I assume it should be ok to cherry-pick part one of this series too, as > > long as you do not cherry-pick part two, as it would be a breaking > change. > > If we do this does this mean we're committing to forwards compatibility? > I'm not sure I like that precedent... While not a requirement, I sure hope we can strive for forward compatibility where it does not impact the older branches negatively. At least in cases where it affects parsability or involves parts that are hard to override in a higher layer. The problem for me in this case is that Mickledore now requires that all URIs have a checksum, which basically requires that parameters are used for the crate:// URIs since it is quite common that recipes depend on multiple versions of the same crate and thus requires the name= parameter. At the same time the crate fetcher in Kirkstone and Langdale will fail if any parameters are specified for the URIs. This means it is not possible to use the same version of the generated crates.inc files, which causes a lot of maintenance burden. It is also very hard to modify the crate fetcher without actually modifying OE-Core. At least I find my Python-fu lacking in regards to how I can monkeypatch the fetcher to use a modified version of crate.py. On the other, it is trivial for you to backport this patch to the two branches in OE-Core. And as long as the second patch in the series (which requires that checksums are used) is not backported, I cannot see that it should have any negative impact. > > Cheers, > > Richard //Peter
On Thu, 2023-04-06 at 10:56 +0000, Peter Kjellerstedt wrote: > > -----Original Message----- > > From: Richard Purdie <richard.purdie@linuxfoundation.org> > > Sent: den 6 april 2023 10:43 > > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; Steve Sakoman > > <steve@sakoman.com> > > Cc: Frederic Martinsons <frederic.martinsons@gmail.com>; bitbake- > > devel@lists.openembedded.org > > Subject: Re: [bitbake-devel][PATCH v2 3/3] crate.py: authorize crate url > > with parameters > > > > On Thu, 2023-04-06 at 03:40 +0000, Peter Kjellerstedt wrote: > > > Please cherry-pick this to Kirkstone and Langdale so that they can read > > > updated recipes that use crate:// URIs with parameters. > > > > > > I assume it should be ok to cherry-pick part one of this series too, as > > > long as you do not cherry-pick part two, as it would be a breaking > > change. > > > > If we do this does this mean we're committing to forwards compatibility? > > I'm not sure I like that precedent... > > While not a requirement, I sure hope we can strive for forward compatibility > where it does not impact the older branches negatively. At least in cases > where it affects parsability or involves parts that are hard to override > in a higher layer. The trouble is I take this patch, then next time something breaks you tell me "in the past such changes were only made so that this didn't break things" and quote it as precedent. I worry a lot about people relying upon us doing this as it isn't a guarantee we've ever made. > The problem for me in this case is that Mickledore now requires that all > URIs have a checksum, which basically requires that parameters are used > for the crate:// URIs since it is quite common that recipes depend on > multiple versions of the same crate and thus requires the name= parameter. > At the same time the crate fetcher in Kirkstone and Langdale will fail > if any parameters are specified for the URIs. This means it is not possible > to use the same version of the generated crates.inc files, which causes > a lot of maintenance burden. I understand the problem but you're effectively asking we would always do this going forward too :/. > It is also very hard to modify the crate fetcher without actually modifying > OE-Core. At least I find my Python-fu lacking in regards to how I can > monkeypatch the fetcher to use a modified version of crate.py. > > On the other, it is trivial for you to backport this patch to the two > branches in OE-Core. And as long as the second patch in the series (which > requires that checksums are used) is not backported, I cannot see that it > should have any negative impact. This change alone is fine. The behaviour where people expect older layers to always work with newer code changes is what worries me, a lot. We have done things to make this happen before and I'm not sure it is good to continue the expectation. Cheers, Richard
diff --git a/lib/bb/fetch2/crate.py b/lib/bb/fetch2/crate.py index f8367ed3..590dc9c1 100644 --- a/lib/bb/fetch2/crate.py +++ b/lib/bb/fetch2/crate.py @@ -56,8 +56,10 @@ class Crate(Wget): if len(parts) < 5: raise bb.fetch2.ParameterError("Invalid URL: Must be crate://HOST/NAME/VERSION", ud.url) - # last field is version - version = parts[len(parts) - 1] + # version is expected to be the last token + # but ignore possible url parameters which will be used + # by the top fetcher class + version, _, _ = parts[len(parts) -1].partition(";") # second to last field is name name = parts[len(parts) - 2] # host (this is to allow custom crate registries to be specified @@ -69,7 +71,8 @@ class Crate(Wget): ud.url = "https://%s/%s/%s/download" % (host, name, version) ud.parm['downloadfilename'] = "%s-%s.crate" % (name, version) - ud.parm['name'] = name + if 'name' not in ud.parm: + ud.parm['name'] = name logger.debug2("Fetching %s to %s" % (ud.url, ud.parm['downloadfilename'])) diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py index fd089bc8..85cf25e7 100644 --- a/lib/bb/tests/fetch.py +++ b/lib/bb/tests/fetch.py @@ -2423,6 +2423,30 @@ class CrateTest(FetcherTest): self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/time-0.1.35/.cargo-checksum.json")) self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/time-0.1.35/src/lib.rs")) + @skipIfNoNetwork() + def test_crate_url_params(self): + + uri = "crate://crates.io/aho-corasick/0.7.20;name=aho-corasick-renamed" + self.d.setVar('SRC_URI', uri) + + uris = self.d.getVar('SRC_URI').split() + d = self.d + + fetcher = bb.fetch2.Fetch(uris, self.d) + ud = fetcher.ud[fetcher.urls[0]] + + self.assertIn("name", ud.parm) + self.assertEqual(ud.parm["name"], "aho-corasick-renamed") + self.assertIn("downloadfilename", ud.parm) + self.assertEqual(ud.parm["downloadfilename"], "aho-corasick-0.7.20.crate") + + fetcher.download() + fetcher.unpack(self.tempdir) + self.assertEqual(sorted(os.listdir(self.tempdir)), ['cargo_home', 'download' , 'unpacked']) + self.assertEqual(sorted(os.listdir(self.tempdir + "/download")), ['aho-corasick-0.7.20.crate', 'aho-corasick-0.7.20.crate.done']) + self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/aho-corasick-0.7.20/.cargo-checksum.json")) + self.assertTrue(os.path.exists(self.tempdir + "/cargo_home/bitbake/aho-corasick-0.7.20/src/lib.rs")) + @skipIfNoNetwork() def test_crate_incorrect_cksum(self): uri = "crate://crates.io/aho-corasick/0.7.20"