[Bug 1322168] Review Request: ibacm - InfiniBand Communication Manager Assistant

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

 



https://bugzilla.redhat.com/show_bug.cgi?id=1322168



--- Comment #3 from Honggang LI <honli@xxxxxxxxxx> ---
(In reply to Michal Schmidt from comment #2)
> > %global _hardened_build 1
> 
> This is actually the default setting since Fedora 23:
>  https://fedoraproject.org/wiki/Changes/Harden_All_Packages
> I see the Packaging Guidelines don't mention that fact, but anyway,
> it's certainly OK to keep it in the spec file as well.
> 

Remove it.

> > # libibacmp.so is a plugin for the ibacm daemon, not a public library.
> > # Do not advertise it in RPM metadata:
> > %global _privatelibs libibacmp[.]so.*
> > %global __provides_exclude ^(%{_privatelibs})$
> > %global __requires_exclude ^(%{_privatelibs})$
> 
> I see this matches the style described in "Filtering provides and requires
> after scanning" in
> https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering.
> I would just suggest moving these lines down, right before the %description,
> to satisfy the recommendation in "Location of macro invocation" on the page.
> 

Moved to just before '%description'.

> > Name: ibacm
> > Version: 1.2.0
> > Release: 1%{?dist}
> > Summary: InfiniBand Communication Manager Assistant
> > Group: System Environment/Daemons
> 
> The Group: tag is unnecessary.

Removed it. 

> > License: GPLv2 or BSD
> > Url: http://www.openfabrics.org/
> > Source: http://downloads.openfabrics.org/downloads/rdmacm/%{name}-%{version}.tar.gz
> > Source1: ibacm.service
> 
> It would be nice to have a systemd unit file in upstream.
> /me looks inside ibacm.service and src/acm.c ... I can see the daemonization
> is implemented carelessly. In such a case Type=forking does not buy us
> anything. We may as well use Type=simple and ExecStart=/usr/bin/ibacm -P
> WantedBy=network.target is unusual and likely wrong.
> I'll look into improving this later myself (will file a BZ to myself once we
> have the component created).
> This is not a problem for the review.
> 
> > Patch0001: 0001-Coverity-and-compile-warning-fixes.patch
> 
> It would be nice to get rid of the patch by getting the changes to upstream.
> I may as well do this when I'm sending the systemd-related changes.
> Not a problem for the review.
> 
> > BuildRequires: libibverbs-devel >= 1.2.0
> > BuildRequires: libibumad-devel >= 1.3.10.2
> > BuildRequires: systemd
> 
> Should also add "BuildRequires: gcc" according to
> https://fedoraproject.org/wiki/Packaging:
> C_and_C%2B%2B#BuildRequires_and_Requires
> 

Added 'BuildRequires: gcc'.

> > Requires(post): systemd
> > Requires(preun): systemd
> > Requires(postun): systemd
> 
> These 3 lines can be expressed by:
> %{?systemd_requires}
> as suggested in https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd
> 

Replaced it as required.

> > [...]
> > %build
> > # ./autogen.sh
> > %configure CFLAGS="$CXXFLAGS -fno-strict-aliasing" LIBS=-lpthread
> 

Removed 'LIBS=-lpthread', and confirmed ibacm had been linked against pthread
as expected.

 ldd usr/bin/ib_acme
        linux-vdso.so.1 (0x00007ffc2f907000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f1f6017a000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f1f5ff76000)
        libibumad.so.3 => not found
        libibverbs.so.1 => not found
        libc.so.6 => /lib64/libc.so.6 (0x00007f1f5fba7000)
        /lib64/ld-linux-x86-64.so.2 (0x000055c674a7b000)


 ldd usr/sbin/ibacm
        linux-vdso.so.1 (0x00007ffc5d7f8000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f17dc092000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f17dbe8e000)
        libibumad.so.3 => not found
        libibverbs.so.1 => not found
        libc.so.6 => /lib64/libc.so.6 (0x00007f17dbabf000)
        /lib64/ld-linux-x86-64.so.2 (0x0000556510936000)

> Setting CFLAGS from CXXFLAGS is surely suspicious.
> A more customary way to add to CFLAGS is:
> 
>   CFLAGS="%{optflags} -fno-strict-aliasing"
>   %configure 
> 

Replaced it.

> I don't know why the setting of LIBS is needed. Maybe it really isn't?

It is unnecessary. Removed it.

-- 
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
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]