Hello!

On Wed, 23 Sep 2020, Dmitry V. Levin wrote:

> On Wed, Sep 23, 2020 at 02:11:43AM +0000, Ivan Zakharyaschev wrote:
> > Update of /people/imz/packages/perl-AptPkg.git
> > 
> > Changes statistics since common ancestor `0.1.26-alt4.qa1.2-1-g0a9f2dc' follows:
> >  perl-AptPkg.spec | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> > 
> > Changelog since common ancestor `0.1.26-alt4.qa1.2-1-g0a9f2dc' follows:

> > commit 1165ebffa2e4788baed39685235f0ddc0df434c5
> > Author: Ivan Zakharyaschev <imz@altlinux.org>
> > Date:   Mon Sep 14 22:41:37 2020 +0300
> > 
> >     (.spec) C++ standard increased for std::optional from the new APT
> >     
> >     Luckily, lcc >= 1.24.11 (in p9_e2k) already knows about c++17.

> > commit 191e2e74f2a57e6393b9977f84f64ced17b94da6
> > Author: Ivan Zakharyaschev <imz@altlinux.org>
> > Date:   Mon Sep 14 21:45:38 2020 +0300
> > 
> >     -Wsuggest-override (as an error) as a style enforcement
> >     
> >     to make sure we haven't missed any overrides.
> >     
> >     If have marked all overrides now, in future, when API changes, the
> >     compiler would detect dependent required changes of the methods that
> >     are intended to be overrides.
> > 
> > commit 9c2d17bbb3283adb897681e9005e71709dbdedaa
> > Author: Ivan Zakharyaschev <imz@altlinux.org>
> > Date:   Mon Sep 14 21:43:54 2020 +0300
> > 
> >     -Woverloaded-virtual made an error (to avoid API misuse)
> >     
> >     This helps prevent coding errors when an override of a method was
> >     intended, but it has a wrong signature: then it would trigger this
> >     warning, because the overriding method in the derived class hides all
> >     other overloaded methods from the base class (according to the C++
> >     standard).

> > Full diff since common ancestor `0.1.26-alt4.qa1.2-1-g0a9f2dc' follows:
> > diff --git a/perl-AptPkg.spec b/perl-AptPkg.spec
> > index 4d23441..f36dd7c 100644
> > --- a/perl-AptPkg.spec
> > +++ b/perl-AptPkg.spec
> > @@ -3,7 +3,7 @@
> >  
> >  Name: perl-AptPkg
> >  Version: 0.1.26
> > -Release: alt5
> > +Release: alt6
> >  
> >  Summary: Perl interface to libapt-pkg
> >  License: GPLv2+
> > @@ -14,7 +14,8 @@ Source: libapt-pkg-perl-%version.tar
> >  Patch: %name-%version-%release.patch
> >  
> >  # Automatically added by buildreq on Wed Oct 12 2011
> > -BuildRequires: apt gcc-c++ libapt-devel perl-devel
> > +BuildRequires: gcc-c++ libapt-devel perl-devel
> > +%{?!_without_test:%{?!_disable_test:BuildPreReq: apt}}
> >  
> >  %description
> >  A Perl interface to APT's libapt-pkg which provides modules
> > @@ -24,13 +25,22 @@ inspection of the binary package cache and source package details.
> >  %prep
> >  %setup -n libapt-pkg-perl-%version
> >  %patch -p1
> > -cp -a /etc/apt/* t/cache/etc/
> > +%{?!_without_test:%{?!_disable_test:cp -a /etc/apt/* t/cache/etc/}}
> >  
> >  %build
> > +# Needed by APT API:
> > +%add_optflags -std=gnu++17
> > +
> >  %ifarch %e2k
> > -%add_optflags -std=c++14
> > +%remove_optflags -Wno-error
> >  %endif
> > -%perl_vendor_build INC=-I%_includedir/rpm ||:
> > +
> > +# To avoid some errors on API change:
> > +%add_optflags -Werror=overloaded-virtual
> > +# A style enforcement: always use the keyword, which helps to avoid API misuse
> > +%add_optflags -Werror=suggest-override
> > +
> > +%perl_vendor_build INC=-I%_includedir/rpm %{?!_without_test:%{?!_disable_test:||:}}
> >  
> >  %install
> >  %perl_vendor_install
> > @@ -45,6 +55,13 @@ cp -a /etc/apt/* t/cache/etc/
> >  	%perl_vendor_autolib/AptPkg/AptPkg.so
> >  
> >  %changelog
> > +* Tue Sep 15 2020 Ivan Zakharyaschev <imz@altlinux.org> 0.1.26-alt6
> > +- (.spec) Just added some compiler flags (which don't change anything)
> > +  to be sure that APT API has not been used wrongly and will not be.
> > +  (Namely, to be sure that if a method override was intended, it would
> > +  actually be overriding and not hiding a virtual method and that it
> > +  would be marked "override" for future.)
> 
> Вот смотрю я на это и думаю, что бы у нас было в репозитории, если бы для
> каждой библиотеки надо было бы каждый раз вручную патчить всех её
> клиентов, чтобы синхронизировать compiler flags?
> 
> Просьба считать это письмо за blanket disapprove на все сборочные задания,
> реализующие такой подход.
> 
> Пожалуйста, используйте pkg-config или аналоги.  Спасибо,

Из бесспорных флагов, которые должны по-хорошему получаться через 
pkg-config -- только -std в данном случае. Тоже об этом задумывался, но не 
реализовал. Эту часть можно принять бесспорно как то, что следует всё ж 
реализовать, чтобы так по-глупому не патчить CXXFLAGS каждый раз, когда 
что-то случается.

Что касается -Wsuggest-override и т.п. -- это, с одной стороны, зона 
ответственности самого проекта, который использует библиотеку APT, 
свойство его исходного кода. (Тогда этому, конечно, более хорошее место не 
в спек-файле, а в Makefile и аналогах.) Приводить его в соответствие при 
возникшем желании можно независимо.

С другой стороны, включение этих флагов придаёт больше уверенности в том, 
что мы избежим поломки задуманного поведения этих клиентов при внесении 
изменения в API APT, поэтому как у изменявшего API APT у меня возникает 
желание blanket-включения этих флагов у всех клиентов (в тех кусочках 
кода, которые используют это API).

Т.е. их включение с одной стороны -- по желанию мейнтейнеров проектов 
клиентов, а с другой стороны -- пожелание (но не строгое требование с 
формальной точки зрения собираемости) мейнтейнера APT.

Мне кажется, их можно один раз добавить в проекты и больше не менять, не 
возвращаться к этому в клиентах. А потом для спокойствия мейнтейнеров APT 
ещё добавить в pkg-config APT-а. :)

-- 
Best regards,
Ivan