https://bugzilla.redhat.com/show_bug.cgi?id=2176206 Gustavo Costa <xfgusta@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(benson_muite@emai | |lplus.org) --- Comment #2 from Gustavo Costa <xfgusta@xxxxxxxxx> --- Hi Benson. There are a couple of things we can improve, I'll point them out and give suggestions. # Source > Source0: https://github.com/fanglingsu/%{name}/archive/refs/tags/%{version}.tar.gz I'd recommend using this URL: https://github.com/fanglingsu/vimb/archive/%{version}/%{name}-%{version}.tar.gz This will give you a more meaningful tarball filename. https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags # BuildRequires > webkit2gtk4.0-devel You should use pkgconfig over -devel whenever possible. In this case it would be pkgconfig(webkit2gtk-4.0) gtk+-3.0 is also a dependency. Add a pkgconfig(gtk+-3.0) BuildRequires. https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires # Build > %make_build PREFIX=%{_prefix} \ > BINPREFIX=%{_bindir} \ > MANPREFIX=%{_mandir} \ > DOTDESKTOPPREFIX=%{_datadir}/applications \ > LIBDIR=%{_libdir} \ > EXTENSIONDIR=%{_libdir}/vimb I think %make_build EXTENSIONDIR=%{_libdir}/%{name} is enough. EXTENSIONDIR is used as a macro in the source code. # Install > mkdir -p %{buildroot}%{_bindir} > install -m 755 src/%{name} %{buildroot}%{_bindir} > mkdir -p %{buildroot}%{_libdir}/%{name} > install -s -m 644 src/webextension/webext_main.so %{buildroot}%{_libdir}/%{name} > mkdir -p %{buildroot}%{_mandir}/man1/ > install -m 644 doc/%{name}.* %{buildroot}%{_mandir}/man1/ > mkdir -p %{buildroot}%{_metainfodir}/ > install -m 644 %{SOURCE1} %{buildroot}%{_metainfodir}/ > mkdir -p %{buildroot}%{_datadir}/applications > install -m 644 %{name}.desktop %{buildroot}%{_datadir}/applications We can simplify all this by using %make_install and installing the metainfo file afterwards: %make_install PREFIX=%{_prefix} \ LIBDIR=%{buildroot}/%{_libdir}/%{name} \ EXTENSIONDIR=%{buildroot}/%{_libdir}/%{name} mkdir -p %{buildroot}/%{_metainfodir} install -p -m 644 %{SOURCE1} %{buildroot}/%{_metainfodir} The -p option in the install command is used to preserve the timestamps. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_timestamps # Check > desktop-file-validate %{buildroot}/%{_datadir}/applications/*.desktop > appstream-util validate-relax --nonet %{buildroot}%{_metainfodir}/*.metainfo.xml I'd recommend using %{name} instead of wildcards. -- 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=2176206 _______________________________________________ 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
