https://bugzilla.redhat.com/show_bug.cgi?id=2107381 --- Comment #2 from Jonathan Wright <jonathan@xxxxxxxxxxxxx> --- 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/UnownedDirectories/ 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 _______________________________________________ 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 on the list, report it: https://pagure.io/fedora-infrastructure
