[devel] PATCH for apt: custom callbacks
Ivan Zakharyaschev
imz на altlinux.org
Чт Июл 15 15:24:48 MSK 2021
Здравствуйте!
Пока ясного согласия мейнтейнеров по общей архитектуре custom callbacks в
apt нет, уделю внимание двум частным вопросам.
Во-первых, виртуальные методы с дефолтными параметрами -- запутывающая
фича в C++. (Потому что дефолтные значения подбираются не через механизм
таблиц виртуальных методов, т.е. не так, как реализация метода.) Пример из
обсуждаемого патча:
@@ -75,17 +101,17 @@ class pkgPackageManager : protected pkgCache::Namespace
// The Actual installation implementation
virtual bool Install(PkgIterator /*Pkg*/,const string &/*File*/) {return false;}
virtual bool Configure(PkgIterator /*Pkg*/) {return false;}
virtual bool Remove(PkgIterator /*Pkg*/,bool /*Purge*/=false) {return false;}
- virtual bool Go() {return true;}
+ virtual bool Go(PackageManagerCallback_t /*callback*/ = nullptr, void * /*callbackData*/ = nullptr) {return true;}
virtual void Reset() {}
public:
Так что я в целом в коде поизбавлялся от всех таких старых мест, и новое
переписал аналогично. См. ветку callbacks у меня. (Вставлю патчи в конце
сообщения.)
Во-вторых, сигнатура метода меняется (при этом дефолтные параметры не надо
учитывать, потому что они другим механизмом подбираются), поэтому обычное
поведение мейнтейнеров было бы изменить soname. Однако, это всё (что
связано с package manager и взиамодействием с rpm) -- довольно внутренняя
часть apt, нечасто используемая внешними клиентами библиотеки (насколько я
понимаю). А также: часть изменений функций у нас в ALT поймает rpm с
set-versions на библиотеки. Эти два соображения могли бы быть аргументом,
чтобы не менять soname и не пересобирать весь набор клиентов. Что думаете?
Менять soname?
Ну и другие мейнтейнеры apt -- за то, чтбы сделать по-своему пока это API
без забирания нарботок из apt-rpm, против, или им всё равно? Если за
сближение с apt-rpm, то кто возьмётся?
Мои патчи с избавлением от дефолтных параметров в виртуаьных методах,
которые я предлагаю оставить в любом случае (за исключением обсуждаемого
вопроса с soname):
$ git --no-pager log -p --reverse test-pk..virtual-has-no-default-param
commit 8f35f2dff70dfe39b853fdef0ac4516dae93f1cb
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Fri Jul 9 20:57:31 2021 +0300
(.spec) move some (non-idempotent) actions into %%prep
diff --git a/apt.spec b/apt.spec
index deee52a23..3506f3560 100644
--- a/apt.spec
+++ b/apt.spec
@@ -225,7 +225,6 @@ This package contains test suite for APT.
%prep
%setup -n %name-%version-%release
-%build
# Fix url.
sed -i 's,/usr/share/common-licenses/GPL,/usr/share/license/GPL,' COPYING
@@ -235,6 +234,7 @@ sed -i 's, > /dev/null 2>&1,,' buildlib/tools.m4
# Add trivial arch translation.
printf '%_target_cpu\t%_target_cpu' >> buildlib/archtable
+%build
gettextize --force --quiet --no-changelog --symlink
%autoreconf
commit 350aaf2dc5fdca23dd447cc3213f091912830f4b
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Fri Jul 9 21:04:01 2021 +0300
(.spec) add a hook to verify that the source conforms to some policies
Will be useful when rebasing our patches (on newer apt-rpm or Debian releases)
and enforcing the policies earlier than in our patches now. And in other ways
during maintenance.
diff --git a/apt.spec b/apt.spec
index 3506f3560..97db63ea6 100644
--- a/apt.spec
+++ b/apt.spec
@@ -225,6 +225,8 @@ This package contains test suite for APT.
%prep
%setup -n %name-%version-%release
+./verify-src.sh
+
# Fix url.
sed -i 's,/usr/share/common-licenses/GPL,/usr/share/license/GPL,' COPYING
diff --git a/verify-src.sh b/verify-src.sh
new file mode 100755
index 000000000..3897da4b2
--- /dev/null
+++ b/verify-src.sh
@@ -0,0 +1,26 @@
+#!/bin/sh -euC
+set -o pipefail
+shopt -s nullglob
+
+# Verify that the source code conforms to some policies.
+# (Some policies--other or the same--are verified by compiler flags.)
+# It's to be invoked from the .spec file; and one could invoke it
+# from a Git hook on commit.
+
+MY="$(dirname -- "$(realpath -- "$0")")"
+readonly MY
+
+for f in "$MY"/verify-src.d/*; do
+ [ -x "$f" ] || continue
+ case "$f" in
+ *~)
+ continue
+ ;;
+ *.verify)
+ "$f"
+ ;;
+ *) # ignore errors
+ "$f" ||:
+ ;;
+ esac
+done
commit a1b9a98c7de2eac657e84a3f8fb98d9c16abc56d
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Fri Jul 9 21:22:38 2021 +0300
add verify-src.d/virtual-has-no-default-param.sh (result ignored)
diff --git a/verify-src.d/virtual-has-no-default-param.sh b/verify-src.d/virtual-has-no-default-param.sh
new file mode 100755
index 000000000..eb3e5cb60
--- /dev/null
+++ b/verify-src.d/virtual-has-no-default-param.sh
@@ -0,0 +1,19 @@
+#!/bin/sh -efuC
+set -o pipefail
+
+# sed is simpler than grep (especially, under xarg) w.r.t. the exit
status.
+# However, it's more complex to output the filename and line number.
+# (= outputs a line number, but it counts total lines in all files.)
+virtual_with_default_param="$(
+ find -type f -'(' -name '*.h' -or -name '*.cc' -')' -print0 \
+ | xargs -0 --no-run-if-empty \
+ sed -nEe '/virtual [^(]*\([^)]*=|=.*\).*override/ {F; p; }'
+)"
+readonly virtual_with_default_param
+
+[ -z "$virtual_with_default_param" ] || {
+ printf '%s: Found default parameters in virtual methods:\n%s\n' \
+ "${0##*/}" \
+ "$virtual_with_default_param"
+ exit 1
+} >&2
commit d531f9f49e491fb9ca7b6bb87fcf9e3f446388dd
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Fri Jul 9 22:44:43 2021 +0300
bump soversion before changing virtual methods' signatures
In general, if a virtual method is added or deleted or modified (the
signature), the table of virtual methods is modified, and old
positions of any methods might be invalid. So, any code using this
base class or derived classes (and calling its virtual methods) would
be invalid without recompilation.
Another upcoming change is in the default parameters of virtual
methods. The default values are taken from the type known at the place
of the invocation (i.e., they are not inherited or looked up
through the table of virtual methods). For them to be seen and take
effect, the code that uses these methods must be recompiled.
And in any case, it's preferable not have default parameters in
virtual methods (because of the confusion described above: different
values can be declared in different derived classes, but they are used
differently from how the definitions of virtual methods are looked
up). So, I want to rewrite this API. Luckily, it is mostly present
only in the API related to package manager (so, mostly internal to APT),
as can be seen from a run of verify-src.d/virtual-has-no-default-param.sh.
The classes (and their methods) that will be affected
in the upcoming changes:
[only moving the default parameters out of virtual methods declaration;
not modifying the signatures and hence probably not affecting ABI and
not requiring the bump of the soname]
* virtual pkgSystem::UnLock() (from pkgsystem.h)
* virtual pkgPackageManager::Remove() (from packagemanager.h)
* virtual RPMHandler::DepsList() (from rpm/rpmhandler.h)
* virtual pkgIndexFile::Describe() (from indexfile.h)
[modifying the signature--if the new API is accepted after a review;
default parameters shouldn't be taken into account when deciding
whether this affects ABI]
* virtual pkgPackageManager::Go(), non-virtual pkgPackageManager::DoInstall()
(from packagemanager.h)
* virtual pkgRPMPM::Process() (from rpm/rpmpm.h)
Let's decide whether a modification of the methods in these classes
deserves a change of the soname... (Usually, when a function is
deleted or its signature is modified or a structure is modified, the
soversion is bumped. But in ALT, the deletion of a function can be
detected by RPM's set-versions for libraries, so it's not that
necessary. However, I'm in doubts concerning the modification of
virtual methods, i.e., will a deletion of the method with the old
signature be detected by RPM if this method is used in other
packages.)
diff --git a/apt-pkg/Makefile.am b/apt-pkg/Makefile.am
index 171d4b771..c940c567c 100644
--- a/apt-pkg/Makefile.am
+++ b/apt-pkg/Makefile.am
@@ -2,7 +2,7 @@
lib_LTLIBRARIES = libapt-pkg.la
libapt_pkg_la_LIBADD = @RPMLIBS@
-libapt_pkg_la_LDFLAGS = -version-info 10:0:1 -release @GLIBC_VER на -@LIBSTDCPP_VER@@FILE_OFFSET_BITS_SUFFIX@
+libapt_pkg_la_LDFLAGS = -version-info 11:0:1 -release @GLIBC_VER на -@LIBSTDCPP_VER@@FILE_OFFSET_BITS_SUFFIX@
AM_CPPFLAGS = -DLIBDIR=\"$(libdir)\"
AM_CFLAGS = $(WARN_CFLAGS)
commit a82df01b332a7a6977e7a5ece342733189c338d1
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Thu Jul 15 12:24:01 2021 +0300
move a default parameter from virtual pkgSystem::UnLock() to avoid confusion
Default parameters in virtual methods are confusing, because they are
not looked up in the same way as a virtual method's implementation
(through the table of virtual methods of the object; but rather from
the "known" type in the code that uses the method).
Luckily, this occurred in pkgSystem class (a thing mostly internal for APT).
This change probably doesn't affect ABI, since the table of virtual
methods is not modified. After recompilation, the new code will behave
equivalently to the old one; but if one changes the default value,
this will probably take effect only after recompilation (because the
new UnLock() looks like an inlined method defined in the header).
diff --git a/apt-pkg/deb/debsystem.h b/apt-pkg/deb/debsystem.h
index c056cc9bf..75e396c47 100644
--- a/apt-pkg/deb/debsystem.h
+++ b/apt-pkg/deb/debsystem.h
@@ -24,7 +24,7 @@ class debSystem : public pkgSystem
public:
virtual bool Lock();
- virtual bool UnLock(bool NoErrors = false);
+ virtual bool UnLock(bool NoErrors);
virtual pkgPackageManager *CreatePM(pkgDepCache *Cache) const;
virtual bool Initialize(Configuration &Cnf);
virtual bool ArchiveSupported(const char *Type);
diff --git a/apt-pkg/pkgsystem.h b/apt-pkg/pkgsystem.h
index 0e63ab984..e20212726 100644
--- a/apt-pkg/pkgsystem.h
+++ b/apt-pkg/pkgsystem.h
@@ -62,7 +62,9 @@ class pkgSystem
/* Prevent other programs from touching shared data not covered by
other locks (cache or state locks) */
virtual bool Lock() = 0;
- virtual bool UnLock(bool NoErrors = false) = 0;
+ virtual bool UnLock(bool NoErrors) = 0;
+ /* a virtual method with default parameter is confusing; instead, define: */
+ bool UnLock() { return UnLock(false); }
// CNC:2002-07-06
virtual bool LockRead() {return true;}
diff --git a/apt-pkg/rpm/rpmsystem.h b/apt-pkg/rpm/rpmsystem.h
index e52490bf4..8847c531e 100644
--- a/apt-pkg/rpm/rpmsystem.h
+++ b/apt-pkg/rpm/rpmsystem.h
@@ -34,7 +34,7 @@ class rpmSystem : public pkgSystem
virtual bool LockRead() override;
virtual bool Lock() override;
- virtual bool UnLock(bool NoErrors = false) override;
+ virtual bool UnLock(bool NoErrors) override;
virtual pkgPackageManager *CreatePM(pkgDepCache *Cache) const override;
virtual bool Initialize(Configuration &Cnf) override;
virtual bool ArchiveSupported(const char *Type) override;
commit 8015eabc1c4273244a5e722a4e31a308e19192e9
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Thu Jul 15 12:50:35 2021 +0300
move a default parameter from virtual pkgPackageManager::Remove() to avoid confusion
Default parameters in virtual methods are confusing, because they are
not looked up in the same way as a virtual method's implementation
(through the table of virtual methods of the object; but rather from
the "known" type in the code that uses the method).
Luckily, this occurred in pkgPackageManager class (a thing mostly internal for APT).
This change probably doesn't affect ABI, since the table of virtual
methods is not modified. After recompilation, the new code will behave
equivalently to the old one; but if one changes the default value,
this will probably take effect only after recompilation (because the
new Remove() looks like an inlined method defined in the header).
diff --git a/apt-pkg/deb/dpkgpm.h b/apt-pkg/deb/dpkgpm.h
index b768850bf..fc4683fbf 100644
--- a/apt-pkg/deb/dpkgpm.h
+++ b/apt-pkg/deb/dpkgpm.h
@@ -39,7 +39,7 @@ class pkgDPkgPM : public pkgPackageManager
// The Actuall installation implementation
virtual bool Install(PkgIterator Pkg,string File);
virtual bool Configure(PkgIterator Pkg);
- virtual bool Remove(PkgIterator Pkg,bool Purge = false);
+ virtual bool Remove(PkgIterator Pkg,bool Purge);
virtual bool Go();
virtual void Reset();
diff --git a/apt-pkg/packagemanager.h b/apt-pkg/packagemanager.h
index 9c5f04825..aeb1c8a02 100644
--- a/apt-pkg/packagemanager.h
+++ b/apt-pkg/packagemanager.h
@@ -75,7 +75,9 @@ class pkgPackageManager : protected pkgCache::Namespace
// The Actual installation implementation
virtual bool Install(PkgIterator /*Pkg*/,const string &/*File*/) {return false;}
virtual bool Configure(PkgIterator /*Pkg*/) {return false;}
- virtual bool Remove(PkgIterator /*Pkg*/,bool /*Purge*/=false) {return false;}
+ virtual bool Remove(PkgIterator /*Pkg*/,bool /*Purge*/) {return false;}
+ /* a virtual method with default parameter is confusing; instead, define: */
+ bool Remove(const PkgIterator Pkg) {return Remove(Pkg,false);}
virtual bool Go() {return true;}
virtual void Reset() {}
diff --git a/apt-pkg/rpm/rpmpm.h b/apt-pkg/rpm/rpmpm.h
index 8a01b80cc..693bb1e1f 100644
--- a/apt-pkg/rpm/rpmpm.h
+++ b/apt-pkg/rpm/rpmpm.h
@@ -45,7 +45,7 @@ class pkgRPMPM : public pkgPackageManager
// The Actuall installation implementation
virtual bool Install(PkgIterator Pkg,const string &File) override;
virtual bool Configure(PkgIterator Pkg) override;
- virtual bool Remove(PkgIterator Pkg,bool Purge = false) override;
+ virtual bool Remove(PkgIterator Pkg,bool Purge) override;
virtual bool Process(const std::vector<apt_item> &install,
const std::vector<apt_item> &upgrade,
commit acf303214c77dc401f5d6bdb8cb5f85a2a4a6b25
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Thu Jul 15 13:05:13 2021 +0300
move a default parameter from virtual RPMHandler::DepsList() to avoid confusion
Default parameters in virtual methods are confusing, because they are
not looked up in the same way as a virtual method's implementation
(through the table of virtual methods of the object; but rather from
the "known" type in the code that uses the method).
Luckily, this occurred in RPMHandler class (a thing internal for apt-rpm).
This change probably doesn't affect ABI, since the table of virtual
methods is not modified. After recompilation, the new code will behave
equivalently to the old one; but if one changes the default value,
this will probably take effect only after recompilation (because the
new DepsList() looks like an inlined method defined in the header).
diff --git a/apt-pkg/rpm/rpmhandler.h b/apt-pkg/rpm/rpmhandler.h
index cd899058c..54dcc4d50 100644
--- a/apt-pkg/rpm/rpmhandler.h
+++ b/apt-pkg/rpm/rpmhandler.h
@@ -87,7 +87,10 @@ class RPMHandler
virtual string SourceRpm() const = 0;
virtual bool DepsList(unsigned int Type, std::vector<Dependency*> &Deps,
- bool checkInternalDep = true) const = 0;
+ bool checkInternalDep) const = 0;
+ /* a virtual method with default parameter is confusing; instead, define: */
+ bool DepsList(const unsigned int Type, std::vector<Dependency*> &Deps) const
+ { return DepsList(Type,Deps,true); }
virtual bool FileList(std::vector<string> &FileList) const = 0;
virtual bool HasFile(const char *File) const;
@@ -127,7 +130,7 @@ class RPMHdrHandler : public RPMHandler
virtual string SourceRpm() const override {return GetSTag(RPMTAG_SOURCERPM);}
virtual bool DepsList(unsigned int Type, std::vector<Dependency*> &Deps,
- bool checkInternalDep = true) const override;
+ bool checkInternalDep) const override;
virtual bool FileList(std::vector<string> &FileList) const override;
RPMHdrHandler() : RPMHandler(), HeaderP(0) {}
commit cc3e7bd160ee521f52f12cd789b766ff888dd71d
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Thu Jul 15 13:13:05 2021 +0300
move a default parameter from virtual pkgIndexFile::Describe() to avoid confusion
Default parameters in virtual methods are confusing, because they are
not looked up in the same way as a virtual method's implementation
(through the table of virtual methods of the object; but rather from
the "known" type in the code that uses the method).
Not sure how much this pkgIndexFile class is used from outside APT (by
the clients of the library)...
This change probably doesn't affect ABI, since the table of virtual
methods is not modified. After recompilation, the new code will behave
equivalently to the old one; but if one changes the default value,
this will probably take effect only after recompilation (because the
new Describe() looks like an inlined method defined in the header).
diff --git a/apt-pkg/indexfile.h b/apt-pkg/indexfile.h
index 8e000b419..0b26ffdad 100644
--- a/apt-pkg/indexfile.h
+++ b/apt-pkg/indexfile.h
@@ -55,7 +55,9 @@ class pkgIndexFile
virtual string ArchiveInfo(pkgCache::VerIterator Ver) const;
virtual string SourceInfo(pkgSrcRecords::Parser const &Record,
pkgSrcRecords::File const &File) const;
- virtual string Describe(bool Short = false) const = 0;
+ virtual string Describe(bool Short) const = 0;
+ /* a virtual method with default parameter is confusing; instead, define: */
+ string Describe() const { return Describe(false); }
// Interface for acquire
virtual string ArchiveURI(const string &/*File*/) const {return string();}
commit 80558860d80b2e44b11761f2f589bf63dee2dc0b (@ALT/virtual-has-no-default-param, virtual-has-no-default-param)
Author: Ivan Zakharyaschev <imz на altlinux.org>
Date: Fri Jul 9 21:25:42 2021 +0300
verify-src.d/virtual-has-no-default-param.sh: turn on
diff --git a/verify-src.d/virtual-has-no-default-param.sh b/verify-src.d/virtual-has-no-default-param.verify
similarity index 100%
rename from verify-src.d/virtual-has-no-default-param.sh
rename to verify-src.d/virtual-has-no-default-param.verify
--
Best regards,
Ivan
Подробная информация о списке рассылки Devel