Bug 2370890

Hi!

I'm new to Fedora Packaging and reviewing some packages as part of working towards sponsorship into the packager group. I use `acme.sh` quite a bit myself, so it seemed fitting to review this one. I can't perform binding reviews yet, but hopefully this helps.

I'll post some things I spotted this comment and then I can post the full fedora-review checklist in the future.

1. The License field isn't a valid SPDX expression because of the use of `GPL-3.0` based on https://spdx.org/licenses/ this needs to be either `GPL-3.0-only` or `GPL-3.0-or-later`.

2. From what I understand, the systemd_post scriptlet needs activating in the %post section too, even if the timer isn't being started by default.

Other than that, this is looking pretty good to me so far.

Thanks Alex for your review!

Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec
SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm

- Changed license to GPL-3.0-or-later
- Added systemd_post scriplet

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2370890-acme.sh/fedora-rawhide-x86_64/09150225-acme.sh/fedora-review/review.txt

Found issues:

- Systemd service file(s) in acme.sh
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets

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.

Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec
SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm

Another try to check if the systemd issue is gone

Created attachment 2093565 [details]
The .spec file difference from Copr build 9150225 to 9150712

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2370890-acme.sh/fedora-rawhide-x86_64/09150712-acme.sh/fedora-review/review.txt

Found issues:

- Systemd service file(s) in acme.sh
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets

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.

Just a few comments:

```
%post
%systemd_post acmesh-renew.timer
# Remind users to start acmesh-renew.timer if they need acme.sh to automatically renew certs
if [ "$1" -eq 1 ] ; then
  echo ""
  echo "acme.sh auto renewal timer is not started by default."
  echo "Run 'systemctl start acmesh-renew.timer' to enable automatic renewals."
fi

%preun
%systemd_preun acmesh-renew.timer

%postun
%systemd_postun_with_restart acmesh-renew.timer
```

I'm pretty sure this not what you want here. I don't think restarting a timer doesn't do anything ... and printing output in RPM scriptlets is very frowned upon.

I would just do what's documented in the Packaging Guidelines for systemd services:

```
%post
%systemd_post acmesh-renew.service

%preun
%systemd_preun acmesh-renew.service

%postun
%systemd_postun_with_restart acmesh-renew.service
```

Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec
SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm

I copied the way certbot package is doing:

https://src.fedoraproject.org/rpms/certbot/blob/rawhide/f/certbot.spec#_273-293

But agree that it might not be the best option, so new spec changes that.