See bug 920095 where we crashed on a stale entry in Packages. We should catch the error in _do_global_updates and give a nicer error in that case. It's also possible, I suppose, that it happens because of binpkg-pkgdir-index-trusted if the PKGDIR is manually modified.
We can make the bindbapi move_ent, move_slot_ent, and update_ents methods handle it by calling binarytree.remove to clean up the state. We can use a different exception type to distinguish this case from any other KeyError, or possibly just use "raise from" to set the __cause__ of the KeyError to the FileNotFoundError.
Thanks Zac! That sounds like a better approach. I'll try that.
Created attachment 880873 [details] test_move_ent.py modified I hit something I find odd so far. 1) With the attached modified test_move_ent.py + 'pytest lib/portage/tests/update/test_move_ent.py -s -vvv -k testMoveEntWithCorruptIndex', I get: /home/sam/git/portage/lib/portage/dbapi/__init__.py:428: UserWarning: gpkg file structure mismatch in /tmp/tmpug3ckom4/pkgdir/dev-libs/B/B-1-1.gpkg.tar; files: ['B-1/gpkg-1', 'B-1-1/metadata.tar.zst', 'B-1/image.tar.zst', 'B-1-1/Manifest'] 2) If I uncomment the os.remove line, I get a portage.exception.FileNotFound. Not yet figured out how to trigger a KeyError in the test yet.
(In reply to Sam James from comment #3) > Created attachment 880873 [details] > test_move_ent.py modified > > I hit something I find odd so far. > > 1) With the attached modified test_move_ent.py + 'pytest > lib/portage/tests/update/test_move_ent.py -s -vvv -k > testMoveEntWithCorruptIndex', I get: > > /home/sam/git/portage/lib/portage/dbapi/__init__.py:428: UserWarning: gpkg > file structure mismatch in > /tmp/tmpug3ckom4/pkgdir/dev-libs/B/B-1-1.gpkg.tar; files: ['B-1/gpkg-1', > 'B-1-1/metadata.tar.zst', 'B-1/image.tar.zst', 'B-1-1/Manifest'] Can you try applying your change(s) on top of https://github.com/gentoo/portage/pull/1220 and see if you still have an issue? > 2) If I uncomment the os.remove line, I get a > portage.exception.FileNotFound. Not yet figured out how to trigger a > KeyError in the test yet. In https://github.com/gentoo/portage/pull/1220 the test case is properly triggering a KeyError after bindb.bintree.remove(cpv).
Created attachment 880879 [details, diff] corrupt.patch (In reply to Zac Medico from comment #4) > (In reply to Sam James from comment #3) > > Created attachment 880873 [details] > > test_move_ent.py modified > > > > I hit something I find odd so far. > > > > 1) With the attached modified test_move_ent.py + 'pytest > > lib/portage/tests/update/test_move_ent.py -s -vvv -k > > testMoveEntWithCorruptIndex', I get: > > > > /home/sam/git/portage/lib/portage/dbapi/__init__.py:428: UserWarning: gpkg > > file structure mismatch in > > /tmp/tmpug3ckom4/pkgdir/dev-libs/B/B-1-1.gpkg.tar; files: ['B-1/gpkg-1', > > 'B-1-1/metadata.tar.zst', 'B-1/image.tar.zst', 'B-1-1/Manifest'] > > Can you try applying your change(s) on top of > https://github.com/gentoo/portage/pull/1220 and see if you still have an > issue? > I do! $ pytest lib/portage/tests/update/test_move_ent.py -s -vvv -k testMoveEntWithCorruptIndex [...] lib/portage/tests/update/test_move_ent.py::MoveEntTestCase::testMoveEntWithCorruptIndex /home/sam/git/portage/lib/portage/dbapi/__init__.py:432: UserWarning: gpkg file structure mismatch in /tmp/tmpo1x85ec1/pkgdir/dev-libs/B/B-1-1.gpkg.tar; files: ['B-1/gpkg-1', 'B-1-1/metadata.tar.zst', 'B-1/image.tar.zst', 'B-1-1/Manifest'] warnings.warn(e) with attached patch on top of your PR
(In reply to Sam James from comment #5) > $ pytest lib/portage/tests/update/test_move_ent.py -s -vvv -k > testMoveEntWithCorruptIndex > [...] > lib/portage/tests/update/test_move_ent.py::MoveEntTestCase:: > testMoveEntWithCorruptIndex > /home/sam/git/portage/lib/portage/dbapi/__init__.py:432: UserWarning: gpkg > file structure mismatch in > /tmp/tmpo1x85ec1/pkgdir/dev-libs/B/B-1-1.gpkg.tar; files: ['B-1/gpkg-1', > 'B-1-1/metadata.tar.zst', 'B-1/image.tar.zst', 'B-1-1/Manifest'] > warnings.warn(e) > > with attached patch on top of your PR There's something wrong with the old_basename logic in the gpkg update_metadata method, since the test passes with this change than causes old_basename to be B-1-1 instead of B-1: --- a/lib/portage/gpkg.py +++ b/lib/portage/gpkg.py @@ -995,13 +995,13 @@ class gpkg: def update_metadata(self, metadata, new_basename=None, force=False): """ Update metadata in the gpkg file. """ self._verify_binpkg() self.checksums = [] - old_basename = self.prefix + old_basename = self.basename if self.signature_exist and not force: raise SignedPackage("Cannot update a signed gpkg file") if new_basename is None: new_basename = old_basename
(In reply to Zac Medico from comment #6) Also, this has nothing to do with the os.remove part of your test case, since it behaves the same if you comment out the os.remove lines. It's just something about the way that aux_update calls the gpkg constructor and update_metadata method. I checked the state in aux_update, and cpv_str was dev-libs/B-1-1, which explains why self.basename has the correct B-1-1 value.
(In reply to Zac Medico from comment #6) > There's something wrong with the old_basename logic in the gpkg > update_metadata method, since the test passes with this change than causes > old_basename to be B-1-1 instead of B-1: > > --- a/lib/portage/gpkg.py > +++ b/lib/portage/gpkg.py > @@ -995,13 +995,13 @@ class gpkg: > def update_metadata(self, metadata, new_basename=None, force=False): > > - old_basename = self.prefix > + old_basename = self.basename This part has been unchanged since: commit 1d94992a2df2b5cc963c26c7978a899dc642deb1 Author: Sheng Yu <syu.os@protonmail.com> Date: Thu Sep 1 10:44:55 2022 -0400 Move all files into basename/DATA structure Signed-off-by: Sheng Yu <syu.os@protonmail.com> Signed-off-by: Michał Górny <mgorny@gentoo.org>
I wonder if this is a leftover from trying to handle corrupted early gpkgs, if it's not just a typo/copypaste error. In _get_inner_tarinfo, for example, we do: ``` if self.basename and self.prefix and not self.prefix.startswith(self.basename): writemsg( colorize("WARN", f"Package basename mismatched, using {self.prefix}") ) ```
(In reply to Sam James from comment #9) > In _get_inner_tarinfo, for example, we do: > Or maybe we should copy that logic for figuring out the old_basename. At least in other places, we prefer basename > prefix > abort.
This will make the test pass without changing the gpkg internals: --- a/lib/portage/dbapi/bintree.py +++ b/lib/portage/dbapi/bintree.py @@ -329,7 +329,7 @@ class bindbapi(fakedbapi): if binpkg_format == "xpak": mytbz2.recompose_mem(portage.xpak.xpak_mem(mydata)) elif binpkg_format == "gpkg": - mybinpkg.update_metadata(mydata) + mybinpkg.update_metadata(mydata, new_basename=cpv_str) else: raise InvalidBinaryPackageFormat( f"Unknown binary package format {binpkg_path}"
Adding BUILD_ID to the gpkg basename/prefix in ResolverPlayground will suppress the aux_update issue: --- a/lib/portage/tests/resolver/ResolverPlayground.py +++ b/lib/portage/tests/resolver/ResolverPlayground.py @@ -394,7 +394,12 @@ class ResolverPlayground: t = portage.xpak.tbz2(binpkg_path) t.recompose_mem(portage.xpak.xpak_mem(metadata)) elif binpkg_format == "gpkg": - t = portage.gpkg.gpkg(self.settings, a.cpv, binpkg_path) + t = portage.gpkg.gpkg( + self.settings, + a.cpv + + (f"-{metadata['BUILD_ID']}" if metadata.get("BUILD_ID") else ""), + binpkg_path, + ) t.compress(os.path.dirname(binpkg_path), metadata) else: raise InvalidBinaryPackageFormat(binpkg_format)
If we do that latter change, I worry we lose a useful test case for package structure mismatches even though it feels okay by itself :( I hit this kind of issue in the wild within the last few weeks but couldn't reproduce it manually until now; all the fixes we did so far were for e.g signed pkgs which my case wasn't, so I think we should go for that caller patch instead of changing the internals? Not sure what to do about the tests then though...
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=fb1d0a22f65747b750143080536a4129e8654f97 commit fb1d0a22f65747b750143080536a4129e8654f97 Author: Zac Medico <zmedico@gentoo.org> AuthorDate: 2023-12-29 00:29:40 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2023-12-30 21:46:04 +0000 dbapi: KeyError tolerance during package moves Raise a new CorruptionKeyError exception type instead of a plain KeyError when os.stat fails. Treat this type of exception as a warning during package moves. Also fix this error that the test case triggered in the binarytree.remove method: NameError: name 'binpkg_path' is not defined Bug: https://bugs.gentoo.org/920828 Signed-off-by: Zac Medico <zmedico@gentoo.org> lib/portage/dbapi/__init__.py | 14 ++++-- lib/portage/dbapi/bintree.py | 28 +++++++++--- lib/portage/dbapi/vartree.py | 9 ++-- lib/portage/exception.py | 6 ++- lib/portage/tests/update/test_update_dbentry.py | 60 +++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 14 deletions(-)
(In reply to Sam James from comment #13) > If we do that latter change, I worry we lose a useful test case for package > structure mismatches even though it feels okay by itself :( Since update_metadata calls _create_tarinfo it doesn't make any sense for update_metadata to use different logic than what _create_tarinfo uses here: if self.basename: basename = self.basename elif self.prefix: basename = self.prefix else: raise InvalidBinaryPackageFormat("No basename or prefix specified") This makes the test pass: diff --git a/lib/portage/gpkg.py b/lib/portage/gpkg.py index 2e1130857b..031b3f87cb 100644 --- a/lib/portage/gpkg.py +++ b/lib/portage/gpkg.py @@ -998,13 +998,16 @@ class gpkg: """ self._verify_binpkg() self.checksums = [] - old_basename = self.prefix - if self.signature_exist and not force: raise SignedPackage("Cannot update a signed gpkg file") if new_basename is None: - new_basename = old_basename + if self.basename: + new_basename = self.basename + elif self.prefix: + new_basename = self.prefix + else: + raise InvalidBinaryPackageFormat("No basename or prefix specified") else: new_basename = new_basename.split("/", maxsplit=1)[-1] self.basename = new_basename
oh great, that works then, then shove https://docs.pytest.org/en/7.1.x/how-to/capture-warnings.html#pytest-mark-filterwarnings onto the test so it fails w/ the warning
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=1e9f25d74fd6df7ef54edffe496499dc1711e911 commit 1e9f25d74fd6df7ef54edffe496499dc1711e911 Author: Sam James <sam@gentoo.org> AuthorDate: 2023-12-31 18:14:33 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2023-12-31 22:23:27 +0000 gpkg: fix basename handling via aux_update When using a binpkg with BUILD_ID, we might sometimes get a UserWarning for mismatched/corrupt gpkg structure: ``` UserWarning: gpkg file structure mismatch in /tmp/tmpo1x85ec1/pkgdir/dev-libs/B/B-1-1.gpkg.tar; files: ['B-1/gpkg-1', 'B-1-1/metadata.tar.zst', 'B-1/image.tar.zst', 'B-1-1/Manifest'] warnings.warn(e) ``` update_metadata directly uses self.prefix instead of self.basename, while e.g. _get_inner_tarinfo prefers self.basename > self.prefix > error. Given update_metadata calls _create_tarinfo, use the same logic as it and _get_inner_tarinfo to avoid corrupting the filename for the image. Bug: https://bugs.gentoo.org/920828 Thanks-to: Zac Medico <zmedico@gentoo.org> Signed-off-by: Sam James <sam@gentoo.org> lib/portage/gpkg.py | 9 ++- lib/portage/tests/update/test_move_ent.py | 92 ++++++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 4 deletions(-)
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=73c980cc99406a9257659adbe10af348897d7a98 commit 73c980cc99406a9257659adbe10af348897d7a98 Author: Sam James <sam@gentoo.org> AuthorDate: 2024-01-02 03:38:51 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2024-01-02 03:38:51 +0000 sys-apps/portage: add 3.0.60 Closes: https://bugs.gentoo.org/920827 Closes: https://bugs.gentoo.org/920828 Closes: https://bugs.gentoo.org/921089 Closes: https://bugs.gentoo.org/921107 Signed-off-by: Sam James <sam@gentoo.org> sys-apps/portage/Manifest | 1 + sys-apps/portage/portage-3.0.60.ebuild | 246 +++++++++++++++++++++++++++++++++ 2 files changed, 247 insertions(+)
