Spec URL: https://download.copr.fedorainfracloud.org/results/codyrobertson/batstat/fedora-rawhide-x86_64/06494691-batstat/batstat.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/codyrobertson/batstat/fedora-rawhide-x86_64/06494691-batstat/batstat-0%5E20230808gite107193-1.fc40.src.rpm Description: CLI battery status for Linux Koji Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=110208733 Fedora Account System Username: codyrobertson Jonathan Wright has agreed to sponsor me (FAS: jonathanspw).
Copr build: https://copr.fedorainfracloud.org/coprs/build/6742431 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2254109-batstat/fedora-rawhide-x86_64/06742431-batstat/fedora-review/review.txt Found issues: - Not a valid SPDX expression 'None'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 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.
Hey Cody, I can give this review a first pass to correct a few things, until Jonathan has time to complete the full review. ================================================================================ The most important problem I see is the lack of a license. I can see that upstream hasn't specified a license, which means they retain all rights and no one is allowed to redistribute the code. Fedora also requires that packages be licensed under open source licenses. I suggest filing an issue upstream to ask them what license the software is available under. Once upstream does specify a license, they'll likely add the license text file to the repository, at which point you need to include it in the package by adding this line to the %files section. %license LICENSE You'll also update the License: field to the appropriate SPDX expression. https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/licensing-a-repository#choosing-the-right-license https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packaged/ https://docs.fedoraproject.org/en-US/legal/license-field/ https://docs.fedoraproject.org/en-US/legal/allowed-licenses/ ================================================================================ Your debug_packages macro definition is disabling debuginfo packages. The guidelines state that packages should produce useful debuginfo packages, and only disable them when it's not possible to generate a useful one. I can see that the upstream makefile as-is doesn't create the debug symbols, which is likely why you added that line in the first place. It's better to resolve the lack of debug symbols rather than disabling the debuginfo packages. It's also required to honor the Fedora system-wide compiler flags. We can solve both of these issues at once with a small change. %make_build CCFLAGS="%{optflags}" https://docs.fedoraproject.org/en-US/packaging-guidelines/#_debuginfo_packages https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags ================================================================================ Unfortunately this upstream has never tagged a version, so the versioning will be a bit complex. The guidelines mention using version 0 in this scenario, but also have seperate instructions for packaging snapshots. We need both in this case, but the guidelines don't have instructions for combining them. I'm in the process of writing this up to improve the docs, but this is what it will look like for this package in the end. Note that this also adjust the Source0 line, not just for the snapshot bits but to use a complete URL to indicate where the tarball comes from, which is also part of the guidelines. %global commit e107193e99fb8d461050358f05aa8343e2fd5bc9 %global shortcommit %(c=%{commit}; echo ${c:0:7}) Version: 0~^1.%{shortcommit} Source0: %{url}/archive/%{commit}/batstat-%{shortcommit}.tar.gz %autosetup -n batstat-%{commit} https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_upstream_has_never_chosen_a_version https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections ================================================================================ It is recommended to use %autochange, especially since you are already using %autorelease. https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs ================================================================================ You can remove the gcc build requirement, since this is a c++ program. ================================================================================ You can remove the %check section since there are no tests to run. ================================================================================ The manual specification of /usr/bin in %install should be switched to %{_bindir} for consistency. mkdir -p %{buildroot}%{_bindir}
Spec URL: https://download.copr.fedorainfracloud.org/results/codyrobertson/batstat/fedora-rawhide-x86_64/08021222-batstat/batstat.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/codyrobertson/batstat/fedora-rawhide-x86_64/08021222-batstat/batstat-0~%5E1.d65abb7-1.fc42.src.rpm
Cody asked me to take over this review from Jonathan. The license handling needs a few more tweaks, and fedora-review noticed a few other things I didn't on the first pass. ================================================================================ I saw that upstream added a license. We need to include that file in %files. %files +%license LICENSE %{_bindir}/%{name} %doc README.md ================================================================================ We also need to use the SPDX identifier in the License tag. -License: GPLv3 +License: GPL-3.0-only URL: https://github.com/Juve45/%{name} Source0: %{url}/archive/%{commit}/%{name}-%{shortcommit}.tar.gz ================================================================================ There is an rpmlint warning about PIE. batstat.x86_64: W: position-independent-executable-suggested /usr/bin/batstat I had to dig into this one a bit, and it seems what's happening is the upstream makefile is overriding the environment LDFLAGS. There's probably a way to patch the upstream makefile to respect the environment LDFLAGS, but I don't know it offhand. This fails to compile without the flags that are in the makefile, so we need to keep those. Sorry I didn't notice this in my initial suggestion to override the makefile's CCFLAGS. I think the best approach is to invoke it with the system flags combined with the flags in the makefile. Doing it this way gets rid of the rpmlint warning and gives us those debug symbols we need. %build -%make_build CCFLAGS="%{optflags}" +%make_build CCFLAGS="%{optflags} -std=c++11" LDFLAGS="%{build_ldflags} -lncurses -pthread" https://docs.fedoraproject.org/en-US/packaging-guidelines/#_pie ================================================================================ rpmlint is complaining about the 775 permissions set by the Makefile. batstat.x86_64: E: non-standard-executable-perm /usr/bin/batstat 775 This one can be fixed with a simple sed in %prep. %prep %autosetup -n batstat-%{commit} +sed -e 's/-m 775/-m 755/' -i Makefile ================================================================================ I think with those fixes this will be ready to approve.
Looks like upstream tagged version 1.0, so along with the other fixes we can move the version field to that and drop the snapshot notation. https://github.com/Juve45/batstat/releases/tag/v1.0
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.