[Bug 1814682] Review Request: rshim - rshim driver for Mellanox BlueField SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux