PR#1: general spec file improvements, cleanups - rpms/libtdb

		`@@ -1,93 +1,82 @@`
		`%if 0%{?fedora} \|\| 0%{?rhel} > 7`
		`- %global with_python3 1`
		`- %else`
		`- %global with_python3 0`
		`+ %bcond_without python3`
		`%endif`

		`- Name: libtdb`
		`- Version: 1.3.15`
		`- Release: 2%{?dist}`
		`- Summary: The tdb library`
		`- License: LGPLv3+`
		`- URL: http://tdb.samba.org/`
		`- Source: http://samba.org/ftp/tdb/tdb-%{version}.tar.gz`
		`-`
		`- BuildRequires: libxslt`
		`- BuildRequires: docbook-style-xsl`
		`- BuildRequires: python2-devel`
		`- %if 0%{?with_python3}`
		`+ Summary: The tdb library`
		`+ Name: libtdb`
		`+ Version: 1.3.15`
		`+ Release: 3%{?dist}`
		`+ License: LGPLv3+`
		`+ URL: http://tdb.samba.org/`
		`+ Source: http://samba.org/ftp/tdb/tdb-%{version}.tar.gz`
		`+ BuildRequires: docbook-style-xsl`
		`+ BuildRequires: libxslt`
		`+ BuildRequires: pkgconfig`
		`+ BuildRequires: python2-devel`
		`+ %if %{with python3}`
		`BuildRequires: python3-devel`
		`%endif`
		`-`
		`Provides: bundled(libreplace)`
		`-`
		`# Patches`

		`%description`
		`A library that implements a trivial database.`

		`%package devel`
		`- Summary: Header files need to link the Tdb library`
		`- Requires: libtdb = %{version}-%{release}`
		`- Requires: pkgconfig`
		`+ Summary: Header files need to link the Tdb library`
		`+ Requires: libtdb = %{version}-%{release}`

		`%description devel`
		`- Header files needed to develop programs that link against the Tdb library.`
		`+ Header files needed to develop programs that link against the Tdb`
		`+ library.`

		`%package -n tdb-tools`
		`- Summary: Developer tools for the Tdb library`
		`- Requires: libtdb = %{version}-%{release}`
		`+ Summary: Developer tools for the Tdb library`
		`+ Requires: libtdb = %{version}-%{release}`

		`%description -n tdb-tools`
		`- Tools to manage Tdb files`
		`+ Tools to manage Tdb files.`

		`%package -n python2-tdb`
		`- Summary: Python bindings for the Tdb library`
		`- Requires: libtdb = %{version}-%{release}`
		`+ Summary: Python bindings for the Tdb library`
		`+ Requires: libtdb = %{version}-%{release}`
		`%{?python_provide:%python_provide python2-tdb}`

		`%description -n python2-tdb`
		`- Python bindings for libtdb`
		`+ Python bindings for libtdb.`

		`- %if 0%{?with_python3}`
		`%package -n python3-tdb`
		`- Summary: Python3 bindings for the Tdb library`
		`- Requires: libtdb = %{version}-%{release}`
		`+ Summary: Python3 bindings for the Tdb library`
		`+ Requires: libtdb = %{version}-%{release}`
		`%{?python_provide:%python_provide python3-tdb}`

		`%description -n python3-tdb`
		`- Python3 bindings for libtdb`
		`- %endif`
		`+ Python3 bindings for libtdb.`

		`%prep`
		`- %setup -q -n tdb-%{version}`
		`+ %autosetup -p1 -n tdb-%{version}`

		`%build`
		`- %if 0%{?with_python3}`
		`- PY3_CONFIG_FLAGS=--extra-python=%{__python3}`
		`- %else`
		`- PY3_CONFIG_FLAGS=""`
		`- %endif`
		`-`
		`- %configure --disable-rpath \`
		`- --bundled-libraries=NONE \`
		`- --builtin-libraries=replace \`
		`- $PY3_CONFIG_FLAGS`
		`-`
		`- make %{?_smp_mflags} V=1`
		`-`
		`- %check`
		`- make %{?_smp_mflags} check`
		`+ %configure \`
		`+ --bundled-libraries=NONE \`
		`+ --builtin-libraries=replace \`
		`+ --disable-rpath \`
		`+ %{?with_python3:--extra-python=%{__python3}} \`
		`+ %{nil}`
		`+ %{make_build}`

		`%install`
		`-`
		`- make install DESTDIR=$RPM_BUILD_ROOT`
		`+ %{make_install}`

		`# Shared libraries need to be marked executable for`
		`# rpmbuild to strip them and include them in debuginfo`
		`- find $RPM_BUILD_ROOT -name ".so" -exec chmod -c +x {} \;`
		`+ find %{buildroot} -name ".so" -exec chmod -c +x {} \;`

		`- rm -f $RPM_BUILD_ROOT%{_libdir}/libtdb.a`
		`+ %check`
		`+ %{make_build} check`
		`+`
		`+ %post -p /sbin/ldconfig`
		`+ %postun -p /sbin/ldconfig`

		`%files`
		`%{_libdir}/libtdb.so.*`
		`@@ -99,41 +88,33 @@`
		`%{_libdir}/pkgconfig/tdb.pc`

		`%files -n tdb-tools`
		`- %{_bindir}/tdbbackup`
		`- %{_bindir}/tdbdump`
		`- %{_bindir}/tdbtool`
		`- %{_bindir}/tdbrestore`
		`- %{_mandir}/man8/tdbbackup.8*`
		`- %{_mandir}/man8/tdbdump.8*`
		`- %{_mandir}/man8/tdbtool.8*`
		`- %{_mandir}/man8/tdbrestore.8*`
		`+ %{_bindir}/*`
		`+ %{_mandir}/man8/*`

		`%files -n python2-tdb`
		`%{python_sitearch}/tdb.so`
		`%{python_sitearch}/_tdb_text.py*`

		`- %if 0%{?with_python3}`
		`+ %if %{with python3}`
		`%files -n python3-tdb`
		`%{python3_sitearch}/__pycache__/_tdb_text.cpython*.py[co]`
		`%{python3_sitearch}/tdb.cpython*.so`
		`%{python3_sitearch}/_tdb_text.py`
		`%endif`

		`- %post -p /sbin/ldconfig`
		`-`
		`- %postun -p /sbin/ldconfig`
		`-`
		`- %post -n python2-tdb -p /sbin/ldconfig`
		`-`
		`- %postun -n python2-tdb -p /sbin/ldconfig`
		`-`
		`- %if 0%{?with_python3}`
		`- %post -n python3-tdb -p /sbin/ldconfig`
		`-`
		`- %postun -n python3-tdb -p /sbin/ldconfig`
		`- %endif`
		`-`
		`%changelog`
		`+ * Mon Jan 08 2018 Tomasz Kłoczko <kloczek@fedoraproject.org> - 1.3.15-3`
		`+ - remove pkgconfig from devel Requires (provide .pc file does not mean that package`
		`+ requires pkgconfig)`
		`+ - added pkgconfig to BuildRequires`
		`+ - executing /sbin/ldconfig in python %%post/%%postun packages is not needed`
		`+ (those packages are not providing libraries but loadable modules)`
		`+ - remove not needed %%ifing`
		`+ - use %%bcond instead %%global`
		`+ - removed delete static libtdb.a in %%install (is not needed)`
		`+ - added use more macros (%%autosetup, %%{make_build} and %{make_install})`
		`+ - indent spec`
		`+`
		`* Thu Nov 30 2017 Lukas Slebodnik <lslebodn@fedoraproject.org> - 1.3.15-2`
		`- Update spec file conditionals`

kloczek commented 6 years ago

Copy of the changelog:
* Mon Jan 08 2018 Tomasz Kłoczko kloczek@fedoraproject.org - 1.3.15-3
- remove pkgconfig from devel Requires (provide .pc file does not mean that package
requires pkgconfig)
- added pkgconfig to BuildRequires
- executing /sbin/ldconfig in python %%post/%%postun packages is not needed
(those packages are not providing libraries but loadable modules)
- remove not needed %%ifing
- use %%bcond instead %%global
- removed delete static libtdb.a in %%install (is not needed)
- added use more macros (%%autosetup, %%{make_build} and %{make_install})
- indent spec

lslebodn commented 6 years ago

"simpler tdb-tools %files"
I do not think it is simpler. And we might unexpectedly include some wrong files.
"interpunction and format %description to 80 cols"
LGTM
"reorder sections (put %check after %install and %post/%postun after %chek and before %files)"
some people prefer to have %check before install; but moving post and postun is fine
"added pkgconfig to BuildRequires"
pkgconf is installed as part of group @buildsys-build. Is there any fedora policy that it should be explicitly listed in BR?
"added use more macros (%%autosetup, %%{make_build} and %{make_install})"
I can see only autosetup and make_install in el6 and that patch would made it a little bit complicated to backport package there
"removed delete static libtdb.a in %%install (is not needed)"
there are some changes in commit which does not fit to commit title
"use %%bcond instead %%global"
have you tested it with epel7 ?
"remove not needed %%ifing"
NACK because it will complicate backporting to el7 and el6
"executing /sbin/ldconfig in python %%post/%%postun packages is not needed"
LGTM
"remove pkgconfig from devel Requires. Provide .pc file does not mean that package"
LGTM

Edited 6 years ago by lslebodn

kloczek commented 6 years ago

I do not think it is simpler. And we might unexpectedly include some wrong files.

Do you expect Spanish Inquisition? :)

some people prefer to have %check before install; but moving post and postun is fine

Some people are reading other people spec files and they want those files as not as expression of packager preferences but as something which has more or less standard form/pattern.

some people prefer to have %check before install; but moving post and postun is fine

pkgconf is installed as part of group @buildsys-build. Is there any fedora policy that it should be explicitly listed in BR?

rpm spec file does not have any possibilities to use/describe any kicksrat profiles.

I can see only autosetup and make_install in el6 and that patch would made it a little bit complicated to backport package there

My patch is not against el6 but against master branch.
Fill free to backport those changes to el6.

have you tested it with epel7 ?

%bcond is available in rpm more than 15 years (pre RedHat 4.x)

NACK because it will complicate backporting to el7 and el6

None of the scriptlets needs to be %ifed because those scriptlets are added/used only if exact %files section is active. In other words such change cannot have any conflicts with el7 or el6.

Edited 6 years ago by kloczek