https://bugzilla.redhat.com/show_bug.cgi?id=2254109 --- Comment #2 from Carl George 🤠 <carl@xxxxxxxxxx> --- 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} -- 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=2254109 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202254109%23c2 -- _______________________________________________ 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
