https://bugzilla.redhat.com/show_bug.cgi?id=2022554 --- Comment #6 from Nils Philippsen <nphilipp@xxxxxxxxxx> --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2022554 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
