https://bugzilla.redhat.com/show_bug.cgi?id=1814682
Michal Schmidt <mschm...@redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |mschm...@redhat.com
--- Comment #2 from Michal Schmidt <mschm...@redhat.com> ---
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 <l...@mellanox.com> - 2.0-1
>- Update the spec file according to fedora packaging-guidelines
> * Mon Dec 16 2019 Liming Sun <l...@mellanox.com>
>- 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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
