Bug 2344534

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8631918
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2344534-awatcher/fedora-rawhide-x86_64/08631918-awatcher/fedora-review/review.txt

Found issues:

- Systemd user unit service file(s) in aw-awatcher
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_units

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Looks mostly OK, with some caveats:

> %global commit a0cdef90cf86cd8d2cc89723f5751c1123ae7e2b
> %global short_commit %(c=%{commit}; echo ${c:0:7})

It would be great if you could make this unambiguous that this refers to the bundled aw-server-rust snapshot, and not awatcher itself.

> # Mozilla Public License 2.0

This is not an SPDX license identifier, it looks like it's from awatcher-0.3.1/Cargo.toml:

> license = "Mozilla Public License 2.0"

This is a bug.

> # prefix with aw- in order to be detected as a watcher in aw-qt
> mv %{buildroot}%{_bindir}/{%{name},%{watcher_name}}

If you rename the binaries anyway, you could just remove the call to `%cargo_install` and install them the way you need from target/rpm/* directly.

> aw-server = { git = "https://github.com/ActivityWatch/aw-server-rust", optional = true, rev = "656f3c9" }
> aw-datastore = { git = "https://github.com/ActivityWatch/aw-server-rust", optional = true, rev = "656f3c9" }

You're patching watchers/Cargo.toml to replace the git dependency there, but these two git dependencies in the root Cargo.toml remain. How does this even work? Is it because the "bundle" feature is not enabled by default?

In general, it's really unfortunate that this project needs to basically import aw-server-rust code via git. Usually this is not a desirable state for packaging in Fedora. At the very least, you'd need to declare virtual Provides for the bundled aw-server-rust code snapshot. But otherwise it seems to be done correctly.

Ideally the common interfaces would be published as a library on crates.io. Is that something that has been considered upstream? I don't think the git snapshot dependency would be a sustainable solution for them long-term, either.

(In reply to Fabio Valentini from comment #2)
> Looks mostly OK, with some caveats:
> 
> > %global commit a0cdef90cf86cd8d2cc89723f5751c1123ae7e2b
> > %global short_commit %(c=%{commit}; echo ${c:0:7})
> 
> It would be great if you could make this unambiguous that this refers to the
> bundled aw-server-rust snapshot, and not awatcher itself.

Done.
 
> > # Mozilla Public License 2.0
> 
> This is not an SPDX license identifier, it looks like it's from
> awatcher-0.3.1/Cargo.toml:
> 
> > license = "Mozilla Public License 2.0"
> 
> This is a bug.

I created a PR.

> > # prefix with aw- in order to be detected as a watcher in aw-qt
> > mv %{buildroot}%{_bindir}/{%{name},%{watcher_name}}
> 
> If you rename the binaries anyway, you could just remove the call to
> `%cargo_install` and install them the way you need from target/rpm/*
> directly.

Done.

> > aw-server = { git = "https://github.com/ActivityWatch/aw-server-rust", optional = true, rev = "656f3c9" }
> > aw-datastore = { git = "https://github.com/ActivityWatch/aw-server-rust", optional = true, rev = "656f3c9" }
> 
> You're patching watchers/Cargo.toml to replace the git dependency there, but
> these two git dependencies in the root Cargo.toml remain. How does this even
> work? Is it because the "bundle" feature is not enabled by default?

Yes. This package can be compiled as bundled with aw-server-rust (for convenience) 
or as a watcher to aw-server-rust. I just use the second option, so I believe only some interface parts are needed.

> In general, it's really unfortunate that this project needs to basically
> import aw-server-rust code via git. Usually this is not a desirable state
> for packaging in Fedora. At the very least, you'd need to declare virtual
> Provides for the bundled aw-server-rust code snapshot. But otherwise it
> seems to be done correctly.

Done with the virtual Provides.

> Ideally the common interfaces would be published as a library on crates.io.
> Is that something that has been considered upstream? I don't think the git
> snapshot dependency would be a sustainable solution for them long-term,
> either.

Not that I know of. The activitywatch app is distributed as a bundle (aw-server included) from https://github.com/ActivityWatch/activitywatch at https://aur.archlinux.org/packages/activitywatch-bin
There was an idea at https://github.com/ActivityWatch/activitywatch/issues/92#issuecomment-1583938452 
to merge the watcher into that bundle, but it didn't happen. The development in general slowed down though.
I added a comment about that to the SPEC file.

Is the SPEC file OK now?

[fedora-review-service-build]

Created attachment 2076902 [details]
The .spec file difference from Copr build 8631918 to 8664955

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8664955
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2344534-awatcher/fedora-rawhide-x86_64/08664955-awatcher/fedora-review/review.txt

Found issues:

- Systemd user unit service file(s) in aw-awatcher
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_units

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

ping

ping

I get so many emails that "ping"ing me here doesn't achieve much ...
I'll get back to it when I have time. Hopefully later today.

(In reply to Fabio Valentini from comment #8)
> I get so many emails that "ping"ing me here doesn't achieve much ...

You mean that I should ping you somewhere else?

> I'll get back to it when I have time. Hopefully later today.

Thanks.

Some problems with associating spec parts with the correct subpackage:

> Requires:       aw-server-rust

and

> # Hopefuly one day it will be a part of aw-server-rust
> # https://github.com/ActivityWatch/activitywatch/issues/92#issuecomment-1583938452
> Provides:       bundled(aw-server-rust)

and all of:

> %post
> %systemd_user_post %{watcher_name}.service
> %preun
> %systemd_user_preun %{watcher_name}.service
> %postun
> %systemd_user_postun_with_restart %{watcher_name}.service

These all need to be associated with the "aw-awatcher" subpackage, otherwise they won't have any effect.

I wonder if it could make sense to just name the "source" package "aw-awatcher" instead?
That would let you avoid this kind of issue.

The project also provides a .desktop file, would you like to package that?

(In reply to Fabio Valentini from comment #10)
> Some problems with associating spec parts with the correct subpackage:
> 
> > Requires:       aw-server-rust
> 
> and
> 
> > # Hopefuly one day it will be a part of aw-server-rust
> > # https://github.com/ActivityWatch/activitywatch/issues/92#issuecomment-1583938452
> > Provides:       bundled(aw-server-rust)
> 
> and all of:
> 
> > %post
> > %systemd_user_post %{watcher_name}.service
> > %preun
> > %systemd_user_preun %{watcher_name}.service
> > %postun
> > %systemd_user_postun_with_restart %{watcher_name}.service
> 
> These all need to be associated with the "aw-awatcher" subpackage, otherwise
> they won't have any effect.

Done.

> I wonder if it could make sense to just name the "source" package
> "aw-awatcher" instead?
> That would let you avoid this kind of issue.

The project is named awatcher though, and I feel comfortable with that name.
That would avoid this kind of issue, but wouldn't make me aware of it.


> The project also provides a .desktop file, would you like to package that?

No. That would be needed if I would target for a bundle with aw-server-rust and would get an executable. I'm targeting for a non-executable plugin that would be loaded by aw-server-rust.

Could you approve now?

[fedora-review-service-build]

Created attachment 2079902 [details]
The .spec file difference from Copr build 8664955 to 8755190

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8755190
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2344534-awatcher/fedora-rawhide-x86_64/08755190-awatcher/fedora-review/review.txt

Found issues:

- Systemd user unit service file(s) in aw-awatcher
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_units

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

(In reply to wojnilowicz from comment #11)
> (In reply to Fabio Valentini from comment #10)
> > Some problems with associating spec parts with the correct subpackage:
> > 
> > > Requires:       aw-server-rust
> > 
> > and
> > 
> > > # Hopefuly one day it will be a part of aw-server-rust
> > > # https://github.com/ActivityWatch/activitywatch/issues/92#issuecomment-1583938452
> > > Provides:       bundled(aw-server-rust)
> > 
> > and all of:
> > 
> > > %post
> > > %systemd_user_post %{watcher_name}.service
> > > %preun
> > > %systemd_user_preun %{watcher_name}.service
> > > %postun
> > > %systemd_user_postun_with_restart %{watcher_name}.service
> > 
> > These all need to be associated with the "aw-awatcher" subpackage, otherwise
> > they won't have any effect.
> 
> Done.

No, this is not done. You moved the Provides and Requires, not didn't adapt the scriptlets.

The scriptlets need to be associated with the correct package name too, just like %package, %description, and %files:

"""
%post -n %{watcher_name}
%systemd_user_post %{watcher_name}.service

%preun -n %{watcher_name}
%systemd_user_preun %{watcher_name}.service

%postun -n %{watcher_name}
%systemd_user_postun_with_restart %{watcher_name}.service
"""

> > I wonder if it could make sense to just name the "source" package
> > "aw-awatcher" instead?
> > That would let you avoid this kind of issue.
> 
> The project is named awatcher though, and I feel comfortable with that name.
> That would avoid this kind of issue, but wouldn't make me aware of it.

What do you mean "wouldn't make me aware of it"? The issue could simply not occur.

> > The project also provides a .desktop file, would you like to package that?
> 
> No. That would be needed if I would target for a bundle with aw-server-rust
> and would get an executable. I'm targeting for a non-executable plugin that
> would be loaded by aw-server-rust.

Ok, this makes sense.

(In reply to Fabio Valentini from comment #14)
> (In reply to wojnilowicz from comment #11)
> > (In reply to Fabio Valentini from comment #10)
> > > Some problems with associating spec parts with the correct subpackage:
> > > 
> > > > Requires:       aw-server-rust
> > > 
> > > and
> > > 
> > > > # Hopefuly one day it will be a part of aw-server-rust
> > > > # https://github.com/ActivityWatch/activitywatch/issues/92#issuecomment-1583938452
> > > > Provides:       bundled(aw-server-rust)
> > > 
> > > and all of:
> > > 
> > > > %post
> > > > %systemd_user_post %{watcher_name}.service
> > > > %preun
> > > > %systemd_user_preun %{watcher_name}.service
> > > > %postun
> > > > %systemd_user_postun_with_restart %{watcher_name}.service
> > > 
> > > These all need to be associated with the "aw-awatcher" subpackage, otherwise
> > > they won't have any effect.
> > 
> > Done.
> 
> No, this is not done. You moved the Provides and Requires, not didn't adapt
> the scriptlets.
> 
> The scriptlets need to be associated with the correct package name too, just
> like %package, %description, and %files:
> 
> """
> %post -n %{watcher_name}
> %systemd_user_post %{watcher_name}.service
> 
> %preun -n %{watcher_name}
> %systemd_user_preun %{watcher_name}.service
> 
> %postun -n %{watcher_name}
> %systemd_user_postun_with_restart %{watcher_name}.service
> """

Fixed that one too. I hope it's done now. Thanks for further hints.

> > > I wonder if it could make sense to just name the "source" package
> > > "aw-awatcher" instead?
> > > That would let you avoid this kind of issue.
> > 
> > The project is named awatcher though, and I feel comfortable with that name.
> > That would avoid this kind of issue, but wouldn't make me aware of it.
> 
> What do you mean "wouldn't make me aware of it"? The issue could simply not
> occur.

True and I wouldn't be aware that it might occur. I would be ignorant to this. That's what I meant.

> > > The project also provides a .desktop file, would you like to package that?
> > 
> > No. That would be needed if I would target for a bundle with aw-server-rust
> > and would get an executable. I'm targeting for a non-executable plugin that
> > would be loaded by aw-server-rust.
> 
> Ok, this makes sense.

[fedora-review-service-build]

Created attachment 2080390 [details]
The .spec file difference from Copr build 8755190 to 8771432

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8771432
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2344534-awatcher/fedora-rawhide-x86_64/08771432-awatcher/fedora-review/review.txt

Found issues:

- Systemd user unit service file(s) in aw-awatcher
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_units

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

I'm still not sure what it brings you to have the source package name be "awatcher" but the package that users actually install be "aw-awatcher". It's a source package that builds one single binary package. Making their names different just creates complications. But if this is a hill you want to die on, that's fine, I guess ...

===

Changes look good to me, so package is APPROVED.

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

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

Issues:
=======
- systemd_user_post is invoked in %post and systemd_user_preun in %preun
  for Systemd user units service files.
  Note: Systemd user unit service file(s) in aw-awatcher
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/Scriptlets/#_user_units

  This seems to be a false postitive, the scriptlets are now called correctly.

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

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]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: %build honors applicable compiler flags or justifies otherwise.
[~]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: 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.
[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]: The License field must be a valid SPDX expression.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[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]: 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 8728 bytes in 1 files.
[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.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: Scriptlets must be sane, if used.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %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]: 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 all installed packages.
     Note: There are rpmlint messages (see attachment).
[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
-------
Checking: aw-awatcher-0.3.1-1.fc43.x86_64.rpm
          awatcher-0.3.1-1.fc43.src.rpm
============================ rpmlint session starts ============================
rpmlint: 2.6.1
configuration:
    /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
rpmlintrc: [PosixPath('/tmp/tmpcan9yat5')]
checks: 32, packages: 2

awatcher.spec:50: W: unversioned-explicit-provides bundled(aw-server-rust)
 2 packages and 0 specfiles checked; 0 errors, 1 warnings, 7 filtered, 0 badness; has taken 0.3 s 

Rpmlint (installed packages)
----------------------------
/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory
/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory
/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory
============================ rpmlint session starts ============================
rpmlint: 2.7.0
configuration:
    /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml
    /etc/xdg/rpmlint/fedora-spdx-licenses.toml
    /etc/xdg/rpmlint/fedora.toml
    /etc/xdg/rpmlint/scoring.toml
    /etc/xdg/rpmlint/users-groups.toml
    /etc/xdg/rpmlint/warn-on-functions.toml
checks: 32, packages: 1

aw-awatcher.x86_64: W: unused-direct-shlib-dependency /usr/bin/aw-awatcher /lib64/libm.so.6
 1 packages and 0 specfiles checked; 0 errors, 1 warnings, 3 filtered, 0 badness; has taken 0.1 s 

Source checksums
----------------
https://github.com/ActivityWatch/aw-server-rust/archive/a0cdef90cf86cd8d2cc89723f5751c1123ae7e2b/aw-server-rust-a0cdef9.tar.gz :
  CHECKSUM(SHA256) this package     : 1871077fdc0a8a0ff4fe2ce0d3c934ac86ab9c1aade3bce7b93ef51665ce8601
  CHECKSUM(SHA256) upstream package : 1871077fdc0a8a0ff4fe2ce0d3c934ac86ab9c1aade3bce7b93ef51665ce8601
https://github.com/2e3s/awatcher/archive/refs/tags/v0.3.1.tar.gz :
  CHECKSUM(SHA256) this package     : 384b3266e5ea869ec670c9fb65667f2ac7232cdcf411151d8216c2470c58b2e3
  CHECKSUM(SHA256) upstream package : 384b3266e5ea869ec670c9fb65667f2ac7232cdcf411151d8216c2470c58b2e3

Requires
--------
aw-awatcher (rpmlib, GLIBC filtered):
    /bin/sh
    aw-server-rust
    ld-linux-x86-64.so.2()(64bit)
    libc.so.6()(64bit)
    libcrypto.so.3()(64bit)
    libcrypto.so.3(OPENSSL_3.0.0)(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3)(64bit)
    libgcc_s.so.1(GCC_4.2.0)(64bit)
    libm.so.6()(64bit)
    libssl.so.3()(64bit)
    libssl.so.3(OPENSSL_3.0.0)(64bit)
    rtld(GNU_HASH)

Provides
--------
aw-awatcher:
    aw-awatcher
    aw-awatcher(x86-64)
    bundled(aw-server-rust)

The Pagure repository was created at https://src.fedoraproject.org/rpms/awatcher

FEDORA-2025-cfd2437945 (awatcher-0.3.1-1.fc43) has been submitted as an update to Fedora 43.
https://bodhi.fedoraproject.org/updates/FEDORA-2025-cfd2437945

(In reply to Fabio Valentini from comment #18)
> I'm still not sure what it brings you to have the source package name be
> "awatcher" but the package that users actually install be "aw-awatcher".
> It's a source package that builds one single binary package. Making their
> names different just creates complications. But if this is a hill you want
> to die on, that's fine, I guess ...

Thanks. I just stick with the naming of the upstream project. One can not go bad with that.

Should some more watchers appear in the future under the name "awatcher", then I wouldn't have to rename the source package.

FEDORA-2025-cfd2437945 (awatcher-0.3.1-1.fc43) has been pushed to the Fedora 43 stable repository.
If problem still persists, please make note of it in this bug report.