Bug 2387625
| Summary: | Review Request: cellbroadcastd - DBus daemon for cellular broadcast messages | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Tomi Lähteenmäki <lihis> |
| Component: | Package Review | Assignee: | Neal Gompa <ngompa13> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | ngompa13, package-review |
| Target Milestone: | --- | Flags: | ngompa13:
fedora-review+
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| URL: | https://gitlab.freedesktop.org/devrtz/%{name} | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | --- | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2025-09-01 19:53:04 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 2387375 | ||
| Attachments: | |||
|
Description
Tomi Lähteenmäki
2025-08-11 14:52:23 UTC
Copr build: https://copr.fedorainfracloud.org/coprs/build/9398494 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2387625-cellbroadcastd/fedora-rawhide-x86_64/09398494-cellbroadcastd/fedora-review/review.txt Found issues: - Systemd user unit service file(s) in cellbroadcastd 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. Taking this review. Spec review notes: > Source: %{url}/-/archive/v%{version}/%{name}-v%{version}.tar.gz > Source: https://gitlab.gnome.org/GNOME/gvdb/-/archive/%{gvdb_commit}/gvdb-%{gvdb_commit}.tar.gz Please number your sources, since you have multiple ones. It may not be required, but it makes it more coherent. > ExcludeArch: i686 This should be "ExcludeArch: %{ix86}" > %autosetup -a1 -n %{name}-v%{version} > mv gvdb-%{gvdb_commit} subprojects/gvdb This can be simplified to the following: %autosetup -n %{name}-v%{version} tar -xf %{S:1} -C subprojects/gvdb > %files -n libcellbroadcast-devel > %{_includedir}/libcellbroadcast-0.0/ > %{_libdir}/libcellbroadcast-0.0.so > %{_libdir}/pkgconfig/libcellbroadcast-0.0.pc > %changelog > %autochangelog This is missing a newline after the last files entry to separate sections. Spec URL: https://codeberg.org/Lihis/cellbroadcastd-spec/raw/commit/57045aafbe12f4cbe7c43fe5c3a999b309cea5f3/cellbroadcastd.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/1658/136171658/cellbroadcastd-0.0.2-2.fc44.src.rpm Numbered the sources, fixed the "ExcludeArch" and added the missing newline. > This can be simplified to the following: > > %autosetup -n %{name}-v%{version} > tar -xf %{S:1} -C subprojects/gvdb Why this is better than having the "-a1" for %autosetup? Created attachment 2103936 [details]
The .spec file difference from Copr build 9398494 to 9437833
Copr build: https://copr.fedorainfracloud.org/coprs/build/9437833 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2387625-cellbroadcastd/fedora-rawhide-x86_64/09437833-cellbroadcastd/fedora-review/review.txt Found issues: - Systemd user unit service file(s) in cellbroadcastd 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. >> This can be simplified to the following:
>>
>> %autosetup -n %{name}-v%{version}
>> tar -xf %{S:1} -C subprojects/gvdb
>
> Why this is better than having the "-a1" for %autosetup?
Well, it removes the need for move and the "-a1" flag.. Posting updated spec soon
Spec URL: https://codeberg.org/Lihis/cellbroadcastd-spec/raw/commit/cdb802d2139a2f70dab1ee0ef62826a4c57ccbef/cellbroadcastd.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/4293/136174293/cellbroadcastd-0.0.2-3.fc44.src.rpm Created attachment 2103940 [details]
The .spec file difference from Copr build 9437833 to 9438236
Copr build: https://copr.fedorainfracloud.org/coprs/build/9438236 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2387625-cellbroadcastd/fedora-rawhide-x86_64/09438236-cellbroadcastd/fedora-review/review.txt Found issues: - Systemd user unit service file(s) in cellbroadcastd 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. Spec review notes:
There's a missing desktop-file-validate call in %check for installed .desktop file in %{_datadir}/applications.
Also, one other piece...
> %build
> %meson -Dsystemd_user_unit_dir="%{_userunitdir}"
This should be in %conf instead of %build unless you plan to build this for < EPEL 10.
For an example, see: https://src.fedoraproject.org/rpms/wayback/blob/rawhide/f/wayback.spec
Spec URL: https://codeberg.org/Lihis/cellbroadcastd-spec/raw/commit/89a6a05699cd79e7d0b49225f3c22eb774474d43/cellbroadcastd.spec SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/990/136400990/cellbroadcastd-0.0.2-4.fc44.src.rpm > There's a missing desktop-file-validate call in %check for installed .desktop file in %{_datadir}/applications. Upstream tests [1] already validates it but added explicit check. > Also, one other piece... > > > %build > > %meson -Dsystemd_user_unit_dir="%{_userunitdir}" > > This should be in %conf instead of %build unless you plan to build this for < EPEL 10. > > For an example, see: https://src.fedoraproject.org/rpms/wayback/blob/rawhide/f/wayback.spec I see, thanks! I'll send a PR to update Packaging Guidelines for Meson as its example SPEC [2] does not contain %conf at all. [1] https://gitlab.freedesktop.org/devrtz/cellbroadcastd/-/blob/7229aad3bb60521a54c1c9a1abfaf4ecd8a1dd51/data/meson.build#L61 [2] https://docs.fedoraproject.org/en-US/packaging-guidelines/Meson/#_example_rpm_spec_file Created attachment 2104577 [details]
The .spec file difference from Copr build 9438236 to 9461965
Copr build: https://copr.fedorainfracloud.org/coprs/build/9461965 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2387625-cellbroadcastd/fedora-rawhide-x86_64/09461965-cellbroadcastd/fedora-review/review.txt Found issues: - Systemd user unit service file(s) in cellbroadcastd 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. @ngompa do you have time to check this? Review notes: * Package follows Fedora packaging guidelines * Package builds and installs * Package licensing is correct and license data is installed * No serious issues from rpmlint PACKAGE APPROVED. The Pagure repository was created at https://src.fedoraproject.org/rpms/cellbroadcastd FEDORA-2025-401cc63851 (cellbroadcastd-0.0.2-1.fc44) has been submitted as an update to Fedora 44. https://bodhi.fedoraproject.org/updates/FEDORA-2025-401cc63851 FEDORA-2025-401cc63851 (cellbroadcastd-0.0.2-1.fc44) has been pushed to the Fedora 44 stable repository. If problem still persists, please make note of it in this bug report. |