https://bugzilla.redhat.com/show_bug.cgi?id=1830712 Carl George 🤠 <carl@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |carl@xxxxxxxxxx Flags| |fedora-review? Assignee|nobody@xxxxxxxxxxxxxxxxx |carl@xxxxxxxxxx Depends On|177841 (FE-NEEDSPONSOR) | Status|NEW |ASSIGNED --- Comment #23 from Carl George 🤠 <carl@xxxxxxxxxx> --- Andrea, I can see that you're in the packager group now, so I'm removing FE-NEEDSPONSOR. I can take this review. ================================================================================ What's the purpose of the bootstrap macro? It doesn't seem to do anything except set the tests macro. It seems like just having the tests macros would be sufficient. Even better, you can switch it to a modern bcond conditional. %bcond tests 1 --or-- %bcond_without tests https://rpm-software-management.github.io/rpm/manual/conditionalbuilds.html ================================================================================ The current macro logic does not run the tests. Run the tests if possible. You might also want to switch to the %ctest macro. While experimenting with this I can't seem to get testtimedisplay to pass, with or without the xvfb-run and dbus-launch commands. If you can sort that out please do, but otherwise it's ok to skip just that test. When skipping testtimedisplay, the xvfb-run and dbus-launch commands don't seem to be necessary. %if %{with tests} %ctest --verbose --exclude-regex testtimedisplay %endif https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites ================================================================================ As expected with the age of this review (not your fault), it will need to be updated to the current version, 2.3.0. Note that the license files are different in this version. Version: 2.3.0 %license LICENSES/GPL-2.0-or-later.txt LICENSES/CC0-1.0.txt ================================================================================ The license field needs to be updated to use SPDX identifiers for the license field. Looking at the 2.3.0 sources, I think it should be set to: # code is GPLv2, appdata file is CC0 License: GPL-2.0-or-later AND CC0-1.0 Your spec file mentions a GFDL license, but I don't see any reference to that in the 2.2.3 or 2.3.0 sources. https://docs.fedoraproject.org/en-US/legal/license-field/ ================================================================================ In the %check section, you have a `|| :` after your desktop-file-validate and appstream-util commands. Running those are mandatory, so ignoring non-zero exit statuses is incorrect. Please remove the `|| :` parts. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/ ================================================================================ As mentioned in comment 20, you're missing a build requirement for a compiler. BuildRequires: gcc-c++ https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ ================================================================================ fedora-review is complaining about /usr/share/man/ca/man1/kronometer.1.gz being listed twice. I think this might be because it matches the wildcard in %files and is also part of the %{name}.lang list. Removing `%{_mandir}/*/man1/kronometer.1*` from %files fixes this. ================================================================================ fedora-review is also complaining about unowned directories created by the package - /usr/share/doc/HTML and subdirectories - /usr/share/icons/hicolor and subdirectories This can be resolve by requiring the packages that own those directories. Requires: kde-filesystem Requires: hicolor-icon-theme https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirectories/ ================================================================================ Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=1830712 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
