Bug 2035944

I've been packaging Touchégg for a while in my elementary-staging COPR:
https://copr.fedorainfracloud.org/coprs/decathorpe/elementary-staging/package/touchegg/

You might want to follow this upstream issue / PR before packaging it:
https://github.com/JoseExposito/touchegg/issues/456
https://github.com/JoseExposito/touchegg/pull/457

Right now, the touchegg systemd service is started unconditionally upon system start, which might conflict with some DEs, and it uses a DBus connection over a unix socket, which is a quite unusual setup for a DBus service.

(In reply to Fabio Valentini from comment #1)
> I've been packaging Touchégg for a while in my elementary-staging COPR:
> https://copr.fedorainfracloud.org/coprs/decathorpe/elementary-staging/
> package/touchegg/
> 
> You might want to follow this upstream issue / PR before packaging it:
> https://github.com/JoseExposito/touchegg/issues/456
> https://github.com/JoseExposito/touchegg/pull/457
> 
> Right now, the touchegg systemd service is started unconditionally upon
> system start, which might conflict with some DEs, and it uses a DBus
> connection over a unix socket, which is a quite unusual setup for a DBus
> service.

Hi Fabio,

Thank you for the information. I did not realize that you already have packaged it, albeit in Copr. I would have contatcted  you 😀.

I have used Touchegg without issue on both GNOME and KDE, of course, on xorg, because Touchegg doesn't support Wayland. Is this issue really significant enough to halt packaging?

Also, I'm happy to make you a co-maintainer if you'd like.

Maxwell

> I would have contatcted  you 😀.

No problem! I just saw the notification for this review ticket on the mailing list and thought to comment on it.

> I have used Touchegg without issue on both GNOME and KDE, of course, on xorg, because Touchegg doesn't support Wayland. Is this issue really significant enough to halt packaging?

I don't think it is a significant issue that prevents packaging. The systemd service is not enabled by default, so that solves the problem. I just wanted you to be aware that touchegg uses an ... interesting DBus client / server setup with its own unix socket instead of using a dbus system or user session. 

This also apparently makes it hard for it to use socket-activation, as other systemd DBus-type services usually do (see the discussion on the linked github issue / PR).

I can review this package tomorrow (basically, compare it to mine, and finish the formal review). It would be great if you then added me as co-maintainer once it's approved :) touchegg is used as an "official" (optional) component of the Pantheon DE, and having it in Fedora proper instead of in COPR would be nice.

Package looks good, some small comments before I finish up the review:

1) Consider not using the Forge macros

I would advise against using the forge macros, except if you're doing Go or Font packages (where you have no choice other than to use them). The "%forge*" macros are unmaintained for a while, and their original author is no longer contributing to Fedora. They are also not entirely compatible with rpmautospec, as far as I can tell.

Instead, just use the standard SourceURL guidelines, for something like:

URL:            https://github.com/JoseExposito/touchegg
Source0:        %{url}/archive/%{version}/%{name}-%{version}.tar.gz

2) BuildRequires:  %{_bindir}/desktop-file-validate

This is not good. %{_bindir} should not be used in BuildRequires, as it is influenced by build-specific settings like %{_prefix}, which might evaluate to /app instead of /usr, for example.

Just use "BuildRequires:  desktop-file-utils" instead.

3) Separate file / directory ownership for /usr/share/touchegg

%dir %{_datadir}/%{name}
%{_datadir}/%{name}/%{name}.conf

This looks weird to me. Why not just have the package own "%{_datadir}/%{name}/" directly?
That already includes the directory and all files under it, without having to list everything explicitly.

4) The order of files in %files is very creative

While this is entirely within the realm of "personal choice", I recommend to follow what other packages do in this regard, i.e. list files that use the special %license and %doc macros first (they execute code, and not only list files), then %config files, followed by an alphabetical list of files, for example, like this:

"""
%files
%license COPYING COPYRIGHT
%doc README.md CHANGELOG.md

%config(noreplace) %{_sysconfdir}/xdg/autostart/%{name}.desktop

%{_bindir}/%{name}
%{_datadir}/%{name}/
%{_unitdir}/%{name}.service
"""

This makes it easy to consistently add new files to the list and keep it easily readable.
Feel free to ignore this point, it is just my advice based on personal experience.

5) The systemd service is not restartable

Restarting the systemd service on package update apparently breaks clients:
https://github.com/JoseExposito/touchegg/issues/453

In this case, the systemd scriptlet guidelines state that you should use %systemd_postun (without _with_restart):
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets

(In reply to Fabio Valentini from comment #4)
> Package looks good, some small comments before I finish up the review:
> 
> 1) Consider not using the Forge macros
> 
> I would advise against using the forge macros, except if you're doing Go or
> Font packages (where you have no choice other than to use them). The
> "%forge*" macros are unmaintained for a while, and their original author is
> no longer contributing to Fedora. They are also not entirely compatible with
> rpmautospec, as far as I can tell.
> 
> Instead, just use the standard SourceURL guidelines, for something like:
> 
> URL:            https://github.com/JoseExposito/touchegg
> Source0:        %{url}/archive/%{version}/%{name}-%{version}.tar.gz

I changed it, as there's not much of a point to use the forge macros in this scenario. I have not noticed any issues between rpmautospec and the forge macros. I have noticed that the way the forge macros handle git snapshots (%commit macro) is incompatible with the new versioning guidelines[0]; `%forgemeta` still adds the commit information to the `Release:` field. Perhaps the SourceURL[1] Packaging Guidelines should be amended to account for the problematic forge macros?

[0]: https://pagure.io/packaging-committee/pull-request/908
[1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/

> 2) BuildRequires:  %{_bindir}/desktop-file-validate
> 
> This is not good. %{_bindir} should not be used in BuildRequires, as it is
> influenced by build-specific settings like %{_prefix}, which might evaluate
> to /app instead of /usr, for example.
> 
> Just use "BuildRequires:  desktop-file-utils" instead.

Done.

> 3) Separate file / directory ownership for /usr/share/touchegg

I know that this isn't necessary, but I wanted to be explicit as a personal reminder and to prevent issues down the line. Still, I ended up liking your layout better, so I adopted it.


> 5) The systemd service is not restartable
> 
> Restarting the systemd service on package update apparently breaks clients:
> https://github.com/JoseExposito/touchegg/issues/453
> 
> In this case, the systemd scriptlet guidelines state that you should use
> %systemd_postun (without _with_restart):
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/
> #_scriptlets

That's a good point. I fixed it and added an explanatory comment to the specfile.

Spec URL: https://git.sr.ht/~gotmax23/touchegg/tree/f1637f258e3a9ad3bc9a2efe0fe20a48ba3b73b7/item/touchegg.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/gotmax23/reviews/fedora-rawhide-x86_64/03129509-touchegg/touchegg-2.0.12-1.fc36.x86_64.rpm
Diff: https://git.sr.ht/~gotmax23/touchegg/commit/f1637f258e3a9ad3bc9a2efe0fe20a48ba3b73b7
Fedora Review URL: https://copr-be.cloud.fedoraproject.org/results/gotmax23/reviews/fedora-rawhide-x86_64/03129509-touchegg/fedora-review/

> - systemd_post is invoked in %post, systemd_preun in %preun, and
>   systemd_postun in %postun for Systemd service files.
>   Note: Systemd service file(s) in touchegg
>  See: https://docs.fedoraproject.org/en-US/packaging-
>  guidelines/Scriptlets/#_scriptlets

It looks like fedora-review does not like the use of macros within the systemd scriplet arguments. I tried replacing %{name}.service with touchegg.service, and the issue went away. I will try to report a bug against fedora-review when I have the chance.

(In reply to Maxwell G from comment #7)
> I will try to report a bug
> against fedora-review when I have the chance.

https://pagure.io/FedoraReview/issue/432

Looks like you accidentally pasted a HTML link instead of the "raw" link for the .spec file?

(In reply to Fabio Valentini from comment #9)
> Looks like you accidentally pasted a HTML link instead of the "raw" link for
> the .spec file?

Spec URL: https://git.sr.ht/~gotmax23/touchegg/blob/f1637f258e3a9ad3bc9a2efe0fe20a48ba3b73b7/touchegg.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/gotmax23/reviews/fedora-rawhide-x86_64/03129509-touchegg/touchegg-2.0.12-1.fc36.x86_64.rpm
Diff: https://git.sr.ht/~gotmax23/touchegg/commit/f1637f258e3a9ad3bc9a2efe0fe20a48ba3b73b7
Fedora Review URL: https://copr-be.cloud.fedoraproject.org/results/gotmax23/reviews/fedora-rawhide-x86_64/03129509-touchegg/fedora-review/

Now the .spec and SRPM don't match :/
(SRPM link seems to point to an older revision) ... maybe a copy-paste-error?

(In reply to Fabio Valentini from comment #11)
> Now the .spec and SRPM don't match :/
> (SRPM link seems to point to an older revision) ... maybe a copy-paste-error?

Sorry, I'm a bit out of it today :). I accidentally linked the built rpm in place of the source one.

Spec URL: https://git.sr.ht/~gotmax23/touchegg/blob/f1637f258e3a9ad3bc9a2efe0fe20a48ba3b73b7/touchegg.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/gotmax23/reviews/fedora-rawhide-x86_64/03129509-touchegg/touchegg-2.0.12-1.fc36.src.rpm
Diff: https://git.sr.ht/~gotmax23/touchegg/commit/f1637f258e3a9ad3bc9a2efe0fe20a48ba3b73b7
Fedora Review URL: https://copr-be.cloud.fedoraproject.org/results/gotmax23/reviews/fedora-rawhide-x86_64/03129509-touchegg/fedora-review/

> %prep
> %forgeautosetup

This is a minor issue that didn't impact the build, but I can fix it to plain `%autosetup` on import.

Good point. You might also want to sort BuildRequires alphabetically, it helps keep the list tidy and duplicate-free.
Other than that: Package APPROVED.

Feel free to add me as co-maintainer after the package has been imported.
Also don't forget to set up release-monitoring and koschei for the package :)


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[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]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[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.
[x]: License file installed when any subpackage combination is installed.
[x]: Package must own all directories that it creates.

     Note: Directories without known owners: /usr/lib/systemd,
     /usr/lib/systemd/system
     Reviewer's Note: This seems to be wrong. systemd owns those directories.

[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.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[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.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[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]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[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]: No %config files under /usr.
[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]: systemd_post is invoked in %post, systemd_preun in %preun, and
     systemd_postun in %postun for Systemd service files.
     Note: Systemd service file(s) in touchegg
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: 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).
[-]: Package functions as described.
[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.
[-]: 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.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[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]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[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]: Rpmlint is run on debuginfo package(s).
[x]: Rpmlint is run on all installed packages.
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness


Rpmlint (debuginfo)
-------------------
touchegg-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/bin/touchegg-2.0.12-1.fc36.x86_64.debug
touchegg-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/bin/touchegg-2.0.12-1.fc36.x86_64.debug
touchegg-debuginfo.x86_64: W: no-documentation
touchegg-debugsource.x86_64: W: no-documentation
touchegg-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/64/cb3bcfd24c21bdfc84c9ed8586087eeb865662 ../../../.build-id/64/cb3bcfd24c21bdfc84c9ed8586087eeb865662
 2 packages and 0 specfiles checked; 1 errors, 4 warnings, 1 badness

(These seem to be false positives.)


Rpmlint (installed packages)
----------------------------
touchegg.x86_64: W: no-manual-page-for-binary touchegg
 1 packages and 0 specfiles checked; 0 errors, 1 warnings, 0 badness

(Missing man page warning can be ignored.)


Source checksums
----------------
https://github.com/JoseExposito/touchegg/archive/2.0.12/touchegg-2.0.12.tar.gz :
  CHECKSUM(SHA256) this package     : f0ee467522c7c9f1295365324515d861888ed7645ffeccecca507ee87eed3e37
  CHECKSUM(SHA256) upstream package : f0ee467522c7c9f1295365324515d861888ed7645ffeccecca507ee87eed3e37


Requires
--------
touchegg (rpmlib, GLIBC filtered):
    /bin/sh
    config(touchegg)
    libX11.so.6()(64bit)
    libXi.so.6()(64bit)
    libXrandr.so.2()(64bit)
    libXtst.so.6()(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)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgtk-3.so.0()(64bit)
    libinput.so.10()(64bit)
    libinput.so.10(LIBINPUT_0.12.0)(64bit)
    libinput.so.10(LIBINPUT_0.20.0)(64bit)
    libm.so.6()(64bit)
    libpugixml.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.5)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    libudev.so.1()(64bit)
    libudev.so.1(LIBUDEV_183)(64bit)
    rtld(GNU_HASH)

touchegg-debuginfo (rpmlib, GLIBC filtered):

touchegg-debugsource (rpmlib, GLIBC filtered):



Provides
--------
touchegg:
    config(touchegg)
    touchegg
    touchegg(x86-64)

touchegg-debuginfo:
    debuginfo(build-id)
    touchegg-debuginfo
    touchegg-debuginfo(x86-64)

touchegg-debugsource:
    touchegg-debugsource
    touchegg-debugsource(x86-64)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 2035944
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, C/C++, Generic
Disabled plugins: PHP, SugarActivity, Ocaml, Haskell, Java, Perl, Python, R, fonts
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

https://pagure.io/releng/fedora-scm-requests/issue/40642

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

FEDORA-2022-96a25b4b68 has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-96a25b4b68

FEDORA-2022-96a25b4b68 has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2022-78d3034682 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-78d3034682

FEDORA-2022-60c4ae0fff has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2022-60c4ae0fff

FEDORA-2022-60c4ae0fff has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-60c4ae0fff \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-60c4ae0fff

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-78d3034682 has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-78d3034682 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-78d3034682

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-b1272e9b4e has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2022-b1272e9b4e`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-b1272e9b4e

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-e5bd78a75e has been pushed to the Fedora 34 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2022-e5bd78a75e`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-e5bd78a75e

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-e5bd78a75e has been pushed to the Fedora 34 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2022-b1272e9b4e has been pushed to the Fedora 35 stable repository.
If problem still persists, please make note of it in this bug report.