https://bugzilla.redhat.com/show_bug.cgi?id=2107381
--- Comment #2 from Jonathan Wright <jonathan(a)almalinux.org> ---
Thanks for the very thorough review!
(In reply to Carl George 🤠 from comment #1)
The Source URL should use the format in the package guidelines.
Optionally
consider using Source instead of Source0, and taking advantage of the %{url}
macro.
-Source0:
https://github.com/aristocratos/btop/archive/refs/tags/v%{version}.tar.gz
+Source: %{url}/archive/v%{version}/%{name}-%{version}.tar.gz
https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
#_git_tags
Excellent point!
=============================================================================
===
Does this need both gcc and gcc-c++ to build? I only see CXX in the
Makefile, not CC, so I think you can remove the former.
-BuildRequires: gcc
Old habits die hard :( gcc should not be needed.
=============================================================================
===
Wrap the description at 80 characters.
https://docs.fedoraproject.org/en-US/packaging-guidelines/
#_summary_and_description
Noted!
=============================================================================
===
Consider including the CHANGELOG.md file as a %doc in %files.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
Good point!
=============================================================================
===
You'll need to run desktop-file-validate on the desktop file, either in
%install or %check.
+desktop-file-validate %{buildroot}%{_datadir}/applications/btop.desktop
https://docs.fedoraproject.org/en-US/packaging-guidelines/
#_desktop_file_install_usage
I've never come across desktop files before so these docs are very helpful!
=============================================================================
===
The file %{_datadir}/btop/README.md is a duplicate of what we put in
%{_docdir} (via %doc). I'd recommend rm-ing that file after the
%make_install.
=============================================================================
===
This current %files structure will result in %{_datadir}/btop being an
unowned directory, which isn't allowed. You can remove the explicit listing
of the contents of that directory and own it recursively.
-%{_datadir}/btop/themes/HotPurpleTrafficLight.theme
...(and the rest)...
+%{_datadir}/btop
Same thing with %{_datadir}/icons/hicolor/, which we should resolve by
requiring the package that does own it.
+Requires: hicolor-icon-theme
https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirector...
Makes perfect sense!
=============================================================================
===
In case upstream adds more resolutions of their icon, it would be better to
use a wildcard to pick them up. You could also glob the extension to avoid
a separate line for the svg.
-%{_datadir}/icons/hicolor/48x48/apps/btop.png
-%{_datadir}/icons/hicolor/scalable/apps/btop.svg
+%{_datadir}/icons/hicolor/*/apps/btop.*
Good to know that wildcards are acceptable!
=============================================================================
===
That's what I noticed manually reviewing the spec file. I wasn't able to
run fedora-review because it seems jonathanwright.me has certificate
problems. Can you fix the cert or post them somewhere else with a working
cert?
All of this information is very helpful. I need to go through all of the
packaging guidelines for various sections. In all the docs I read somehow I
didn't read most of them regarding various best practices which is how we ended
up here :)
I'll get this all patched up tomorrow. The certificate is fixed so you can run
fedora-review.
--
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=2107381