https://bugzilla.redhat.com/show_bug.cgi?id=1873199
Bug ID: 1873199 Summary: Review Request: malcontent - Parental controls implementation Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: bnocera@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://hadess.fedorapeople.org/malcontent/malcontent.spec SRPM URL: https://hadess.fedorapeople.org/malcontent/malcontent-0.8.0-1.fc32.src.rpm Description: libmalcontent implements parental controls support which can be used by applications to filter or limit the access of child accounts to inappropriate content. Fedora Account System Username: hadess
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #1 from Bastien Nocera bnocera@redhat.com --- Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=50254466
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
Andy Mender andymenderunix@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |andymenderunix@gmail.com Assignee|nobody@fedoraproject.org |andymenderunix@gmail.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #2 from Andy Mender andymenderunix@gmail.com ---
URL: https://gitlab.freedesktop.org/pwithnall/malcontent/ Source0: https://gitlab.freedesktop.org/pwithnall/malcontent/-/archive/%%7Bversion%7D...
You can replace the URL part in Source0 with %{url} to make it a little shorter and avoid duplication.
%package ui-devel Summary: Development files for libmalcontent-ui License: LGPLv2+ Requires: %{name}-ui-libs%{?_isa} = %{version}-%{release} Requires: gtk3-devel
gtk3 provides a pkgconfig file so you can use the "pkgconfig(gtk3)" syntax in the last line.
I see the project provides a README.md. Could you attach it to all of the stand-alone packages?
Main review below: Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - Package installs a %{name}.desktop using desktop-file-install or desktop- file-validate if there is such a file. Review: Yes, a desktop file is listed and should use either of the above. %files control %license COPYING %{_bindir}/malcontent-control %{_datadir}/applications/org.freedesktop.MalcontentControl.desktop
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. Review: SO is needed internally by the PAM module. That's okay. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs.
Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages Review: Tested by submitter in Koji. [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU Lesser General Public License GNU General Public License", "GNU Lesser General Public License (v2.1 or later)", "GPL (v2 or later)". 109 files have unknown license. Detailed output of licensecheck in /home/amender/rpmbuild/SPECS/malcontent/malcontent/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/dbus-1, /usr/share/help/C, /usr/share/help/pt_BR, /usr/share/help/pl, /usr/share/polkit-1, /usr/lib64/girepository-1.0, /usr/share/accountsservice, /usr/share/gir-1.0, /usr/share/polkit-1/actions, /usr/share/polkit-1/rules.d, /usr/share/help/id, /usr/share/help/uk [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [x]: Development files must be in a -devel package [?]: Package uses nothing in %doc for runtime. [x]: The spec file handles locales properly. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [x]: Reviewer should test that the package builds in mock. [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [ ]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in malcontent-control , malcontent-pam , malcontent-tools , malcontent- ui-devel , malcontent-ui-libs , malcontent-devel , malcontent-libs Review: I'm assuming this is correct, because these can be installed separately, yes? [x]: Package functions as described. Review: Yes, but requires a running ServiceAccounts daemon. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. Review: Tested in Koji. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: The placement of pkgconfig(.pc) files are correct. [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Note: Arch-ed rpms have a total of 1198080 bytes in /usr/share [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages.
Rpmlint ------- Checking: malcontent-0.8.0-1.fc34.x86_64.rpm malcontent-control-0.8.0-1.fc34.x86_64.rpm malcontent-pam-0.8.0-1.fc34.x86_64.rpm malcontent-tools-0.8.0-1.fc34.x86_64.rpm malcontent-ui-devel-0.8.0-1.fc34.x86_64.rpm malcontent-ui-libs-0.8.0-1.fc34.x86_64.rpm malcontent-devel-0.8.0-1.fc34.x86_64.rpm malcontent-libs-0.8.0-1.fc34.x86_64.rpm malcontent-debuginfo-0.8.0-1.fc34.x86_64.rpm malcontent-debugsource-0.8.0-1.fc34.x86_64.rpm malcontent-0.8.0-1.fc32.src.rpm malcontent.x86_64: E: no-binary malcontent.x86_64: W: no-documentation malcontent-control.x86_64: W: no-documentation malcontent-control.x86_64: W: no-manual-page-for-binary malcontent-control malcontent-pam.x86_64: W: spelling-error %description -l en_US logins -> losing, login, loins malcontent-pam.x86_64: W: no-documentation malcontent-ui-devel.x86_64: W: spelling-error Summary(en_US) libmalcontent -> lib malcontent, lib-malcontent, malcontent malcontent-ui-devel.x86_64: W: spelling-error %description -l en_US libmalcontent -> lib malcontent, lib-malcontent, malcontent malcontent-ui-devel.x86_64: E: description-line-too-long C This package contains the pkg-config file and development headers for libmalcontent-ui. malcontent-ui-devel.x86_64: W: no-documentation malcontent-ui-libs.x86_64: W: spelling-error %description -l en_US libmalcontent -> lib malcontent, lib-malcontent, malcontent malcontent-ui-libs.x86_64: W: no-documentation malcontent-devel.x86_64: E: description-line-too-long C This package contains the pkg-config file and development headers for malcontent. malcontent-devel.x86_64: W: no-documentation malcontent-libs.x86_64: W: spelling-error %description -l en_US libmalcontent -> lib malcontent, lib-malcontent, malcontent malcontent-libs.x86_64: W: no-documentation 11 packages and 0 specfiles checked; 3 errors, 13 warnings.
Rpmlint (debuginfo) ------------------- Checking: malcontent-control-debuginfo-0.8.0-1.fc34.x86_64.rpm malcontent-debuginfo-0.8.0-1.fc34.x86_64.rpm malcontent-libs-debuginfo-0.8.0-1.fc34.x86_64.rpm malcontent-pam-debuginfo-0.8.0-1.fc34.x86_64.rpm malcontent-ui-libs-debuginfo-0.8.0-1.fc34.x86_64.rpm 5 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages) ---------------------------- (none): E: no installed packages by name malcontent-pam (none): E: no installed packages by name malcontent-ui-devel (none): E: no installed packages by name malcontent-debuginfo (none): E: no installed packages by name malcontent-pam-debuginfo (none): E: no installed packages by name malcontent (none): E: no installed packages by name malcontent-debugsource (none): E: no installed packages by name malcontent-ui-libs-debuginfo (none): E: no installed packages by name malcontent-tools (none): E: no installed packages by name malcontent-libs-debuginfo (none): E: no installed packages by name malcontent-ui-libs (none): E: no installed packages by name malcontent-control (none): E: no installed packages by name malcontent-libs (none): E: no installed packages by name malcontent-devel (none): E: no installed packages by name malcontent-control-debuginfo 0 packages and 0 specfiles checked; 0 errors, 0 warnings.
Unversioned so-files -------------------- malcontent-pam: /usr/lib64/security/pam_malcontent.so
Source checksums ---------------- https://gitlab.freedesktop.org/pwithnall/malcontent/-/archive/0.8.0/malconte... : CHECKSUM(SHA256) this package : f7f1416c5db61738f1bb5b68a96aa6f7ce4965734325649232d1f629ff614a42 CHECKSUM(SHA256) upstream package : f7f1416c5db61738f1bb5b68a96aa6f7ce4965734325649232d1f629ff614a42
Requires -------- malcontent (rpmlib, GLIBC filtered):
malcontent-control (rpmlib, GLIBC filtered): libaccountsservice.so.0()(64bit) libc.so.6()(64bit) libcairo.so.2()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libgdk-3.so.0()(64bit) libgdk_pixbuf-2.0.so.0()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libgtk-3.so.0()(64bit) libmalcontent-ui-0.so.0()(64bit) libpolkit-gobject-1.so.0()(64bit) rtld(GNU_HASH)
malcontent-pam (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libmalcontent-0.so.0()(64bit) libpam.so.0()(64bit) libpam.so.0(LIBPAM_1.0)(64bit) libpam.so.0(LIBPAM_EXTENSION_1.0)(64bit) libpam.so.0(LIBPAM_MODUTIL_1.0)(64bit) rtld(GNU_HASH)
malcontent-tools (rpmlib, GLIBC filtered): /usr/bin/python3
malcontent-ui-devel (rpmlib, GLIBC filtered): /usr/bin/pkg-config gtk3-devel libmalcontent-ui-0.so.0()(64bit) malcontent-ui-libs(x86-64) pkgconfig(accountsservice) pkgconfig(flatpak) pkgconfig(gio-2.0) pkgconfig(glib-2.0) pkgconfig(gobject-2.0) pkgconfig(gtk+-3.0) pkgconfig(malcontent-0)
malcontent-ui-libs (rpmlib, GLIBC filtered): libaccountsservice.so.0()(64bit) libc.so.6()(64bit) libflatpak.so.0()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libgdk-3.so.0()(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libgtk-3.so.0()(64bit) libmalcontent-0.so.0()(64bit) rtld(GNU_HASH)
malcontent-devel (rpmlib, GLIBC filtered): /usr/bin/pkg-config libmalcontent-0.so.0()(64bit) malcontent-libs(x86-64) pkgconfig(gio-2.0) pkgconfig(gio-unix-2.0) pkgconfig(glib-2.0) pkgconfig(gobject-2.0)
malcontent-libs (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) rtld(GNU_HASH)
malcontent-debuginfo (rpmlib, GLIBC filtered):
malcontent-debugsource (rpmlib, GLIBC filtered):
Provides -------- malcontent: malcontent malcontent(x86-64)
malcontent-control: application() application(org.freedesktop.MalcontentControl.desktop) malcontent-control malcontent-control(x86-64) metainfo() metainfo(org.freedesktop.MalcontentControl.appdata.xml)
malcontent-pam: malcontent-pam malcontent-pam(x86-64)
malcontent-tools: malcontent-tools malcontent-tools(x86-64)
malcontent-ui-devel: malcontent-ui-devel malcontent-ui-devel(x86-64) pkgconfig(malcontent-ui-0)
malcontent-ui-libs: libmalcontent-ui-0.so.0()(64bit) malcontent-ui-libs malcontent-ui-libs(x86-64)
malcontent-devel: malcontent-devel malcontent-devel(x86-64) pkgconfig(malcontent-0)
malcontent-libs: libmalcontent-0.so.0()(64bit) malcontent-libs malcontent-libs(x86-64)
malcontent-debuginfo: malcontent-debuginfo malcontent-debuginfo(x86-64)
malcontent-debugsource: malcontent-debugsource malcontent-debugsource(x86-64)
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #3 from Bastien Nocera bnocera@redhat.com --- (In reply to Andy Mender from comment #2)
URL: https://gitlab.freedesktop.org/pwithnall/malcontent/ Source0: https://gitlab.freedesktop.org/pwithnall/malcontent/-/archive/%%7Bversion%7D...
You can replace the URL part in Source0 with %{url} to make it a little shorter and avoid duplication.
Sure.
%package ui-devel Summary: Development files for libmalcontent-ui License: LGPLv2+ Requires: %{name}-ui-libs%{?_isa} = %{version}-%{release} Requires: gtk3-devel
gtk3 provides a pkgconfig file so you can use the "pkgconfig(gtk3)" syntax in the last line.
Done.
I see the project provides a README.md. Could you attach it to all of the stand-alone packages?
OK.
Main review below: Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues:
- Package installs a %{name}.desktop using desktop-file-install or desktop- file-validate if there is such a file. Review: Yes, a desktop file is listed and should use either of the above. %files control %license COPYING %{_bindir}/malcontent-control %{_datadir}/applications/org.freedesktop.MalcontentControl.desktop
I've added a desktop-file-validate in %check, is that enough?
v2 is in the same place at: https://hadess.fedorapeople.org/malcontent/
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=50316270
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #4 from Andy Mender andymenderunix@gmail.com ---
I've added a desktop-file-validate in %check, is that enough?
Yup, that should be enough according to the packaging guidelines.
[ ]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/help/C, /usr/share/gir-1.0, /usr/share/polkit-1/actions, /usr/share/accountsservice, /usr/share/help/id, /usr/share/polkit-1/rules.d, /usr/share/help/pt_BR, /usr/share/polkit-1, /usr/share/help/uk, /usr/share/dbus-1, /usr/share/help/pl, /usr/lib64/girepository-1.0
These directories still require proper ownership. Either you make the subpackages which use them own them or add Requires lines for packages which provide them. For instance, /usr/share/polkit-1 is provided by meson or polkit, but I'd recommend polkit in this case. Check which directory is provided by what with "dnf provides <path>".
A couple of extra things picked up by rpmlint:
malcontent-pam.x86_64: W: no-documentation malcontent-ui-devel.x86_64: W: no-documentation malcontent-devel.x86_64: W: no-documentation
Since these are stand-alone subpackages, each needs its own %doc entry with the README.md file.
malcontent-ui-devel.x86_64: E: description-line-too-long C This package contains the pkg-config file and development headers for libmalcontent-ui.
%description lines are 80 characters max. Either split it into multiple lines or change it to something like "Contains the pkg-config file and development headers for libmalcontent-ui."
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #5 from Bastien Nocera bnocera@redhat.com --- (In reply to Andy Mender from comment #4)
I've added a desktop-file-validate in %check, is that enough?
Yup, that should be enough according to the packaging guidelines.
[ ]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/help/C, /usr/share/gir-1.0, /usr/share/polkit-1/actions, /usr/share/accountsservice, /usr/share/help/id, /usr/share/polkit-1/rules.d, /usr/share/help/pt_BR, /usr/share/polkit-1, /usr/share/help/uk, /usr/share/dbus-1, /usr/share/help/pl, /usr/lib64/girepository-1.0
These directories still require proper ownership. Either you make the subpackages which use them own them or add Requires lines for packages which provide them. For instance, /usr/share/polkit-1 is provided by meson or polkit, but I'd recommend polkit in this case. Check which directory is provided by what with "dnf provides <path>".
Will require: accountsservice-0.6.55-5
All the /usr/share/help/*/ directories should be owned by filesystem, because there's no way that gnome-user-docs will be able to own all the same dirs random packages could own, so it'll be filesystem's job to do that.
I believe the rest is now fixed.
A couple of extra things picked up by rpmlint:
malcontent-pam.x86_64: W: no-documentation malcontent-ui-devel.x86_64: W: no-documentation malcontent-devel.x86_64: W: no-documentation
Since these are stand-alone subpackages, each needs its own %doc entry with the README.md file.
What does "stand-alone subpackages" actually mean? All those depend on packages from the same SRPM which contain the README, and if those are considered "stand-alone" despite having deps on other RPMs from the same source, what's considered "non stand-alone"?
I haven't made any changes here.
malcontent-ui-devel.x86_64: E: description-line-too-long C This package contains the pkg-config file and development headers for libmalcontent-ui.
%description lines are 80 characters max. Either split it into multiple lines or change it to something like "Contains the pkg-config file and development headers for libmalcontent-ui."
Done.
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=50755715
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #6 from Andy Mender andymenderunix@gmail.com ---
What does "stand-alone subpackages" actually mean? All those depend on packages from the same SRPM which contain the README, and if those are considered "stand-alone" despite having deps on other RPMs from the same source, what's considered "non stand-alone"?
"Stand-alone subpackages" would be subpackages which can be installed completely independently of the rest of the subpackages in the SPEC file. However, I looked more closely at the SPEC file again and it seems it's rather the following: - malcontent-pam - malcontent-control - malcontent-tools - malcontent-ui-libs - malcontent-libs
Out of these, malcontent-pam and malcontent-tools don't provide a %doc.
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #7 from Kalev Lember klember@redhat.com --- I don't think there's any requirement for shipping a docs within all subpackages. It's just dumb rpmlint warning about not having docs and that warning is fine to ignore. rpmlint output doesn't 100% match our packaging guidelines and best practices.
What stand-alone subpackages need to have is a license file specified with %license (which they already have).
I do have some other random comments here I noticed while going through the spec file:
Requires: gnome-user-docs
Why is this hard dep necessary? It seems wrong from minimization point of view to drag in an unrelated docs package if one just wants to install the library.
%package ui-devel Summary: Development files for libmalcontent-ui License: LGPLv2+ Requires: %{name}-ui-libs%{?_isa} = %{version}-%{release} Requires: pkgconfig(gtk+-3.0)
I don't think you need to manually specify 'Requires: pkgconfig(gtk+-3.0)' as it's normally automatically generated based on the .pc file. If that doesn't happen, it's probably a bug in the .pc file.
Requires: gobject-introspection-devel
Instead of this, please just use multiple directory ownership for the gir directories as we do in other gnome packages instead of hard-depending on gobject-introspection-devel.
%package ui-libs Summary: Libraries for %{name} License: LGPLv2+ Requires: gobject-introspection
Same thing here, please just use multiple directory ownership for the gir directories.
I see that you are specifying "License: LGPLv2+" in all subpackages. This is fine, but unnecessary: it's enough to just specify it for the main package and then all subpackages inheriy the license tag of the main packages.
%check desktop-file-validate %{buildroot}%{_datadir}/applications/org.freedesktop.MalcontentControl.desktop
It should also have appdata validation here.
%ldconfig_scriptlets libs %ldconfig_scriptlets ui-libs
These are not needed in current Fedora versions and can just go (they evaluate to nothing). We have ldconfig triggers in place that automatically rebuild the ldconfig cache.
%{_mandir}/man8/malcontent-client.8.gz
Can you use malcontent-client.8* glob here? This works better for flatpak builds that don't currently have man page compression applied.
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #8 from Bastien Nocera bnocera@redhat.com --- (In reply to Kalev Lember from comment #7)
I don't think there's any requirement for shipping a docs within all subpackages. It's just dumb rpmlint warning about not having docs and that warning is fine to ignore. rpmlint output doesn't 100% match our packaging guidelines and best practices.
What stand-alone subpackages need to have is a license file specified with %license (which they already have).
I do have some other random comments here I noticed while going through the spec file:
Requires: gnome-user-docs
Why is this hard dep necessary? It seems wrong from minimization point of view to drag in an unrelated docs package if one just wants to install the library.
It was so that the /usr/share/help sub-directories were created. I've tried to fix that up in the latest version. Let me know if you wanted it done some other way, I couldn't actually find examples in the checked out packages.
%package ui-devel Summary: Development files for libmalcontent-ui License: LGPLv2+ Requires: %{name}-ui-libs%{?_isa} = %{version}-%{release} Requires: pkgconfig(gtk+-3.0)
I don't think you need to manually specify 'Requires: pkgconfig(gtk+-3.0)' as it's normally automatically generated based on the .pc file. If that doesn't happen, it's probably a bug in the .pc file.
OK, removed.
Requires: gobject-introspection-devel
Instead of this, please just use multiple directory ownership for the gir directories as we do in other gnome packages instead of hard-depending on gobject-introspection-devel.
Done.
%package ui-libs Summary: Libraries for %{name} License: LGPLv2+ Requires: gobject-introspection
Same thing here, please just use multiple directory ownership for the gir directories.
I see that you are specifying "License: LGPLv2+" in all subpackages. This is fine, but unnecessary: it's enough to just specify it for the main package and then all subpackages inheriy the license tag of the main packages.
OK.
%check desktop-file-validate %{buildroot}%{_datadir}/applications/org.freedesktop.MalcontentControl.desktop
It should also have appdata validation here.
Done, but disabled for now: https://gitlab.freedesktop.org/pwithnall/malcontent/-/issues/23
%ldconfig_scriptlets libs %ldconfig_scriptlets ui-libs
These are not needed in current Fedora versions and can just go (they evaluate to nothing). We have ldconfig triggers in place that automatically rebuild the ldconfig cache.
Done.
%{_mandir}/man8/malcontent-client.8.gz
Can you use malcontent-client.8* glob here? This works better for flatpak builds that don't currently have man page compression applied.
Sure.
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=50946663
-4 version copied to the same URL.
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #9 from Kalev Lember klember@redhat.com --- Nice, thanks for the fixes! I'll let Andy do the final pass but looking good to me :)
Just a few more remarks below:
(In reply to Bastien Nocera from comment #8)
(In reply to Kalev Lember from comment #7)
I don't think there's any requirement for shipping a docs within all subpackages. It's just dumb rpmlint warning about not having docs and that warning is fine to ignore. rpmlint output doesn't 100% match our packaging guidelines and best practices.
What stand-alone subpackages need to have is a license file specified with %license (which they already have).
I do have some other random comments here I noticed while going through the spec file:
Requires: gnome-user-docs
Why is this hard dep necessary? It seems wrong from minimization point of view to drag in an unrelated docs package if one just wants to install the library.
It was so that the /usr/share/help sub-directories were created. I've tried to fix that up in the latest version. Let me know if you wanted it done some other way, I couldn't actually find examples in the checked out packages.
Ahh, that's usually done with '%find_lang --with_gnome' which then picks up all the individual directories under /usr/share/help and adds the correct %files lang() attributes etc and writes it out to malcontent.lang, which you then include with %files -f malcontent.lang. I think the only missing part is adding --with-gnome to %find_lang and then you can drop the gnome-user-docs requires and all of the %{_datadir}/help references from %files.
%check desktop-file-validate %{buildroot}%{_datadir}/applications/org.freedesktop.MalcontentControl.desktop
It should also have appdata validation here.
Done, but disabled for now: https://gitlab.freedesktop.org/pwithnall/malcontent/-/issues/23
It should be validate-relax which should make it pass. appstream-util uses a bit weird naming there, but the idea was for 'validate' to mean "validate stuff that flathub wants" and 'validate-relax' to be "validate stuff that package-based distros want".
%files ui-libs %license COPYING %doc README.md %{_libdir}/girepository-1.0/MalcontentUi-0.typelib
This should have '%dir %{_libdir}/girepository-1.0' to own the directory as well.
%files libs %license COPYING %doc README.md %{_libdir}/girepository-1.0/Malcontent-0.typelib
... and same here, '%dir %{_libdir}/girepository-1.0', which would then allow dropping 'Requires: gobject-introspection' from above.
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #10 from Bastien Nocera bnocera@redhat.com --- All done, same location.
Scratch build here: https://koji.fedoraproject.org/koji/taskinfo?taskID=50989830
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #11 from Kalev Lember klember@redhat.com --- Looks good to me now, thanks!
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
Andy Mender andymenderunix@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #12 from Andy Mender andymenderunix@gmail.com --- Great, approved!
Thanks a lot for the feedback, Kalev :).
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
--- Comment #13 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/malcontent
https://bugzilla.redhat.com/show_bug.cgi?id=1873199
Bastien Nocera bnocera@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE Last Closed| |2020-09-10 14:11:48
--- Comment #14 from Bastien Nocera bnocera@redhat.com --- Built in malcontent-0.8.0-5.fc34
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com