https://bugzilla.redhat.com/show_bug.cgi?id=1814682 Michal Schmidt <mschmidt@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mschmidt@xxxxxxxxxx --- Comment #2 from Michal Schmidt <mschmidt@xxxxxxxxxx> --- Commenting on a few lines in rshim.spec: > URL: https://github.com/mellanox/rshim-user-space Seeing the name "rshim-user-space" made me curious if there's a kernel part and I found: https://github.com/mellanox/rshim "BlueField RSHIM driver" What is the relation between the kernel and the userspace drivers? Was the kernel driver the original approach which later got obsoleted by the userspace implementation? > Obsoletes: %{name} < 2.0 This line says: rshim-2.0 obsoletes rshim < 2.0. That seems redundant. > %global with_systemd %(if ( test -d "%{_unitdir}" > /dev/null); then echo -n '1'; else echo -n '0'; fi) Why not assume systemd always? > %prep > rm -fr %{name}-%{version} > mkdir %{name}-%{version} > tar -axf %{SOURCE0} -C %{name}-%{version} --strip-components 1 > %setup -q -D -T Would this simpler way work too?: %prep %setup -n rshim-user-space-%{name}-%{version} Or maybe, since you are the upstream maintainer, you could change the upstream tarball generation to avoid the odd directory name? > %install > %makeinstall -C src INSTALL_DIR="%{buildroot}%{_bindir}" > %if "%{with_systemd}" == "1" > %{__install} -d %{buildroot}%{_unitdir} > %{__install} -m 0644 bfrshim.service %{buildroot}%{_unitdir} > %endif > %{__install} -d %{buildroot}%{_mandir}/man1 > %{__install} -m 0644 man/bfrshim.1 %{buildroot}%{_mandir}/man1 Why don't you let %makeinstall install these? > %__spec_install_post This looks like duplicating rpm-build's work. > %post > %if "%{with_systemd}" == "1" > systemctl daemon-reload > systemctl enable bfrshim.service > %endif No, you need to use the macros from the packaging guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd > %files > %license LICENSE > %defattr(-,root,root,-) This looks like the default, so the %defattr line is redundant. > %%doc README.md Double % sign, typo? > %changelog > * Fri Mar 13 2020 Liming Sun <lsun@xxxxxxxxxxxx> - 2.0-1 >- Update the spec file according to fedora packaging-guidelines > * Mon Dec 16 2019 Liming Sun <lsun@xxxxxxxxxxxx> >- Initial packaging Please add an empty line between changelog entries. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ 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
