https://bugzilla.redhat.com/show_bug.cgi?id=1826439 Michal Schmidt <mschmidt@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(mschmidt@redhat.c | |om) | --- Comment #21 from Michal Schmidt <mschmidt@xxxxxxxxxx> --- 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 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/ > %build > export revision=%{use_rel} This seems unnecessary. I did not find anything referencing the "revision" environment variable in your build system. > %install > %{make_build} DESTDIR=${RPM_BUILD_ROOT} install You should use %{make_install} instead. > %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 ? 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". 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. 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? vma.service: > [Unit] > Description=VMA Daemon. Version: 9.1.0-0 Please consider dropping the version from the Description. No other service has that. > 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. > [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.) > 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. -- 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
