https://bugzilla.redhat.com/show_bug.cgi?id=1826439 --- Comment #22 from igor.ivanov.va@xxxxxxxxx --- Hi Michal, I have updated libvma.spec to meet most of your requirements and added additional patch as https://raw.githubusercontent.com/igor-ivanov/libvma/pr/fedora-package/fedora/0002-Update-systemctl-files.patch For others please look at my comments inline. Thanks, Igor (In reply to Michal Schmidt from comment #21) > Hi Igor, > I have not added you to the packagers group yet. I have some questions and > comments about the package. > > libvma.spec: > > BuildRequires: libibverbs-devel > > BuildRequires: rdma-core-devel > Done > These days, libibverbs-devel is provided by rdma-core-devel. > > > BuildRequires: libnl3-devel > > Your config/m4/nl.m4 uses PKG_CHECK_MODULES, so you should BR > pkgconfig(libnl-route-3.0) instead. > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > PkgConfigBuildRequires/ > Done > > %build > > export revision=%{use_rel} > > This seems unnecessary. I did not find anything referencing the "revision" > environment variable in your build system. > It is used during configure to set VMA_SVN_REVISION. > > %install > > %{make_build} DESTDIR=${RPM_BUILD_ROOT} install > > You should use %{make_install} instead. > Corrected but following rules as https://docs.fedoraproject.org/en-US/packaging-guidelines/#_why_the_makeinstall_macro_should_not_be_used permit original usage. > > %files > > %{_libdir}/%{name}.so* > > So the package contains something like this: > libvma.so -> libvma.so.9.1.0 > libvma.so.9 -> libvma.so.9.1.0 > libvma.so.9.1.0 > > Since libvma.so is to be used only for LD_PRELOAD and no program should be > linked against it (and have DT_NEEDED for it),: > 1. is there a point in versioning the library? > 2. does it have to be installed in the linker's default path? Would it make > more sense to have it as %{_libdir}/libvma/libvma.so ? > livma has own extra api with vma_extra.h header file and admits direct linkage. > > Do you actually use configure_options for anything? I don't think it's > common for Fedora packages to have macros like that "just in case". > This option is used to provide flexibility in final binary libvma version. For example: this option can be used to enable tso support (--enable-tso) that increases throughput but not ready to be included by default. > use_rel - This is fascinating. The macro not only determines the value of > the Release tag, it also influences the build where its value gets used for > version compatibility checks and encoded in version strings. To me the > mechanism seems needlessly complicated and I fail to see the benefit of it. > Also, are you aware that sometimes automatic scripts (e.g. release > engineering mass rebuilds) bump the Release fields in Fedora spec files? > Normally in RPM, Version is the upstream version and Release serves > packaging needs. > I removed it but it is used in pair with configure_options to differentiate packages for the customers on our side. > 30-libvma-limits.conf: > > # Default limits that are needed for proper work of libvma > > # Read more about this topic in the VMA's User Manual > > Can you point me to where in the manual is this discussed? > > > * - memlock unlimited > > * soft memlock unlimited > > * hard memlock unlimited > > Does having the package installed give every user on the system the > permission to mlock unlimited amounts of RAM? Is that really necessary? > Could it be at least limited to a user group? > There is such recommendation at https://docs.mellanox.com/display/VMAv902/VMA+Installation+Options that forces to https://github.com/Mellanox/libvma/wiki/VMA-over-RHEL-7.x-with-inbox-driver#4-common-configurations > vma.service: > > [Unit] > > Description=VMA Daemon. Version: 9.1.0-0 > > Please consider dropping the version from the Description. No other service > has that. > Done > > After=network.target syslog.target > > syslog.target became irrelevant and was removed in 2013. Please remove that > dependency. I am aware some packages still have that, but it's just > cargo-cult now. > > > Requires=network.target > > This does not make sense for your daemon. See man systemd.special(7) about > the intended use of network.target. > Done > > [Service] > > Type=forking > > Restart=on-failure > > ExecStart=/usr/sbin/vma start > > ExecStop=/usr/sbin/vma stop > > /usr/sbin/vma is a SysV initscript. That's terrible. > Why not start the deamon binary directly?: > ExecStart=/usr/sbin/vmad > (And maybe set KillSignal if needed.) > It does support of some systems easier for us. > > ExecReload=/usr/sbin/vma restart > > You must not fake the reload operation with restart. If the service cannot > do a reload, just do not define ExecReload. > > > RestartForceExitStatus=1 SIGTERM > > It's unusual to need to use this setting. There may be a good reason for it, > but please double check. SIGTERM is generated by kill utility by default. So it is used to restart vmad in this case too. -- 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
