Bug 1873199

Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=50254466

> URL:            https://gitlab.freedesktop.org/pwithnall/malcontent/
> Source0:        https://gitlab.freedesktop.org/pwithnall/malcontent/-/archive/%{version}/%{name}-%{version}.tar.bz2

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/malcontent-0.8.0.tar.bz2 :
  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)

(In reply to Andy Mender from comment #2)
> > URL:            https://gitlab.freedesktop.org/pwithnall/malcontent/
> > Source0:        https://gitlab.freedesktop.org/pwithnall/malcontent/-/archive/%{version}/%{name}-%{version}.tar.bz2
> 
> 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

> 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."

(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

> 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.

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.

(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.

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.

All done, same location.

Scratch build here:
https://koji.fedoraproject.org/koji/taskinfo?taskID=50989830

Looks good to me now, thanks!

Great, approved!

Thanks a lot for the feedback, Kalev :).

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/malcontent

Built in malcontent-0.8.0-5.fc34