https://bugzilla.redhat.com/show_bug.cgi?id=2022554
--- Comment #6 from Nils Philippsen nphilipp@redhat.com --- Hey Ben, thanks for reviewing this!
(In reply to Ben Beasley from comment #5)
Package Review
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
Issues:
Dist tag is present.
OK; fedora-review does not understand rpmautospec.
I used %autorelease/%autochangelog for this one in part to identify the warts with reviewing an rpmautospec-enabled package. Sorry for making you a guinea pig. 🤷😉
https://pagure.io/fedora-infra/rpmautospec/issue/238
- This is the first time I have seen rpmautospec combined with package unretirement. It makes sense to do. Just make sure that you end up with a release number higher than the last build (-34) to avoid unintended downgrades, using the -b option to %autorelease if needed.
Oh, Koji just wouldn't allow me to build a package of the same N-V-R again (we have plotmm in F34, so this wouldn't have gone unnoticed). I didn't consider unretirement when cooking up the release counting algorithm, but adding -b to compensate shouldn't be necessary:
https://pagure.io/fedora-infra/rpmautospec/issue/237
- Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 1075200 bytes in 174 files. See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_documentation
A -doc subpackage is for end user documentation, so it probably should be -devel-doc. Anyway, while it's not mandatory to have large documentation in a subpackage – you just should consider it… 😏 – it makes sense to do so, -devel would only account for ~84k without the docs and that should make builds quicker.
- Be warned that modern Doxygen-generated HTML documentation is not suitable for packaging, as it has relatively intractable issues with bundled and minified JavaScript. See
https://bugzilla.redhat.com/show_bug.cgi?id=2006555 for details. However, the ancient pre-generated documentation here is OK: it lacks JavaScript, and the CSS is not minified.
If you ever re-build the documentation in the RPM build, which is possible to do, then you will need to deal with this. The best solution is probably to build a PDF instead of HTML. See https://src.fedoraproject.org/rpms/cairomm/blob/rawhide/f/cairomm.spec for an example of doing this. (The example also builds the HTML with the JavaScript stripped out, but that is only so that DevHelp documentation—which plotmm does not provide—keeps working.)
Wow, I wasn't aware of that issue and it's a shame it didn't reach a better conclusion. This really should be spelled out in the packaging guidelines. Dodged a bullet there… 😬
- The option “--enable-docs” to the configure script is not doing anything
and should be removed.
Done.
Since the dependency on gtkmm is via pkgconfig, please change
BuildRequires: gtkmm24-devel >= 2.4.0
to
BuildRequires: pkgconfig(gtkmm-2.4)
and
Requires: gtkmm24-devel
to
Requires: pkgconfig(gtkmm-2.4)
See
https://docs.fedoraproject.org/en-US/packaging-guidelines/ PkgConfigBuildRequires/, and also
https://docs.fedoraproject.org/en-US/packaging-guidelines/ #_package_dependencies regarding versioned dependencies.
Done.
- While the URL only works with plain HTTP, the Source0 URL still works with HTTPS. Please adjust it to HTTPS.
Done.
- This project is thoroughly dead upstream. It’s okay to keep packaging it,
but you will have a higher burden as maintainer should any issues arise.
Thanks, I'm aware of it. 🙂
- You should not glob the shared library in a way that would miss an
soversion bump. See
https://docs.fedoraproject.org/en-US/packaging-guidelines/ #_listing_shared_library_files.
Plus, it’s best not to use extremely broad globs under shared directories. There are some arguments for not doing so in
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ #_explicit_lists.
At least change:
%{_libdir}/*.so.*
to:
%{_libdir}/*.so.0*
but preferably:
%{_libdir}/libplotmm.so.0*
or:
%{_libdir}/libplotmm.so.0 %{_libdir}/libplotmm.so.0.*
Done, even if I don't expect it to change its SONAME anytime soon (or anytime at all).
This is obsolete in all current Fedora releases and should be removed:
%ldconfig_scriptlets
Done.
While the make invocations are acceptable, it would be even better to write
make %{?_smp_mflags}
as
%make_build
and
make DESTDIR=%{buildroot} install
as
%make_install
Done.
This is a nit-pick, but
find %{buildroot} -type f -name "*.la" -exec rm -f {} ';'
could be more simply written as
find '%{buildroot}' -type f -name '*.la' -delete
Yes it can 😀, and done.
The spec file contains mixed spaces and tabs. You can fix it with:
sed -r -i 's/\t/ /g' plotmm.spec
See rpmlint diagnostics:
plotmm.spec:44: W: mixed-use-of-spaces-and-tabs (spaces: line 10, tab:
line 44) plotmm.spec:44: W: mixed-use-of-spaces-and-tabs (spaces: line 10, tab: line 44)
Done.
I think all of these rpmlint messages are spurious and require no action:
plotmm.src: W: strange-permission plotmm.spec 600
That looks like `fedpkg srpm` created it that way. Will have to investigate this at some point.
plotmm.x86_64: E: shlib-policy-name-error 0 plotmm-examples.x86_64: W: no-documentation plotmm-devel.x86_64: W: missing-dependency-on
plotmm*/plotmm-libs/libplotmm* = 0.1.2
Yeah, the %{?_isa}-ified Requires: is there.
plotmm.x86_64: E: invalid-ldconfig-symlink /usr/lib64/libplotmm.so.0.0.0
libplotmm.so.0.0.0
🤔 Maybe it's weirded out because the package is called `plotmm` instead of `libplotmm`?
plotmm-devel.x86_64: W: files-duplicate
/usr/share/doc/plotmm-devel/html/ class_plot_m_m_1_1_error_curve__inherit__graph.dot /usr/share/doc/plotmm-devel/html/class_plot_m_m_1_1_error_curve__coll__graph. dot …
I'll run hardlink to deduplicate the installed files.
Simple man pages explaining what the examples do would be nice, but are not required. See rpmlint diagnostics:
plotmm-examples.x86_64: W: no-manual-page-for-binary plotmm-curves plotmm-examples.x86_64: W: no-manual-page-for-binary plotmm-simple
and also https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages.
To be honest, I'm not sure if anyone even needs these. At this point it's mere inertia to drag the example programs along. 😉
This should be reported upstream, although given plotmm seems to be abandoned, I wouldn’t expect doing so to accomplish anything.
plotmm.x86_64: E: incorrect-fsf-address
/usr/share/licenses/plotmm/COPYING
You are required to report this, but you must not patch the file yourself; see https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address.
Filed here: https://sourceforge.net/p/plotmm/bugs/5/
Because there is no (new) git history, the release number didn't change. You can therefore find the updated files in the same places, I've moved the old versions into a subdirectory (…/old/1):
Spec URL: https://nphilipp.fedorapeople.org/review/plotmm/plotmm.spec SRPM URL: https://nphilipp.fedorapeople.org/review/plotmm/plotmm-0.1.2-2.fc36.src.rpm